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

Reply via email to