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

[Guenter Roeck <linux@roeck-us.net>]

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