[PATCH v2] ntb:Fix an NULL vs IS_ERR() bug for debugfs_create_dir() in tool_setup_dbgfs()

[PATCH v2] ntb:Fix an NULL vs IS_ERR() bug for debugfs_create_dir() in tool_setup_dbgfs()

[Wang Ming]

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@vivo.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))
 		return;
 
 	debugfs_create_file("port", 0600, tc->dbgfs_dir,
-- 
2.25.1

Re: [PATCH v2] ntb:Fix an NULL vs IS_ERR() bug for debugfs_create_dir() in tool_setup_dbgfs()

[Dan Carpenter]

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@vivo.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

回复: [PATCH v2] ntb:Fix an NULL vs IS_ERR() bug for debugfs_create_dir() in tool_setup_dbgfs()

[王明-软件底层技术部]

Hi dan carpenter
You mean that this modification is correct, but there is no need to do so, is that the understanding?

regards
Wang Ming

-----邮件原件-----
发件人: Dan Carpenter <dan.carpenter@linaro.org> 
发送时间: 2023年7月12日 21:51
收件人: 王明-软件底层技术部 <machel@vivo.com>
抄送: Jon Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; Allen Hubbe <allenbh@gmail.com>; Serge Semin <fancer.lancer@gmail.com>; Dan Carpenter <error27@gmail.com>; Jiasheng Jiang <jiasheng@iscas.ac.cn>; ntb@lists.linux.dev; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com>
主题: 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@vivo.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

Re: 回复: [PATCH v2] ntb:Fix an NULL vs IS_ERR() bug for debugfs_create_dir() in tool_setup_dbgfs()

[Dan Carpenter]

On Thu, Jul 13, 2023 at 02:13:32AM +0000, 王明-软件底层技术部 wrote:
> Hi dan carpenter
> You mean that this modification is correct, but there is no need to do
> so, is that the understanding?

No, this patch is wrong.  Possibly harmless, possibly harmful but either
way it is wrong.  The correct way is:

diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c
index eeeb4b1c97d2..e0acc11d29ba 100644
--- a/drivers/ntb/test/ntb_tool.c
+++ b/drivers/ntb/test/ntb_tool.c
@@ -1495,8 +1495,6 @@ 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)
-		return;
 
 	debugfs_create_file("port", 0600, tc->dbgfs_dir,
 			    tc, &tool_port_fops);

回复: 回复: [PATCH v2] ntb:Fix an NULL vs IS_ERR() bug for debugfs_create_dir() in tool_setup_dbgfs()

[王明-软件底层技术部]

Hi Dan Carpenter

Thank you for your guidance to me every time. I will submit it again. Thank you very much

regards,
Ming

-----邮件原件-----
发件人: Dan Carpenter <dan.carpenter@linaro.org> 
发送时间: 2023年7月13日 14:00
收件人: 王明-软件底层技术部 <machel@vivo.com>
抄送: Jon Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; Allen Hubbe <allenbh@gmail.com>; Serge Semin <fancer.lancer@gmail.com>; Dan Carpenter <error27@gmail.com>; Jiasheng Jiang <jiasheng@iscas.ac.cn>; ntb@lists.linux.dev; LKML <linux-kernel@vger.kernel.org>; opensource.kernel <opensource.kernel@vivo.com>
主题: Re: 回复: [PATCH v2] ntb:Fix an NULL vs IS_ERR() bug for debugfs_create_dir() in tool_setup_dbgfs()

On Thu, Jul 13, 2023 at 02:13:32AM +0000, 王明-软件底层技术部 wrote:
> Hi dan carpenter
> You mean that this modification is correct, but there is no need to do 
> so, is that the understanding?

No, this patch is wrong.  Possibly harmless, possibly harmful but either way it is wrong.  The correct way is:

diff --git a/drivers/ntb/test/ntb_tool.c b/drivers/ntb/test/ntb_tool.c index eeeb4b1c97d2..e0acc11d29ba 100644
--- a/drivers/ntb/test/ntb_tool.c
+++ b/drivers/ntb/test/ntb_tool.c
@@ -1495,8 +1495,6 @@ 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)
-		return;
 
 	debugfs_create_file("port", 0600, tc->dbgfs_dir,
 			    tc, &tool_port_fops);