Re: [PATCH net-next v3 3/9] sysfs: add sysfs_group{s}_change_owner()

[Greg Kroah-Hartman]

On Tue, Feb 18, 2020 at 05:29:37PM +0100, Christian Brauner wrote:
> Add helpers to change the owner of sysfs groups.
> 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@xxxxxxxxxx>
> ---
> /* v2 */
> -  Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>:
>    - Add comment how ownership of sysfs object is changed.
> 
> /* v3 */
> -  Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>:
>    - Add explicit uid/gid parameters.
> - Christian Brauner <christian.brauner@xxxxxxxxxx>:
>   - Collapse groups ownership helper patches into a single patch.
> ---
>  fs/sysfs/group.c      | 117 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h |  20 ++++++++
>  2 files changed, 137 insertions(+)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index c4ab045926b7..bae562d3cba1 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -13,6 +13,7 @@
>  #include <linux/dcache.h>
>  #include <linux/namei.h>
>  #include <linux/err.h>
> +#include <linux/fs.h>
>  #include "sysfs.h"
>  
>  
> @@ -457,3 +458,119 @@ int __compat_only_sysfs_link_entry_to_kobj(struct kobject *kobj,
>  	return PTR_ERR_OR_ZERO(link);
>  }
>  EXPORT_SYMBOL_GPL(__compat_only_sysfs_link_entry_to_kobj);
> +
> +static int sysfs_group_attrs_change_owner(struct kernfs_node *grp_kn,
> +					  const struct attribute_group *grp,
> +					  struct iattr *newattrs)
> +{
> +	struct kernfs_node *kn;
> +	int error;
> +
> +	if (grp->attrs) {
> +		struct attribute *const *attr;
> +
> +		for (attr = grp->attrs; *attr; attr++) {
> +			kn = kernfs_find_and_get(grp_kn, (*attr)->name);
> +			if (!kn)
> +				return -ENOENT;
> +
> +			error = kernfs_setattr(kn, newattrs);
> +			kernfs_put(kn);
> +			if (error)
> +				return error;
> +		}
> +	}
> +
> +	if (grp->bin_attrs) {
> +		struct bin_attribute *const *bin_attr;
> +
> +		for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> +			kn = kernfs_find_and_get(grp_kn, (*bin_attr)->attr.name);
> +			if (!kn)
> +				return -ENOENT;
> +
> +			error = kernfs_setattr(kn, newattrs);
> +			kernfs_put(kn);
> +			if (error)
> +				return error;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * sysfs_group_change_owner - change owner of an attribute group.
> + * @kobj:	The kobject containing the group.
> + * @grp:	The attribute group.
> + * @kuid:	new owner's kuid
> + * @kgid:	new owner's kgid
> + *
> + * Returns 0 on success or error code on failure.

This is fine to document, just funny it's the only one documented about
the return value so far in this series.

Anyway, looks good to me:

Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>