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

[Rajat Jain]

Hi Greg,

Thanks for your review.

On Sat, Jun 16, 2018 at 12:11 AM, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> 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?

Sorry, my bad, I need to provide more clarification: None of the
existing users of this API use it on a named attribute group, thus
will not see this issue. I hit this issue while writing new code
(However, since then my code logic has changed so that it does not
need to use this API anymore).

I'm just trying to fix an issue with this API, that I stumbled upon,
which might be seen in future by any new code that uses this API with
named attribute groups. At the minimum, I think we should clarify in
comments (or better yet, ensure) that this API cannot be misused.


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

Agreed, I will turn into a normal error.

>
> But maybe we should just get rid of this function entirely, it feels
> very ackward and I can't remember why we added it...

There are a couple of existing in-tree users of this API, and I do not
understand a lot about that code.

Thanks & Best Regards,

Rajat

>
> thanks,
>
> greg k-h