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

[Jamal Hadi Salim]

On Mon, Jun 1, 2026 at 4:26 AM Jamal Hadi Salim <jhs@xxxxxxxxxxxx> wrote:
>
> On Sun, May 31, 2026 at 2:52 PM David Laight
> <david.laight.linux@xxxxxxxxx> wrote:
> >
> > On Sun, 31 May 2026 19:42:41 +0100
> > David Laight <david.laight.linux@xxxxxxxxx> wrote:
> >
> > > On Sun, 31 May 2026 13:25:36 -0400
> > > Jamal Hadi Salim <jhs@xxxxxxxxxxxx> wrote:
> > ....
> > > > >
> > > > > > +                             goto bad;
> > > > > > +                     if (skb_ensure_writable(skb, min_t(int, skb->len,
> > > > > > +                                                        write_len)))
> > >
> > > You've completely missed that the above has to include the write_offset.
> >
> > And I've missed that write_len isn't the length you are going to write :-(
> >
> > This version is unreadable....
> >
> > A few one line comments can make all the difference.
> > I put these two in for a reason.
> > It took some effort to find out exactly what they did.
>
> Double check this so i didnt miss anything you said.
>
> --------
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index bd3b1da3cd63..1607aaff3ed6 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -394,7 +394,7 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>         tkey_ex = parms->tcfp_keys_ex;
>
>         for (i = parms->tcfp_nkeys; i > 0; i--, tkey++) {
> -               int write_offset, write_len;
> +               int write_offset;
>                 int offset = tkey->off;
>                 int hoffset = 0;
>                 u32 cur_val, val;
> @@ -447,23 +447,16 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>                         goto bad;
>                 }
>
> -               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_t(int, skb->len,
> -
> write_offset + (int)sizeof(*ptr))))
> -                                       goto bad;
> -                       }
> -               } else {
> -                       if (check_add_overflow(write_offset, (int)sizeof(*ptr),
> -                                              &write_len))
> -                               goto bad;
> -                       if (skb_ensure_writable(skb, min_t(int, skb->len,
> -                                                          write_len)))
> -                               goto bad;
> -               }
> +               /* Check that any needed headroom is writeable */
> +               if (write_offset < 0 && skb_cow(skb, -write_offset))
> +                       goto bad;
> +
> +               /* Ensure the skb is writeable upto to the end of the area to be
> +                * written. Note: that data is pulled into the skb header
> +                */
> +               if ((write_offset >= 0 || write_offset +
> (int)sizeof(*ptr) > 0) &&
> +                   skb_ensure_writable(skb, write_offset + (int)sizeof(*ptr)))
> +                       goto bad;
>
>                 ptr = (u32 *)(skb->data + write_offset);
>                 cur_val = get_unaligned(ptr);
> --------------
>
> And please _no more_ changes after this. I will only make these
> changes if the bots complain about something meaningful.
>

I note that the patch hasnt been applied yet. Since none of the
sashiko's found an issue I wasnt planning on re-iterating the patch as
i stated above to avoid the review again, test again cycle.
Paolo/Jakub ..

cheers,
jamal