[PATCH] debugfs: Check module state before warning in {full/open}_proxy_open()

[PATCH] debugfs: Check module state before warning in {full/open}_proxy_open()

[Taehee Yoo]

When the module is being removed, the module state is set to
MODULE_STATE_GOING. At this point, try_module_get() fails.
And when {full/open}_proxy_open() is being called,
it calls try_module_get() to try to hold module reference count.
If it fails, it warns about the possibility of debugfs file leak.

If {full/open}_proxy_open() is called while the module is being removed,
it fails to hold the module.
So, It warns about debugfs file leak. But it is not the debugfs file
leak case. So, this patch just adds module state checking routine
in the {full/open}_proxy_open().

Test commands:
    #SHELL1
    while :
    do
        modprobe netdevsim
	echo 1 > /sys/bus/netdevsim/new_device
	modprobe -rv netdevsim
    done

    #SHELL2
    while :
    do
        cat /sys/kernel/debug/netdevsim/netdevsim1/ports/0/ipsec
    done

Splat looks like:
[  298.766738][T14664] debugfs file owner did not clean up at exit: ipsec
[  298.766766][T14664] WARNING: CPU: 2 PID: 14664 at fs/debugfs/file.c:312 full_proxy_open+0x10f/0x650
[  298.768595][T14664] Modules linked in: netdevsim(-) openvswitch nsh nf_conncount nf_nat nf_conntrack nf_defrag_ipv6 n]
[  298.771343][T14664] CPU: 2 PID: 14664 Comm: cat Tainted: G        W         5.5.0+ #1
[  298.772373][T14664] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[  298.773545][T14664] RIP: 0010:full_proxy_open+0x10f/0x650
[  298.774247][T14664] Code: 48 c1 ea 03 80 3c 02 00 0f 85 c1 04 00 00 49 8b 3c 24 e8 e4 b5 78 ff 84 c0 75 2d 4c 89 ee 48
[  298.776782][T14664] RSP: 0018:ffff88805b7df9b8 EFLAGS: 00010282
[  298.777583][T14664] RAX: dffffc0000000008 RBX: ffff8880511725c0 RCX: 0000000000000000
[  298.778610][T14664] RDX: 0000000000000000 RSI: 0000000000000006 RDI: ffff8880540c5c14
[  298.779637][T14664] RBP: 0000000000000000 R08: fffffbfff15235ad R09: 0000000000000000
[  298.780664][T14664] R10: 0000000000000001 R11: 0000000000000000 R12: ffffffffc06b5000
[  298.781702][T14664] R13: ffff88804c234a88 R14: ffff88804c22dd00 R15: ffffffff8a1b5660
[  298.782722][T14664] FS:  00007fafa13a8540(0000) GS:ffff88806c800000(0000) knlGS:0000000000000000
[  298.783845][T14664] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  298.784672][T14664] CR2: 00007fafa0e9cd10 CR3: 000000004b286005 CR4: 00000000000606e0
[  298.785739][T14664] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  298.786769][T14664] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  298.787785][T14664] Call Trace:
[  298.788237][T14664]  do_dentry_open+0x63c/0xf50
[  298.788872][T14664]  ? open_proxy_open+0x270/0x270
[  298.789524][T14664]  ? __x64_sys_fchdir+0x180/0x180
[  298.790169][T14664]  ? inode_permission+0x65/0x390
[  298.790832][T14664]  path_openat+0xc45/0x2680
[  298.791425][T14664]  ? save_stack+0x69/0x80
[  298.791988][T14664]  ? save_stack+0x19/0x80
[  298.792544][T14664]  ? path_mountpoint+0x2e0/0x2e0
[  298.793233][T14664]  ? check_chain_key+0x236/0x5d0
[  298.793910][T14664]  ? sched_clock_cpu+0x18/0x170
[  298.794527][T14664]  ? find_held_lock+0x39/0x1d0
[  298.795153][T14664]  do_filp_open+0x16a/0x260
[ ... ]

