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