linux-kernel - RE: [PATCH v2] mei: 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]

Message-ID: <5B8DA87D05A7694D9FA63FD143655C1B9DC4B6C4@hasmsx108.ger.corp.intel.com>
Date:   Wed, 19 Jun 2019 08:25:58 +0000
From:   "Winkler, Tomas" <tomas.winkler@...el.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Arnd Bergmann <arnd@...db.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] mei: no need to check return value of debugfs_create
 functions

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

Maybe need to mention that API has changed in patch ' ff9fb72bc07705c00795ca48631f7fffe24d2c6b' in 5.0 
and create_dir() doesn't return NULL but ERR_PTR() and proper checking is done inside the debugfs functions.
Not sure how critical is that but, but this should go probably to stable 5.0+ as well. 
Ack otherwise. 
> 
> Cc: Tomas Winkler <tomas.winkler@...el.com>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> ---
> v2: break the patch up properly
> 
>  drivers/misc/mei/debugfs.c | 47 +++++++++-----------------------------
>  drivers/misc/mei/main.c    |  8 +------
>  drivers/misc/mei/mei_dev.h |  7 ++----
>  3 files changed, 14 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/misc/mei/debugfs.c b/drivers/misc/mei/debugfs.c index
> 0970142bcace..df6bf8b81936 100644
> --- a/drivers/misc/mei/debugfs.c
> +++ b/drivers/misc/mei/debugfs.c
> @@ -233,47 +233,22 @@ void mei_dbgfs_deregister(struct mei_device *dev)
>   *
>   * @dev: the mei device structure
>   * @name: the mei device name
> - *
> - * Return: 0 on success, <0 on failure.
>   */
> -int mei_dbgfs_register(struct mei_device *dev, const char *name)
> +void mei_dbgfs_register(struct mei_device *dev, const char *name)
>  {
> -	struct dentry *dir, *f;
> +	struct dentry *dir;
> 
>  	dir = debugfs_create_dir(name, NULL);
> -	if (!dir)
> -		return -ENOMEM;
> -
>  	dev->dbgfs_dir = dir;
> 
> -	f = debugfs_create_file("meclients", S_IRUSR, dir,
> -				dev, &mei_dbgfs_fops_meclients);
> -	if (!f) {
> -		dev_err(dev->dev, "meclients: registration failed\n");
> -		goto err;
> -	}
> -	f = debugfs_create_file("active", S_IRUSR, dir,
> -				dev, &mei_dbgfs_fops_active);
> -	if (!f) {
> -		dev_err(dev->dev, "active: registration failed\n");
> -		goto err;
> -	}
> -	f = debugfs_create_file("devstate", S_IRUSR, dir,
> -				dev, &mei_dbgfs_fops_devstate);
> -	if (!f) {
> -		dev_err(dev->dev, "devstate: registration failed\n");
> -		goto err;
> -	}
> -	f = debugfs_create_file("allow_fixed_address", S_IRUSR | S_IWUSR,
> dir,
> -				&dev->allow_fixed_address,
> -				&mei_dbgfs_fops_allow_fa);
> -	if (!f) {
> -		dev_err(dev->dev, "allow_fixed_address: registration
> failed\n");
> -		goto err;
> -	}
> -	return 0;
> -err:
> -	mei_dbgfs_deregister(dev);
> -	return -ENODEV;
> +	debugfs_create_file("meclients", S_IRUSR, dir, dev,
> +			    &mei_dbgfs_fops_meclients);
> +	debugfs_create_file("active", S_IRUSR, dir, dev,
> +			    &mei_dbgfs_fops_active);
> +	debugfs_create_file("devstate", S_IRUSR, dir, dev,
> +			    &mei_dbgfs_fops_devstate);
> +	debugfs_create_file("allow_fixed_address", S_IRUSR | S_IWUSR, dir,
> +			    &dev->allow_fixed_address,
> +			    &mei_dbgfs_fops_allow_fa);
>  }
> 
> diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c index
> ad02097d7fee..f894d1f8a53e 100644
> --- a/drivers/misc/mei/main.c
> +++ b/drivers/misc/mei/main.c
> @@ -984,16 +984,10 @@ int mei_register(struct mei_device *dev, struct
> device *parent)
>  		goto err_dev_create;
>  	}
> 
> -	ret = mei_dbgfs_register(dev, dev_name(clsdev));
> -	if (ret) {
> -		dev_err(clsdev, "cannot register debugfs ret = %d\n", ret);
> -		goto err_dev_dbgfs;
> -	}
> +	mei_dbgfs_register(dev, dev_name(clsdev));
> 
>  	return 0;
> 
> -err_dev_dbgfs:
> -	device_destroy(mei_class, devno);
>  err_dev_create:
>  	cdev_del(&dev->cdev);
>  err_dev_add:
> diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h index
> fca832fcac57..f71a023aed3c 100644
> --- a/drivers/misc/mei/mei_dev.h
> +++ b/drivers/misc/mei/mei_dev.h
> @@ -718,13 +718,10 @@ bool mei_hbuf_acquire(struct mei_device *dev);
> bool mei_write_is_idle(struct mei_device *dev);
> 
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> -int mei_dbgfs_register(struct mei_device *dev, const char *name);
> +void mei_dbgfs_register(struct mei_device *dev, const char *name);
>  void mei_dbgfs_deregister(struct mei_device *dev);  #else -static inline int
> mei_dbgfs_register(struct mei_device *dev, const char *name) -{
> -	return 0;
> -}
> +static inline void mei_dbgfs_register(struct mei_device *dev, const
> +char *name) {}
>  static inline void mei_dbgfs_deregister(struct mei_device *dev) {}  #endif /*
> CONFIG_DEBUG_FS */
> 
> --
> 2.22.0