[PATCH net] net/sched: act_pedit: really ensure the skb is writable

[PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Paolo Abeni]

Currently pedit tries to ensure that the accessed skb offset
is writeble via skb_unclone(). The action potentially allows
touching any skb bytes, so it may end-up modifying shared data.

The above causes some sporadic MPTCP self-test failures.

Address the issue keeping track of a rough over-estimate highest skb
offset accessed by the action and ensure such offset is really
writable.

Note that this may cause performance regressions in some scenario,
but hopefully pedit is not critical path.

Fixes: db2c24175d14 ("act_pedit: access skb->data safely")
Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Tested-by: Geliang Tang <geliang.tang@suse.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Note: AFAICS the issue is present since 1da177e4c3f4
("Linux-2.6.12-rc2"), but before the "Fixes" commit this change
is irrelevant, because accessing any data out of the skb head
will cause an oops.
---
 include/net/tc_act/tc_pedit.h |  1 +
 net/sched/act_pedit.c         | 23 +++++++++++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
index 748cf87a4d7e..3e02709a1df6 100644
--- a/include/net/tc_act/tc_pedit.h
+++ b/include/net/tc_act/tc_pedit.h
@@ -14,6 +14,7 @@ struct tcf_pedit {
 	struct tc_action	common;
 	unsigned char		tcfp_nkeys;
 	unsigned char		tcfp_flags;
+	u32			tcfp_off_max_hint;
 	struct tc_pedit_key	*tcfp_keys;
 	struct tcf_pedit_key_ex	*tcfp_keys_ex;
 };
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 31fcd279c177..a8ab6c3f1ea2 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -149,7 +149,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	struct nlattr *pattr;
 	struct tcf_pedit *p;
 	int ret = 0, err;
-	int ksize;
+	int i, ksize;
 	u32 index;
 
 	if (!nla) {
@@ -228,6 +228,20 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 		p->tcfp_nkeys = parm->nkeys;
 	}
 	memcpy(p->tcfp_keys, parm->keys, ksize);
