Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 16 Jun 2018 09:11:08 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Rajat Jain <rajatja@google.com>
Cc:     linux-kernel@vger.kernel.org, rajatxjain@gmail.com
Subject: Re: [PATCH] sysfs: Fix internal_create_group() for named group
 updates
Message-ID: <20180616071108.GB30558@kroah.com>
References: <20180616012910.152694-1-rajatja@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180616012910.152694-1-rajatja@google.com>
User-Agent: Mutt/1.10.0 (2018-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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