Fixes: 9fd4dcece43a ("debugfs: prevent access to possibly dead file_operations at file open")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 fs/debugfs/file.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 634b09d18b77..890903543678 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -175,8 +175,11 @@ static int open_proxy_open(struct inode *inode, struct file *filp)
 	if (r)
 		goto out;
 
-	real_fops = fops_get(real_fops);
-	if (!real_fops) {
+	if (!fops_get(real_fops)) {
+		if (real_fops->owner &&
+		    real_fops->owner->state == MODULE_STATE_GOING)
+			goto out;
+
 		/* Huh? Module did not clean up after itself at exit? */
 		WARN(1, "debugfs file owner did not clean up at exit: %pd",
 			dentry);
@@ -305,8 +308,11 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
 	if (r)
 		goto out;
 
-	real_fops = fops_get(real_fops);
-	if (!real_fops) {
+	if (!fops_get(real_fops)) {
+		if (real_fops->owner &&
+		    real_fops->owner->state == MODULE_STATE_GOING)
+			goto out;
+
 		/* Huh? Module did not cleanup after itself at exit? */
 		WARN(1, "debugfs file owner did not clean up at exit: %pd",
 			dentry);
-- 
2.17.1

Re: [PATCH] debugfs: Check module state before warning in {full/open}_proxy_open()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3371 bytes --]

Hi Taehee,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on v5.5 next-20200204]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Taehee-Yoo/debugfs-Check-module-state-before-warning-in-full-open-_proxy_open/20200206-045726
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 6992ca0dd017ebaa2bf8e9dcc49f1c3b7033b082
config: arm-mps2_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/debugfs/file.c: In function 'open_proxy_open':
>> fs/debugfs/file.c:180:23: error: dereferencing pointer to incomplete type 'struct module'
          real_fops->owner->state == MODULE_STATE_GOING)
                          ^~
>> fs/debugfs/file.c:180:34: error: 'MODULE_STATE_GOING' undeclared (first use in this function); did you mean 'DL_STATE_NONE'?
          real_fops->owner->state == MODULE_STATE_GOING)
                                     ^~~~~~~~~~~~~~~~~~
                                     DL_STATE_NONE
   fs/debugfs/file.c:180:34: note: each undeclared identifier is reported only once for each function it appears in
   fs/debugfs/file.c: In function 'full_proxy_open':
   fs/debugfs/file.c:313:34: error: 'MODULE_STATE_GOING' undeclared (first use in this function); did you mean 'DL_STATE_NONE'?
          real_fops->owner->state == MODULE_STATE_GOING)
                                     ^~~~~~~~~~~~~~~~~~
                                     DL_STATE_NONE

vim +180 fs/debugfs/file.c

   161	
   162	static int open_proxy_open(struct inode *inode, struct file *filp)
   163	{
   164		struct dentry *dentry = F_DENTRY(filp);
   165		const struct file_operations *real_fops = NULL;
   166		int r;
   167	
   168		r = debugfs_file_get(dentry);
   169		if (r)
   170			return r == -EIO ? -ENOENT : r;
   171	
   172		real_fops = debugfs_real_fops(filp);
   173	
   174		r = debugfs_locked_down(inode, filp, real_fops);
   175		if (r)
   176			goto out;
   177	
   178		if (!fops_get(real_fops)) {
   179			if (real_fops->owner &&
 > 180			    real_fops->owner->state == MODULE_STATE_GOING)
   181				goto out;
   182	
   183			/* Huh? Module did not clean up after itself at exit? */
   184			WARN(1, "debugfs file owner did not clean up at exit: %pd",
   185				dentry);
   186			r = -ENXIO;
   187			goto out;
   188		}
   189		replace_fops(filp, real_fops);
   190	
   191		if (real_fops->open)
   192			r = real_fops->open(inode, filp);
   193	
   194	out:
   195		debugfs_file_put(dentry);
   196		return r;
   197	}
   198	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 9518 bytes --]