netdev - Re: [PATCH net-next 06/10] net: sched: conditionally obtain rtnl lock in cls hw offloads API

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: <vbfef1c17tv.fsf@mellanox.com>
Date:   Fri, 23 Aug 2019 10:42:39 +0000
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     Vlad Buslov <vladbu@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "pablo@...filter.org" <pablo@...filter.org>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH net-next 06/10] net: sched: conditionally obtain rtnl lock
 in cls hw offloads API


On Fri 23 Aug 2019 at 02:03, Jakub Kicinski <jakub.kicinski@...ronome.com> wrote:
> On Thu, 22 Aug 2019 15:43:49 +0300, Vlad Buslov wrote:
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 02a547aa77c0..bda42f1b5514 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -3076,11 +3076,28 @@ __tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>>  int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>>  		     void *type_data, bool err_stop, bool rtnl_held)
>>  {
>> +	bool take_rtnl = false;
>
> Should we perhaps speculatively:
>
> 	 bool take_rtnl = READ_ONCE(block->lockeddevcnt);
>
> here? It shouldn't hurt, really, and otherwise every offload that
> requires rtnl will have to re-lock cb_lock, every single time..

Great idea! This looks like a straightforward opportunity for
optimization.

>
>>  	int ok_count;
>>
>> +retry:
>> +	if (take_rtnl)
>> +		rtnl_lock();
>>  	down_read(&block->cb_lock);
>> +	/* Need to obtain rtnl lock if block is bound to devs that require it.
>> +	 * In block bind code cb_lock is obtained while holding rtnl, so we must
>> +	 * obtain the locks in same order here.
>> +	 */
>> +	if (!rtnl_held && !take_rtnl && block->lockeddevcnt) {
>> +		up_read(&block->cb_lock);
>> +		take_rtnl = true;
>> +		goto retry;
>> +	}
>> +
>>  	ok_count = __tc_setup_cb_call(block, type, type_data, err_stop);
>> +
>>  	up_read(&block->cb_lock);
>> +	if (take_rtnl)
>> +		rtnl_unlock();
>>  	return ok_count;
>>  }
>>  EXPORT_SYMBOL(tc_setup_cb_call);