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:   Tue, 7 Aug 2018 22:37:28 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Vlad Buslov <vladbu@...lanox.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 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?
Or maybe it is because the current one is explicitly aimed towards the
stats?

  Marcelo