[PATCH] sysfs: group: allow is_visible to drop permissions

[PATCH] sysfs: group: allow is_visible to drop permissions

[Vivien Didelot]

Using the optional is_visible function, it is actually possible to
either hide an attribute, or add a new permission, but not remove one.

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.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.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);
 			}
 			error = sysfs_add_file_mode_ns(parent, *attr, false,
-						       (*attr)->mode | mode,
-						       NULL);
+						       mode, NULL);
 			if (unlikely(error))
 				break;
 		}
-- 
2.2.2

Re: [PATCH] sysfs: group: allow is_visible to drop permissions

[Greg Kroah-Hartman]

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?

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

> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.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?

thanks,

greg k-h

Re: [PATCH] sysfs: group: allow is_visible to drop permissions

[Vivien Didelot]

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?

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?

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

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.

thanks,

-v

Re: [PATCH] sysfs: group: allow is_visible to drop permissions

[Guenter Roeck]

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

Re: [PATCH] sysfs: group: allow is_visible to drop permissions

[Greg Kroah-Hartman]

On Fri, Jan 16, 2015 at 07:22:25PM -0500, 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?
> 
> 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.

Sorry, I meant to say, "what code wants to remove permissions".

> > > 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?
> 
> 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@savoirfairelinux.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.

You somehow ignored this comment :(

Re: [PATCH] sysfs: group: allow is_visible to drop permissions

[Vivien Didelot]

Hi Guenter, Greg,

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

BTW Guenter, does this patch make sense to you?

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

Sorry Greg this wasn't clear, I kinda group the reply below. Here it is:

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

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

I said "usually" because I gave a look at some is_visible functions in
drivers/ (but not all) and noted this usage. And "should not" because
I'm not 100% sure since I didn't go through all is_visible functions as
you said.

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

This is exactly my point. this code expects to remove the write
permission since change_queue_depth is not provided, but this will never
happen.

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

Yes, there are other bits like S_ISUID, S_ISGID, S_ISVTX (see
include/linux/stat.h) and some internal usage bits like SYSFS_PREALLOC
(see include/linux/sysfs.h).

My assumption here was that the attribute group is_visible function
should just be able to adjust the UGO bits. Am I correct?

I'm not even sure about the execute permission though. Only one driver
uses it for an attribute and it seems wrong, in drivers/hid/hid-lg4ff.c:

static DEVICE_ATTR(range, S_IRWXU | S_IRWXG | S_IROTH, lg4ff_range_show, lg4ff_range_store);

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

Indeed, this can be 2 patches here.

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

If you return ~S_IRWXUGO now, the file is visible, with the default
attribute mode. If you return ~S_IRWXUGO with this patch, the file is
not visible.

The actual behavior seems wrong to me. Again, what happens is you return
SYSFS_PREALLOC, that the underlying sysfs_add_file_mode_ns() function is
actually checking?

IMHO, if we want an attribute group to only be able to "hide or show" an
attribute, then is_visible (as the name suggests) should return a
boolean. If we want it be able to adjust permissions (as it seems
correct, given the examples), we should identify which permissions are
OK to change, deprecate is_visible function (to avoid code break) in
favor of a new one which limits the bits to that scope.

Thanks,
-v

Re: [PATCH] sysfs: group: allow is_visible to drop permissions

[Guenter Roeck]

On 01/17/2015 02:09 PM, Vivien Didelot wrote:
> Hi Guenter, Greg,
>
[ .. ]

>
> BTW Guenter, does this patch make sense to you?
>

It does make sense to me to only use the return value from is_visible
for the mode.

As for which bits to use, I am not entirely sure. I think it would be
more important to first decide which bits should be acceptable to start with.

Then I would _always_ only use the bits from mode, masked against the
valid bits, whatever they are.

	umode_t mode = (*attr)->mode;
	...
	if (grp->is_visible) {
		mode = grp->is_visible(kobj, *attr, i);
		if (!mode)
			continue;
	}

	WARN(mode & ~(S_IRUGO | S_IWUGO | SYSFS_PREALLOC),	/* optional */
	     "Attribute %s: Invalid permission 0x%x\n", (*attr)->name, mode);

	mode &= S_IRUGO | S_IWUGO | SYSFS_PREALLOC;
	error = sysfs_add_file_mode_ns(parent, *attr, false, mode, NULL);
	...

>
> My assumption here was that the attribute group is_visible function
> should just be able to adjust the UGO bits. Am I correct?
>
I would think so.

> I'm not even sure about the execute permission though. Only one driver
> uses it for an attribute and it seems wrong, in drivers/hid/hid-lg4ff.c:
>
> static DEVICE_ATTR(range, S_IRWXU | S_IRWXG | S_IROTH, lg4ff_range_show, lg4ff_range_store);
>
That seems wrong.

>
> The actual behavior seems wrong to me. Again, what happens is you return
> SYSFS_PREALLOC, that the underlying sysfs_add_file_mode_ns() function is
> actually checking?
>
Ultimately, the implementor asked for it.

> IMHO, if we want an attribute group to only be able to "hide or show" an
> attribute, then is_visible (as the name suggests) should return a
> boolean. If we want it be able to adjust permissions (as it seems
> correct, given the examples), we should identify which permissions are
> OK to change, deprecate is_visible function (to avoid code break) in
> favor of a new one which limits the bits to that scope.
>

Up to Greg to decide. From my perspective, we have lived with is_visible
for several years and overall it seems to work. Sure, it lacks a clear
API, but that can be fixed without changing a lot of code just to replace
the function name.

Guenter

Re: [PATCH] sysfs: group: allow is_visible to drop permissions

[Greg Kroah-Hartman]

On Sat, Jan 17, 2015 at 05:45:15PM -0800, Guenter Roeck wrote:
> On 01/17/2015 02:09 PM, Vivien Didelot wrote:
> >Hi Guenter, Greg,
> >
> [ .. ]
> 
> >
> >BTW Guenter, does this patch make sense to you?
> >
> 
> It does make sense to me to only use the return value from is_visible
> for the mode.
> 
> As for which bits to use, I am not entirely sure. I think it would be
> more important to first decide which bits should be acceptable to start with.
> 
> Then I would _always_ only use the bits from mode, masked against the
> valid bits, whatever they are.
> 
> 	umode_t mode = (*attr)->mode;
> 	...
> 	if (grp->is_visible) {
> 		mode = grp->is_visible(kobj, *attr, i);
> 		if (!mode)
> 			continue;
> 	}
> 
> 	WARN(mode & ~(S_IRUGO | S_IWUGO | SYSFS_PREALLOC),	/* optional */
> 	     "Attribute %s: Invalid permission 0x%x\n", (*attr)->name, mode);
> 
> 	mode &= S_IRUGO | S_IWUGO | SYSFS_PREALLOC;
> 	error = sysfs_add_file_mode_ns(parent, *attr, false, mode, NULL);
> 	...
> 
> >
> >My assumption here was that the attribute group is_visible function
> >should just be able to adjust the UGO bits. Am I correct?
> >
> I would think so.
> 
> >I'm not even sure about the execute permission though. Only one driver
> >uses it for an attribute and it seems wrong, in drivers/hid/hid-lg4ff.c:
> >
> >static DEVICE_ATTR(range, S_IRWXU | S_IRWXG | S_IROTH, lg4ff_range_show, lg4ff_range_store);
> >
> That seems wrong.
> 
> >
> >The actual behavior seems wrong to me. Again, what happens is you return
> >SYSFS_PREALLOC, that the underlying sysfs_add_file_mode_ns() function is
> >actually checking?
> >
> Ultimately, the implementor asked for it.
> 
> >IMHO, if we want an attribute group to only be able to "hide or show" an
> >attribute, then is_visible (as the name suggests) should return a
> >boolean. If we want it be able to adjust permissions (as it seems
> >correct, given the examples), we should identify which permissions are
> >OK to change, deprecate is_visible function (to avoid code break) in
> >favor of a new one which limits the bits to that scope.
> >
> 
> Up to Greg to decide. From my perspective, we have lived with is_visible
> for several years and overall it seems to work. Sure, it lacks a clear
> API, but that can be fixed without changing a lot of code just to replace
> the function name.

If someone wants to submit a "cleaner" patch, I'm always willing to
review it, but the one submitted here I can't take for the reasons I
gave at the least.

thanks,

greg k-h