+	p->tcfp_off_max_hint = 0;
+	for (i = 0; i < p->tcfp_nkeys; ++i) {
+		u32 cur = p->tcfp_keys[i].off;
+
+		/* The AT option can read a single byte, we can bound the actual
+		 * value with uchar max. Each key touches 4 bytes starting from
+		 * the computed offset
+		 */
+		if (p->tcfp_keys[i].offmask) {
+			cur += 255 >> p->tcfp_keys[i].shift;
+			cur = max(p->tcfp_keys[i].at, cur);
+		}
+		p->tcfp_off_max_hint = max(p->tcfp_off_max_hint, cur + 4);
+	}
 
 	p->tcfp_flags = parm->flags;
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
@@ -308,9 +322,14 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
 			 struct tcf_result *res)
 {
 	struct tcf_pedit *p = to_pedit(a);
+	u32 max_offset;
 	int i;
 
-	if (skb_unclone(skb, GFP_ATOMIC))
+	max_offset = (skb_transport_header_was_set(skb) ?
+		      skb_transport_offset(skb) :
+		      skb_network_offset(skb)) +
+		     p->tcfp_off_max_hint;
+	if (skb_ensure_writable(skb, min(skb->len, max_offset)))
 		return p->tcf_action;
 
 	spin_lock(&p->tcf_lock);
-- 
2.35.1

Re: [PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Jamal Hadi Salim]

What was the tc pedit command that triggered this?
Can we add it to tdc tests?

cheers,
jamal

On 2022-05-03 10:05, Paolo Abeni wrote:
> Currently pedit tries to ensure that the accessed skb offset
> is writeble via skb_unclone(). The action potentially allows
> touching any skb bytes, so it may end-up modifying shared data.
> 
> The above causes some sporadic MPTCP self-test failures.
> 
> Address the issue keeping track of a rough over-estimate highest skb
> offset accessed by the action and ensure such offset is really
> writable.
> 
> Note that this may cause performance regressions in some scenario,
> but hopefully pedit is not critical path.
> 
> Fixes: db2c24175d14 ("act_pedit: access skb->data safely")
> Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Tested-by: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> Note: AFAICS the issue is present since 1da177e4c3f4
> ("Linux-2.6.12-rc2"), but before the "Fixes" commit this change
> is irrelevant, because accessing any data out of the skb head
> will cause an oops.
> ---
>   include/net/tc_act/tc_pedit.h |  1 +
>   net/sched/act_pedit.c         | 23 +++++++++++++++++++++--
>   2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
> index 748cf87a4d7e..3e02709a1df6 100644
> --- a/include/net/tc_act/tc_pedit.h
> +++ b/include/net/tc_act/tc_pedit.h
> @@ -14,6 +14,7 @@ struct tcf_pedit {
>   	struct tc_action	common;
>   	unsigned char		tcfp_nkeys;
>   	unsigned char		tcfp_flags;
> +	u32			tcfp_off_max_hint;
>   	struct tc_pedit_key	*tcfp_keys;
>   	struct tcf_pedit_key_ex	*tcfp_keys_ex;
>   };
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index 31fcd279c177..a8ab6c3f1ea2 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -149,7 +149,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>   	struct nlattr *pattr;
>   	struct tcf_pedit *p;
>   	int ret = 0, err;
> -	int ksize;
> +	int i, ksize;
>   	u32 index;
>   
>   	if (!nla) {
> @@ -228,6 +228,20 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>   		p->tcfp_nkeys = parm->nkeys;
>   	}
>   	memcpy(p->tcfp_keys, parm->keys, ksize);
> +	p->tcfp_off_max_hint = 0;
> +	for (i = 0; i < p->tcfp_nkeys; ++i) {
> +		u32 cur = p->tcfp_keys[i].off;
> +
> +		/* The AT option can read a single byte, we can bound the actual
> +		 * value with uchar max. Each key touches 4 bytes starting from
> +		 * the computed offset
> +		 */
> +		if (p->tcfp_keys[i].offmask) {
> +			cur += 255 >> p->tcfp_keys[i].shift;
> +			cur = max(p->tcfp_keys[i].at, cur);
> +		}
> +		p->tcfp_off_max_hint = max(p->tcfp_off_max_hint, cur + 4);
> +	}
>   
>   	p->tcfp_flags = parm->flags;
>   	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> @@ -308,9 +322,14 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
>   			 struct tcf_result *res)
>   {
>   	struct tcf_pedit *p = to_pedit(a);
> +	u32 max_offset;
>   	int i;
>   
> -	if (skb_unclone(skb, GFP_ATOMIC))
> +	max_offset = (skb_transport_header_was_set(skb) ?
> +		      skb_transport_offset(skb) :
> +		      skb_network_offset(skb)) +
> +		     p->tcfp_off_max_hint;
> +	if (skb_ensure_writable(skb, min(skb->len, max_offset)))
>   		return p->tcf_action;
>   
>   	spin_lock(&p->tcf_lock);

Re: [PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Paolo Abeni]

On Tue, 2022-05-03 at 16:10 -0400, Jamal Hadi Salim wrote:
> What was the tc pedit command that triggered this?

From the mptcp self-tests, mptcp_join.sh:

tc -n $ns2 filter add dev ns2eth$i egress \
		protocol ip prio 1000 \
		handle 42 fw \
		action pedit munge offset 148 u8 invert \
		pipe csum tcp \
		index 100 || exit 1

It's used to corrupt a packet so that TCP csum is still correct while
the MPTCP one is not.

The relevant part is that the touched offset is outside the skb head.

> Can we add it to tdc tests?

What happens in the mptcp self-tests it that an almost simultaneous
mptcp-level reinjection on another device using the same cloned data
get unintentionally corrupted and we catch it - when it sporadically
happens - via the MPTCP mibs.

While we could add the above pedit command, but I fear that a
meaningful test for the issue addressed here not fit the tdc
infrastructure easily.

Thanks,

Paolo

Re: [PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Jakub Kicinski]

On Wed, 04 May 2022 10:52:59 +0200 Paolo Abeni wrote:
> On Tue, 2022-05-03 at 16:10 -0400, Jamal Hadi Salim wrote:
> > What was the tc pedit command that triggered this?  
> 
> From the mptcp self-tests, mptcp_join.sh:
> 
> tc -n $ns2 filter add dev ns2eth$i egress \
> 		protocol ip prio 1000 \
> 		handle 42 fw \
> 		action pedit munge offset 148 u8 invert \
> 		pipe csum tcp \
> 		index 100 || exit 1
> 
> It's used to corrupt a packet so that TCP csum is still correct while
> the MPTCP one is not.
> 
> The relevant part is that the touched offset is outside the skb head.
> 
> > Can we add it to tdc tests?  
> 
> What happens in the mptcp self-tests it that an almost simultaneous
> mptcp-level reinjection on another device using the same cloned data
> get unintentionally corrupted and we catch it - when it sporadically
> happens - via the MPTCP mibs.
> 
> While we could add the above pedit command, but I fear that a
> meaningful test for the issue addressed here not fit the tdc
> infrastructure easily.

For testing stuff like this would it be possible to inject packets
with no headers pulled and frags in pages we marked read-only?
We can teach netdevsim to do it.

Obviously not as a pre-requisite for this patch.

Re: [PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Paolo Abeni]

On Wed, 2022-05-04 at 07:47 -0700, Jakub Kicinski wrote:
> On Wed, 04 May 2022 10:52:59 +0200 Paolo Abeni wrote:
> > On Tue, 2022-05-03 at 16:10 -0400, Jamal Hadi Salim wrote:
> > > What was the tc pedit command that triggered this?  
> > 
> > From the mptcp self-tests, mptcp_join.sh:
> > 
> > tc -n $ns2 filter add dev ns2eth$i egress \
> > 		protocol ip prio 1000 \
> > 		handle 42 fw \
> > 		action pedit munge offset 148 u8 invert \
> > 		pipe csum tcp \
> > 		index 100 || exit 1
> > 
> > It's used to corrupt a packet so that TCP csum is still correct while
> > the MPTCP one is not.
> > 
> > The relevant part is that the touched offset is outside the skb head.
> > 
> > > Can we add it to tdc tests?  
> > 
> > What happens in the mptcp self-tests it that an almost simultaneous
> > mptcp-level reinjection on another device using the same cloned data
> > get unintentionally corrupted and we catch it - when it sporadically
> > happens - via the MPTCP mibs.
> > 
> > While we could add the above pedit command, but I fear that a
> > meaningful test for the issue addressed here not fit the tdc
> > infrastructure easily.
> 
> For testing stuff like this would it be possible to inject packets
> with no headers pulled and frags in pages we marked read-only?
> We can teach netdevsim to do it.

We additionally need to ensure that the crafted packets are cloned,
otherwise the current code is AFAICS fine. And at the point we likely
want to configure the packet layout (hdrs/address) created by
netdevsim. 

> Obviously not as a pre-requisite for this patch.

I agree it looks a bit out-of-scope here ;)

