Re: [PATCH net-next v3 5/9] device: add device_change_owner()

[Christian Brauner]

On Thu, Feb 20, 2020 at 12:25:13PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 18, 2020 at 05:29:39PM +0100, Christian Brauner wrote:
> > Add a helper to change the owner of a device's sysfs entries. This
> > needs to happen when the ownership of a device is changed, e.g. when
> > moving network devices between network namespaces.
> > This function will be used to correctly account for ownership changes,
> > e.g. when moving network devices between network namespaces.
> > 
> > Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > ---
> > /* v2 */
> > unchanged
> > 
> > /* v3 */
> > -  Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>:
> >    - Add explicit uid/gid parameters.
> > ---
> >  drivers/base/core.c    | 80 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/device.h |  1 +
> >  2 files changed, 81 insertions(+)
> > 
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 42a672456432..ec0d5e8cfd0f 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -3458,6 +3458,86 @@ int device_move(struct device *dev, struct device *new_parent,
> >  }
> >  EXPORT_SYMBOL_GPL(device_move);
> >  
> > +static int device_attrs_change_owner(struct device *dev, kuid_t kuid,
> > +				     kgid_t kgid)
> > +{
> > +	struct kobject *kobj = &dev->kobj;
> > +	struct class *class = dev->class;
> > +	const struct device_type *type = dev->type;
> > +	int error;
> > +
> > +	if (class) {
> > +		error = sysfs_groups_change_owner(kobj, class->dev_groups, kuid,
> > +						  kgid);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	if (type) {
> > +		error = sysfs_groups_change_owner(kobj, type->groups, kuid,
> > +						  kgid);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	error = sysfs_groups_change_owner(kobj, dev->groups, kuid, kgid);
> > +	if (error)
> > +		return error;
> > +
> > +	if (device_supports_offline(dev) && !dev->offline_disabled) {
> > +		error = sysfs_file_change_owner_by_name(
> > +			kobj, dev_attr_online.attr.name, kuid, kgid);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * device_change_owner - change the owner of an existing device.
> 
> The "owner" and what else gets changed here?  Please document this
> better.
> 
> 
> > + * @dev: device.
> > + * @kuid: new owner's kuid
> > + * @kgid: new owner's kgid
> > + */
> > +int device_change_owner(struct device *dev, kuid_t kuid, kgid_t kgid)
> > +{
> > +	int error;
> > +	struct kobject *kobj = &dev->kobj;
> > +
> > +	dev = get_device(dev);
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> > +	error = sysfs_change_owner(kobj, kuid, kgid);
> 
> the kobject of the device is changed, good.
> 
> > +	if (error)
> > +		goto out;
> > +
> > +	error = sysfs_file_change_owner_by_name(kobj, dev_attr_uevent.attr.name,
> > +						kuid, kgid);
> 
> Why call out the uevent file explicitly here?

This again, mirrors the creation of a kobject in sysfs. The uevent file
is created separately and thus should be chowned separately.

> 
> > +	if (error)
> > +		goto out;
> > +
> > +	error = device_attrs_change_owner(dev, kuid, kgid);
> > +	if (error)
> > +		goto out;
> 
> Doesn't this also change the uevent file?

No, not as far as I can tell. The uevent file is created in an extra
step when the kobject/sysfs entries are created.

> 
> > +
> > +#ifdef CONFIG_BLOCK
> > +	if (sysfs_deprecated && dev->class == &block_class)
> > +		goto out;
> > +#endif
> 
> Ugh, we still need this?

Yeah, apparently. It's what I gather from how a device is added.

> 
> > +
> > +	error = sysfs_link_change_owner(&dev->class->p->subsys.kobj, &dev->kobj,
> > +					dev_name(dev), kuid, kgid);
> 
> Now what is this changing?

So, this changed the ownership of the class link for the device to match
the directory entry for that device, so e.g. given a network device
symlink (or any other type) that points to the actual directory entry
for that device:

/sys/class/net/my-dev -> ../../devices/virtual/net/my-dev

it makes my-dev show the same permissions as the directory my-dev has.
If we don't do this this will look weird, because the symlink will show
different permissions than the target it is pointoing to.

> 
> Again, more documentation please as to exactly what is being changed in
> this function is needed.

Sure!