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: <19322579-a24b-679a-051b-c202eb3750f7@huawei.com>
Date: Thu, 24 Oct 2024 21:26:30 +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 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.

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