Paolo 

Re: [PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Jakub Kicinski]

On Wed, 04 May 2022 17:11:25 +0200 Paolo Abeni wrote:
> > For testing stuff like this would it be possible to inject packets
> > with no headers pulled and frags in pages we marked read-only?
> > We can teach netdevsim to do it.  
> 
> We additionally need to ensure that the crafted packets are cloned,
> otherwise the current code is AFAICS fine.

Depends whether skb frags are writable, or not. Or to put it differently
why does skb_cow_data() exist and does what it does.

Re: [PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Jakub Kicinski]

On Tue,  3 May 2022 16:05:42 +0200 Paolo Abeni wrote:
> Currently pedit tries to ensure that the accessed skb offset
> is writeble via skb_unclone(). The action potentially allows
> touching any skb bytes, so it may end-up modifying shared data.
> 
> The above causes some sporadic MPTCP self-test failures.
> 
> Address the issue keeping track of a rough over-estimate highest skb
> offset accessed by the action and ensure such offset is really
> writable.
> 
> Note that this may cause performance regressions in some scenario,
> but hopefully pedit is not critical path.
> 
> Fixes: db2c24175d14 ("act_pedit: access skb->data safely")
> Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Tested-by: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> Note: AFAICS the issue is present since 1da177e4c3f4
> ("Linux-2.6.12-rc2"), but before the "Fixes" commit this change
> is irrelevant, because accessing any data out of the skb head
> will cause an oops.
> ---
>  include/net/tc_act/tc_pedit.h |  1 +
>  net/sched/act_pedit.c         | 23 +++++++++++++++++++++--
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
> index 748cf87a4d7e..3e02709a1df6 100644
> --- a/include/net/tc_act/tc_pedit.h
> +++ b/include/net/tc_act/tc_pedit.h
> @@ -14,6 +14,7 @@ struct tcf_pedit {
>  	struct tc_action	common;
>  	unsigned char		tcfp_nkeys;
>  	unsigned char		tcfp_flags;
> +	u32			tcfp_off_max_hint;
>  	struct tc_pedit_key	*tcfp_keys;
>  	struct tcf_pedit_key_ex	*tcfp_keys_ex;
>  };
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index 31fcd279c177..a8ab6c3f1ea2 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -149,7 +149,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>  	struct nlattr *pattr;
>  	struct tcf_pedit *p;
>  	int ret = 0, err;
> -	int ksize;
> +	int i, ksize;
>  	u32 index;
>  
>  	if (!nla) {
> @@ -228,6 +228,20 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>  		p->tcfp_nkeys = parm->nkeys;
>  	}
>  	memcpy(p->tcfp_keys, parm->keys, ksize);
> +	p->tcfp_off_max_hint = 0;

This gets zeroed here... [1]

