Hi Guenter, > For sysfs file attributes, only read and write permisssions make sense.
Minor typo, there's an extra 's' to permissions. > Mask provided attribute permissions accordingly and send a warning > to the console if invalid permission bits are set. > > Cc: Vivien Didelot <[email protected]> > Signed-off-by: Guenter Roeck <[email protected]> > --- > fs/sysfs/group.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c > index 305eccb..0de6473 100644 > --- a/fs/sysfs/group.c > +++ b/fs/sysfs/group.c > @@ -55,6 +55,12 @@ static int create_files(struct kernfs_node *parent, struct > kobject *kobj, > if (!mode) > continue; > } > + > + WARN(mode & ~(S_IRUGO | S_IWUGO | SYSFS_PREALLOC), > + "Attribute %s: Invalid permission 0x%x\n", > + (*attr)->name, mode); To print permissions, I would suggest unsigned octal ("0%o"). > + > + mode &= S_IRUGO | S_IWUGO | SYSFS_PREALLOC; As readable attributes are created with S_IRUGO and writable attributes are created with S_IWUSR, I would limit the scope of is_visible to only: S_IRUGO | S_IWUSR. Write permission for group and others feels wrong. Then, I think we may want to keep the extra bits (all mode bits > 0777) from the default attribute mode. Can they be used for sysfs attributes? My suggestion is something like this: /* Limit the scope to S_IRUGO | S_IWUSR */ if (mode & ~(S_IRUGO | S_IWUSR)) pr_warn("Attribute %s: Invalid permissions 0%o\n", (*attr)->name, mode); mode &= S_IRUGO | S_IWUSR; /* Use only returned bits and defaults > 0777 */ mode |= (*attr)->mode & ~S_IRWXUGO; > error = sysfs_add_file_mode_ns(parent, *attr, false, > mode, NULL); > if (unlikely(error)) The code hitting this warning actually is drivers/pci/pci-sysfs.c, which declares write-only attributes with S_IWUSR|S_IWGRP (0220). Is that correct to have write access for group for these attributes? Thanks, -v -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
