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

[Geliang Tang <geliangtang@gmail.com>]

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