> +	for (i = 0; i < p->tcfp_nkeys; ++i) {
> +		u32 cur = p->tcfp_keys[i].off;
> +
> +		/* The AT option can read a single byte, we can bound the actual
> +		 * value with uchar max. Each key touches 4 bytes starting from
> +		 * the computed offset
> +		 */
> +		if (p->tcfp_keys[i].offmask) {
> +			cur += 255 >> p->tcfp_keys[i].shift;

Could be written as:

		cur += (0xff & p->tcfp_keys[i].offmask) >>
			p->tcfp_keys[i].shift;

without the if? That would be closer to the:

		offset += (*d & tkey->offmask) >> tkey->shift;

which ends up getting executed.

> +			cur = max(p->tcfp_keys[i].at, cur);

We never write under ->at, tho, so this shouldn't be needed?

> +		}
> +		p->tcfp_off_max_hint = max(p->tcfp_off_max_hint, cur + 4);
> +	}
>  
>  	p->tcfp_flags = parm->flags;
>  	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> @@ -308,9 +322,14 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
>  			 struct tcf_result *res)
>  {
>  	struct tcf_pedit *p = to_pedit(a);
> +	u32 max_offset;
>  	int i;
>  
> -	if (skb_unclone(skb, GFP_ATOMIC))
> +	max_offset = (skb_transport_header_was_set(skb) ?
> +		      skb_transport_offset(skb) :
> +		      skb_network_offset(skb)) +
> +		     p->tcfp_off_max_hint;

[1] ... and used here outside of the lock. Isn't it racy?

> +	if (skb_ensure_writable(skb, min(skb->len, max_offset)))
>  		return p->tcf_action;
>  
>  	spin_lock(&p->tcf_lock);

Re: [PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Paolo Abeni]

On Wed, 2022-05-04 at 20:04 -0700, Jakub Kicinski wrote:
> On Tue,  3 May 2022 16:05:42 +0200 Paolo Abeni wrote:
> > Currently pedit tries to ensure that the accessed skb offset
> > is writeble via skb_unclone(). The action potentially allows
> > touching any skb bytes, so it may end-up modifying shared data.
> > 
> > The above causes some sporadic MPTCP self-test failures.
> > 
> > Address the issue keeping track of a rough over-estimate highest skb
> > offset accessed by the action and ensure such offset is really
> > writable.
> > 
> > Note that this may cause performance regressions in some scenario,
> > but hopefully pedit is not critical path.
> > 
> > Fixes: db2c24175d14 ("act_pedit: access skb->data safely")
> > Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> > Tested-by: Geliang Tang <geliang.tang@suse.com>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > Note: AFAICS the issue is present since 1da177e4c3f4
> > ("Linux-2.6.12-rc2"), but before the "Fixes" commit this change
> > is irrelevant, because accessing any data out of the skb head
> > will cause an oops.
> > ---
> >  include/net/tc_act/tc_pedit.h |  1 +
> >  net/sched/act_pedit.c         | 23 +++++++++++++++++++++--
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
> > index 748cf87a4d7e..3e02709a1df6 100644
> > --- a/include/net/tc_act/tc_pedit.h
> > +++ b/include/net/tc_act/tc_pedit.h
> > @@ -14,6 +14,7 @@ struct tcf_pedit {
> >  	struct tc_action	common;
> >  	unsigned char		tcfp_nkeys;
> >  	unsigned char		tcfp_flags;
> > +	u32			tcfp_off_max_hint;
> >  	struct tc_pedit_key	*tcfp_keys;
> >  	struct tcf_pedit_key_ex	*tcfp_keys_ex;
> >  };
> > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> > index 31fcd279c177..a8ab6c3f1ea2 100644
> > --- a/net/sched/act_pedit.c
> > +++ b/net/sched/act_pedit.c
> > @@ -149,7 +149,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> >  	struct nlattr *pattr;
> >  	struct tcf_pedit *p;
> >  	int ret = 0, err;
> > -	int ksize;
> > +	int i, ksize;
> >  	u32 index;
> >  
> >  	if (!nla) {
> > @@ -228,6 +228,20 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> >  		p->tcfp_nkeys = parm->nkeys;
> >  	}
> >  	memcpy(p->tcfp_keys, parm->keys, ksize);
> > +	p->tcfp_off_max_hint = 0;
> 
> This gets zeroed here... [1]
> 
> > +	for (i = 0; i < p->tcfp_nkeys; ++i) {
> > +		u32 cur = p->tcfp_keys[i].off;
> > +
> > +		/* The AT option can read a single byte, we can bound the actual
> > +		 * value with uchar max. Each key touches 4 bytes starting from
> > +		 * the computed offset
> > +		 */
> > +		if (p->tcfp_keys[i].offmask) {
> > +			cur += 255 >> p->tcfp_keys[i].shift;
> 
> Could be written as:
> 
> 		cur += (0xff & p->tcfp_keys[i].offmask) >>
> 			p->tcfp_keys[i].shift;
> 
> without the if? That would be closer to the:
> 
> 		offset += (*d & tkey->offmask) >> tkey->shift;
> 
> which ends up getting executed.
> 
> > +			cur = max(p->tcfp_keys[i].at, cur);
> 
> We never write under ->at, tho, so this shouldn't be needed?

Every thing you mentioned looks correct, I'll take care in v2.

> 
> > +		}
> > +		p->tcfp_off_max_hint = max(p->tcfp_off_max_hint, cur + 4);
> > +	}
> >  
> >  	p->tcfp_flags = parm->flags;
> >  	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > @@ -308,9 +322,14 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
> >  			 struct tcf_result *res)
> >  {
> >  	struct tcf_pedit *p = to_pedit(a);
> > +	u32 max_offset;
> >  	int i;
> >  
> > -	if (skb_unclone(skb, GFP_ATOMIC))
> > +	max_offset = (skb_transport_header_was_set(skb) ?
> > +		      skb_transport_offset(skb) :
> > +		      skb_network_offset(skb)) +
> > +		     p->tcfp_off_max_hint;
> 
> [1] ... and used here outside of the lock. Isn't it racy?

Indeed it is. I'll fix in v2 extending the lock over here (including
the allocation, sigh).

