netdev - Re: [PATCH 03/17] mlx5: no need to check return value of debugfs

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]

Date:   Tue, 6 Aug 2019 19:41:57 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "leon@...nel.org" <leon@...nel.org>
Subject: Re: [PATCH 03/17] mlx5: no need to check return value of
 debugfs_create functions

On Tue, 2019-08-06 at 18:11 +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic
> should
> never do something different based on this.
> 
> This cleans up a lot of unneeded code and logic around the debugfs
> files, making all of this much simpler and easier to understand as we
> don't need to keep the dentries saved anymore.
> 

Hi Greg, 

Basically i am ok with this patch and i like it very much.., but i am
concerned about some of the driver internal flows that are dependent on
these debug fs entries being valid.

for example mlx5_debug_eq_add if failed, it will fail the whole flow. I
know it is wrong even before your patch.. but maybe we should deal with
it now ? or let me know if you want me to follow up with my own patch. 

All we need to improve in this patch is to void out add_res_tree()
implemented in 
drivers/net/ethernet/mellanox/mlx5/core/debugfs.c 
as i will comment below.


> Cc: Saeed Mahameed <saeedm@...lanox.com>
> Cc: Leon Romanovsky <leon@...nel.org>
> Cc: netdev@...r.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/cmd.c |  51 ++-------
>  .../net/ethernet/mellanox/mlx5/core/debugfs.c | 102 ++------------
> ----
>  drivers/net/ethernet/mellanox/mlx5/core/eq.c  |  11 +-
>  .../net/ethernet/mellanox/mlx5/core/lib/eq.h  |   2 +-
>  .../net/ethernet/mellanox/mlx5/core/main.c    |   7 +-
>  .../ethernet/mellanox/mlx5/core/mlx5_core.h   |   2 +-
>  include/linux/mlx5/driver.h                   |  12 +--
>  7 files changed, 24 insertions(+), 163 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> index 8cdd7e66f8df..973f90888b1f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
> @@ -1368,49 +1368,19 @@ static void clean_debug_files(struct
> mlx5_core_dev *dev)
>  	debugfs_remove_recursive(dbg->dbg_root);
>  }
>  

[...]

>  void mlx5_cq_debugfs_cleanup(struct mlx5_core_dev *dev)
>  {
> -	if (!mlx5_debugfs_root)
> -		return;
> -
>  	debugfs_remove_recursive(dev->priv.cq_debugfs);
>  }
>  
> @@ -484,7 +418,6 @@ static int add_res_tree(struct mlx5_core_dev 

Basically this function is a debugfs wrapper that should behave the
same as debug_fs_*, we should fix it to return void as well, and
improve all the its callers to ignore the return value.

callers are:
mlx5_debug_qp_add()
mlx5_debug_eq_add()
mlx5_debug_cq_add()

> *dev, enum dbg_rsc_type type,
>  {
>  	struct mlx5_rsc_debug *d;
>  	char resn[32];
> -	int err;
>  	int i;
>  
>  	d = kzalloc(struct_size(d, fields, nfile), GFP_KERNEL);
> @@ -496,30 +429,15 @@ static int add_res_tree(struct mlx5_core_dev
> *dev, enum dbg_rsc_type type,
>  	d->type = type;
>  	sprintf(resn, "0x%x", rsn);
>  	d->root = debugfs_create_dir(resn,  root);
> -	if (!d->root) {
> -		err = -ENOMEM;
> -		goto out_free;
> -	}
>  
>  	for (i = 0; i < nfile; i++) {
>  		d->fields[i].i = i;
> -		d->fields[i].dent = debugfs_create_file(field[i], 0400,
> -							d->root, &d-
> >fields[i],
> -							&fops);
> -		if (!d->fields[i].dent) {
> -			err = -ENOMEM;
> -			goto out_rem;
> -		}
> +		debugfs_create_file(field[i], 0400, d->root, &d-
> >fields[i],
> +				    &fops);
>  	}
>  	*dbg = d;
>  
>  	return 0;
> -out_rem:
> -	debugfs_remove_recursive(d->root);
> -
> -out_free:
> -	kfree(d);
> -	return err;
>  }