Re: [PATCH net v4 1/1] net/sched: fix pedit partial COW leading to page cache corruption

[Jamal Hadi Salim]

"

On Sat, May 30, 2026 at 12:15 PM Jamal Hadi Salim <jhs@xxxxxxxxxxxx> wrote:
>
> On Sat, May 30, 2026 at 11:19 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
> >
> > Jamal Hadi Salim <jhs@xxxxxxxxxxxx> writes:
> >

[..]

> > Re-tested and LGTM. Let's hope this is the last one ;)
> >
>
> Fingers crossed ;-> Let's see what our new overlords say tomorrow morning.
>

Sigh. Now both sashikos are complaining about the min() macro.

Sashiko 1:
=========

 + if (write_offset < 0) {
 + if (skb_cow(skb, -write_offset))
 + goto bad;
 + if (write_offset + (int)sizeof(*ptr) > 0) {
 + if (skb_ensure_writable(skb,
 + min(skb->len,
 +     write_offset + (int)sizeof(*ptr))))

Could this min() macro trigger a build failure?
skb->len is an unsigned int, but write_offset + (int)sizeof(*ptr) evaluates
to a signed int.
The min() macro enforces strict type checking and usually triggers a static
assertion failure when mixing signed and unsigned variables.
--------

And Sashiko 2, on the same issue
===========================
Will this build?  In the negative-offset arm the call is
min(skb->len, write_offset + (int)sizeof(*ptr))
skb->len is unsigned int and write_offset is a runtime int local, so
the two arguments differ in signedness.  The min() macro in
include/linux/minmax.h enforces signedness compatibility:
#define __careful_cmp(op, x, y) \
__cmp_once_check(op, x, y, ...)
with
BUILD_BUG_ON_MSG(!__types_ok(ux, uy), #op "(" #x ", " #y ") signedness error")
__is_nonneg() relies on __builtin_constant_p(), which returns 0 for a
runtime int such as write_offset, so the int side ends up signed-only
while skb->len is unsigned-only and __types_ok() is 0.
The matching positive-offset arm in the same hunk already uses
skb_ensure_writable(skb, min_t(int, skb->len, write_len))
and existing call sites such as those in net/core/skbuff.c and
net/ipv4/tcp_output.c likewise use min_t(int, skb->len, ...).  Should
the negative-offset arm use min_t(int, ...) for consistency and to
avoid the signedness BUILD_BUG_ON?
------------

So i looked at why this thing compiles despite the statements above...
I could be wrong but I think the compiler optimizes away with -O2
because under that if statement  "if (write_offset + (int)sizeof(*ptr)
> 0)" IOW, the compiler already knows this is a positive outcome.

David, this is what i had asked in the earlier discussion. It dos feel
like it is safe as is,  I suppose given the compiler I am using but is
it possible that older compilers may not do this optimization? It will
also very likely fail at lower optimizations..
Shall i restore to min_t()?

cheers,
jamal





David, I am going back to min_t.