| Message ID | 20200218162943.2488012-5-christian.brauner@ubuntu.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: fix sysfs permssions when device changes network | expand |
On Tue, Feb 18, 2020 at 05:29:38PM +0100, Christian Brauner wrote: > Add a helper to change the owner of sysfs objects. > 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>: > - Add comment how ownership of sysfs object is changed. > > /* v3 */ > - Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > - Add explicit uid/gid parameters. > --- > fs/sysfs/file.c | 39 +++++++++++++++++++++++++++++++++++++++ > include/linux/sysfs.h | 6 ++++++ > 2 files changed, 45 insertions(+) > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c > index df5107d7b3fd..02f7e852aad4 100644 > --- a/fs/sysfs/file.c > +++ b/fs/sysfs/file.c > @@ -665,3 +665,42 @@ int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid) > return error; > } > EXPORT_SYMBOL_GPL(sysfs_file_change_owner); > + > +/** > + * sysfs_change_owner - change owner of the given object. "and all of the files associated with this kobject", right? > + * @kobj: object. > + * @kuid: new owner's kuid > + * @kgid: new owner's kgid > + */ > +int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid) > +{ > + int error; > + const struct kobj_type *ktype; > + > + if (!kobj->state_in_sysfs) > + return -EINVAL; > + > + error = sysfs_file_change_owner(kobj, kuid, kgid); Ok, this changes the attributes of the sysfs directory for the kobject itself. > + if (error) > + return error; > + > + ktype = get_ktype(kobj); > + if (ktype) { > + struct attribute **kattr; > + > + for (kattr = ktype->default_attrs; kattr && *kattr; kattr++) { > + error = sysfs_file_change_owner_by_name( > + kobj, (*kattr)->name, kuid, kgid); > + if (error) > + return error; > + } And here you change all of the files of the kobject. But what about files that have a subdir? Does that also happen here? > + > + error = sysfs_groups_change_owner(kobj, ktype->default_groups, > + kuid, kgid); Then what are you changing here? I think the kerneldoc needs a lot more explaination as to what is going on in this function and why you would call it, and not some of the other functions you are adding. thanks, greg k-h
On Thu, Feb 20, 2020 at 12:23:14PM +0100, Greg Kroah-Hartman wrote: > On Tue, Feb 18, 2020 at 05:29:38PM +0100, Christian Brauner wrote: > > Add a helper to change the owner of sysfs objects. > > 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>: > > - Add comment how ownership of sysfs object is changed. > > > > /* v3 */ > > - Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > > - Add explicit uid/gid parameters. > > --- > > fs/sysfs/file.c | 39 +++++++++++++++++++++++++++++++++++++++ > > include/linux/sysfs.h | 6 ++++++ > > 2 files changed, 45 insertions(+) > > > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c > > index df5107d7b3fd..02f7e852aad4 100644 > > --- a/fs/sysfs/file.c > > +++ b/fs/sysfs/file.c > > @@ -665,3 +665,42 @@ int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid) > > return error; > > } > > EXPORT_SYMBOL_GPL(sysfs_file_change_owner); > > + > > +/** > > + * sysfs_change_owner - change owner of the given object. > > "and all of the files associated with this kobject", right? > > > + * @kobj: object. > > + * @kuid: new owner's kuid > > + * @kgid: new owner's kgid > > + */ > > +int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid) > > +{ > > + int error; > > + const struct kobj_type *ktype; > > + > > + if (!kobj->state_in_sysfs) > > + return -EINVAL; > > + > > + error = sysfs_file_change_owner(kobj, kuid, kgid); > > Ok, this changes the attributes of the sysfs directory for the kobject > itself. Yes. > > > + if (error) > > + return error; > > + > > + ktype = get_ktype(kobj); > > + if (ktype) { > > + struct attribute **kattr; > > + > > + for (kattr = ktype->default_attrs; kattr && *kattr; kattr++) { > > + error = sysfs_file_change_owner_by_name( > > + kobj, (*kattr)->name, kuid, kgid); > > + if (error) > > + return error; > > + } > > And here you change all of the files of the kobject. This changes the default attributes associated with that ktype (if any) and mirrors how a kobject is registered in sysfs. > > But what about files that have a subdir? Does that also happen here? Maybe that was all to brief on my end, sorry: So all of this mirrors how a kobject is added through driver core which in its guts is done via kobject_add_internal() which in summary does: - create the main directory via create_dir() - populate the directory with the groups associated with that ktype (if any) - populate the directory with the basic attributes associated with that ktype (if any) These are the basic steps that are associated with adding a kobject in sysfs. Any additional properties are added by the specific subsystem itself (not by driver core) after it has registered the device. So for the example of network devices, a network device will e.g. register a queue subdirectory under the basic sysfs directory for the network device and than further subdirectories within that queues subdirectory. But that is all specific to network devices and they call the corresponding sysfs functions to do that directly when they create those queue objects. So anything that a subsystem adds outside of what driver core does must also be changed by them (That's already true for removal of files it created outside of driver core.) and it's the same for ownership changes. :) I'll document that. > > > + > > + error = sysfs_groups_change_owner(kobj, ktype->default_groups, > > + kuid, kgid); > > Then what are you changing here? So this changes the default groups associated with the ktype for that kobject and again mirrors how a kobject is registered in sysfs. > > I think the kerneldoc needs a lot more explaination as to what is going > on in this function and why you would call it, and not some of the other > functions you are adding. Will do for sure. Thanks! Christian
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index df5107d7b3fd..02f7e852aad4 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -665,3 +665,42 @@ int sysfs_file_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid) return error; } EXPORT_SYMBOL_GPL(sysfs_file_change_owner); + +/** + * sysfs_change_owner - change owner of the given object. + * @kobj: object. + * @kuid: new owner's kuid + * @kgid: new owner's kgid + */ +int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid) +{ + int error; + const struct kobj_type *ktype; + + if (!kobj->state_in_sysfs) + return -EINVAL; + + error = sysfs_file_change_owner(kobj, kuid, kgid); + if (error) + return error; + + ktype = get_ktype(kobj); + if (ktype) { + struct attribute **kattr; + + for (kattr = ktype->default_attrs; kattr && *kattr; kattr++) { + error = sysfs_file_change_owner_by_name( + kobj, (*kattr)->name, kuid, kgid); + if (error) + return error; + } + + error = sysfs_groups_change_owner(kobj, ktype->default_groups, + kuid, kgid); + if (error) + return error; + } + + return 0; +} +EXPORT_SYMBOL_GPL(sysfs_change_owner); diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index 564a2e57b90a..fa1a37dd0f2b 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -313,6 +313,7 @@ static inline void sysfs_enable_ns(struct kernfs_node *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); +int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid); int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ, const char *name, kuid_t kuid, kgid_t kgid); int sysfs_groups_change_owner(struct kobject *kobj, @@ -555,6 +556,11 @@ static inline int sysfs_link_change_owner(struct kobject *kobj, return 0; } +static inline int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid) +{ + return 0; +} + static inline int sysfs_groups_change_owner(struct kobject *kobj, const struct attribute_group **groups, kuid_t kuid, kgid_t kgid)
Add a helper to change the owner of sysfs objects. 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>: - Add comment how ownership of sysfs object is changed. /* v3 */ - Greg Kroah-Hartman <gregkh@linuxfoundation.org>: - Add explicit uid/gid parameters. --- fs/sysfs/file.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/sysfs.h | 6 ++++++ 2 files changed, 45 insertions(+)