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

[Taehee Yoo <ap420073@gmail.com>]

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