From: Mat Martineau <mathew.j.martineau@linux.intel.com>
To: Paolo Abeni <pabeni@redhat.com>
Cc: mptcp@lists.linux.dev
Subject: Re: [PATCH mptcp-net v2] net/sched: act_pedit: really ensure the skb is writable
Date: Fri, 29 Apr 2022 13:56:01 -0700 (PDT) [thread overview]
Message-ID: <7c93496c-5153-5b83-48a9-6dd75da42542@linux.intel.com> (raw)
In-Reply-To: <26445210b10b18b39129c4ede9d7fde0e37fe21f.1651253087.git.pabeni@redhat.com>
On Fri, 29 Apr 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.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
> - fix build issue
> - account for the skb hdr offset, too
>
> 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
>
> Note that a much simpler alternatives would be simply replacing
> skb_unshare() with skb_ensure_writable(skb, skb->len), but that
> really could causes more visible regressions
To make sure I'm understanding correctly: skb_ensure_writable(skb,
skb->len) would copy the entire packet payload on every edited
packet, but this patch will only copy the part that might be modified (and
maybe a little extra). Seems like the full copy is worth avoiding, and
that users shouldn't be depending on pedit modifying shared data.
I did run the associated test for a while (with the other patches for
#265) and the changes look good from a MPTCP perspective:
Acked-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Do you plan to upstream this one yourself, or should I include it with the
other mptcp-net patches?
Thanks,
Mat
> ---
> 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 e01ef7f109f4..301ad7f19da9 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
>
>
>
--
Mat Martineau
Intel
next prev parent reply other threads:[~2022-04-29 20:56 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-29 17:29 [PATCH mptcp-net v2] net/sched: act_pedit: really ensure the skb is writable Paolo Abeni
2022-04-29 20:48 ` net/sched: act_pedit: really ensure the skb is writable: Tests Results MPTCP CI
2022-04-29 20:56 ` Mat Martineau [this message]
2022-05-02 7:55 ` [PATCH mptcp-net v2] net/sched: act_pedit: really ensure the skb is writable Paolo Abeni
2022-05-02 15:52 ` Geliang Tang
2022-05-02 16:58 ` Paolo Abeni
2022-05-02 20:31 ` Matthieu Baerts
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7c93496c-5153-5b83-48a9-6dd75da42542@linux.intel.com \
--to=mathew.j.martineau@linux.intel.com \
--cc=mptcp@lists.linux.dev \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.