diff mbox series

[net-next,05/10] sysfs: add sysfs_change_owner()

Message ID 20200212104321.43570-6-christian.brauner@ubuntu.com
State Deferred
Delegated to: David Miller
Headers show
Series net: fix sysfs permssions when device changes network | expand

Commit Message

Christian Brauner Feb. 12, 2020, 10:43 a.m. UTC
Add a helper to change the owner of sysfs objects.
The ownership of a sysfs object is determined based on the ownership of
the corresponding kobject, i.e. only if the ownership of a kobject is
changed will this function change the ownership of the corresponding
sysfs entry.
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>
---
 fs/sysfs/file.c       | 35 +++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  6 ++++++
 2 files changed, 41 insertions(+)

Comments

Greg Kroah-Hartman Feb. 12, 2020, 1:18 p.m. UTC | #1
On Wed, Feb 12, 2020 at 11:43:16AM +0100, Christian Brauner wrote:
> Add a helper to change the owner of sysfs objects.

Seems sane, but:

> The ownership of a sysfs object is determined based on the ownership of
> the corresponding kobject, i.e. only if the ownership of a kobject is
> changed will this function change the ownership of the corresponding
> sysfs entry.

A "sysfs object" is a kobject.  So I don't understand this sentance,
sorry.

> 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>
> ---
>  fs/sysfs/file.c       | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h |  6 ++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 6239d9584f0b..6a0fe88061fd 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -642,3 +642,38 @@ int sysfs_file_change_owner(struct kobject *kobj, const char *name)
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
> +
> +/**
> + *	sysfs_change_owner - change owner of the given object.
> + *	@kobj:	object.
> + */
> +int sysfs_change_owner(struct kobject *kobj)

What does this change the owner of the given object _to_?

> +{
> +	int error;
> +	const struct kobj_type *ktype;
> +
> +	if (!kobj->state_in_sysfs)
> +		return -EINVAL;
> +
> +	error = sysfs_file_change_owner(kobj, NULL);

It passes NULL?


> +	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(kobj, (*kattr)->name);
> +			if (error)
> +				return error;
> +		}
> +
> +		error = sysfs_groups_change_owner(kobj, ktype->default_groups);
> +		if (error)
> +			return error;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(sysfs_change_owner);

I can understand wanting to change owners/groups/whatever of existing
sysfs objects and their files, but I can't figure out how to call this
function to set the attribute I want to change.

With only one parameter, how does this work?  It guesses?  :)

thanks,

greg k-h
Christian Brauner Feb. 12, 2020, 3:07 p.m. UTC | #2
On Wed, Feb 12, 2020 at 05:18:08AM -0800, Greg Kroah-Hartman wrote:
> On Wed, Feb 12, 2020 at 11:43:16AM +0100, Christian Brauner wrote:
> > Add a helper to change the owner of sysfs objects.
> 
> Seems sane, but:
> 
> > The ownership of a sysfs object is determined based on the ownership of
> > the corresponding kobject, i.e. only if the ownership of a kobject is
> > changed will this function change the ownership of the corresponding
> > sysfs entry.
> 
> A "sysfs object" is a kobject.  So I don't understand this sentance,
> sorry.

I meant that only if you change the uid/gid the underlying kobject is
associated with will this function do anything, meaning that you can't
pass in uids/gids directly. I'll explain why I did this down below [1].
Sorry if that was confusing.

> 
> > 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>
> > ---
> >  fs/sysfs/file.c       | 35 +++++++++++++++++++++++++++++++++++
> >  include/linux/sysfs.h |  6 ++++++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > index 6239d9584f0b..6a0fe88061fd 100644
> > --- a/fs/sysfs/file.c
> > +++ b/fs/sysfs/file.c
> > @@ -642,3 +642,38 @@ int sysfs_file_change_owner(struct kobject *kobj, const char *name)
> >  	return error;
> >  }
> >  EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
> > +
> > +/**
> > + *	sysfs_change_owner - change owner of the given object.
> > + *	@kobj:	object.
> > + */
> > +int sysfs_change_owner(struct kobject *kobj)
> 
> What does this change the owner of the given object _to_?

[1]:
So ownership only changes if the kobject's uid/gid have been changed.
So when to stick with the networking example, when a network device is
moved into a new network namespace, the uid/gid of the kobject will be
changed to the root user of the owning user namespace of that network
namespace. So when the move of the network device has completed and
kobject_get_ownership() is called it will now return a different
uid/gid.
So my reasoning was that ownership is determined dynamically that way. I
guess what you're hinting at is that we could simply add uid_t uid,
gid_t gid arguments to these sysfs helpers. That's fine with me too. It
means that callers are responsible to either retrieve the ownership from
the kobject (in case it was changed through another call) or the call to
syfs_change_owner(kobj, uid, gid) sets the new owner of the kobject. I
don't know what the best approach is. Maybe a hybrid whereby we allow
passing in uid/gid but also allow passing in ({g,u}id_t - 1) to indicate
that we want the ownership to be taken from the kobject itself (e.g.
when a network device has been updated by dev_change_net_namespace()).

> 
> > +{
> > +	int error;
> > +	const struct kobj_type *ktype;
> > +
> > +	if (!kobj->state_in_sysfs)
> > +		return -EINVAL;
> > +
> > +	error = sysfs_file_change_owner(kobj, NULL);
> 
> It passes NULL?

Which means, change the ownership of "kobj" itself and not lookup a file
relative to "kobj".

> 
> 
> > +	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(kobj, (*kattr)->name);
> > +			if (error)
> > +				return error;
> > +		}
> > +
> > +		error = sysfs_groups_change_owner(kobj, ktype->default_groups);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(sysfs_change_owner);
> 
> I can understand wanting to change owners/groups/whatever of existing
> sysfs objects and their files, but I can't figure out how to call this
> function to set the attribute I want to change.
> 
> With only one parameter, how does this work?  It guesses?  :)

