linux-kernel - Re: [PATCH] iio: core: print error message on debugfs register failure

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: <20191217151940.GB3654493@kroah.com>
Date:   Tue, 17 Dec 2019 16:19:40 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Alexandru Ardelean <alexandru.ardelean@...log.com>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        jic23@...nel.org, Michael Hennerich <michael.hennerich@...log.com>
Subject: Re: [PATCH] iio: core: print error message on debugfs register
 failure

On Wed, Dec 11, 2019 at 05:16:36PM +0200, Alexandru Ardelean wrote:
> From: Michael Hennerich <michael.hennerich@...log.com>
> 
> If there's a failure when registering a debugfs entry for a device, don't
> silently ignore the failure. Instead, print an error message and an error
> code signaling the failure.

No, never do that.

> Signed-off-by: Michael Hennerich <michael.hennerich@...log.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> ---
>  drivers/iio/industrialio-core.c | 34 +++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index dab67cb69fe6..662dabf8b08c 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -364,23 +364,45 @@ static const struct file_operations iio_debugfs_reg_fops = {
>  static void iio_device_unregister_debugfs(struct iio_dev *indio_dev)
>  {
>  	debugfs_remove_recursive(indio_dev->debugfs_dentry);
> +	indio_dev->debugfs_dentry = NULL;
>  }
>  
>  static void iio_device_register_debugfs(struct iio_dev *indio_dev)
>  {
> +	struct dentry *d;
> +	int ret;
> +
>  	if (indio_dev->info->debugfs_reg_access == NULL)
>  		return;
>  
>  	if (!iio_debugfs_dentry)
>  		return;
>  
> -	indio_dev->debugfs_dentry =
> -		debugfs_create_dir(dev_name(&indio_dev->dev),
> -				   iio_debugfs_dentry);
> +	d = debugfs_create_dir(dev_name(&indio_dev->dev), iio_debugfs_dentry);
> +	if (IS_ERR_OR_NULL(d))
> +		goto error;

No, don't do that, I spent a lot of time removing all of these pointless
checks.

No kernel code shoudl ever care if debugfs is workign or not, just make
the call and move on.   You can always pass the result of a debugfs call
into another one with no problems.

> +
> +	indio_dev->debugfs_dentry = d;
> +
> +	d = debugfs_create_file("direct_reg_access", 0644,
> +				indio_dev->debugfs_dentry, indio_dev,
> +				&iio_debugfs_reg_fops);
> +
> +	if (IS_ERR_OR_NULL(d))
> +		goto error;

This check isn't even correct :)

So this isn't going to work no matter what, sorry.

just don't do this.

The code is just fine as-is.

thanks,

greg k-h