[PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove

[PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove

[Taehee Yoo]

When module is being removed, it couldn't be held by try_module_get().
debugfs's open function internally tries to hold file_operation->owner
if .owner is set.
If holding owner operation is failed, it prints a warning message.

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:
[  412.227709][ T1720] debugfs file owner did not clean up at exit: ipsec
[  412.227728][ T1720] WARNING: CPU: 3 PID: 1720 at fs/debugfs/file.c:309 full_proxy_open+0x10f/0x650
[  412.231755][ T1720] Modules linked in: netdevsim(-) veth openvswitch nsh nf_conncount nf_nat nf_conntrack nf_d]
[  412.236495][ T1720] CPU: 3 PID: 1720 Comm: cat Tainted: G    B   W         5.5.0-rc5+ #270
[  412.237468][ T1720] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[  412.239480][ T1720] RIP: 0010:full_proxy_open+0x10f/0x650
[  412.240321][ T1720] Code: 48 c1 ea 03 80 3c 02 00 0f 85 c1 04 00 00 49 8b 3c 24 e8 24 fb 78 ff 84 c0 75 2d 4c 8
[  412.247099][ T1720] RSP: 0018:ffff8880c9787a38 EFLAGS: 00010286
[  412.247905][ T1720] RAX: dffffc0000000008 RBX: ffff8880ccb94b80 RCX: ffffffff912c2234
[  412.248961][ T1720] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff93d6dcf4
[  412.252583][ T1720] RBP: 0000000000000000 R08: fffffbfff2722f5d R09: fffffbfff2722f5d
[  412.253677][ T1720] R10: 0000000000000001 R11: fffffbfff2722f5c R12: ffffffffc02a7a80
[  412.258939][ T1720] R13: ffff8880acd07728 R14: ffff8880ae9a9b60 R15: ffffffff931b3b60
[  412.260021][ T1720] FS:  00007f24a0b9c540(0000) GS:ffff8880da800000(0000) knlGS:0000000000000000
[  412.261208][ T1720] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  412.262131][ T1720] CR2: 0000561898255068 CR3: 00000000c6a6c004 CR4: 00000000000606e0
[  412.263201][ T1720] Call Trace:
[  412.263650][ T1720]  do_dentry_open+0x63c/0xf50
[  412.264304][ T1720]  ? open_proxy_open+0x270/0x270
[  412.265015][ T1720]  ? __x64_sys_fchdir+0x180/0x180
[  412.265708][ T1720]  ? inode_permission+0x65/0x390
[  412.266566][ T1720]  path_openat+0x701/0x2810
[  412.267445][ T1720]  ? save_stack+0x69/0x80
[  412.268288][ T1720]  ? path_lookupat+0x880/0x880
[  412.269218][ T1720]  ? getname_flags+0xba/0x500
[  412.270556][ T1720]  ? do_sys_open+0x15d/0x350
[  412.271195][ T1720]  ? do_syscall_64+0x99/0x4f0
[  412.271843][ T1720]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  412.272676][ T1720]  ? _raw_spin_unlock+0x1f/0x30
[  412.273349][ T1720]  ? deactivate_slab.isra.77+0x62e/0x800
[  412.274149][ T1720]  ? check_object+0xaf/0x260
[  412.274784][ T1720]  ? init_object+0x6b/0x80
[  412.275385][ T1720]  do_filp_open+0x17a/0x270
[ ... ]

In order to avoid the warning message, this patch makes netdevsim module
does not set .owner. Unsetting .owner is safe because these are protected
by inode_lock().

Fixes: 7699353da875 ("netdevsim: add ipsec offload testing")
Fixes: 31d3ad832948 ("netdevsim: add bpf offload support")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 drivers/net/netdevsim/bpf.c   | 13 ++++++++++++-
 drivers/net/netdevsim/ipsec.c |  1 -
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
index 2b74425822ab..13e17c82d71c 100644
--- a/drivers/net/netdevsim/bpf.c
+++ b/drivers/net/netdevsim/bpf.c
@@ -57,7 +57,18 @@ static int nsim_bpf_string_show(struct seq_file *file, void *data)
 
 	return 0;
 }
-DEFINE_SHOW_ATTRIBUTE(nsim_bpf_string);
+
+static int nsim_debugfs_bpf_string_open(struct inode *inode, struct file *f)
+{
+	return single_open(f, nsim_bpf_string_show, inode->i_private);
+}
+
+static const struct file_operations nsim_bpf_string_fops = {
+	.open = nsim_debugfs_bpf_string_open,
+	.release = single_release,
+	.read = seq_read,
+	.llseek = seq_lseek
+};
 
 static int
 nsim_bpf_verify_insn(struct bpf_verifier_env *env, int insn_idx, int prev_insn)
diff --git a/drivers/net/netdevsim/ipsec.c b/drivers/net/netdevsim/ipsec.c
index e27fc1a4516d..63aff6399d11 100644
--- a/drivers/net/netdevsim/ipsec.c
+++ b/drivers/net/netdevsim/ipsec.c
@@ -60,7 +60,6 @@ static ssize_t nsim_dbg_netdev_ops_read(struct file *filp,
 }
 
 static const struct file_operations ipsec_dbg_fops = {
-	.owner = THIS_MODULE,
 	.open = simple_open,
 	.read = nsim_dbg_netdev_ops_read,
 };
-- 
2.17.1

