Message-ID: <54B9C05E.1020309@roeck-us.net>
Date:	Fri, 16 Jan 2015 17:52:30 -0800
From:	Guenter Roeck <linux@...ck-us.net>
To:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	linux-kernel@...r.kernel.org, kernel@...oirfairelinux.com
Subject: Re: [PATCH] sysfs: group: allow is_visible to drop permissions

On 01/16/2015 04:22 PM, Vivien Didelot wrote:
> Hi Greg,
>
>> On Fri, Jan 16, 2015 at 04:29:10PM -0500, Vivien Didelot wrote:
>>> Using the optional is_visible function, it is actually possible to
>>> either hide an attribute, or add a new permission, but not remove
>>> one.
>>
>> What code wants to remove attributes?
>
Actually, that can happen and is supported (if is_visible returns 0
when updating an attribute group).

> Sorry, I meant removing a permission. Actually, drivers hide attributes
> (if a feature isn't supported by a device) by returning 0 in is_visible.
>
> E.g, if we consider a read-only attribute, a driver can add the write
> permission by returning S_IWUSR, but removing S_IRUGO isn't possible.
>
>>> This commit uses all the UGO bits returned by is_visible instead of
>>> OR'ing them with the default attribute mode.
>>>
>>> Concretely, this allows a driver to use macros like DEVICE_ATTR_RW
>>> to
>>> set the attribute show and store functions and remove the S_IWUSR
>>> permission in is_visible if the implementation doesn't provide a
>>> setter.
>>
>> What bus wants to do this?
>
Every driver which has an attribute which is not always writable.
The scsi code fragment I copied below would be another example.

> Some high level frameworks such as DSA. My motivation behind this was to
> clarify how modes are set for temperature attributes in DSA. Optional
> functions can be provided by switch drivers to get temperatures or set
> limits. I hope the following patch helps clarifying this point:
> https://gist.github.com/vivien/72734ba0673ad0b79a6b
>
> (I Cc: Guenter as he is the author of NET_DSA_HWMON, see 51579c3).
>
>>> Signed-off-by: Vivien Didelot <vivien.didelot@...oirfairelinux.com>
>>> ---
>>>   fs/sysfs/group.c | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
>>> index 7d2a860..a8cfe03 100644
>>> --- a/fs/sysfs/group.c
>>> +++ b/fs/sysfs/group.c
>>> @@ -41,7 +41,7 @@ static int create_files(struct kernfs_node
>>> *parent, struct kobject *kobj,
>>>
>>>   	if (grp->attrs) {
>>>   		for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
>>> -			umode_t mode = 0;
>>> +			umode_t mode = (*attr)->mode;
>>>
>>>   			/*
>>>   			 * In update mode, we're changing the permissions or
>>> @@ -51,13 +51,15 @@ static int create_files(struct kernfs_node
>>> *parent, struct kobject *kobj,
>>>   			if (update)
>>>   				kernfs_remove_by_name(parent, (*attr)->name);
>>>   			if (grp->is_visible) {
>>> -				mode = grp->is_visible(kobj, *attr, i);
>>> -				if (!mode)
>>> +				umode_t ugo = grp->is_visible(kobj, *attr, i);
>>> +
>>> +				if (!(ugo & S_IRWXUGO))
>>>   					continue;
>>> +
>>> +				mode = (mode & ~S_IRWXUGO) | (ugo & S_IRWXUGO);
>>
>> Please document what you are doing here in the code, it's not obvious
>> at first glance.
>>
>>>   			}
>>>   			error = sysfs_add_file_mode_ns(parent, *attr, false,
>>> -						       (*attr)->mode | mode,
>>> -						       NULL);
>>> +						       mode, NULL);
>>
>> Any chance this is going to break existing code that isn't expecting
>> this type of change in functionality?
>
> Usually, drivers return 0 to hide the attribute, or the attr->mode to
> show it. This change should not break this expectation.
>

I am a bit concerned with 'Usually' and 'should not'. While you are correct,
the only way to know for sure would be to go through all is_visible functions
and make sure. And we don't really know why the original commit (0f4238958d)
chose to use "(*attr)->mode | mode" instead of just mode.

In this context, it is interesting that the scsi code, for which is_visible
was changed to return umode_t, appears to use it in a way that doesn't work.
The following code fragment is from drivers/scsi/scsi_sysfs.c.

static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth,
                    sdev_store_queue_depth);
...
         if (attr == &dev_attr_queue_depth.attr &&
             !sdev->host->hostt->change_queue_depth)
                 return S_IRUGO;

Oops ...

Given that, one could argue that the change to just use the return value
of is_visible may cause some trouble with lost permissions here or there,
but that it is already used in a wrong way and therefore _should_ be
changed.

> In the meantime, as the returned value is OR'ed with the actual mode,
> I'm wondering if a driver can break anything by returning, let's say ~0?
>
> That's why I kept the eventual extra bits from the attribute mode and OR
> them with only the UGO bits from the return of is_visible, similar to
> what sysfs_chmod_file() does.
>
Are there mode flags which have bits other than S_IRWXUGO set, or is that
another assumption ? If there are, why would or should is_visible be limited
to the S_IRWXUGO bits ?

So ultimately you have two semantic changes: One to limit the scope of
is_valid to S_IRWXUGO, and one to only use the bits from is_visible
within that scope, but keep using the other bits from the original mode.
Plus, returning ~S_IRWXUGO from asn in_visible function now results in
the file not being visible at all. Wonder if that should be more than
one patch, and if the changes should be discussed separately.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists