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

[David Laight <david.laight.linux@gmail.com>]

On Tue, 26 May 2026 07:57:08 -0400
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> On Tue, May 26, 2026 at 5:48 AM David Laight
> <david.laight.linux@gmail.com> wrote:
> >
> > On Sat, 23 May 2026 12:57:08 -0400
> > Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >  
> > > On Sat, May 23, 2026 at 12:46 PM Jakub Kicinski <kuba@kernel.org> wrote:  
> > > >
> > > > On Sat, 23 May 2026 08:13:21 -0400 Jamal Hadi Salim wrote:  
> > > > > > > @@ -474,8 +473,6 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
> > > > > > >                 }
> > > > > > >
> > > > > > >                 *ptr = ((*ptr & tkey->mask) ^ val);
> > > > > > > -               if (ptr == &hdata)
> > > > > > > -                       skb_store_bits(skb, hoffset + offset, ptr, 4);
> > > > > > >         }
> > > > > > >
> > > > > > >         goto done;  
> > > > > >
> > > > > > I see you are trying to get rid of the skb_header_pointer() /
> > > > > > skb_store_bits() piece. Sure looks cleaner if we must linearize.  
> > > > >
> > > > > The other thing (i may be over thinking) with pskb_may_pull is: if the
> > > > > data is already linear (in a clone), wouldn't we corrupt the shared
> > > > > linear data of the clone?  
> > > >
> > > > I said
> > > >
> > > >   for the portion of the problem that's "we are writing to frags"
> > > >
> > > > IOW not replacing the rest of the patch (assuming we care).  
> > >
> > > So as an alternative to the piece i posted? i.e this:
> > >
> > > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> > > index 79921b8d89ba..8f0f84b50c85 100644
> > > --- a/net/sched/act_pedit.c
> > > +++ b/net/sched/act_pedit.c
> > > @@ -474,6 +474,12 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
> > >                 if (write_offset < 0) {
> > >                         if (skb_cow(skb, -write_offset))
> > >                                 goto bad;
> > > +                       if (write_offset + (int)sizeof(hdata) > 0) {
> > > +                               if (skb_ensure_writable(skb,
> > > +                                               min_t(int, skb->len,
> > > +                                                     write_offset +
> > > (int)sizeof(hdata))))  
> >
> > I don't think that needs to be min_t(), a plain min() will do.
> > (Possibly with the (int) cast removed from the sizeof.)
> >  
> 
> what is the benefit? would min() allow a mixed type (eg skb_len not being int)?

min_t() is really a broken idea.
You wouldn't really write:
	if ((int)skb->len < (int)(write_offset + (int)sizeof(hdata)))
which it what it effectively expands to.
IMHO it was always better to cast the one side that was 'wrong'.
Then there wouldn't be all the broken/pointless max_t(u8, x, 255).

Here min(skb->len, write_offset + (int)sizeof(hdata)) is actually likely to
be allowed even though they types are unsigned and signed because the
compiler knows the preceding 'if' stops negative values getting through.
(Sometimes the KASAN (etc) builds (randomly) error such cases.)

But min(skb->len, write_offset + sizeof(hdata)) will always be fine.
Both sides are unsigned, a negative 32bit write_offset is sign extended
to 64bits before being converted to unsigned (as 2s compliment) and the
sum is positive.
But the compiler might not do CSE with the previous condition.

-- David

> 
> cheers,
> jamal
> 
> > -- David
> >  
> > > +                                       goto bad;
> > > +                       }
> > >                 } else {
> > >                         if (unlikely(check_add_overflow(write_offset,
> > >                                                         (int)sizeof(hdata),
> > >
> > > cheers,
> > > jamal
> > >  
> >