linux-kernel - Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE

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: <20200329103818.2fce9529@archlinux>
Date:   Sun, 29 Mar 2020 10:38:18 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Rohit Sarkar <rohitsarkar5398@...il.com>
Cc:     linux-iio@...r.kernel.org, dragos.bogdan@...log.com,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Stefan Popa <stefan.popa@...log.com>,
        Hartmut Knaack <knaack.h@....de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-kernel@...r.kernel.org,
        "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>,
        zhong jiang <zhongjiang@...wei.com>
Subject: Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of
 DEFINE_SIMPLE_ATTRIBUTE

On Sat, 28 Mar 2020 12:04:53 +0530
Rohit Sarkar <rohitsarkar5398@...il.com> wrote:

> The debugfs_create_file_unsafe method does not protect the fops given to
> it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
> which makes the fops aware of the file lifetime.
> 
> Further using DEFINE_DEBUGFS_ATTRIBUTE along with
> debugfs_create_file_unsafe significantly reduces the overhead introduced by
> debugfs_create_file which creates a lifetime managing proxy around each
> fops handed in. Refer [1] for more on this.
> 
> Fixes the following warnings reported by coccinelle:
> drivers/iio/imu//adis16460.c:126:0-23: WARNING: adis16460_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> drivers/iio/imu//adis16460.c:108:0-23: WARNING: adis16460_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> drivers/iio/imu//adis16460.c:90:0-23: WARNING: adis16460_serial_number_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> drivers/iio/imu//adis16400.c:278:0-23: WARNING: adis16400_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> drivers/iio/imu//adis16400.c:261:0-23: WARNING: adis16400_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> 
> [1]: https://lists.gt.net/linux/kernel/2369498
> 
> Rohit Sarkar (2):
>   iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
>     DEFINE_SIMPLE_ATTRIBUTE
>   iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
>     DEFINE_SIMPLE_ATTRIBUTE
> 
>  drivers/iio/imu/adis16400.c | 4 ++--
>  drivers/iio/imu/adis16460.c | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
Hi Rohit,

You've opened a can of worms with this one.  There as a previous series
posted doing exactly this change back in 2019 by Zhong Jiang (cc'd)

At the time I did a bit of looking into why this had been universally taken
up cross tree and turned out there are some potential issues.

Alexandru added it to the list of things to test, but I guess it got
buried under other work and is still outstanding.

https://lkml.org/lkml/2019/10/30/144

Has Greg KH raising the point that file reference counting is changed (as you
mention) but that can cause subtle bugs.  It 'might' be fine but is
definitely one that needs a tested-by from someone with the hardware.

Jonathan