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

From: David Laight <[email-masked]>
Date: Tue, 26 May 2026 10:48:21 +0100

On Sat, 23 May 2026 12:57:08 -0400
Jamal Hadi Salim <[email-masked]> wrote:

> On Sat, May 23, 2026 at 12:46 PM Jakub Kicinski <[email-masked]> 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.)

-- David

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