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

Message-ID: <20241025120917.GQ1202098@kernel.org>
Date: Fri, 25 Oct 2024 13:09:17 +0100
From: Simon Horman <horms@...nel.org>
To: "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
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 Fri, Oct 25, 2024 at 12:17:17PM +0800, Leizhen (ThunderTown) wrote:
> 
> 
> 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.

Thanks. Sorry that I was not clearer.

...