Later, for net-next I think we could refactor the code to avoid the
lock in the data path with something alike c749cdda9089 ("net/sched:
act_skbedit: don't use spinlock in the data path")


Thanks!

Paolo

Re: [PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Paolo Abeni]

On Thu, 2022-05-05 at 13:30 -0700, Mat Martineau wrote:
> On Thu, 5 May 2022, Paolo Abeni wrote:
> 
> > Currently pedit tries to ensure that the accessed skb offset
> > is writeble via skb_unclone(). The action potentially allows
> > touching any skb bytes, so it may end-up modifying shared data.
> > 
> > The above causes some sporadic MPTCP self-test failures.
> > 
> > Address the issue keeping track of a rough over-estimate highest skb
> > offset accessed by the action and ensure such offset is really
> > writable.
> > 
> > Note that this may cause performance regressions in some scenario,
> > but hopefully pedit is not critical path.
> > 
> > v1 -> v2:
> > - cleanup hint update (Jakub)
> > - avoid raices while accessing the hint (Jakub)
> > - re-organize the comments for clarity
> > 
> > Fixes: db2c24175d14 ("act_pedit: access skb->data safely")
> > Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> > Tested-by: Geliang Tang <geliang.tang@suse.com>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > include/net/tc_act/tc_pedit.h |  1 +
> > net/sched/act_pedit.c         | 25 +++++++++++++++++++++----
> > 2 files changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
> > index 748cf87a4d7e..3e02709a1df6 100644
> > --- a/include/net/tc_act/tc_pedit.h
> > +++ b/include/net/tc_act/tc_pedit.h
> > @@ -14,6 +14,7 @@ struct tcf_pedit {
> > 	struct tc_action	common;
> > 	unsigned char		tcfp_nkeys;
> > 	unsigned char		tcfp_flags;
> > +	u32			tcfp_off_max_hint;
> > 	struct tc_pedit_key	*tcfp_keys;
> > 	struct tcf_pedit_key_ex	*tcfp_keys_ex;
> > };
> > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> > index e01ef7f109f4..0fc07532e6f6 100644
> > --- a/net/sched/act_pedit.c
> > +++ b/net/sched/act_pedit.c
> > @@ -149,7 +149,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> > 	struct nlattr *pattr;
> > 	struct tcf_pedit *p;
> > 	int ret = 0, err;
> > -	int ksize;
> > +	int i, ksize;
> > 	u32 index;
> > 
> > 	if (!nla) {
> > @@ -228,6 +228,18 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> > 		p->tcfp_nkeys = parm->nkeys;
> > 	}
> > 	memcpy(p->tcfp_keys, parm->keys, ksize);
> > +	p->tcfp_off_max_hint = 0;
> > +	for (i = 0; i < p->tcfp_nkeys; ++i) {
> > +		u32 cur = p->tcfp_keys[i].off;
> > +
> > +		/* The AT option can read a single byte, we can bound the actual
> > +		 * value with uchar max.
> > +		 */
> > +		cur += (0xff & p->tcfp_keys[i].offmask) >> p->tcfp_keys[i].shift;
> > +
> > +		/* Each key touches 4 bytes starting from the computed offset */
> > +		p->tcfp_off_max_hint = max(p->tcfp_off_max_hint, cur + 4);
> > +	}
> > 
> > 	p->tcfp_flags = parm->flags;
> > 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > @@ -308,13 +320,18 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
> > 			 struct tcf_result *res)
> > {
> > 	struct tcf_pedit *p = to_pedit(a);
> > +	u32 max_offset;
> > 	int i;
> > 
> > -	if (skb_unclone(skb, GFP_ATOMIC))
> > -		return p->tcf_action;
> > -
> > 	spin_lock(&p->tcf_lock);
> > 
> > +	max_offset = (skb_transport_header_was_set(skb) ?
> > +		      skb_transport_offset(skb) :
> > +		      skb_network_offset(skb)) +
> > +		     p->tcfp_off_max_hint;
> > +	if (skb_ensure_writable(skb, min(skb->len, max_offset)))
> > +		return p->tcf_action;
> 
> Now that this hunk of code happens after the spinlock is acquired, that 
> lock needs to be released before returning.

Oh my! I really should take more care. Thank you for catching it!

I'll send a v3.

/P

Re: [PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Mat Martineau]

On Thu, 5 May 2022, Paolo Abeni wrote:

> Currently pedit tries to ensure that the accessed skb offset
> is writeble via skb_unclone(). The action potentially allows
> touching any skb bytes, so it may end-up modifying shared data.
>
> The above causes some sporadic MPTCP self-test failures.
>
> Address the issue keeping track of a rough over-estimate highest skb
> offset accessed by the action and ensure such offset is really
> writable.
>
> Note that this may cause performance regressions in some scenario,
> but hopefully pedit is not critical path.
>
> v1 -> v2:
> - cleanup hint update (Jakub)
> - avoid raices while accessing the hint (Jakub)
> - re-organize the comments for clarity
>
> Fixes: db2c24175d14 ("act_pedit: access skb->data safely")
> Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Tested-by: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> include/net/tc_act/tc_pedit.h |  1 +
> net/sched/act_pedit.c         | 25 +++++++++++++++++++++----
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
> index 748cf87a4d7e..3e02709a1df6 100644
> --- a/include/net/tc_act/tc_pedit.h
> +++ b/include/net/tc_act/tc_pedit.h
> @@ -14,6 +14,7 @@ struct tcf_pedit {
> 	struct tc_action	common;
> 	unsigned char		tcfp_nkeys;
> 	unsigned char		tcfp_flags;
> +	u32			tcfp_off_max_hint;
> 	struct tc_pedit_key	*tcfp_keys;
> 	struct tcf_pedit_key_ex	*tcfp_keys_ex;
> };
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index e01ef7f109f4..0fc07532e6f6 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -149,7 +149,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> 	struct nlattr *pattr;
> 	struct tcf_pedit *p;
> 	int ret = 0, err;
> -	int ksize;
> +	int i, ksize;
> 	u32 index;
>
> 	if (!nla) {
> @@ -228,6 +228,18 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> 		p->tcfp_nkeys = parm->nkeys;
> 	}
> 	memcpy(p->tcfp_keys, parm->keys, ksize);
> +	p->tcfp_off_max_hint = 0;
> +	for (i = 0; i < p->tcfp_nkeys; ++i) {
> +		u32 cur = p->tcfp_keys[i].off;
> +
> +		/* The AT option can read a single byte, we can bound the actual
> +		 * value with uchar max.
> +		 */
> +		cur += (0xff & p->tcfp_keys[i].offmask) >> p->tcfp_keys[i].shift;
> +
> +		/* Each key touches 4 bytes starting from the computed offset */
> +		p->tcfp_off_max_hint = max(p->tcfp_off_max_hint, cur + 4);
> +	}
>
> 	p->tcfp_flags = parm->flags;
> 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> @@ -308,13 +320,18 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
> 			 struct tcf_result *res)
> {
> 	struct tcf_pedit *p = to_pedit(a);
> +	u32 max_offset;
> 	int i;
>
> -	if (skb_unclone(skb, GFP_ATOMIC))
> -		return p->tcf_action;
> -
> 	spin_lock(&p->tcf_lock);
>
> +	max_offset = (skb_transport_header_was_set(skb) ?
> +		      skb_transport_offset(skb) :
> +		      skb_network_offset(skb)) +
> +		     p->tcfp_off_max_hint;
> +	if (skb_ensure_writable(skb, min(skb->len, max_offset)))
> +		return p->tcf_action;

Now that this hunk of code happens after the spinlock is acquired, that 
lock needs to be released before returning.

> +
> 	tcf_lastuse_update(&p->tcf_tm);
>
> 	if (p->tcfp_nkeys > 0) {
> -- 
> 2.35.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH net] net/sched: act_pedit: really ensure the skb is writable

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 11635 bytes --]

CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <dfcfe03e01c364599e92715828b24713d955e7c8.1651764848.git.pabeni@redhat.com>
References: <dfcfe03e01c364599e92715828b24713d955e7c8.1651764848.git.pabeni@redhat.com>
TO: Paolo Abeni <pabeni@redhat.com>

Hi Paolo,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Paolo-Abeni/net-sched-act_pedit-really-ensure-the-skb-is-writable/20220505-235513
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 4071bf121d59944d5cd2238de0642f3d7995a997
:::::: branch date: 2 hours ago
:::::: commit date: 2 hours ago
config: i386-randconfig-c001 (https://download.01.org/0day-ci/archive/20220506/202205060152.sIspNBBq-lkp(a)intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>


cocci warnings: (new ones prefixed by >>)
>> net/sched/act_pedit.c:333:2-8: preceding lock on line 326

vim +333 net/sched/act_pedit.c

71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  318  
6a2b401cd17d41 net/sched/act_pedit.c Jamal Hadi Salim  2018-08-12  319  static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
e9ce1cd3cf6cf3 net/sched/act_pedit.c David S. Miller   2006-08-21  320  			 struct tcf_result *res)
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  321  {
a85a970af265f1 net/sched/act_pedit.c WANG Cong         2016-07-25  322  	struct tcf_pedit *p = to_pedit(a);
5453f7744a582e net/sched/act_pedit.c Paolo Abeni       2022-05-05  323  	u32 max_offset;
4749c3ef854e3a net/sched/act_pedit.c Florian Westphal  2015-04-30  324  	int i;
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  325  
e9ce1cd3cf6cf3 net/sched/act_pedit.c David S. Miller   2006-08-21 @326  	spin_lock(&p->tcf_lock);
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  327  
5453f7744a582e net/sched/act_pedit.c Paolo Abeni       2022-05-05  328  	max_offset = (skb_transport_header_was_set(skb) ?
5453f7744a582e net/sched/act_pedit.c Paolo Abeni       2022-05-05  329  		      skb_transport_offset(skb) :
5453f7744a582e net/sched/act_pedit.c Paolo Abeni       2022-05-05  330  		      skb_network_offset(skb)) +
5453f7744a582e net/sched/act_pedit.c Paolo Abeni       2022-05-05  331  		     p->tcfp_off_max_hint;
5453f7744a582e net/sched/act_pedit.c Paolo Abeni       2022-05-05  332  	if (skb_ensure_writable(skb, min(skb->len, max_offset)))
5453f7744a582e net/sched/act_pedit.c Paolo Abeni       2022-05-05 @333  		return p->tcf_action;
5453f7744a582e net/sched/act_pedit.c Paolo Abeni       2022-05-05  334  
9c4a4e488bc8f5 net/sched/act_pedit.c Jamal Hadi Salim  2016-06-06  335  	tcf_lastuse_update(&p->tcf_tm);
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  336  
e9ce1cd3cf6cf3 net/sched/act_pedit.c David S. Miller   2006-08-21  337  	if (p->tcfp_nkeys > 0) {
e9ce1cd3cf6cf3 net/sched/act_pedit.c David S. Miller   2006-08-21  338  		struct tc_pedit_key *tkey = p->tcfp_keys;
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  339  		struct tcf_pedit_key_ex *tkey_ex = p->tcfp_keys_ex;
80f0f574cc615b net/sched/act_pedit.c Roman Mashak      2018-06-27  340  		enum pedit_header_type htype =
80f0f574cc615b net/sched/act_pedit.c Roman Mashak      2018-06-27  341  			TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK;
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  342  		enum pedit_cmd cmd = TCA_PEDIT_KEY_EX_CMD_SET;
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  343  
e9ce1cd3cf6cf3 net/sched/act_pedit.c David S. Miller   2006-08-21  344  		for (i = p->tcfp_nkeys; i > 0; i--, tkey++) {
544377cd2545f3 net/sched/act_pedit.c Roman Mashak      2018-06-27  345  			u32 *ptr, hdata;
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  346  			int offset = tkey->off;
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  347  			int hoffset;
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  348  			u32 val;
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  349  			int rc;
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  350  
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  351  			if (tkey_ex) {
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  352  				htype = tkey_ex->htype;
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  353  				cmd = tkey_ex->cmd;
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  354  
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  355  				tkey_ex++;
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  356  			}
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  357  
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  358  			rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  359  			if (rc) {
95b0d2dc13c7e7 net/sched/act_pedit.c Roman Mashak      2018-06-27  360  				pr_info("tc action pedit bad header type specified (0x%x)\n",
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  361  					htype);
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  362  				goto bad;
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  363  			}
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  364  
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  365  			if (tkey->offmask) {
430527415398cf net/sched/act_pedit.c Roman Mashak      2018-06-27  366  				u8 *d, _d;
db2c24175d149b net/sched/act_pedit.c Changli Gao       2010-06-02  367  
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  368  				if (!offset_valid(skb, hoffset + tkey->at)) {
95b0d2dc13c7e7 net/sched/act_pedit.c Roman Mashak      2018-06-27  369  					pr_info("tc action pedit 'at' offset %d out of bounds\n",
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  370  						hoffset + tkey->at);
95c2027bfeda21 net/sched/act_pedit.c Amir Vadai        2016-11-28  371  					goto bad;
95c2027bfeda21 net/sched/act_pedit.c Amir Vadai        2016-11-28  372  				}
80f0f574cc615b net/sched/act_pedit.c Roman Mashak      2018-06-27  373  				d = skb_header_pointer(skb, hoffset + tkey->at,
6ff7586e382cb4 net/sched/act_pedit.c Roman Mashak      2018-06-27  374  						       sizeof(_d), &_d);
db2c24175d149b net/sched/act_pedit.c Changli Gao       2010-06-02  375  				if (!d)
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  376  					goto bad;
db2c24175d149b net/sched/act_pedit.c Changli Gao       2010-06-02  377  				offset += (*d & tkey->offmask) >> tkey->shift;
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  378  			}
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  379  
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  380  			if (offset % 4) {
95b0d2dc13c7e7 net/sched/act_pedit.c Roman Mashak      2018-06-27  381  				pr_info("tc action pedit offset must be on 32 bit boundaries\n");
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  382  				goto bad;
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  383  			}
95c2027bfeda21 net/sched/act_pedit.c Amir Vadai        2016-11-28  384  
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  385  			if (!offset_valid(skb, hoffset + offset)) {
95b0d2dc13c7e7 net/sched/act_pedit.c Roman Mashak      2018-06-27  386  				pr_info("tc action pedit offset %d out of bounds\n",
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  387  					hoffset + offset);
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  388  				goto bad;
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  389  			}
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  390  
80f0f574cc615b net/sched/act_pedit.c Roman Mashak      2018-06-27  391  			ptr = skb_header_pointer(skb, hoffset + offset,
6ff7586e382cb4 net/sched/act_pedit.c Roman Mashak      2018-06-27  392  						 sizeof(hdata), &hdata);
db2c24175d149b net/sched/act_pedit.c Changli Gao       2010-06-02  393  			if (!ptr)
db2c24175d149b net/sched/act_pedit.c Changli Gao       2010-06-02  394  				goto bad;
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  395  			/* just do it, baby */
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  396  			switch (cmd) {
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  397  			case TCA_PEDIT_KEY_EX_CMD_SET:
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  398  				val = tkey->val;
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  399  				break;
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  400  			case TCA_PEDIT_KEY_EX_CMD_ADD:
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  401  				val = (*ptr + tkey->val) & ~tkey->mask;
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  402  				break;
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  403  			default:
95b0d2dc13c7e7 net/sched/act_pedit.c Roman Mashak      2018-06-27  404  				pr_info("tc action pedit bad command (%d)\n",
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  405  					cmd);
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  406  				goto bad;
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  407  			}
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  408  
853a14ba4682f8 net/sched/act_pedit.c Amir Vadai        2017-02-07  409  			*ptr = ((*ptr & tkey->mask) ^ val);
544377cd2545f3 net/sched/act_pedit.c Roman Mashak      2018-06-27  410  			if (ptr == &hdata)
71d0ed7079dffb net/sched/act_pedit.c Amir Vadai        2017-02-07  411  				skb_store_bits(skb, hoffset + offset, ptr, 4);
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  412  		}
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  413  
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  414  		goto done;
80f0f574cc615b net/sched/act_pedit.c Roman Mashak      2018-06-27  415  	} else {
6ff9c3644e72bf net/sched/act_pedit.c stephen hemminger 2010-05-12  416  		WARN(1, "pedit BUG: index %d\n", p->tcf_index);
80f0f574cc615b net/sched/act_pedit.c Roman Mashak      2018-06-27  417  	}
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  418  
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  419  bad:
e9ce1cd3cf6cf3 net/sched/act_pedit.c David S. Miller   2006-08-21  420  	p->tcf_qstats.overlimits++;
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  421  done:
bfe0d0298f2a67 net/sched/act_pedit.c Eric Dumazet      2011-01-09  422  	bstats_update(&p->tcf_bstats, skb);
e9ce1cd3cf6cf3 net/sched/act_pedit.c David S. Miller   2006-08-21  423  	spin_unlock(&p->tcf_lock);
e9ce1cd3cf6cf3 net/sched/act_pedit.c David S. Miller   2006-08-21  424  	return p->tcf_action;
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  425  }
^1da177e4c3f41 net/sched/pedit.c     Linus Torvalds    2005-04-16  426  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH net] net/sched: act_pedit: really ensure the skb is writable

