netdev - Re: [PATCH 1/2] bna: Fix return value check for debugfs create APIs

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: <c04f1ab2-54de-368c-d80b-f9716a944c30@huawei.com>
Date: Fri, 25 Oct 2024 12:17:17 +0800
From: "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
To: Simon Horman <horms@...nel.org>
CC: Rasesh Mody <rmody@...vell.com>, Sudarsana Kalluru <skalluru@...vell.com>,
	<GR-Linux-NIC-Dev@...vell.com>, Andrew Lunn <andrew+netdev@...n.ch>, "David S
 . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH 1/2] bna: Fix return value check for debugfs create APIs



On 2024/10/24 23:27, Simon Horman wrote:
> On Thu, Oct 24, 2024 at 09:26:30PM +0800, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2024/10/24 20:13, Simon Horman wrote:
>>> On Wed, Oct 23, 2024 at 04:09:20PM +0800, Zhen Lei wrote:
>>>> Fix the incorrect return value check for debugfs_create_dir() and
>>>> debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL
>>>> when it fails.
>>>>
>>>> Commit 4ad23d2368cc ("bna: Remove error checking for
>>>> debugfs_create_dir()") allows the program to continue execution if the
>>>> creation of bnad->port_debugfs_root fails, which causes the atomic count
>>>> bna_debugfs_port_count to be unbalanced. The corresponding error check
>>>> need to be added back.
>>>
>>> Hi Zhen Lei,
>>>
>>> The documentation for debugfs_create_dir states:
>>>
>>>  * NOTE: it's expected that most callers should _ignore_ the errors returned
>>>  * by this function. Other debugfs functions handle the fact that the "dentry"
>>>  * passed to them could be an error and they don't crash in that case.
>>>  * Drivers should generally work fine even if debugfs fails to init anyway.
>>>
>>> Which makes me wonder why we are checking the return value of
>>> debugfs_create_dir() at all. Can't we just take advantage of
>>> it not mattering, to debugfs functions, if the return value
>>> is an error or not?
>>
>> Do you want to ignore all the return values of debugfs_create_dir() and debugfs_create_file()?
>> "bna_debugfs_root = debugfs_create_dir("bna", NULL);" and debugfs_create_file() is OK.
>> I've carefully analyzed the current code, and "bnad->port_debugfs_root = debugfs_create_dir(...);"
>> is also OK for now.
> 
> What I'm saying is that it is unusual to depend on the return value of
> debugfs_create_dir() for anything. And it would be best to avoid doing so.
> 
> But perhaps that isn't possible for some reason?

OK, I understand now. Please forgive my poor English. Combine Andrew's reply
and my analysis above. The return value check for the remaining two places
should now be removed.

> 
>>
>> bnad_debugfs_init():
>> 	bnad->port_debugfs_root = debugfs_create_dir(name, bna_debugfs_root);	//IS_ERR() if fails
>> (1)
>> 	atomic_inc(&bna_debugfs_port_count);
>>
>> bnad_debugfs_uninit():
>> (2)	if (bnad->port_debugfs_root)						//It still works when it's IS_ERR()
>> 		atomic_dec(&bna_debugfs_port_count);
>>
>> 	if (atomic_read(&bna_debugfs_port_count) == 0)
>> 		debugfs_remove(bna_debugfs_root);
>>
>> If we want the code to be more robust or easier to understand, it is better
>> to modify (1) and (2) above as follows:
>> (1) if (IS_ERR(bnad->port_debugfs_root))
>> 	return;
>> (2) if (!IS_ERR_OR_NULL(bnad->port_debugfs_root))
>>
>>>
>>>> Fixes: 4ad23d2368cc ("bna: Remove error checking for debugfs_create_dir()")
>>>> Fixes: 7afc5dbde091 ("bna: Add debugfs interface.")
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@...wei.com>
>>>
>>> ...
>>> .
>>>
>>
>> -- 
>> Regards,
>>   Zhen Lei
>>
> .
> 

-- 
Regards,
  Zhen Lei