Re: [PATCH] sysfs: Fix internal_create_group() for named group updates

[Greg Kroah-Hartman]

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 <rajatja@xxxxxxxxxx>
> ---
>  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