[net-next,v3,4/9] sysfs: add sysfs_change_owner()
diff mbox series

Message ID 20200218162943.2488012-5-christian.brauner@ubuntu.com
State Superseded, archived
Headers show
Series
  • net: fix sysfs permssions when device changes network
Related show

Commit Message

Christian Brauner Feb. 18, 2020, 4:29 p.m. UTC
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(+)

Comments

Greg Kroah-Hartman Feb. 20, 2020, 11:23 a.m. UTC | #1
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
Christian Brauner Feb. 20, 2020, 8:13 p.m. UTC | #2
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

Patch
diff mbox series

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)