On Fri, Jun 15, 2018 at 06:29:10PM -0700, Rajat Jain wrote: > There are a couple of problems with named group updates in the code > today: > > * sysfs_update_group() will always fail for a named group, because > internal_create_group() will try to create a new sysfs directory > unconditionally, which will ofcourse fail with -EEXIST. > > * We can leak the kernfs_node for grp->name if some one tries to: > - rename a group (change grp->name), or > - update a named group, to an unnamed group > > It appears that the whole purpose of sysfs_update_group() was to > allow changing the permissions or visibility of attributes and not > the names. So make it clear in the comments, and allow it to update > an existing named group.
Who uses sysfs_update_group() today that has these problems? Or do you want to use it in new code? How can it be broken today so badly that it does not work? > Signed-off-by: Rajat Jain <raja...@google.com> > --- > fs/sysfs/group.c | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c > index 4802ec0e1e3a..8bd10dc730ae 100644 > --- a/fs/sysfs/group.c > +++ b/fs/sysfs/group.c > @@ -119,12 +119,23 @@ static int internal_create_group(struct kobject *kobj, > int update, > return -EINVAL; > } > if (grp->name) { > - kn = kernfs_create_dir(kobj->sd, grp->name, > - S_IRWXU | S_IRUGO | S_IXUGO, kobj); > - if (IS_ERR(kn)) { > - if (PTR_ERR(kn) == -EEXIST) > - sysfs_warn_dup(kobj->sd, grp->name); > - return PTR_ERR(kn); > + if (update) { > + kn = kernfs_find_and_get(kobj->sd, grp->name); > + if (!kn) { > + WARN(1, > + "Can't update unknown attr grp name: > %s/%s\n", > + kobj->name, grp->name); > + return -EINVAL; This is going to cause the syzbot to bug the heck out of us, as people do run with panic-on-warning. Just make this a "normal" error message and dump the stack if you want that. But maybe we should just get rid of this function entirely, it feels very ackward and I can't remember why we added it... thanks, greg k-h