Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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 -- 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/
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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 -- 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/
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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 == _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,
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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 > > > --- > > > 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 :( -- 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/
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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 -- 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/
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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 -- 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/
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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.dide...@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 :( -- 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/
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 --- 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 == _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
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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 > > --- > > 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 -- 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/
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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 > --- > 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 -- 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/
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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.dide...@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 -- 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/
Re: [PATCH] sysfs: group: allow is_visible to drop permissions
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.dide...@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 -- 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/
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.dide...@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