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

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

[Jamal Hadi Salim]

From: Rajat Gupta <rajat.gupta@oss.qualcomm.com>

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.

Additionally, linearize skbs with shared frags upfront to prevent
silent data corruption when pedit operates on zero-copy pages
(e.g. from sendfile).

Fixes: 8b796475fd78 ("net/sched: act_pedit: really ensure the skb is writable")
Reported-by: Yiming Qian <yimingqian591@gmail.com>
Reported-by: Keenan Dong <keenanat2000@gmail.com>
Reported-by: Han Guidong <2045gemini@gmail.com>
Reported-by: Zhang Cen <rollkingzzc@gmail.com>
Tested-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Rajat Gupta <rajat.gupta@oss.qualcomm.com>
---
Changes v1->v2:
 1. Do better boundary analysis to cover cloned skbs with frags. Pointed
    out by sashiko-nipa:
    https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260519033950.2037-1-rajat.gupta%40oss.qualcomm.com
 2. As a result of fix #1 remove the skb_has_shared_frag() check, unnecessary.
    Also Jakub has plans where the shared frags is not going to be a "thing"
 3. Make small adjustments everywhere for integer checks, suggested by D. Laight
 4. Remove all reviewers and testers since this is a large enough change.
    Please retest and re-review.
 5. Remove Rajat as reporter since he is the author (which implies he is a reporter)
---
 net/sched/act_pedit.c | 49 ++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index bc20f08a2789..719bee335e1f 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -16,6 +16,7 @@
 #include <linux/ip.h>
 #include <linux/ipv6.h>
 #include <linux/slab.h>
+#include <linux/overflow.h>
 #include <net/ipv6.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
@@ -323,7 +324,7 @@ static bool offset_valid(struct sk_buff *skb, int offset)
 	if (offset > 0 && offset > skb->len)
 		return false;
 
-	if  (offset < 0 && -offset > skb_headroom(skb))
+	if (offset < 0 && offset < -(int)skb_headroom(skb))
 		return false;
 
 	return true;
@@ -393,18 +394,10 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 	struct tcf_pedit_key_ex *tkey_ex;
 	struct tcf_pedit_parms *parms;
 	struct tc_pedit_key *tkey;
-	u32 max_offset;
 	int i;
 
 	parms = rcu_dereference_bh(p->parms);
 
-	max_offset = (skb_transport_header_was_set(skb) ?
-		      skb_transport_offset(skb) :
-		      skb_network_offset(skb)) +
-		     parms->tcfp_off_max_hint;
-	if (skb_ensure_writable(skb, min(skb->len, max_offset)))
-		goto done;
-
 	tcf_lastuse_update(&p->tcf_tm);
 	tcf_action_update_bstats(&p->common, skb);
 
@@ -412,9 +405,10 @@ 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 offset = tkey->off;
 		int hoffset = 0;
-		u32 *ptr, hdata;
+		u32 *ptr;
 		u32 val;
 		int rc;
 
@@ -451,15 +445,38 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 			}
 		}
 
