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

[Jamal Hadi Salim]

On Sun, May 31, 2026 at 5:58 AM Jamal Hadi Salim <jhs@xxxxxxxxxxxx> wrote:
>
> "
>
> 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()?

>
> David, I am going back to min_t.

That was definetely subliminal ;-> I am going to make another update.
David you can improve things after, I just want this thing to go in so
going back to min_t.

Although these changes should not affect functionality, to everyone
who tested and reviewed if you can go ahead and review/test.

cheers,
jamal