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 the (estimated) 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: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
this almost solves issues/265 here. I'm still getting some rare
failure with MPTcpExtMPFailTx==0: sometimes the transfer completes
before we are able to use the 2nd/failing link. The relevant fix
is a purely seft-test one
---
 include/net/tc_act/tc_pedit.h |  1 +
 net/sched/act_pedit.c         | 14 ++++++++++++--
 2 files changed, 13 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 e01ef7f109f4..5ff37da2f9c3 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,16 @@ 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;
+
+		/* AT reads a single byte, we can bound the offset with UCHAR_MAX,
+		 * each key will touch 4 bytes
+		 */
+		cur = p->tcfp_keys[i].off + p->tcfp_keys[i].offmask ? UCHAR_MAX >> p->tcfp_keys[i].shift: 0;
+		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);
@@ -310,7 +320,7 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
 	struct tcf_pedit *p = to_pedit(a);
 	int i;
 
-	if (skb_unclone(skb, GFP_ATOMIC))
+	if (skb_ensure_writable(skb, min(skb->len, p->tcfp_off_max_hint)))
 		return p->tcf_action;
 
 	spin_lock(&p->tcf_lock);
-- 
2.35.1

Hi Paolo,

Paolo Abeni <pabeni@redhat.com> 于2022年4月29日周五 23:52写道：
>
> 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 the (estimated) 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: 1da177e4c3f4 ("Linux-2.6.12-rc2")

Is it better to use this Fixes tag:

Fixes: 9dacaf17a6010 ("net sched: make pedit check for clones instead")

skb_cloned() is introduced by this commit.

But I'm not sure.

Thanks,
-Geliang

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> this almost solves issues/265 here. I'm still getting some rare
> failure with MPTcpExtMPFailTx==0: sometimes the transfer completes
> before we are able to use the 2nd/failing link. The relevant fix
> is a purely seft-test one
> ---
>  include/net/tc_act/tc_pedit.h |  1 +
>  net/sched/act_pedit.c         | 14 ++++++++++++--
>  2 files changed, 13 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 e01ef7f109f4..5ff37da2f9c3 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,16 @@ 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;
> +
> +               /* AT reads a single byte, we can bound the offset with UCHAR_MAX,
> +                * each key will touch 4 bytes
> +                */
> +               cur = p->tcfp_keys[i].off + p->tcfp_keys[i].offmask ? UCHAR_MAX >> p->tcfp_keys[i].shift: 0;
> +               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);
> @@ -310,7 +320,7 @@ static int tcf_pedit_act(struct sk_buff *skb, const struct tc_action *a,
>         struct tcf_pedit *p = to_pedit(a);
>         int i;
>
> -       if (skb_unclone(skb, GFP_ATOMIC))
> +       if (skb_ensure_writable(skb, min(skb->len, p->tcfp_off_max_hint)))
>                 return p->tcf_action;
>
>         spin_lock(&p->tcf_lock);
> --
> 2.35.1
>
>

On Fri, 2022-04-29 at 17:52 +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 the (estimated) 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: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> this almost solves issues/265 here. I'm still getting some rare
> failure with MPTcpExtMPFailTx==0: sometimes the transfer completes
> before we are able to use the 2nd/failing link. The relevant fix
> is a purely seft-test one
> ---
>  include/net/tc_act/tc_pedit.h |  1 +
>  net/sched/act_pedit.c         | 14 ++++++++++++--
>  2 files changed, 13 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 e01ef7f109f4..5ff37da2f9c3 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,16 @@ 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;
> +
> +		/* AT reads a single byte, we can bound the offset with UCHAR_MAX,
> +		 * each key will touch 4 bytes
> +		 */
> +		cur = p->tcfp_keys[i].off + p->tcfp_keys[i].offmask ? UCHAR_MAX >> p->tcfp_keys[i].shift: 0;

I'm dumb: I did some cosmetic editing before submitting this one
without re-testing, and they broke the build. I'll send a v2.

/P