[Paolo Abeni]

Currently pedit tries to ensure that the accessed skb offset
is writeble via skb_unclone(). The action potentially allows
touching any skb bytes, so it may end-up modifying shared data.

The above causes some sporadic MPTCP self-test failures.

Address the issue keeping track of a rough over-estimate highest skb
offset accessed by the action and ensure such offset is really
writable.

Note that this may cause performance regressions in some scenario,
but hopefully pedit is not critical path.

v1 -> v2:
 - cleanup hint update (Jakub)
 - avoid raices while accessing the hint (Jakub)
 - re-organize the comments for clarity

Fixes: db2c24175d14 ("act_pedit: access skb->data safely")
Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Tested-by: Geliang Tang <geliang.tang@suse.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/tc_act/tc_pedit.h |  1 +
 net/sched/act_pedit.c         | 25 +++++++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/net/tc_act/tc_pedit.h b/include/net/tc_act/tc_pedit.h
index 748cf87a4d7e..3e02709a1df6 100644
--- a/include/net/tc_act/tc_pedit.h
+++ b/include/net/tc_act/tc_pedit.h
@@ -14,6 +14,7 @@ struct tcf_pedit {
 	struct tc_action	common;
 	unsigned char		tcfp_nkeys;
 	unsigned char		tcfp_flags;
+	u32			tcfp_off_max_hint;
 	struct tc_pedit_key	*tcfp_keys;
 	struct tcf_pedit_key_ex	*tcfp_keys_ex;
 };
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index e01ef7f109f4..0fc07532e6f6 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -149,7 +149,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	struct nlattr *pattr;
 	struct tcf_pedit *p;
 	int ret = 0, err;