-		if (!offset_valid(skb, hoffset + offset)) {
-			pr_info_ratelimited("tc action pedit offset %d out of bounds\n", hoffset + offset);
+		if (unlikely(check_add_overflow(hoffset, offset,
+						&write_offset))) {
+			pr_info_ratelimited("tc action pedit offset overflow\n");
 			goto bad;
 		}
 
-		ptr = skb_header_pointer(skb, hoffset + offset,
-					 sizeof(hdata), &hdata);
-		if (!ptr)
+		if (!offset_valid(skb, write_offset)) {
+			pr_info_ratelimited("tc action pedit offset %d out of bounds\n",
+					    write_offset);
 			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(skb->len,
+							    write_offset + sizeof(*ptr))))
+					goto bad;
+			}
+		} else {
+			if (unlikely(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;
+		}
+
+		ptr = (u32 *)(skb->data + write_offset);
 		/* just do it, baby */
 		switch (cmd) {
 		case TCA_PEDIT_KEY_EX_CMD_SET:
@@ -474,8 +491,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;
-- 
2.34.1

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

[Toke Høiland-Jørgensen]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> From: Rajat Gupta <rajat.gupta@oss.qualcomm.com>
>
> 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.
>
> Additionally, linearize skbs with shared frags upfront to prevent
> silent data corruption when pedit operates on zero-copy pages
> (e.g. from sendfile).
>
> Fixes: 8b796475fd78 ("net/sched: act_pedit: really ensure the skb is writable")
> Reported-by: Yiming Qian <yimingqian591@gmail.com>
> Reported-by: Keenan Dong <keenanat2000@gmail.com>
> Reported-by: Han Guidong <2045gemini@gmail.com>
> Reported-by: Zhang Cen <rollkingzzc@gmail.com>
> Tested-by: Victor Nogueira <victor@mojatatu.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Rajat Gupta <rajat.gupta@oss.qualcomm.com>

Re-ran the tests, and everything looks good, so:

Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>

Also looked at the code, and I have a few nits below, but I'm really
nitpicking here, so whether you end up fixing those or not:

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>


[...]

> @@ -323,7 +324,7 @@ static bool offset_valid(struct sk_buff *skb, int offset)
>  	if (offset > 0 && offset > skb->len)
>  		return false;
>  
> -	if  (offset < 0 && -offset > skb_headroom(skb))
> +	if (offset < 0 && offset < -(int)skb_headroom(skb))
>  		return false;

This change makes it really obvious that this is really just:

	if (offset < -(int)skb_headroom(skb))
  		return false;

so, well, that would be clearer, IMO.

But then I guess the same could be said of the positive case, so:

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

	return true;
}

> @@ -393,18 +394,10 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>  	struct tcf_pedit_key_ex *tkey_ex;
>  	struct tcf_pedit_parms *parms;
>  	struct tc_pedit_key *tkey;
> -	u32 max_offset;
>  	int i;
>  
>  	parms = rcu_dereference_bh(p->parms);
>  
> -	max_offset = (skb_transport_header_was_set(skb) ?
> -		      skb_transport_offset(skb) :
> -		      skb_network_offset(skb)) +
> -		     parms->tcfp_off_max_hint;
> -	if (skb_ensure_writable(skb, min(skb->len, max_offset)))
> -		goto done;
> -
>  	tcf_lastuse_update(&p->tcf_tm);
>  	tcf_action_update_bstats(&p->common, skb);
>  
> @@ -412,9 +405,10 @@ 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 offset = tkey->off;
>  		int hoffset = 0;
> -		u32 *ptr, hdata;
> +		u32 *ptr;
>  		u32 val;
>  		int rc;
>  
> @@ -451,15 +445,38 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>  			}
>  		}
>  
> -		if (!offset_valid(skb, hoffset + offset)) {
> -			pr_info_ratelimited("tc action pedit offset %d out of bounds\n", hoffset + offset);
> +		if (unlikely(check_add_overflow(hoffset, offset,

It's a bit weird that this has the unlikely(), but the offset_valid()
check doesn't?

> +						&write_offset))) {
> +			pr_info_ratelimited("tc action pedit offset overflow\n");
>  			goto bad;
>  		}
>  
> -		ptr = skb_header_pointer(skb, hoffset + offset,
> -					 sizeof(hdata), &hdata);
> -		if (!ptr)
> +		if (!offset_valid(skb, write_offset)) {
> +			pr_info_ratelimited("tc action pedit offset %d out of bounds\n",
> +					    write_offset);
>  			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(skb->len,
> +							    write_offset + sizeof(*ptr))))

Combining these with && instead of the double indentation would be more
readable IMO (shorter lines, aligning the 'goto bad' labels).

