| 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
| ||
|
Date: Tue, 31 Jan 2023 13:37:03 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Pedro Tammela <pctammela@...atatu.com>
Cc: netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next v4 1/2] net/sched: transition act_pedit to rcu
and percpu stats
On Mon, Jan 30, 2023 at 01:02:32PM -0300, Pedro Tammela wrote:
> The software pedit action didn't get the same love as some of the
> other actions and it's still using spinlocks and shared stats in the
> datapath.
> Transition the action to rcu and percpu stats as this improves the
> action's performance on multiple cpu deployments.
>
> Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> ---
> include/net/tc_act/tc_pedit.h | 81 +++++++++++++++----
> net/sched/act_pedit.c | 144 +++++++++++++++++++++-------------
> 2 files changed, 154 insertions(+), 71 deletions(-)
Hi Pedro,
thanks for the update.
A few questions from my side below.
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index a0378e9f0121..c8e8748dc258 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -134,6 +134,17 @@ static int tcf_pedit_key_ex_dump(struct sk_buff *skb,
> return -EINVAL;
> }
>
> +static void tcf_pedit_cleanup_rcu(struct rcu_head *head)
> +{
> + struct tcf_pedit_parms *parms =
> + container_of(head, struct tcf_pedit_parms, rcu);
> +
> + kfree(parms->tcfp_keys_ex);
> + kfree(parms->tcfp_keys);
> +
> + kfree(parms);
> +}
> +
> static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> struct nlattr *est, struct tc_action **a,
> struct tcf_proto *tp, u32 flags,
> @@ -143,8 +154,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> bool bind = flags & TCA_ACT_FLAGS_BIND;
> struct nlattr *tb[TCA_PEDIT_MAX + 1];
> struct tcf_chain *goto_ch = NULL;
> - struct tc_pedit_key *keys = NULL;
> - struct tcf_pedit_key_ex *keys_ex;
> + struct tcf_pedit_parms *oparms, *nparms;
> struct tc_pedit *parm;
> struct nlattr *pattr;
> struct tcf_pedit *p;
> @@ -181,18 +191,25 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> return -EINVAL;
> }
>
> - keys_ex = tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys);
> - if (IS_ERR(keys_ex))
> - return PTR_ERR(keys_ex);
> + nparms = kzalloc(sizeof(*nparms), GFP_KERNEL);
> + if (!nparms)
> + return -ENOMEM;
> +
> + nparms->tcfp_keys_ex =
> + tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys);
> + if (IS_ERR(nparms->tcfp_keys_ex)) {
> + ret = PTR_ERR(nparms->tcfp_keys_ex);
> + goto out_free;
> + }
>
> index = parm->index;
> err = tcf_idr_check_alloc(tn, &index, a, bind);
> if (!err) {
> - ret = tcf_idr_create(tn, index, est, a,
> - &act_pedit_ops, bind, false, flags);
> + ret = tcf_idr_create_from_flags(tn, index, est, a,
> + &act_pedit_ops, bind, flags);
> if (ret) {
> tcf_idr_cleanup(tn, index);
> - goto out_free;
> + goto out_free_ex;
> }
> ret = ACT_P_CREATED;
> } else if (err > 0) {
> @@ -204,7 +221,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> }
> } else {
> ret = err;
> - goto out_free;
> + goto out_free_ex;
> }
>
> err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
> @@ -212,68 +229,79 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> ret = err;
> goto out_release;
> }
> +
> + nparms->tcfp_off_max_hint = 0;
> + nparms->tcfp_flags = parm->flags;
> +
> p = to_pedit(*a);
> spin_lock_bh(&p->tcf_lock);
>
> + oparms = rcu_dereference_protected(p->parms, 1);
> +
> if (ret == ACT_P_CREATED ||
> - (p->tcfp_nkeys && p->tcfp_nkeys != parm->nkeys)) {
> - keys = kmalloc(ksize, GFP_ATOMIC);
> - if (!keys) {
> + (oparms->tcfp_nkeys && oparms->tcfp_nkeys != parm->nkeys)) {
> + nparms->tcfp_keys = kmalloc(ksize, GFP_ATOMIC);
> + if (!nparms->tcfp_keys) {
> spin_unlock_bh(&p->tcf_lock);
> ret = -ENOMEM;
> - goto put_chain;
> + goto out_release;
I'm a little unclear on why put_chain is no longer needed.
It seems to me that there can be a reference to goto_ch held here,
as was the case before this patch.
> }
> - kfree(p->tcfp_keys);
> - p->tcfp_keys = keys;
> - p->tcfp_nkeys = parm->nkeys;
> + nparms->tcfp_nkeys = parm->nkeys;
> + } else {
> + nparms->tcfp_keys = oparms->tcfp_keys;
I feel that I am missing something obvious:
* Here oparms->tcfp_keys is assigned to nparms->tcfp_keys.
* Later on there is a call to call_rcu(..., tcf_pedit_cleanup_rcu),
which will free oparms->tcfp_keys some time in the future.
* But the memory bay still be accessed via tcfp_keys.
Is there a life cycle issue here?
> + nparms->tcfp_nkeys = oparms->tcfp_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;
> +
> + memcpy(nparms->tcfp_keys, parm->keys, ksize);
> +
> + for (i = 0; i < nparms->tcfp_nkeys; ++i) {
> + u32 cur = nparms->tcfp_keys[i].off;
>
> /* sanitize the shift value for any later use */
> - p->tcfp_keys[i].shift = min_t(size_t, BITS_PER_TYPE(int) - 1,
> - p->tcfp_keys[i].shift);
> + nparms->tcfp_keys[i].shift = min_t(size_t,
> + BITS_PER_TYPE(int) - 1,
> + nparms->tcfp_keys[i].shift);
>
> /* 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;
> + cur += (0xff & nparms->tcfp_keys[i].offmask) >> nparms->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);
> + nparms->tcfp_off_max_hint =
> + max(nparms->tcfp_off_max_hint, cur + 4);
> }
>
> - p->tcfp_flags = parm->flags;
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>
> - kfree(p->tcfp_keys_ex);
> - p->tcfp_keys_ex = keys_ex;
> + rcu_assign_pointer(p->parms, nparms);
>
> spin_unlock_bh(&p->tcf_lock);
> +
> + if (oparms)
> + call_rcu(&oparms->rcu, tcf_pedit_cleanup_rcu);
Here there is a condition on oparms being non-NULL.
But further above oparms is dereference unconditionally.
Is there an inconsistency here?
> +
> if (goto_ch)
> tcf_chain_put_by_act(goto_ch);
> +
> return ret;
>
> -put_chain:
> - if (goto_ch)
> - tcf_chain_put_by_act(goto_ch);
> out_release:
> tcf_idr_release(*a, bind);
> +out_free_ex:
> + kfree(nparms->tcfp_keys_ex);
> out_free:
> - kfree(keys_ex);
> + kfree(nparms);
> return ret;
> -
> }
...
Powered by blists - more mailing lists