See [1]. :)
And sorry if that wasn't clear!

Thanks!
Christian
Greg Kroah-Hartman Feb. 12, 2020, 4:04 p.m. UTC | #3
On Wed, Feb 12, 2020 at 04:07:43PM +0100, Christian Brauner wrote:
> On Wed, Feb 12, 2020 at 05:18:08AM -0800, Greg Kroah-Hartman wrote:
> > On Wed, Feb 12, 2020 at 11:43:16AM +0100, Christian Brauner wrote:
> > > Add a helper to change the owner of sysfs objects.
> > 
> > Seems sane, but:
> > 
> > > The ownership of a sysfs object is determined based on the ownership of
> > > the corresponding kobject, i.e. only if the ownership of a kobject is
> > > changed will this function change the ownership of the corresponding
> > > sysfs entry.
> > 
> > A "sysfs object" is a kobject.  So I don't understand this sentance,
> > sorry.
> 
> I meant that only if you change the uid/gid the underlying kobject is
> associated with will this function do anything, meaning that you can't
> pass in uids/gids directly. I'll explain why I did this down below [1].
> Sorry if that was confusing.
> 
> > 
> > > 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>
> > > ---
> > >  fs/sysfs/file.c       | 35 +++++++++++++++++++++++++++++++++++
> > >  include/linux/sysfs.h |  6 ++++++
> > >  2 files changed, 41 insertions(+)
> > > 
> > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > > index 6239d9584f0b..6a0fe88061fd 100644
> > > --- a/fs/sysfs/file.c
> > > +++ b/fs/sysfs/file.c
> > > @@ -642,3 +642,38 @@ int sysfs_file_change_owner(struct kobject *kobj, const char *name)
> > >  	return error;
> > >  }
> > >  EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
> > > +
> > > +/**
> > > + *	sysfs_change_owner - change owner of the given object.
> > > + *	@kobj:	object.
> > > + */
> > > +int sysfs_change_owner(struct kobject *kobj)
> > 
> > What does this change the owner of the given object _to_?
> 
> [1]:
> So ownership only changes if the kobject's uid/gid have been changed.
> So when to stick with the networking example, when a network device is
> moved into a new network namespace, the uid/gid of the kobject will be
> changed to the root user of the owning user namespace of that network
> namespace. So when the move of the network device has completed and
> kobject_get_ownership() is called it will now return a different
> uid/gid.

Ok, then this needs to say "change the uid/gid of the kobject to..." in
order to explain what it is now being set to.  Otherwise this is really
confusing if you only read the kerneldoc, right?

> So my reasoning was that ownership is determined dynamically that way. I
> guess what you're hinting at is that we could simply add uid_t uid,
> gid_t gid arguments to these sysfs helpers. That's fine with me too.

It's fine if you want to set it to the "root owner", just say that
somewhere :)

> It
> means that callers are responsible to either retrieve the ownership from
> the kobject (in case it was changed through another call) or the call to
> syfs_change_owner(kobj, uid, gid) sets the new owner of the kobject. I
> don't know what the best approach is. Maybe a hybrid whereby we allow
> passing in uid/gid but also allow passing in ({g,u}id_t - 1) to indicate
> that we want the ownership to be taken from the kobject itself (e.g.
> when a network device has been updated by dev_change_net_namespace()).
> 
> > 
> > > +{
> > > +	int error;
> > > +	const struct kobj_type *ktype;
> > > +
> > > +	if (!kobj->state_in_sysfs)
> > > +		return -EINVAL;
> > > +
> > > +	error = sysfs_file_change_owner(kobj, NULL);
> > 
> > It passes NULL?
> 
> Which means, change the ownership of "kobj" itself and not lookup a file
> relative to "kobj".

Ok, that's totally not obvious at all :(

Better naming please, I know it's hard, but it matters.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 6239d9584f0b..6a0fe88061fd 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -642,3 +642,38 @@  int sysfs_file_change_owner(struct kobject *kobj, const char *name)
 	return error;
 }
 EXPORT_SYMBOL_GPL(sysfs_file_change_owner);
+
+/**
+ *	sysfs_change_owner - change owner of the given object.
+ *	@kobj:	object.
+ */
+int sysfs_change_owner(struct kobject *kobj)
+{
+	int error;
+	const struct kobj_type *ktype;
+
+	if (!kobj->state_in_sysfs)
+		return -EINVAL;
+
+	error = sysfs_file_change_owner(kobj, NULL);
+	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(kobj, (*kattr)->name);
+			if (error)
+				return error;
+		}
+
+		error = sysfs_groups_change_owner(kobj, ktype->default_groups);
+		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 3b9770c5ecb7..b9ce60261e38 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -310,6 +310,7 @@  static inline void sysfs_enable_ns(struct kernfs_node *kn)
 	return kernfs_enable_ns(kn);
 }
 
+int sysfs_change_owner(struct kobject *kobj);
 int sysfs_file_change_owner(struct kobject *kobj, const char *name);
 int sysfs_link_change_owner(struct kobject *kobj, struct kobject *targ,
 			    const char *name);
@@ -542,6 +543,11 @@  static inline int sysfs_link_change_owner(struct kobject *kobj,
 	return 0;
 }
 
+static inline int sysfs_change_owner(struct kobject *kobj)
+{
+	return 0;
+}
+
 static inline int sysfs_groups_change_owner(struct kobject *kobj,
 			  const struct attribute_group **groups)
 {