Re: [PATCH 09/14] xattr: move user limits for xattrs to generic infra

[Jan Kara <jack@suse.cz>]

On Mon 16-02-26 14:32:05, Christian Brauner wrote:
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/kernfs/inode.c           | 75 ++-------------------------------------------
>  fs/kernfs/kernfs-internal.h |  3 +-
>  fs/xattr.c                  | 65 +++++++++++++++++++++++++++++++++++++++
>  include/linux/kernfs.h      |  2 --
>  include/linux/xattr.h       | 18 +++++++++++
>  5 files changed, 87 insertions(+), 76 deletions(-)
> 
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index dfc3315b5afc..1de10500842d 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -45,8 +45,7 @@ static struct kernfs_iattrs *__kernfs_iattrs(struct kernfs_node *kn, bool alloc)
>  	ret->ia_mtime = ret->ia_atime;
>  	ret->ia_ctime = ret->ia_atime;
>  
> -	atomic_set(&ret->nr_user_xattrs, 0);
> -	atomic_set(&ret->user_xattr_size, 0);
> +	simple_xattr_limits_init(&ret->xattr_limits);
>  
>  	/* If someone raced us, recognize it. */
>  	if (!try_cmpxchg(&kn->iattr, &attr, ret))
> @@ -355,69 +354,6 @@ static int kernfs_vfs_xattr_set(const struct xattr_handler *handler,
>  	return kernfs_xattr_set(kn, name, value, size, flags);
>  }
>  
> -static int kernfs_vfs_user_xattr_add(struct kernfs_node *kn,
> -				     const char *full_name,
> -				     struct simple_xattrs *xattrs,
> -				     const void *value, size_t size, int flags)
> -{
> -	struct kernfs_iattrs *attr = kernfs_iattrs_noalloc(kn);
> -	atomic_t *sz = &attr->user_xattr_size;
> -	atomic_t *nr = &attr->nr_user_xattrs;
> -	struct simple_xattr *old_xattr;
> -	int ret;
> -
> -	if (atomic_inc_return(nr) > KERNFS_MAX_USER_XATTRS) {
> -		ret = -ENOSPC;
> -		goto dec_count_out;
> -	}
> -
> -	if (atomic_add_return(size, sz) > KERNFS_USER_XATTR_SIZE_LIMIT) {
> -		ret = -ENOSPC;
> -		goto dec_size_out;
> -	}
> -
> -	old_xattr = simple_xattr_set(xattrs, full_name, value, size, flags);
> -	if (!old_xattr)
> -		return 0;
> -
> -	if (IS_ERR(old_xattr)) {
> -		ret = PTR_ERR(old_xattr);
> -		goto dec_size_out;
> -	}
> -
> -	ret = 0;
> -	size = old_xattr->size;
> -	simple_xattr_free_rcu(old_xattr);
> -dec_size_out:
> -	atomic_sub(size, sz);
> -dec_count_out:
> -	atomic_dec(nr);
> -	return ret;
> -}
> -
> -static int kernfs_vfs_user_xattr_rm(struct kernfs_node *kn,
> -				    const char *full_name,
> -				    struct simple_xattrs *xattrs,
> -				    const void *value, size_t size, int flags)
> -{
> -	struct kernfs_iattrs *attr = kernfs_iattrs_noalloc(kn);
> -	atomic_t *sz = &attr->user_xattr_size;
> -	atomic_t *nr = &attr->nr_user_xattrs;
> -	struct simple_xattr *old_xattr;
> -
> -	old_xattr = simple_xattr_set(xattrs, full_name, value, size, flags);
> -	if (!old_xattr)
> -		return 0;
> -
> -	if (IS_ERR(old_xattr))
> -		return PTR_ERR(old_xattr);
> -
> -	atomic_sub(old_xattr->size, sz);
> -	atomic_dec(nr);
> -	simple_xattr_free_rcu(old_xattr);
> -	return 0;
> -}
> -
>  static int kernfs_vfs_user_xattr_set(const struct xattr_handler *handler,
>  				     struct mnt_idmap *idmap,
>  				     struct dentry *unused, struct inode *inode,
> @@ -440,13 +376,8 @@ static int kernfs_vfs_user_xattr_set(const struct xattr_handler *handler,
>  	if (IS_ERR_OR_NULL(xattrs))
>  		return PTR_ERR(xattrs);
>  
> -	if (value)
> -		return kernfs_vfs_user_xattr_add(kn, full_name, xattrs,
> -						 value, size, flags);
> -	else
> -		return kernfs_vfs_user_xattr_rm(kn, full_name, xattrs,
> -						value, size, flags);
> -
> +	return simple_xattr_set_limited(xattrs, &attrs->xattr_limits,
> +					full_name, value, size, flags);
>  }
>  
>  static const struct xattr_handler kernfs_trusted_xattr_handler = {
> diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
> index 1324ed8c0661..1d3831e3a270 100644
> --- a/fs/kernfs/kernfs-internal.h
> +++ b/fs/kernfs/kernfs-internal.h
> @@ -27,8 +27,7 @@ struct kernfs_iattrs {
>  	struct timespec64	ia_ctime;
>  
>  	struct simple_xattrs	*xattrs;
> -	atomic_t		nr_user_xattrs;
> -	atomic_t		user_xattr_size;
> +	struct simple_xattr_limits xattr_limits;
>  };
>  
>  struct kernfs_root {
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 328ed7558dfc..5e559b1c651f 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -1427,6 +1427,71 @@ struct simple_xattr *simple_xattr_set(struct simple_xattrs *xattrs,
>  	return old_xattr;
>  }
>  
> +static inline void simple_xattr_limits_dec(struct simple_xattr_limits *limits,
> +					   size_t size)
> +{
> +	atomic_sub(size, &limits->xattr_size);
> +	atomic_dec(&limits->nr_xattrs);
> +}
> +
> +static inline int simple_xattr_limits_inc(struct simple_xattr_limits *limits,
> +					  size_t size)
> +{
> +	if (atomic_inc_return(&limits->nr_xattrs) > SIMPLE_XATTR_MAX_NR) {
> +		atomic_dec(&limits->nr_xattrs);
> +		return -ENOSPC;
> +	}
> +
> +	if (atomic_add_return(size, &limits->xattr_size) <= SIMPLE_XATTR_MAX_SIZE)
> +		return 0;
> +
> +	simple_xattr_limits_dec(limits, size);
> +	return -ENOSPC;
> +}
> +
> +/**
> + * simple_xattr_set_limited - set an xattr with per-inode user.* limits
> + * @xattrs: the header of the xattr object
> + * @limits: per-inode limit counters for user.* xattrs
> + * @name: the name of the xattr to set or remove
> + * @value: the value to store (NULL to remove)
> + * @size: the size of @value
> + * @flags: XATTR_CREATE, XATTR_REPLACE, or 0
> + *
> + * Like simple_xattr_set(), but enforces per-inode count and total value size
> + * limits for user.* xattrs. Uses speculative pre-increment of the atomic
> + * counters to avoid races without requiring external locks.
> + *
> + * Return: On success zero is returned. On failure a negative error code is
> + * returned.
> + */
> +int simple_xattr_set_limited(struct simple_xattrs *xattrs,
> +			     struct simple_xattr_limits *limits,
> +			     const char *name, const void *value,
> +			     size_t size, int flags)
> +{
> +	struct simple_xattr *old_xattr;
> +	int ret;
> +
> +	if (value) {
> +		ret = simple_xattr_limits_inc(limits, size);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	old_xattr = simple_xattr_set(xattrs, name, value, size, flags);
> +	if (IS_ERR(old_xattr)) {
> +		if (value)
> +			simple_xattr_limits_dec(limits, size);
> +		return PTR_ERR(old_xattr);
> +	}
> +	if (old_xattr) {
> +		simple_xattr_limits_dec(limits, old_xattr->size);
> +		simple_xattr_free_rcu(old_xattr);
> +	}
> +	return 0;
> +}
> +
>  static bool xattr_is_trusted(const char *name)
>  {
>  	return !strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN);
> diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
> index b5a5f32fdfd1..d8f57f0af5e4 100644
> --- a/include/linux/kernfs.h
> +++ b/include/linux/kernfs.h
> @@ -99,8 +99,6 @@ enum kernfs_node_type {
>  
>  #define KERNFS_TYPE_MASK		0x000f
>  #define KERNFS_FLAG_MASK		~KERNFS_TYPE_MASK
> -#define KERNFS_MAX_USER_XATTRS		128
> -#define KERNFS_USER_XATTR_SIZE_LIMIT	(128 << 10)
>  
>  enum kernfs_node_flag {
>  	KERNFS_ACTIVATED	= 0x0010,
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index f60357d9f938..90a43a117127 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -118,6 +118,20 @@ struct simple_xattr {
>  	char value[];
>  };
>  
> +#define SIMPLE_XATTR_MAX_NR		128
> +#define SIMPLE_XATTR_MAX_SIZE		(128 << 10)
> +
> +struct simple_xattr_limits {
> +	atomic_t	nr_xattrs;	/* current user.* xattr count */
> +	atomic_t	xattr_size;	/* current total user.* value bytes */
> +};
> +
> +static inline void simple_xattr_limits_init(struct simple_xattr_limits *limits)
> +{
> +	atomic_set(&limits->nr_xattrs, 0);
> +	atomic_set(&limits->xattr_size, 0);
> +}
> +
>  int simple_xattrs_init(struct simple_xattrs *xattrs);
>  struct simple_xattrs *simple_xattrs_alloc(void);
>  struct simple_xattrs *simple_xattrs_lazy_alloc(struct simple_xattrs **xattrsp,
> @@ -132,6 +146,10 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
>  struct simple_xattr *simple_xattr_set(struct simple_xattrs *xattrs,
>  				      const char *name, const void *value,
>  				      size_t size, int flags);
> +int simple_xattr_set_limited(struct simple_xattrs *xattrs,
> +			     struct simple_xattr_limits *limits,
> +			     const char *name, const void *value,
> +			     size_t size, int flags);
>  ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
>  			  char *buffer, size_t size);
>  int simple_xattr_add(struct simple_xattrs *xattrs,
> 
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR