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

[PATCH mptcp-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 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

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

[Paolo Abeni]

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

Re: net/sched: act_pedit: really ensure the skb is writable: Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: Script error! ❓:
  - :
  - Task: https://cirrus-ci.com/task/5879453103423488
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5879453103423488/summary/summary.txt

- KVM Validation: Script error! ❓:
  - :
  - Task: https://cirrus-ci.com/task/5316503150002176
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5316503150002176/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/935cecef44e7


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

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

[Geliang Tang]

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
>
>