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

[Jamal Hadi Salim]

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.

cheers,
jamal
> > >         /* Ensure any needed headroom is writeable */
> > >         if (hoffset < 0 && skb_cow(skb, -hoffset)
> > >                 goto bad;
> > >         /* Ensure the skb is writeable to the end of the area to be written.
> > >          * The data is pulled into the skb header. */
>
> -- David