| Message ID | 20241023080921.326-2-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bna: Fix return value check for debugfs create APIs | expand |
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? > Fixes: 4ad23d2368cc ("bna: Remove error checking for debugfs_create_dir()") > Fixes: 7afc5dbde091 ("bna: Add debugfs interface.") > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> ...
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@huawei.com> > > ... > . >
> Do you want to ignore all the return values of debugfs_create_dir() and debugfs_create_file()?
All return values from all debugfs_foo() calls.
debugfs has been designed so that it should be robust if any previous
call to debugfs failed. It will not crash, it will just keep going.
It does not matter if the contents of debugfs are messed up as a
result, it is not an ABI, you cannot trust it to contain anything
useful, and it might be missing all together, since it is optional.
Andrew
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? > > 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@huawei.com> > > > > ... > > . > > > > -- > Regards, > Zhen Lei >
On 2024/10/24 22:04, Andrew Lunn wrote: >> Do you want to ignore all the return values of debugfs_create_dir() and debugfs_create_file()? > > All return values from all debugfs_foo() calls. I searched. Currently, bna only involves functions debugfs_create_dir() and debugfs_create_file(). debugfs_remove() has no return value. git grep -n "\bdebugfs_" drivers/net/ethernet/brocade/bna/ drivers/net/ethernet/brocade/bna/bnad_debugfs.c:501: bna_debugfs_root = debugfs_create_dir("bna", NULL); drivers/net/ethernet/brocade/bna/bnad_debugfs.c:514: debugfs_create_dir(name, bna_debugfs_root); drivers/net/ethernet/brocade/bna/bnad_debugfs.c:521: debugfs_create_file(file->name, drivers/net/ethernet/brocade/bna/bnad_debugfs.c:544: debugfs_remove(bnad->bnad_dentry_files[i]); drivers/net/ethernet/brocade/bna/bnad_debugfs.c:551: debugfs_remove(bnad->port_debugfs_root); drivers/net/ethernet/brocade/bna/bnad_debugfs.c:558: debugfs_remove(bna_debugfs_root); > > debugfs has been designed so that it should be robust if any previous > call to debugfs failed. It will not crash, it will just keep going. > > It does not matter if the contents of debugfs are messed up as a > result, it is not an ABI, you cannot trust it to contain anything > useful, and it might be missing all together, since it is optional. Okay, thank you for your detailed explanation. > > Andrew > . >
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@huawei.com> >>> >>> ... >>> . >>> >> >> -- >> Regards, >> Zhen Lei >> > . >
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. ...
diff --git a/drivers/net/ethernet/brocade/bna/bnad_debugfs.c b/drivers/net/ethernet/brocade/bna/bnad_debugfs.c index 97291bfbeea589e..ad0b29391f990f3 100644 --- a/drivers/net/ethernet/brocade/bna/bnad_debugfs.c +++ b/drivers/net/ethernet/brocade/bna/bnad_debugfs.c @@ -493,25 +493,29 @@ void bnad_debugfs_init(struct bnad *bnad) { const struct bnad_debugfs_entry *file; + struct dentry *de; char name[64]; int i; /* Setup the BNA debugfs root directory*/ if (!bna_debugfs_root) { - bna_debugfs_root = debugfs_create_dir("bna", NULL); + de = debugfs_create_dir("bna", NULL); atomic_set(&bna_debugfs_port_count, 0); - if (!bna_debugfs_root) { + if (IS_ERR(de)) { netdev_warn(bnad->netdev, "debugfs root dir creation failed\n"); return; } + bna_debugfs_root = de; } /* Setup the pci_dev debugfs directory for the port */ snprintf(name, sizeof(name), "pci_dev:%s", pci_name(bnad->pcidev)); if (!bnad->port_debugfs_root) { - bnad->port_debugfs_root = - debugfs_create_dir(name, bna_debugfs_root); + de = debugfs_create_dir(name, bna_debugfs_root); + if (IS_ERR(de)) + return; + bnad->port_debugfs_root = de; atomic_inc(&bna_debugfs_port_count); @@ -523,7 +527,7 @@ bnad_debugfs_init(struct bnad *bnad) bnad->port_debugfs_root, bnad, file->fops); - if (!bnad->bnad_dentry_files[i]) { + if (IS_ERR(bnad->bnad_dentry_files[i])) { netdev_warn(bnad->netdev, "create %s entry failed\n", file->name);
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. Fixes: 4ad23d2368cc ("bna: Remove error checking for debugfs_create_dir()") Fixes: 7afc5dbde091 ("bna: Add debugfs interface.") Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- drivers/net/ethernet/brocade/bna/bnad_debugfs.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)