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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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