Re: [PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove

[Jakub Kicinski]

On Sat, 11 Jan 2020 16:37:23 +0000, Taehee Yoo wrote:
> When module is being removed, it couldn't be held by try_module_get().
> debugfs's open function internally tries to hold file_operation->owner
> if .owner is set.
> If holding owner operation is failed, it prints a warning message.

> [  412.227709][ T1720] debugfs file owner did not clean up at exit: ipsec

> In order to avoid the warning message, this patch makes netdevsim module
> does not set .owner. Unsetting .owner is safe because these are protected
> by inode_lock().

So inode_lock will protect from the code getting unloaded/disappearing?
At a quick glance at debugs code it doesn't seem that inode_lock would
do that. Could you explain a little more to a non-fs developer like
myself? :)

Alternatively should we perhaps hold a module reference for each device
created and force user space to clean up the devices? That may require
some fixes to the test which use netdevsim.

> Fixes: 7699353da875 ("netdevsim: add ipsec offload testing")
> Fixes: 31d3ad832948 ("netdevsim: add bpf offload support")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>

Re: [PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove

[Taehee Yoo]

On Sun, 12 Jan 2020 at 23:45, Jakub Kicinski <kuba@kernel.org> wrote:
>

Hi Jakub,
Thank you for catching the problem!

> On Sat, 11 Jan 2020 16:37:23 +0000, Taehee Yoo wrote:
> > When module is being removed, it couldn't be held by try_module_get().
> > debugfs's open function internally tries to hold file_operation->owner
> > if .owner is set.
> > If holding owner operation is failed, it prints a warning message.
>
> > [  412.227709][ T1720] debugfs file owner did not clean up at exit: ipsec
>
> > In order to avoid the warning message, this patch makes netdevsim module
> > does not set .owner. Unsetting .owner is safe because these are protected
> > by inode_lock().
>
> So inode_lock will protect from the code getting unloaded/disappearing?
> At a quick glance at debugs code it doesn't seem that inode_lock would
> do that. Could you explain a little more to a non-fs developer like
> myself? :)
>
> Alternatively should we perhaps hold a module reference for each device
> created and force user space to clean up the devices? That may require
> some fixes to the test which use netdevsim.
>

Sorry, I misunderstood the debugfs logic.
inode_lock() is called by debugfs_remove() and debugfs_create_file().
It doesn't protect read and write operations.

Currently, I have been taking look at debugfs_file_{get/put}() function,
which increases and decreases the reference counter.
In the __debugfs_file_removed(), this reference counter is used for
waiting read and write operations. Unfortunately, the
__debugfs_file_removed() isn't used because of "dentry->d_flags" value.
So, I'm looking for a way to use these functions.

Thanks a lot!
Taehee Yoo

Re: [PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove

[Taehee Yoo]

On Thu, 16 Jan 2020 at 23:54, Taehee Yoo <ap420073@gmail.com> wrote:
>
> On Sun, 12 Jan 2020 at 23:45, Jakub Kicinski <kuba@kernel.org> wrote:
> >
>
> Hi Jakub,

Hi again!

> Thank you for catching the problem!
>
> > On Sat, 11 Jan 2020 16:37:23 +0000, Taehee Yoo wrote:
> > > When module is being removed, it couldn't be held by try_module_get().
> > > debugfs's open function internally tries to hold file_operation->owner
> > > if .owner is set.
> > > If holding owner operation is failed, it prints a warning message.
> >
> > > [  412.227709][ T1720] debugfs file owner did not clean up at exit: ipsec
> >
> > > In order to avoid the warning message, this patch makes netdevsim module
> > > does not set .owner. Unsetting .owner is safe because these are protected
> > > by inode_lock().
> >
> > So inode_lock will protect from the code getting unloaded/disappearing?
> > At a quick glance at debugs code it doesn't seem that inode_lock would
> > do that. Could you explain a little more to a non-fs developer like
> > myself? :)
> >
> > Alternatively should we perhaps hold a module reference for each device
> > created and force user space to clean up the devices? That may require
> > some fixes to the test which use netdevsim.
> >
>
> Sorry, I misunderstood the debugfs logic.
> inode_lock() is called by debugfs_remove() and debugfs_create_file().
> It doesn't protect read and write operations.
>
> Currently, I have been taking look at debugfs_file_{get/put}() function,
> which increases and decreases the reference counter.
> In the __debugfs_file_removed(), this reference counter is used for
> waiting read and write operations. Unfortunately, the
> __debugfs_file_removed() isn't used because of "dentry->d_flags" value.
> So, I'm looking for a way to use these functions.

I will drop this patch from this patchset because .owner should be set.
If I understood debugfs logic correctly, only .owner protect the whole
.owner module. There are other locks in there, which are "d_lockref"
and "active_users" counter.

1. "active_users" protects it "temporarily" when operations are
being executed. So, it doesn't protect the whole .owner module.

static ret_type full_proxy_ ## name(proto)                              \
{                                                                       \
        struct dentry *dentry = F_DENTRY(filp);                 \
        const struct file_operations *real_fops;                        \
        ret_type r;                                                     \
                                                                        \
        r = debugfs_file_get(dentry);                                   \
        if (unlikely(r))                                                \
                return r;                                               \
        real_fops = debugfs_real_fops(filp);                            \
        r = real_fops->name(args);                                      \
        debugfs_file_put(dentry);                                       \
        return r;                                                       \
}

2. "d_lockref.count" means how many users are using this dentry.
This is also a counter value.
This is increased when ->open() is being called.
And this is decreased when ->released() is being called.
I think this counter is a good way to protect the .owner module.
But, debugfs_remove() doesn't wait for ->release() with this value.
So actually it couldn't protect the module.

So, there is no other way to protect the module disappearing while the
file is being used.
I think avoiding a warning message is up to the debugfs code.

So, I will drop this patch from the patchset.

Thanks again for the review.
Taehee Yoo