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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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