| 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
| ||
|
Message-ID: <50a30aa3-3924-4fd1-f644-2fd2b184ec0e@mellanox.com>
Date: Tue, 5 Nov 2019 00:48:16 +0000
From: Mark Bloch <markb@...lanox.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Leon Romanovsky <leon@...nel.org>,
Doug Ledford <dledford@...hat.com>,
Jason Gunthorpe <jgg@...pe.ca>
CC: "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] IB: mlx5: no need to check return value of debugfs_create
functions
On 11/3/19 11:41 PM, 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.
>
> Cc: Leon Romanovsky <leon@...nel.org>
> Cc: Doug Ledford <dledford@...hat.com>
> Cc: Jason Gunthorpe <jgg@...pe.ca>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> ---
> drivers/infiniband/hw/mlx5/main.c | 62 +++++++---------------------
> drivers/infiniband/hw/mlx5/mlx5_ib.h | 9 +---
> 2 files changed, 16 insertions(+), 55 deletions(-)
>
> Note, I kind of need to take this through my tree now as I broke the
> build due to me changing the use of debugfs_create_atomic_t() in my
> tree and not noticing this being used here. Sorry about that, any
> objections?
>
> And 0-day seems really broken to have missed this for the past months,
> ugh, I need to stop relying on it...
>
>
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 831539419c30..059db0610445 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -5710,11 +5710,10 @@ static int mlx5_ib_rn_get_params(struct ib_device *device, u8 port_num,
>
> static void delay_drop_debugfs_cleanup(struct mlx5_ib_dev *dev)
> {
> - if (!dev->delay_drop.dbg)
> + if (!dev->delay_drop.dir_debugfs)
Shouldn't this be:
if (IS_ERR(dev->delay_drop.dir_debugfs))
return;
?
> return;
> - debugfs_remove_recursive(dev->delay_drop.dbg->dir_debugfs);
> - kfree(dev->delay_drop.dbg);
> - dev->delay_drop.dbg = NULL;
> + debugfs_remove_recursive(dev->delay_drop.dir_debugfs);
> + dev->delay_drop.dir_debugfs = NULL;
Thinking about this more, we already do something like this:
if (IS_ERR_OR_NULL(dentry))
return;
inside debugfs_remove_recursive(), so this entire function can be reduced
to just calling debugfs_remove_recursive().
Mark
> }
>
> static void cancel_delay_drop(struct mlx5_ib_dev *dev)
> @@ -5765,52 +5764,22 @@ static const struct file_operations fops_delay_drop_timeout = {
> .read = delay_drop_timeout_read,
> };
>
> -static int delay_drop_debugfs_init(struct mlx5_ib_dev *dev)
> +static void delay_drop_debugfs_init(struct mlx5_ib_dev *dev)
> {
> - struct mlx5_ib_dbg_delay_drop *dbg;
> + struct dentry *root;
>
> if (!mlx5_debugfs_root)
> - return 0;
> -
> - dbg = kzalloc(sizeof(*dbg), GFP_KERNEL);
> - if (!dbg)
> - return -ENOMEM;
> -
> - dev->delay_drop.dbg = dbg;
> -
> - dbg->dir_debugfs =
> - debugfs_create_dir("delay_drop",
> - dev->mdev->priv.dbg_root);
> - if (!dbg->dir_debugfs)
> - goto out_debugfs;
> -
> - dbg->events_cnt_debugfs =
> - debugfs_create_atomic_t("num_timeout_events", 0400,
> - dbg->dir_debugfs,
> - &dev->delay_drop.events_cnt);
> - if (!dbg->events_cnt_debugfs)
> - goto out_debugfs;
> -
> - dbg->rqs_cnt_debugfs =
> - debugfs_create_atomic_t("num_rqs", 0400,
> - dbg->dir_debugfs,
> - &dev->delay_drop.rqs_cnt);
> - if (!dbg->rqs_cnt_debugfs)
> - goto out_debugfs;
> -
> - dbg->timeout_debugfs =
> - debugfs_create_file("timeout", 0600,
> - dbg->dir_debugfs,
> - &dev->delay_drop,
> - &fops_delay_drop_timeout);
> - if (!dbg->timeout_debugfs)
> - goto out_debugfs;
> + return;
>
> - return 0;
> + root = debugfs_create_dir("delay_drop", dev->mdev->priv.dbg_root);
> + dev->delay_drop.dir_debugfs = root;
>
> -out_debugfs:
> - delay_drop_debugfs_cleanup(dev);
> - return -ENOMEM;
> + debugfs_create_atomic_t("num_timeout_events", 0400, root,
> + &dev->delay_drop.events_cnt);
> + debugfs_create_atomic_t("num_rqs", 0400, root,
> + &dev->delay_drop.rqs_cnt);
> + debugfs_create_file("timeout", 0600, root, &dev->delay_drop,
> + &fops_delay_drop_timeout);
> }
>
> static void init_delay_drop(struct mlx5_ib_dev *dev)
> @@ -5826,8 +5795,7 @@ static void init_delay_drop(struct mlx5_ib_dev *dev)
> atomic_set(&dev->delay_drop.rqs_cnt, 0);
> atomic_set(&dev->delay_drop.events_cnt, 0);
>
> - if (delay_drop_debugfs_init(dev))
> - mlx5_ib_warn(dev, "Failed to init delay drop debugfs\n");
> + delay_drop_debugfs_init(dev);
> }
>
> static void mlx5_ib_unbind_slave_port(struct mlx5_ib_dev *ibdev,
> diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> index 1a98ee2e01c4..55ce599db803 100644
> --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> @@ -792,13 +792,6 @@ enum {
> MLX5_MAX_DELAY_DROP_TIMEOUT_MS = 100,
> };
>
> -struct mlx5_ib_dbg_delay_drop {
> - struct dentry *dir_debugfs;
> - struct dentry *rqs_cnt_debugfs;
> - struct dentry *events_cnt_debugfs;
> - struct dentry *timeout_debugfs;
> -};
> -
> struct mlx5_ib_delay_drop {
> struct mlx5_ib_dev *dev;
> struct work_struct delay_drop_work;
> @@ -808,7 +801,7 @@ struct mlx5_ib_delay_drop {
> bool activate;
> atomic_t events_cnt;
> atomic_t rqs_cnt;
> - struct mlx5_ib_dbg_delay_drop *dbg;
> + struct dentry *dir_debugfs;
> };
>
> enum mlx5_ib_stages {
>
Powered by blists - more mailing lists