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

Paolo Abeni posted 1 patch 1 week, 3 days ago
Failed in applying to current master (apply log)
include/net/tc_act/tc_pedit.h |  1 +
net/sched/act_pedit.c         | 26 ++++++++++++++++++++++----
2 files changed, 23 insertions(+), 4 deletions(-)
[PATCH net v3] net/sched: act_pedit: really ensure the skb is writable
Posted by Paolo Abeni 1 week, 3 days ago
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>
---
v2 -> v3:
 - release lock on error (Mat)

note: part of the changelog is here, because -net will see only v2
(hopefully)
---
 include/net/tc_act/tc_pedit.h |  1 +
 net/sched/act_pedit.c         | 26 ++++++++++++++++++++++----
 2 files changed, 23 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..d1221daa0952 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)))
+		goto unlock;
+
 	tcf_lastuse_update(&p->tcf_tm);
 
 	if (p->tcfp_nkeys > 0) {
@@ -403,6 +420,7 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
 	p->tcf_qstats.overlimits++;
 done:
 	bstats_update(&p->tcf_bstats, skb);
+unlock:
 	spin_unlock(&p->tcf_lock);
 	return p->tcf_action;
 }
-- 
2.35.1


Re: [PATCH net v3] net/sched: act_pedit: really ensure the skb is writable
Posted by Matthieu Baerts 1 week, 2 days ago
Hi Paolo, Mat,

On 06/05/2022 11:21, 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.

Thank you for the patch and the review!

Now in our tree (replacing the v1):

New patches for t/upstream and t/upstream-net:
- 45e0f5044b92: net/sched: act_pedit: really ensure the skb is writable
- 3b7454ae1d97: tg:msg: switch to v3
- Results: 825394fc0bc5..bc6528b2b1d7 (export)
- Results: ca8b19c6d86b..90e1181966d4 (export-net)

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220506T173804
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20220506T173804
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export-net

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH net v3] net/sched: act_pedit: really ensure the skb is writable
Posted by Mat Martineau 1 week, 2 days ago
On Fri, 6 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>
> ---
> v2 -> v3:
> - release lock on error (Mat)

LGTM. My Acked-by tag is re-affirmed :)

- Mat

>
> note: part of the changelog is here, because -net will see only v2
> (hopefully)
> ---
> include/net/tc_act/tc_pedit.h |  1 +
> net/sched/act_pedit.c         | 26 ++++++++++++++++++++++----
> 2 files changed, 23 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..d1221daa0952 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)))
> +		goto unlock;
> +
> 	tcf_lastuse_update(&p->tcf_tm);
>
> 	if (p->tcfp_nkeys > 0) {
> @@ -403,6 +420,7 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
> 	p->tcf_qstats.overlimits++;
> done:
> 	bstats_update(&p->tcf_bstats, skb);
> +unlock:
> 	spin_unlock(&p->tcf_lock);
> 	return p->tcf_action;
> }
> -- 
> 2.35.1
>
>
>

--
Mat Martineau
Intel