On Mon, Sep 20, 2021 at 07:53:16AM -1000, Tejun Heo wrote:
Hello,
On Fri, Sep 17, 2021 at 10:04:27PM -0700, Luis Chamberlain wrote:
> If try_module_get() fails we fail the operation on the kernfs node.
>
> We use a try method as a full lock means we'd then make our sysfs
> attributes busy us out from possible module removal, and so userspace
> could force denying module removal, a silly form of "DOS" against module
> removal. A try lock on the module removal ensures we give priority to
> module removal and interacting with sysfs attributes only comes second.
> Using a full lock could mean for instance that if you don't stop poking
> at sysfs files you cannot remove a module.
I find this explanation odd because there's no real equivalent to locking
the module (as opposed to try locking)
Actually there is, __module_get() but I suspect some of these users are
probably incorrect and should be be moved to try. The documentation
about "rmmod --wait" for __module_get() is also outdated as that option
is no longer supported. I'll send an update for that later.
because you can't wait for the
removal to finish and then grant the lock, so any operation which increases
the reference *has* to be a try method unless the caller already holds a
reference to the same module and thus knows that the module is already
pinned.
Right, the reason I mention the alternative is that we technically don't
need to use try in this case since during a kernfs op it is implied the
module will be pinned, but we have further motivations to use a try
method here: to avoid a possible DOS from module removal by userspace
mucking with ops.
The code isn't wrong, so maybe just drop the related paragraphs
in
the commit message?
Does it make sense to clarify the above a bit more somehow? Or do think
its not needed?
> static struct kernfs_node *__kernfs_new_node(struct kernfs_root
*root,
> struct kernfs_node *parent,
> const char *name, umode_t mode,
> + struct module *owner,
> kuid_t uid, kgid_t gid,
> unsigned flags)
Is there a particular reason why @owner is added between @mode and @uid?
Sitting between two fs attributes seems a bit awkward. Maybe it can just be
the last one?
No, I just picked an arbitrary place. Sure I'll move it to the end.
Luis