> +					goto bad;
> +			}
> +		} else {
> +			if (unlikely(check_add_overflow(write_offset,
> +							(int)sizeof(*ptr),
> +							&write_len)))

Same comment wrt unlikely()

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

[Davide Caratti]

On Tue, May 26, 2026 at 11:59:13AM -0400, Jamal Hadi Salim wrote:
> From: Rajat Gupta <rajat.gupta@oss.qualcomm.com>
> 
> 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.
> 
> Additionally, linearize skbs with shared frags upfront to prevent
> silent data corruption when pedit operates on zero-copy pages
> (e.g. from sendfile).
> 
> Fixes: 8b796475fd78 ("net/sched: act_pedit: really ensure the skb is writable")
> Reported-by: Yiming Qian <yimingqian591@gmail.com>
> Reported-by: Keenan Dong <keenanat2000@gmail.com>
> Reported-by: Han Guidong <2045gemini@gmail.com>
> Reported-by: Zhang Cen <rollkingzzc@gmail.com>
> Tested-by: Victor Nogueira <victor@mojatatu.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Rajat Gupta <rajat.gupta@oss.qualcomm.com>
> ---
> Changes v1->v2:
>  1. Do better boundary analysis to cover cloned skbs with frags. Pointed
>     out by sashiko-nipa:
>     https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260519033950.2037-1-rajat.gupta%40oss.qualcomm.com
>  2. As a result of fix #1 remove the skb_has_shared_frag() check, unnecessary.
>     Also Jakub has plans where the shared frags is not going to be a "thing"
>  3. Make small adjustments everywhere for integer checks, suggested by D. Laight
>  4. Remove all reviewers and testers since this is a large enough change.
>     Please retest and re-review.
>  5. Remove Rajat as reporter since he is the author (which implies he is a reporter)

re-ran mp_join + pedit_l4port + pedit_ip kselftests on patch v2, no issues found.

Reviewed-and-tested-by: Davide Caratti <dcaratti@redhat.com>

Thanks!
-- 
davide

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

[Han Guidong]

On Tue, May 26, 2026 at 11:59 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> From: Rajat Gupta <rajat.gupta@oss.qualcomm.com>
>
> 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.
>
> Additionally, linearize skbs with shared frags upfront to prevent
> silent data corruption when pedit operates on zero-copy pages
> (e.g. from sendfile).
>
> Fixes: 8b796475fd78 ("net/sched: act_pedit: really ensure the skb is writable")
> Reported-by: Yiming Qian <yimingqian591@gmail.com>
> Reported-by: Keenan Dong <keenanat2000@gmail.com>
> Reported-by: Han Guidong <2045gemini@gmail.com>
> Reported-by: Zhang Cen <rollkingzzc@gmail.com>
> Tested-by: Victor Nogueira <victor@mojatatu.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Rajat Gupta <rajat.gupta@oss.qualcomm.com>

Reviewed-and-tested-by: Han Guidong <2045gemini@gmail.com>

I tested this v2 both with the PoC and with the module-based corner
case we discussed earlier, and both look good to me.

Thanks for the careful and thorough rework here.

> ---
> Changes v1->v2:
>  1. Do better boundary analysis to cover cloned skbs with frags. Pointed
>     out by sashiko-nipa:
>     https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260519033950.2037-1-rajat.gupta%40oss.qualcomm.com
>  2. As a result of fix #1 remove the skb_has_shared_frag() check, unnecessary.
>     Also Jakub has plans where the shared frags is not going to be a "thing"
>  3. Make small adjustments everywhere for integer checks, suggested by D. Laight
>  4. Remove all reviewers and testers since this is a large enough change.
>     Please retest and re-review.
>  5. Remove Rajat as reporter since he is the author (which implies he is a reporter)
> ---
>  net/sched/act_pedit.c | 49 ++++++++++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index bc20f08a2789..719bee335e1f 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -16,6 +16,7 @@
>  #include <linux/ip.h>
>  #include <linux/ipv6.h>
>  #include <linux/slab.h>
> +#include <linux/overflow.h>
>  #include <net/ipv6.h>
>  #include <net/netlink.h>
>  #include <net/pkt_sched.h>
> @@ -323,7 +324,7 @@ static bool offset_valid(struct sk_buff *skb, int offset)
>         if (offset > 0 && offset > skb->len)
>                 return false;
>
> -       if  (offset < 0 && -offset > skb_headroom(skb))
> +       if (offset < 0 && offset < -(int)skb_headroom(skb))
>                 return false;
>
>         return true;
> @@ -393,18 +394,10 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>         struct tcf_pedit_key_ex *tkey_ex;
>         struct tcf_pedit_parms *parms;
>         struct tc_pedit_key *tkey;
> -       u32 max_offset;
>         int i;
>
>         parms = rcu_dereference_bh(p->parms);
>
> -       max_offset = (skb_transport_header_was_set(skb) ?
> -                     skb_transport_offset(skb) :
> -                     skb_network_offset(skb)) +
> -                    parms->tcfp_off_max_hint;
> -       if (skb_ensure_writable(skb, min(skb->len, max_offset)))
> -               goto done;
> -
>         tcf_lastuse_update(&p->tcf_tm);
>         tcf_action_update_bstats(&p->common, skb);
>
> @@ -412,9 +405,10 @@ 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 offset = tkey->off;
>                 int hoffset = 0;
> -               u32 *ptr, hdata;
> +               u32 *ptr;
>                 u32 val;
>                 int rc;
>
> @@ -451,15 +445,38 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>                         }
>                 }
>
> -               if (!offset_valid(skb, hoffset + offset)) {
> -                       pr_info_ratelimited("tc action pedit offset %d out of bounds\n", hoffset + offset);
> +               if (unlikely(check_add_overflow(hoffset, offset,
> +                                               &write_offset))) {
> +                       pr_info_ratelimited("tc action pedit offset overflow\n");
>                         goto bad;
>                 }
>
> -               ptr = skb_header_pointer(skb, hoffset + offset,
> -                                        sizeof(hdata), &hdata);
> -               if (!ptr)
> +               if (!offset_valid(skb, write_offset)) {
> +                       pr_info_ratelimited("tc action pedit offset %d out of bounds\n",
> +                                           write_offset);
>                         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(skb->len,
> +                                                           write_offset + sizeof(*ptr))))
> +                                       goto bad;
> +                       }
> +               } else {
> +                       if (unlikely(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;
> +               }
> +
> +               ptr = (u32 *)(skb->data + write_offset);
>                 /* just do it, baby */
>                 switch (cmd) {
>                 case TCA_PEDIT_KEY_EX_CMD_SET:
> @@ -474,8 +491,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;
> --
> 2.34.1
>

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

[David Laight]

On Tue, 26 May 2026 21:22:52 +0200
Toke Høiland-Jørgensen <toke@redhat.com> wrote:

> Jamal Hadi Salim <jhs@mojatatu.com> writes:
> 
> > From: Rajat Gupta <rajat.gupta@oss.qualcomm.com>
> >
> > 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.
> >
> > Additionally, linearize skbs with shared frags upfront to prevent
> > silent data corruption when pedit operates on zero-copy pages
> > (e.g. from sendfile).
> >
> > Fixes: 8b796475fd78 ("net/sched: act_pedit: really ensure the skb is writable")
> > Reported-by: Yiming Qian <yimingqian591@gmail.com>
> > Reported-by: Keenan Dong <keenanat2000@gmail.com>
> > Reported-by: Han Guidong <2045gemini@gmail.com>
> > Reported-by: Zhang Cen <rollkingzzc@gmail.com>
> > Tested-by: Victor Nogueira <victor@mojatatu.com>
> > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > Signed-off-by: Rajat Gupta <rajat.gupta@oss.qualcomm.com>  
> 
> Re-ran the tests, and everything looks good, so:
> 
> Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> Also looked at the code, and I have a few nits below, but I'm really
> nitpicking here, so whether you end up fixing those or not:
> 
> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> 
> [...]
> 
> > @@ -323,7 +324,7 @@ static bool offset_valid(struct sk_buff *skb, int offset)
> >  	if (offset > 0 && offset > skb->len)
> >  		return false;
> >  
> > -	if  (offset < 0 && -offset > skb_headroom(skb))
> > +	if (offset < 0 && offset < -(int)skb_headroom(skb))
> >  		return false;  
> 
> This change makes it really obvious that this is really just:
> 
> 	if (offset < -(int)skb_headroom(skb))
>   		return false;
> 
> so, well, that would be clearer, IMO.
> 
> But then I guess the same could be said of the positive case, so:
> 
> static bool offset_valid(struct sk_buff *skb, int offset)
> {
> 	if (offset > skb->len || offset < -(int)skb_headroom(skb))
> 		return false;
> 
> 	return true;
> }

There are all sorts of integer conversions going on.
IIRC Both skb->len and skb_headroom() are 32bit unsigned.
skb_headroom() is relatively small, skb->len can be over 64k but nowhere
near MAX_INT.
offset is signed 32bit and the code is allowing for it being -MAX_INT
(but I'm not at all sure whether that can happen without overflow being likely).

So I think the single test:
	if (offset + skb_headroom(skb) >= skb->len + skb_headroom(skb))
		return false;
is correct.
If offset is 'too negative' the LHS will be 'very large postitive' and
the test fails.

-- David

> 
> > @@ -393,18 +394,10 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
> >  	struct tcf_pedit_key_ex *tkey_ex;
> >  	struct tcf_pedit_parms *parms;
> >  	struct tc_pedit_key *tkey;
> > -	u32 max_offset;
> >  	int i;
> >  
> >  	parms = rcu_dereference_bh(p->parms);
> >  
> > -	max_offset = (skb_transport_header_was_set(skb) ?
> > -		      skb_transport_offset(skb) :
> > -		      skb_network_offset(skb)) +
> > -		     parms->tcfp_off_max_hint;
> > -	if (skb_ensure_writable(skb, min(skb->len, max_offset)))
> > -		goto done;
> > -
> >  	tcf_lastuse_update(&p->tcf_tm);
> >  	tcf_action_update_bstats(&p->common, skb);
> >  
> > @@ -412,9 +405,10 @@ 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 offset = tkey->off;
> >  		int hoffset = 0;
> > -		u32 *ptr, hdata;
> > +		u32 *ptr;
> >  		u32 val;
> >  		int rc;
> >  
> > @@ -451,15 +445,38 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
> >  			}
> >  		}
> >  
> > -		if (!offset_valid(skb, hoffset + offset)) {
> > -			pr_info_ratelimited("tc action pedit offset %d out of bounds\n", hoffset + offset);
> > +		if (unlikely(check_add_overflow(hoffset, offset,  
> 
> It's a bit weird that this has the unlikely(), but the offset_valid()
> check doesn't?
> 
> > +						&write_offset))) {
> > +			pr_info_ratelimited("tc action pedit offset overflow\n");
> >  			goto bad;
> >  		}
> >  
> > -		ptr = skb_header_pointer(skb, hoffset + offset,
> > -					 sizeof(hdata), &hdata);
> > -		if (!ptr)
> > +		if (!offset_valid(skb, write_offset)) {
> > +			pr_info_ratelimited("tc action pedit offset %d out of bounds\n",
> > +					    write_offset);
> >  			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(skb->len,
> > +							    write_offset + sizeof(*ptr))))  
> 
> Combining these with && instead of the double indentation would be more
> readable IMO (shorter lines, aligning the 'goto bad' labels).
> 
> > +					goto bad;
> > +			}
> > +		} else {
> > +			if (unlikely(check_add_overflow(write_offset,
> > +							(int)sizeof(*ptr),
> > +							&write_len)))  
> 
> Same comment wrt unlikely()
> 

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

[Toke Høiland-Jørgensen]

David Laight <david.laight.linux@gmail.com> writes:

> On Tue, 26 May 2026 21:22:52 +0200
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
>> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>> 
>> > From: Rajat Gupta <rajat.gupta@oss.qualcomm.com>
>> >
>> > 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.
>> >
>> > Additionally, linearize skbs with shared frags upfront to prevent
>> > silent data corruption when pedit operates on zero-copy pages
>> > (e.g. from sendfile).
>> >
>> > Fixes: 8b796475fd78 ("net/sched: act_pedit: really ensure the skb is writable")
>> > Reported-by: Yiming Qian <yimingqian591@gmail.com>
>> > Reported-by: Keenan Dong <keenanat2000@gmail.com>
>> > Reported-by: Han Guidong <2045gemini@gmail.com>
>> > Reported-by: Zhang Cen <rollkingzzc@gmail.com>
>> > Tested-by: Victor Nogueira <victor@mojatatu.com>
>> > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> > Signed-off-by: Rajat Gupta <rajat.gupta@oss.qualcomm.com>  
>> 
>> Re-ran the tests, and everything looks good, so:
>> 
>> Tested-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> Also looked at the code, and I have a few nits below, but I'm really
>> nitpicking here, so whether you end up fixing those or not:
>> 
>> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> 
>> [...]
>> 
>> > @@ -323,7 +324,7 @@ static bool offset_valid(struct sk_buff *skb, int offset)
>> >  	if (offset > 0 && offset > skb->len)
>> >  		return false;
>> >  
>> > -	if  (offset < 0 && -offset > skb_headroom(skb))
>> > +	if (offset < 0 && offset < -(int)skb_headroom(skb))
>> >  		return false;  
>> 
>> This change makes it really obvious that this is really just:
>> 
>> 	if (offset < -(int)skb_headroom(skb))
>>   		return false;
>> 
>> so, well, that would be clearer, IMO.
>> 
>> But then I guess the same could be said of the positive case, so:
>> 
>> static bool offset_valid(struct sk_buff *skb, int offset)
>> {
>> 	if (offset > skb->len || offset < -(int)skb_headroom(skb))
>> 		return false;
>> 
>> 	return true;
>> }
>
> There are all sorts of integer conversions going on.
> IIRC Both skb->len and skb_headroom() are 32bit unsigned.
> skb_headroom() is relatively small, skb->len can be over 64k but nowhere
> near MAX_INT.

Right, I had the implicit signed/unsigned conversions the wrong way
'round in my head. So what's missing above is a cast of skb->len to int,
i.e.:

 	if (offset > (int)skb->len || offset < -(int)skb_headroom(skb))
 		return false;

right?

> offset is signed 32bit and the code is allowing for it being -MAX_INT
> (but I'm not at all sure whether that can happen without overflow being likely).
>
> So I think the single test:
> 	if (offset + skb_headroom(skb) >= skb->len + skb_headroom(skb))
> 		return false;
> is correct.
> If offset is 'too negative' the LHS will be 'very large postitive' and
> the test fails.

Yeah, I agree. FWIW, they turn out to be the same number of
instructions, the difference being that one contains a jump and the
other doesn't. It seems clang needs an unlikely() to put that jump in
the 'false' path, though:

https://godbolt.org/z/5o5Gxqe3W

OTOH, I think the two tests are more readable; the single-test version
would need a comment explaining your "too negative" rationale. But given
the subtleties of the implicit integer conversions, maybe that's better
anyway.

And they're both half the instructions of the version currently in the
patch, so if we're code golfing (which I guess we are at this point) I
guess we should pick one of those :)

-Toke