| Message ID | 20200218162943.2488012-2-christian.brauner@ubuntu.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | net: fix sysfs permssions when device changes network | expand |
On Tue, Feb 18, 2020 at 05:29:35PM +0100, Christian Brauner wrote: > Add helpers to change the owner of a sysfs files. > This function will be used to correctly account for kobject ownership > changes, e.g. when moving network devices between network namespaces. > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Feb 18, 2020 at 05:29:35PM +0100, Christian Brauner wrote: > Add helpers to change the owner of a sysfs files. > This function will be used to correctly account for kobject ownership > changes, e.g. when moving network devices between network namespaces. > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > --- > /* v2 */ > - Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > - Better naming for sysfs_file_change_owner() to reflect the fact that it > can be used to change the owner of the kobject itself by passing NULL as > argument. > - Christian Brauner <christian.brauner@ubuntu.com>: > - Split sysfs_file_change_owner() into two helpers sysfs_change_owner() and > sysfs_change_owner_by_name(). The former changes the owner of the kobject > itself, the latter the owner of the kobject looked up via the name > argument. > > /* v3 */ > - Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > - Add explicit uid/gid parameters. Looks much better, thanks for doing these changes. I'll review the whole series now... greg k-h
On Tue, Feb 18, 2020 at 05:29:35PM +0100, Christian Brauner wrote: > +/** > + * sysfs_file_change_owner - change owner of a file. > + * @kobj: object. > + * @kuid: new owner's kuid > + * @kgid: new owner's kgid > + */ > +int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid) > +{ > + struct kernfs_node *kn; > + int error; > + > + if (!kobj->state_in_sysfs) > + return -EINVAL; > + > + kernfs_get(kobj->sd); > + > + kn = kobj->sd; > + error = internal_change_owner(kn, kobj, kuid, kgid); > + > + kernfs_put(kn); > + > + return error; > +} > +EXPORT_SYMBOL_GPL(sysfs_file_change_owner); Oops, wait, what "file" are you changing here? You aren't changing the kobject's attributes, but rather a file in the kobject's directory, right? But kobj->sd is the directory of the kobject itself, so why isn't this function just the same thing as sysfs_change_owner()? Why would you call this function at all? confused, greg k-h
On Thu, Feb 20, 2020 at 12:20:49PM +0100, Greg Kroah-Hartman wrote: > On Tue, Feb 18, 2020 at 05:29:35PM +0100, Christian Brauner wrote: > > +/** > > + * sysfs_file_change_owner - change owner of a file. > > + * @kobj: object. > > + * @kuid: new owner's kuid > > + * @kgid: new owner's kgid > > + */ > > +int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid) > > +{ > > + struct kernfs_node *kn; > > + int error; > > + > > + if (!kobj->state_in_sysfs) > > + return -EINVAL; > > + > > + kernfs_get(kobj->sd); > > + > > + kn = kobj->sd; > > + error = internal_change_owner(kn, kobj, kuid, kgid); > > + > > + kernfs_put(kn); > > + > > + return error; > > +} > > +EXPORT_SYMBOL_GPL(sysfs_file_change_owner); > > Oops, wait, what "file" are you changing here? You aren't changing the > kobject's attributes, but rather a file in the kobject's directory, > right? But kobj->sd is the directory of the kobject itself, so why > isn't this function just the same thing as sysfs_change_owner()? I've moved it directly into sysfs_change_owner(), removed the function, and renamed "_by_name()" back to sysfs_file_change_owner() which is easier to parse and makes more sense.
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index 130fc6fbcc03..32bb04b4d9d9 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -558,3 +558,70 @@ void sysfs_remove_bin_file(struct kobject *kobj, kernfs_remove_by_name(kobj->sd, attr->attr.name); } EXPORT_SYMBOL_GPL(sysfs_remove_bin_file); + +static int internal_change_owner(struct kernfs_node *kn, struct kobject *kobj, + kuid_t kuid, kgid_t kgid) +{ + struct iattr newattrs = { + .ia_valid = ATTR_UID | ATTR_GID, + .ia_uid = kuid, + .ia_gid = kgid, + }; + return kernfs_setattr(kn, &newattrs); +} + +/** + * sysfs_file_change_owner_by_name - change owner of a file. + * @kobj: object. + * @name: name of the file to change. + * @kuid: new owner's kuid + * @kgid: new owner's kgid + */ +int sysfs_file_change_owner_by_name(struct kobject *kobj, const char *name, + kuid_t kuid, kgid_t kgid) +{ + struct kernfs_node *kn; + int error; + + if (!name) + return -EINVAL; + + if (!kobj->state_in_sysfs) + return -EINVAL; + + kn = kernfs_find_and_get(kobj->sd, name); + if (!kn) + return -ENOENT; + + error = internal_change_owner(kn, kobj, kuid, kgid); + + kernfs_put(kn); + + return error; +} +EXPORT_SYMBOL_GPL(sysfs_file_change_owner_by_name); + +/** + * sysfs_file_change_owner - change owner of a file. + * @kobj: object. + * @kuid: new owner's kuid + * @kgid: new owner's kgid + */ +int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid) +{ + struct kernfs_node *kn; + int error; + + if (!kobj->state_in_sysfs) + return -EINVAL; + + kernfs_get(kobj->sd); + + kn = kobj->sd; + error = internal_change_owner(kn, kobj, kuid, kgid); + + kernfs_put(kn); + + return error; +} +EXPORT_SYMBOL_GPL(sysfs_file_change_owner); diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index fa7ee503fb76..c11d11c78713 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -310,6 +310,10 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn) return kernfs_enable_ns(kn); } +int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid); +int sysfs_file_change_owner_by_name(struct kobject *kobj, const char *name, + kuid_t kuid, kgid_t kgid); + #else /* CONFIG_SYSFS */ static inline int sysfs_create_dir_ns(struct kobject *kobj, const void *ns) @@ -522,6 +526,19 @@ static inline void sysfs_enable_ns(struct kernfs_node *kn) { } +static inline int int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, + kgid_t kgid) +{ + return 0; +} + +static inline int sysfs_file_change_owner_by_name(struct kobject *kobj, + const char *name, kuid_t kuid, + kgid_t kgid) +{ + return 0; +} + #endif /* CONFIG_SYSFS */ static inline int __must_check sysfs_create_file(struct kobject *kobj,
Add helpers to change the owner of a sysfs files. This function will be used to correctly account for kobject ownership changes, e.g. when moving network devices between network namespaces. Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- /* v2 */ - Greg Kroah-Hartman <gregkh@linuxfoundation.org>: - Better naming for sysfs_file_change_owner() to reflect the fact that it can be used to change the owner of the kobject itself by passing NULL as argument. - Christian Brauner <christian.brauner@ubuntu.com>: - Split sysfs_file_change_owner() into two helpers sysfs_change_owner() and sysfs_change_owner_by_name(). The former changes the owner of the kobject itself, the latter the owner of the kobject looked up via the name argument. /* v3 */ - Greg Kroah-Hartman <gregkh@linuxfoundation.org>: - Add explicit uid/gid parameters. --- fs/sysfs/file.c | 67 +++++++++++++++++++++++++++++++++++++++++++ include/linux/sysfs.h | 17 +++++++++++ 2 files changed, 84 insertions(+)