-	int ksize;
+	int i, ksize;
 	u32 index;
 
 	if (!nla) {
@@ -228,6 +228,18 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 		p->tcfp_nkeys = parm->nkeys;
 	}
 	memcpy(p->tcfp_keys, parm->keys, ksize);
+	p->tcfp_off_max_hint = 0;
+	for (i = 0; i < p->tcfp_nkeys; ++i) {
+		u32 cur = p->tcfp_keys[i].off;
+
+		/* The AT option can read a single byte, we can bound the actual
+		 * value with uchar max.
+		 */
+		cur += (0xff & p->tcfp_keys[i].offmask) >> p->tcfp_keys[i].shift;
+
+		/* Each key touches 4 bytes starting from the computed offset */
+		p->tcfp_off_max_hint = max(p->tcfp_off_max_hint, cur + 4);
+	}
 
 	p->tcfp_flags = parm->flags;
 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
@@ -308,13 +320,18 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
 			 struct tcf_result *res)
 {
 	struct tcf_pedit *p = to_pedit(a);
+	u32 max_offset;
 	int i;
 
-	if (skb_unclone(skb, GFP_ATOMIC))
-		return p->tcf_action;
-
 	spin_lock(&p->tcf_lock);
 
+	max_offset = (skb_transport_header_was_set(skb) ?
+		      skb_transport_offset(skb) :
+		      skb_network_offset(skb)) +
+		     p->tcfp_off_max_hint;
+	if (skb_ensure_writable(skb, min(skb->len, max_offset)))
+		return p->tcf_action;
+
 	tcf_lastuse_update(&p->tcf_tm);
 
 	if (p->tcfp_nkeys > 0) {
-- 
2.35.1