Message ID | 20200111163723.4260-1-ap420073@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | netdevsim: fix a several bugs in netdevsim module | expand |
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>
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
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
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, };
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(-)