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

[Jamal Hadi Salim]

On Thu, May 28, 2026 at 6:26 AM Jamal Hadi Salim <jhs@xxxxxxxxxxxx> wrote:
>
> On Thu, May 28, 2026 at 6:03 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
> >
> > Jamal Hadi Salim <jhs@xxxxxxxxxxxx> writes:
> >
> > > From: Rajat Gupta <rajat.gupta@xxxxxxxxxxxxxxxx>
> > >
> > > tcf_pedit_act() computes the COW range for skb_ensure_writable()
> > > once before the key loop using tcfp_off_max_hint, but the hint does
> > > not account for the runtime header offset added by typed keys. This
> > > can leave part of the write region un-COW'd.
> > >
> > > Fix by moving skb_ensure_writable() inside the per-key loop where
> > > the actual write offset is known, and add overflow checking on the
> > > offset arithmetic. For negative offsets (e.g. Ethernet header edits
> > > at ingress), use skb_cow() to COW the headroom instead. Guard
> > > offset_valid() against INT_MIN, where negation is undefined.
> >
> > So you did tell us not to nitpick, but...
> >
>
> Actually, an opportunity to nitpick has opened up;-> I have to resend.
> In my rush to send the patch out i accidentally deleted the "Fixes"
> while adding names and removing obsolete commit log. Probably missed
> something else.
>

Sigh. There's another issue that both sashikos pointed out in v2 but i
wasnt paying attention.
https://sashiko.dev/#/patchset/20260526155913.1060694-1-jhs%40mojatatu.com
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260526155913.1060694-1-jhs%40mojatatu.com

I believe this is because we removed
skb_header_pointer/skb_store_bits() (which ensures you get an error if
you try to write beyond skb->len)

Toke/David - Would this be ok? It passes the tests - but i am afraid
it will require another round of reviews/tests

static bool offset_valid(struct sk_buff *skb, int offset, int len)
{
        if (offset < -(int)skb_headroom(skb))
                return false;

        return offset + len <= (int)skb->len;
}

If this is agreable can people test? then i will send the patch

cheers,
jamal


> > > 2) Add more optimal boundary checks (Toke & David L.)
> >
> > [..]
> >
> > > -     if  (offset < 0 && -offset > skb_headroom(skb))
> > > +     if (offset < 0 && offset < -(int)skb_headroom(skb))
> >
> > Seems that bit of the changelog isn't actually accurate.
> >
> > However, I don't think this matters, this version is not actually buggy;
> > so let's just get this merged, and we can code-golf the offset check on
> > top :)
> >
> > I did re-run the tests on this version, and they look fine, so
> > re-affirming my tags.
> >
> > -Toke
> >