| 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: <bf8f3ae0-02e7-dae3-2b93-c7088db1a424@mojatatu.com>
Date: Tue, 31 Jan 2023 11:20:02 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: Simon Horman <simon.horman@...igine.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 31/01/2023 09:37, Simon Horman wrote:
[...]
>> 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.
Correct, initially I thought it was assigned unconditionally after the
for loop. I will restore it, thanks!
>
>> }
>> - 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?
Correct, this is wrong.
I got the wrong impression we could avoid the memory allocation in the
update case.
>
>> + 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?
oparms is NULL when we create the action instance.
I believe this will be way clearer in the next version.
>
>> +
>> 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