Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data

[Greg Kroah-Hartman]

On Tue, Feb 09, 2016 at 11:03:49PM +0100, Nicolai Stange wrote:
> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> >> Last time I checked the tree (Nov.), there weren't any users of this
> >> kind (debugfs file removal w/o module unload).
> >> Obviously I missed ion though... I will recheck.
> 
> Uhm, I must have been drinking back then.

I too want to drink when dealing with debugfs removal issues :)

> The real statistics are:
> There are 101 DEFINE_SIMPE_ATTRIBUTE fops handed to debugfs.
> From these, 49 suffer from "dynamic" debugfs file removal races.
> These 49 are distributed over the following 15 files:
> 
>   drivers/gpu/drm/i915/i915_debugfs.c
>   drivers/hwmon/asus_atk0110.c
>   drivers/iio/gyro/adis16136.c
>   drivers/iio/imu/adis16400_core.c
>   drivers/iio/imu/adis16480.c
>   drivers/input/touchscreen/edt-ft5x06.c
>   drivers/misc/cxl/debugfs.c
>   drivers/mmc/core/debugfs.c
>   drivers/net/wimax/i2400m/debugfs.c
>   drivers/net/wireless/ath/wil6210/debugfs.c
>   drivers/net/wireless/mac80211_hwsim.c
>   fs/ceph/debugfs.c
>   fs/ocfs2/blockcheck.c
>   lib/fault-inject.c
>   net/bluetooth/hci_debugfs.c
> 
> Details of my manual analysis can be found at
> http://nicst.de/debugfs_dsa_users.html

Nice summary.

> Now, a quick grep shows that there are 1002 call sites of
> debugfs_create_file() in total. Minus 101 yields 901 users of debugfs
> who hand in custom file_operations (assuming a 1:1 relationship between
> DEFINE_SIMPLE_ATTRIBUTE() and debugfs_create_file(), which mostly holds).

Ok, those will be easy to fix.

> Another 342 of these custom file_operations hand-ins are of the seqfile
> style, i.e. have their ->read set to seq_read().
> (A coccinelle patch for finding these 342 is available at
>  http://nicst.de/debugfs_seq.cocci
>  Run spatch with '-D org' or '-D report')
> 
> Migrating those to a debugfs_create_seqfile() might be good in terms of
> both, avoiding the removal race and refactorization in general.
> If only they were not so many...

That's what .cocci scripts are for :)

> However, there are still 901-342=559 debugfs_create_file() invocations I
> haven't even looked at yet. No clue how safe these are against removal
> races.

Ugh, that seems really high.  I found one such example in
sound/soc/soc-dapm.c and it should be simple to convert to use a seqfile
operation instead, so maybe the number of overall problems are smaller
than we think.  Will take a bunch of auditing to ensure it though.

> Before I proceed with the current approach, let me note that there might
> be an alternative that would not require modification of each and every
> debugfs user:
> We could rather not replace the ->f_op in debugfs' proxy_open() (from
> this series) with the original one, but instead keep an "extended" proxy
> there forever.
> This extended proxy would simply proxy every file_operations method to
> the original one, under protection of the SRCU lock.

Oh, that would be nice, and solve the issue for almost everyone right
away.

> However, the question is what to do with file_operation methods that
> are NULL in the original. Probably, -ENOTSUPP most of the time.

Yes, that should be fine.

> Now it's up to you to decide which way to go: mass patching throughout
> the tree or the full proxy?

Try the "full proxy" way and see how it goes, that might make this
simpler than touching every user in the tree, which would be good to not
have to do unless necessary.

thanks again for looking into this,

greg k-h