netdev - Re: [patch net-next 0/3] net/sched: Improve getting objects by indexes

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-next>] [day] [month] [year] [list]

Date:   Wed, 16 Aug 2017 10:16:31 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Christian König <christian.koenig@....com>
Cc:     Chris Mi <chrism@...lanox.com>, netdev@...r.kernel.org
Subject: Re: [patch net-next 0/3] net/sched: Improve getting objects by
 indexes

Wed, Aug 16, 2017 at 09:49:07AM CEST, christian.koenig@....com wrote:
>Am 16.08.2017 um 04:12 schrieb Chris Mi:
>> Using current TC code, it is very slow to insert a lot of rules.
>> 
>> In order to improve the rules update rate in TC,
>> we introduced the following two changes:
>>          1) changed cls_flower to use IDR to manage the filters.
>>          2) changed all act_xxx modules to use IDR instead of
>>             a small hash table
>> 
>> But IDR has a limitation that it uses int. TC handle uses u32.
>> To make sure there is no regression, we also changed IDR to use
>> unsigned long. All clients of IDR are changed to use new IDR API.
>
>WOW, wait a second. The idr change is touching a lot of drivers and to be
>honest doesn't looks correct at all.
>
>Just look at the first chunk of your modification:
>> @@ -998,8 +999,9 @@ int bsg_register_queue(struct request_queue *q, struct device *parent,
>>   	mutex_lock(&bsg_mutex);
>> -	ret = idr_alloc(&bsg_minor_idr, bcd, 0, BSG_MAX_DEVS, GFP_KERNEL);
>> -	if (ret < 0) {
>> +	ret = idr_alloc(&bsg_minor_idr, bcd, &idr_index, 0, BSG_MAX_DEVS,
>> +			GFP_KERNEL);
>> +	if (ret) {
>>   		if (ret == -ENOSPC) {
>>   			printk(KERN_ERR "bsg: too many bsg devices\n");
>>   			ret = -EINVAL;
>The condition "if (ret)" will now always be true after the first allocation
>and so we always run into the error handling after that.

On success, idr_alloc returns 0.


>
>I've never read the bsg code before, but that's certainly not correct. And
>that incorrect pattern repeats over and over again in this code.
>
>Apart from that why the heck do you want to allocate more than 1<<31 handles?

tc action indexes for example. That is part of this patchset.