netdev - Re: [Patch net] net_sched: move tcf_lock down after gen_replace

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]

Message-ID: <41b1d17b-d8ba-6d70-9cc7-fcc5f5306dda@mojatatu.com>
Date:   Wed, 14 Jun 2017 08:48:26 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
Cc:     nicholashuber@...il.com, labbott@...hat.com
Subject: Re: [Patch net] net_sched: move tcf_lock down after
 gen_replace_estimator()

On 17-06-13 04:36 PM, Cong Wang wrote:
> Laura reported a sleep-in-atomic kernel warning inside
> tcf_act_police_init() which calls gen_replace_estimator() with
> spinlock protection.
> 
> It is not necessary in this case, we already have RTNL lock here
> so it is enough to protect concurrent writers. For the reader,
> i.e. tcf_act_police(), it needs to make decision based on this
> rate estimator, in the worst case we drop more/less packets than
> necessary while changing the rate in parallel, it is still acceptable.
> 
> Reported-by: Laura Abbott <labbott@...hat.com>
> Reported-by: Nick Huber <nicholashuber@...il.com>
> Cc: Jamal Hadi Salim <jhs@...atatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> ---
>   net/sched/act_police.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index f42008b..b062bc8 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -132,21 +132,21 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
>   		}
>   	}
>   
> -	spin_lock_bh(&police->tcf_lock);
>   	if (est) {
>   		err = gen_replace_estimator(&police->tcf_bstats, NULL,
>   					    &police->tcf_rate_est,
>   					    &police->tcf_lock,
>   					    NULL, est);
>   		if (err)
> -			goto failure_unlock;
> +			goto failure;
>   	} else if (tb[TCA_POLICE_AVRATE] &&
>   		   (ret == ACT_P_CREATED ||
>   		    !gen_estimator_active(&police->tcf_rate_est))) {
>   		err = -EINVAL;
> -		goto failure_unlock;
> +		goto failure;
>   	}
>   
> +	spin_lock_bh(&police->tcf_lock);
>   	/* No failure allowed after this point */
>   	police->tcfp_mtu = parm->mtu;
>   	if (police->tcfp_mtu == 0) {
> @@ -192,8 +192,6 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
>   
>   	return ret;
>   
> -failure_unlock:
> -	spin_unlock_bh(&police->tcf_lock);
>   failure:
>   	qdisc_put_rtab(P_tab);
>   	qdisc_put_rtab(R_tab);
> 


Looks good to me.

Acked-by: Jamal Hadi Salim <jhs@...atatu.com>

cheers,
jamal