| Message ID | 004a9eddf22a44b415a6573bdc67040b995c14dc.1652095998.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net/sched: act_pedit: really ensure the skb is writable | expand |
On 2022-05-09 07:33, Paolo Abeni wrote: [..] > 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; > + goto bad; would have been better so we can record it in the stats? Other than that LGTM. The commit message is good - but it would be better if you put that example that triggered it. cheers, jamal
On Mon, 2022-05-09 at 08:16 -0400, Jamal Hadi Salim wrote: > On 2022-05-09 07:33, Paolo Abeni wrote: > > [..] > > 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; > > + > > goto bad; would have been better so we can record it in the stats? I thought about that, but it looked like an unexpected behavioral change: currently skb_unclone() failures do not touch stats. > > Other than that LGTM. > The commit message is good - but it would be better if you put that > example that triggered it. Please let me know if you prefer another revision to cope with the above. Thanks! Paolo
On 2022-05-09 09:14, Paolo Abeni wrote: > On Mon, 2022-05-09 at 08:16 -0400, Jamal Hadi Salim wrote: >> On 2022-05-09 07:33, Paolo Abeni wrote: >> >> [..] >>> 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; >>> + >> >> goto bad; would have been better so we can record it in the stats? > > I thought about that, but it looked like an unexpected behavioral > change: currently skb_unclone() failures do not touch stats. Fair enough. >> Other than that LGTM. >> The commit message is good - but it would be better if you put that >> example that triggered it. > > Please let me know if you prefer another revision to cope with the > above. > Only if you have the energy to do it. Otherwise Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal
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 31fcd279c177..0eaaf1f45de1 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; }
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> --- include/net/tc_act/tc_pedit.h | 1 + net/sched/act_pedit.c | 26 ++++++++++++++++++++++---- 2 files changed, 23 insertions(+), 4 deletions(-)