linux-kernel - Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16

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

Date:   Tue, 15 Nov 2016 08:54:16 +0100
From:   Shiva Kerdel <shiva@...ev.nl>
To:     Stuart Yoder <stuart.yoder@....com>,
        Dan Carpenter <dan.carpenter@...cle.com>
Cc:     "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Nipun Gupta <nipun.gupta@....com>,
        "treding@...dia.com" <treding@...dia.com>,
        Laurentiu Tudor <laurentiu.tudor@....com>
Subject: Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16'
 preferred over 'int16_t'


>> -----Original Message-----
>> From: Dan Carpenter [mailto:dan.carpenter@...cle.com]
>> Sent: Monday, November 14, 2016 4:06 AM
>> To: Stuart Yoder <stuart.yoder@....com>
>> Cc: Shiva Kerdel <shiva@...ev.nl>; devel@...verdev.osuosl.org; gregkh@...uxfoundation.org; linux-
>> kernel@...r.kernel.org; Nipun Gupta <nipun.gupta@....com>; treding@...dia.com; Laurentiu Tudor
>> <laurentiu.tudor@....com>
>> Subject: Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'
>>
>> On Fri, Nov 11, 2016 at 02:52:31PM +0000, Stuart Yoder wrote:
>>>>> diff --git a/drivers/staging/fsl-mc/include/mc-bus.h b/drivers/staging/fsl-mc/include/mc-bus.h
>>>>> index e915574..c7cad87 100644
>>>>> --- a/drivers/staging/fsl-mc/include/mc-bus.h
>>>>> +++ b/drivers/staging/fsl-mc/include/mc-bus.h
>>>>> @@ -42,8 +42,8 @@ struct msi_domain_info;
>>>>>    */
>>>>>   struct fsl_mc_resource_pool {
>>>>>   	enum fsl_mc_pool_type type;
>>>>> -	int16_t max_count;
>>>>> -	int16_t free_count;
>>>>> +	s16 max_count;
>>>> My understanding is that this has to be signed because the design of
>>>> this driver is that we keep adding devices until the the counter
>>>> overflows.  After that there are a couple tests for
>>>> "if (WARN_ON(res_pool->max_count < 0)) " which prevent the driver from
>>>> working again.
>>>>
>>>> This all seems pretty horrible.
>>> Can you elaborate?
>>>
>>> The resource pools managed by this driver are populated by hardware objects
>>> discovered when the fsl-mc bus probes a DPRC/container.
>>>
>>> The number of potential objects discovered of a given type is in the hundreds,
>>> so a signed 16-bit number is order of magnitudes larger than anything we will
>>> ever encounter.
>>>
>>> Would you feel better about this if max_count was an int?
>> Yeah.
>>
>>> The max_count reflects the total number of objects discovered.  If that is
>>> exceeded we display a warning, because something is horribly wrong.  Nothing
>>> stops working, the allocator simply refuses to add anything else to the
>>> free list.
>> I didn't look at this carefully...  Anyway we can't remove devices
>> either.  If we just had an upper bound instead of overflowing the s16
>> then we could still remove devices.
>>
>>> The only reason max_count is there at all is as an internal check against
>>> bugs and resource leaks.  If the driver is being removed and a resource
>>> pool is being freed, max_count must be zero...i.e. all objects should have
>>> been removed.  If not, there is a leak somewhere.  So, it's a sanity check.
>>>
>> Just use a normal upper bound with a #define instead of an magic number
>> hidden and then disguised as an integer overflow.
> Ok, agree that it would be clearer like that.
>
> Shiva, can you respin this patch and just make both max_count and free_count
> to be of type "int".
>
> I will get Dan's suggestion sent as a separate patch...to #define the upper bound
> instead of relying on integer overflow.
>
> Thanks,
> Stuart
I will do that, thank you for the clarification of what I should do.

Thanks,
Shiva Kerdel