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