lkml.org 
[lkml]   [2020]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next 05/10] sysfs: add sysfs_change_owner()
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

\
 
 \ /
  Last update: 2020-02-12 17:04    [W:0.096 / U:3.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site