* [PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove
@ 2020-01-11 16:37 Taehee Yoo
2020-01-12 14:45 ` Jakub Kicinski
0 siblings, 1 reply; 4+ messages in thread
From: Taehee Yoo @ 2020-01-11 16:37 UTC (permalink / raw)
To: davem, jakub.kicinski, netdev; +Cc: ap420073
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove
2020-01-11 16:37 [PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove Taehee Yoo
@ 2020-01-12 14:45 ` Jakub Kicinski
2020-01-16 14:54 ` Taehee Yoo
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2020-01-12 14:45 UTC (permalink / raw)
To: Taehee Yoo; +Cc: davem, netdev
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove
2020-01-12 14:45 ` Jakub Kicinski
@ 2020-01-16 14:54 ` Taehee Yoo
2020-01-19 11:28 ` Taehee Yoo
0 siblings, 1 reply; 4+ messages in thread
From: Taehee Yoo @ 2020-01-16 14:54 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: David Miller, Netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove
2020-01-16 14:54 ` Taehee Yoo
@ 2020-01-19 11:28 ` Taehee Yoo
0 siblings, 0 replies; 4+ messages in thread
From: Taehee Yoo @ 2020-01-19 11:28 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: David Miller, Netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-01-19 11:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-11 16:37 [PATCH net 3/5] netdevsim: avoid debugfs warning message when module is remove Taehee Yoo
2020-01-12 14:45 ` Jakub Kicinski
2020-01-16 14:54 ` Taehee Yoo
2020-01-19 11:28 ` Taehee Yoo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.