2025-11-05, 15:47:00 -0800, Jacob Keller wrote:
On 11/5/2025 3:12 PM, Nate Karstens wrote:
Thanks, Jake!
So, without the ssize_t, I guess everything switches back to unsigned here when subtracting skb->len..
That's right. In C, if there is a mix of signed an unsigned, then signed are converted to unsigned and unsigned arithmetic is used.
Not if the signed type is bigger than the unsigned?
on x86_64 (with long = s64 and unsigned int = u32): (long)1 - (unsigned int)100 < 0 (int)1 - (unsigned int)100 > 0
Are you testing on some 32b arch? Otherwise ssize_t would be s64 and int/unsigned int should be 32b so the missing cast would not matter?
I don't quite recall the signed vs unsigned rules for this. Is stm.strp.offset also unsigned? which means that after head->len - skb->len resolves to unsigned 0 then we underflow?
Here is a summary of the types for the variables involved:
len => ssize_t (signed) (ssize_t)head->len => unsigned int cast to ssize_t skb->len => unsigned int, causes the whole comparison to use unsigned arithmetic stm->strp.offset => int (see struct strp_msg)
Ah, right so if we don't cast skb->len then the entire thing uses unsigned arithmetic which results in the bad outcome for certain values of input.
Casting would fix this. Another alternative would be to re-write the checks so that they don't fail when using unsigned arithmetic.
Given that we already cast one to ssize_t, it does seem reasonable to just add the other cast as your patch did.
Agree. And adding a summary of the information in this thread to the commit message would be really useful (clearly, this stuff is not so obvious :)).
If we don't actually use the strparser code anywhere then it could be dropped
It is still used elsewhere, and ktls still uses some of the data structures.
Right. Fixing it makes the most sense, so that other users don't accidentally behave unexpectedly.
Agree. I didn't mean to dismiss the presence of a bug, sorry if it sounded like that. But I was a bit unclear on the conditions, this discussion is helpful.