| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y85bxLzv9wgwh+h5@corigine.com>
Date: Mon, 23 Jan 2023 11:04:52 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Paul Blakey <paulb@...dia.com>
Cc: netdev@...r.kernel.org, Saeed Mahameed <saeedm@...dia.com>,
Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Cong Wang <xiyou.wangcong@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Oz Shlomo <ozsh@...dia.com>, Jiri Pirko <jiri@...dia.com>,
Roi Dayan <roid@...dia.com>, Vlad Buslov <vladbu@...dia.com>
Subject: Re: [PATCH net-next v4 1/6] net/sched: cls_api: Support hardware
miss to tc action
On Sun, Jan 22, 2023 at 04:55:07PM +0200, Paul Blakey wrote:
> For drivers to support partial offload of a filter's action list,
> add support for action miss to specify an action instance to
> continue from in sw.
>
> CT action in particular can't be fully offloaded, as new connections
> need to be handled in software. This imposes other limitations on
> the actions that can be offloaded together with the CT action, such
> as packet modifications.
>
> Assign each action on a filter's action list a unique miss_cookie
> which drivers can then use to fill action_miss part of the tc skb
> extension. On getting back this miss_cookie, find the action
> instance with relevant cookie and continue classifying from there.
>
> Signed-off-by: Paul Blakey <paulb@...dia.com>
> Reviewed-by: Jiri Pirko <jiri@...dia.com>
...
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 4cabb32a2ad94..9ef85cf9b5328 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
...
> @@ -240,21 +243,11 @@ struct tcf_exts {
> static inline int tcf_exts_init(struct tcf_exts *exts, struct net *net,
> int action, int police)
> {
> -#ifdef CONFIG_NET_CLS_ACT
> - exts->type = 0;
> - exts->nr_actions = 0;
> - /* Note: we do not own yet a reference on net.
> - * This reference might be taken later from tcf_exts_get_net().
> - */
> - exts->net = net;
> - exts->actions = kcalloc(TCA_ACT_MAX_PRIO, sizeof(struct tc_action *),
> - GFP_KERNEL);
> - if (!exts->actions)
> - return -ENOMEM;
> +#ifdef CONFIG_NET_CLS
> + return tcf_exts_init_ex(exts, net, action, police, NULL, 0, false);
> +#else
> + return -EOPNOTSUPP;
> #endif
nit: I think it would be nicer if there was a dummy implementation
of tcf_exts_init_ex for the !CONFIG_NET_CLS case and #ifdefs
could disappear from tcf_exts_init() entirely.
Likewise, elsewhere in this patch there seems to be new #if/#ifdefs
Which may be in keeping with the style of this file. But I also
think it's a style we ought to be moving away from.
> - exts->action = action;
> - exts->police = police;
> - return 0;
> }
>
> /* Return false if the netns is being destroyed in cleanup_net(). Callers
...
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 5b4a95e8a1ee0..46524ae353c5a 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
...
> +static struct tcf_exts_miss_cookie_node *
> +tcf_exts_miss_cookie_lookup(u64 miss_cookie, int *act_index)
> +{
> + union tcf_exts_miss_cookie mc = { .miss_cookie = miss_cookie, };
> +
> + *act_index = mc.act_index;
> + return xa_load(&tcf_exts_miss_cookies_xa, mc.miss_cookie_base);
> +}
> +#endif /* IS_ENABLED(CONFIG_NET_TC_SKB_EXT) */
> +
> +static u64 tcf_exts_miss_cookie_get(u32 miss_cookie_base, int act_index)
> +{
> + union tcf_exts_miss_cookie mc = { .act_index = act_index, };
> +
> + if (!miss_cookie_base)
> + return 0;
nit: perhaps the compiler optimises this out, or it is otherwise
unimportant,
but doesn't the assignment
of mc zero it's fields, other than .act_index only for:
1. Any assignment of mc to be unused in the !miss_cookie_base case
2. mc.miss_cookie_base to be reassigned, otherwise
FWIIW, I might have gone for something more like this (*untested*).
union tcf_exts_miss_cookie mc;
if (!miss_cookie_base)
return 0;
mc.act_index = act_index;
mc.miss_cookie_base = miss_cookie_base;
return mc.miss_cookie;
> +
> + mc.miss_cookie_base = miss_cookie_base;
> + return mc.miss_cookie;
> +}
> +
> #ifdef CONFIG_NET_CLS_ACT
> DEFINE_STATIC_KEY_FALSE(tc_skb_ext_tc);
> EXPORT_SYMBOL(tc_skb_ext_tc);
> @@ -1549,6 +1642,8 @@ static inline int __tcf_classify(struct sk_buff *skb,
> const struct tcf_proto *orig_tp,
> struct tcf_result *res,
> bool compat_mode,
> + struct tcf_exts_miss_cookie_node *n,
> + int act_index,
> u32 *last_executed_chain)
> {
> #ifdef CONFIG_NET_CLS_ACT
> @@ -1560,13 +1655,40 @@ static inline int __tcf_classify(struct sk_buff *skb,
> #endif
> for (; tp; tp = rcu_dereference_bh(tp->next)) {
> __be16 protocol = skb_protocol(skb, false);
> - int err;
> + int err = 0;
>
> - if (tp->protocol != protocol &&
> - tp->protocol != htons(ETH_P_ALL))
> - continue;
> + if (n) {
> + struct tcf_exts *exts;
>
> - err = tc_classify(skb, tp, res);
> + if (n->tp_prio != tp->prio)
> + continue;
> +
> + /* We re-lookup the tp and chain based on index instead
> + * of having hard refs and locks to them, so do a sanity
> + * check if any of tp,chain,exts was replaced by the
> + * time we got here with a cookie from hardware.
> + */
> + if (unlikely(n->tp != tp || n->tp->chain != n->chain ||
> + !tp->ops->get_exts))
> + return TC_ACT_SHOT;
> +
> + exts = tp->ops->get_exts(tp, n->handle);
I see that this is probably safe, but it's not entirely obvious
that tp->ops->get_exts will never by NULL here.
1) It is invariant on n, which is set in a different function and is in turn
2) invariant on ext->act_miss being set, several patches away, in the driver.
3) Which is lastly invariant on being a code path relating to for
hardware offload of a classifier where tp->ops->get_exts is defined.
Or perhaps I mixed it up somehow. But I do think at a minimum this
ought to be documented.
Which brings me to a more central concern with this series: it's very
invasive and sets up a complex relationship between the core and the
driver. Is this the right abstraction for the problem at hand?
> + if (unlikely(!exts || n->exts != exts))
> + return TC_ACT_SHOT;
> +
> + n = NULL;
> +#ifdef CONFIG_NET_CLS_ACT
> + err = tcf_action_exec(skb, exts->actions + act_index,
> + exts->nr_actions - act_index,
> + res);
> +#endif
> + } else {
> + if (tp->protocol != protocol &&
> + tp->protocol != htons(ETH_P_ALL))
> + continue;
> +
> + err = tc_classify(skb, tp, res);
> + }
> #ifdef CONFIG_NET_CLS_ACT
> if (unlikely(err == TC_ACT_RECLASSIFY && !compat_mode)) {
> first_tp = orig_tp;
...
> @@ -1606,21 +1731,33 @@ int tcf_classify(struct sk_buff *skb,
> #if !IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
> u32 last_executed_chain = 0;
>
> - return __tcf_classify(skb, tp, tp, res, compat_mode,
> + return __tcf_classify(skb, tp, tp, res, compat_mode, NULL, 0,
> &last_executed_chain);
> #else
> u32 last_executed_chain = tp ? tp->chain->index : 0;
> + struct tcf_exts_miss_cookie_node *n = NULL;
> const struct tcf_proto *orig_tp = tp;
> struct tc_skb_ext *ext;
> + int act_index = 0;
> int ret;
>
> if (block) {
> ext = skb_ext_find(skb, TC_SKB_EXT);
>
> - if (ext && ext->chain) {
> + if (ext && (ext->chain || ext->act_miss)) {
> struct tcf_chain *fchain;
> + u32 chain = ext->chain;
nit: ext->chain seems to only be used once, as it was before this patch.
Perhaps the chain local variable is an artifact of development
and is best not added?
> +
> + if (ext->act_miss) {
> + n = tcf_exts_miss_cookie_lookup(ext->act_miss_cookie,
> + &act_index);
> + if (!n)
> + return TC_ACT_SHOT;
>
> - fchain = tcf_chain_lookup_rcu(block, ext->chain);
> + chain = n->chain_index;
> + }
> +
> + fchain = tcf_chain_lookup_rcu(block, chain);
> if (!fchain)
> return TC_ACT_SHOT;
>
...
Powered by blists - more mailing lists