netdev - Re: [PATCH net-next 13/14] net: core: add new/replace rate estimator lock parameter

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 08 Aug 2018 15:30:41 +0300
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jhs@...atatu.com,
        xiyou.wangcong@...il.com, jiri@...nulli.us, pablo@...filter.org,
        kadlec@...ckhole.kfki.hu, fw@...len.de, ast@...nel.org,
        daniel@...earbox.net, edumazet@...gle.com, keescook@...omium.org
Subject: Re: [PATCH net-next 13/14] net: core: add new/replace rate estimator lock parameter

On Wed 08 Aug 2018 at 01:37, Marcelo Ricardo Leitner <marcelo.leitner@...il.com> wrote:
> On Mon, Aug 06, 2018 at 09:54:24AM +0300, Vlad Buslov wrote:
>> Extend rate estimator 'new' and 'replace' APIs with additional spinlock
>> parameter to be used by rtnl-unlocked actions to protect rate_est pointer
>> from concurrent modification.
>
> I'm wondering if this additional parameter is really needed. So far,
> the only case in which it is not NULL, the same lock that is used to
> protect the stats is also used in this new parameter.
>
> ...
>
>> --- a/net/sched/act_police.c
>> +++ b/net/sched/act_police.c
>> @@ -138,7 +138,7 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
>>  
>>  	if (est) {
>>  		err = gen_replace_estimator(&police->tcf_bstats, NULL,
>> -					    &police->tcf_rate_est,
>> +					    &police->tcf_rate_est, NULL,
>>  					    &police->tcf_lock,
>>  					    NULL, est);
>
> Which is here, and this new NULL arg is replaced by &police->tcf_lock
> in the next patch.
>
> Do you foresee a case in which a different lock will be used?

Not in my use-case, no.

> Or maybe it is because the current one is explicitly aimed towards the
> stats?

Yes, stats lock is only taken when fetching counters. You think better
approach would be to rely on the fact that, in case of police action,
same lock is already passed as stats lock? Having it as standalone
argument looked like cleaner approach to me. If you think this change is
too much code for very little benefit, I can reuse stats lock.

>
>   Marcelo

Thank you for reviewing my code!