linux-kernel - Re: [PATCH v2] ntb:Fix an NULL vs IS_ERR() bug for debugfs_create_dir() in tool_setup

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: <138ab604-27de-4ac6-88fb-0886ee8f6b6d@kadam.mountain>
Date:   Wed, 12 Jul 2023 16:50:34 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Wang Ming <machel@...o.com>
Cc:     Jon Mason <jdmason@...zu.us>, Dave Jiang <dave.jiang@...el.com>,
        Allen Hubbe <allenbh@...il.com>,
        Serge Semin <fancer.lancer@...il.com>,
        Dan Carpenter <error27@...il.com>,
        Jiasheng Jiang <jiasheng@...as.ac.cn>, ntb@...ts.linux.dev,
        linux-kernel@...r.kernel.org, opensource.kernel@...o.com
Subject: Re: [PATCH v2] ntb:Fix an NULL vs IS_ERR() bug for
 debugfs_create_dir() in tool_setup_dbgfs()

On Wed, Jul 12, 2023 at 08:39:59PM +0800, Wang Ming wrote:
> The debugfs_create_dir() function returns error pointers.
> It never returns NULL. Most incorrect error checks were fixed,
> but the one in tool_setup_dbgfs() was forgotten.
> 
> Fix the remaining error check.
> 
> Signed-off-by: Wang Ming <machel@...o.com>
> ---
>  drivers/ntb/test/ntb_tool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
> index eeeb4b1c97d2..4fa69ea4331d 100644
> --- a/drivers/ntb/test/ntb_tool.c
> +++ b/drivers/ntb/test/ntb_tool.c
> @@ -1495,7 +1495,7 @@ static void tool_setup_dbgfs(struct tool_ctx *tc)
>  
>  	tc->dbgfs_dir = debugfs_create_dir(dev_name(&tc->ntb->dev),
>  					   tool_dbgfs_topdir);
> -	if (!tc->dbgfs_dir)
> +	if (IS_ERR(tc->dbgfs_dir))

No, this will break the driver if debugfs is disabled in the .config.

(I haven't checked, it's possible that this code is #ifdeffed out when
CONFIG_DEBUGFS is disabled so possibly this change is harmless.  But
either way, this change is wrong).

Normally this would be the correct change, but debugfs is weird.  It's
not supposed to be checked for errors in the normal case.  If the
driver pokes around in the debugfs internals then you might need to
check but you should avoid doing that and it doesn't apply here.

As I was saying, this change would normally be the correct thing, and it
used to work.  But we changed it so that now it's impossible to check
for errors.  Making it impossible to check for errors helps people feel
better about deleting error checking.

The correct change is to delete this dead code, but it's a headache
to convince people to it.  It would be better to do it as a mass delete
so everyone can see the thread.  Trying to convince people one by one
does not work.

regards,
dan carpenter