[RFC PATCH] kernfs: enable per-inode limits for all xattr types

[RFC PATCH] kernfs: enable per-inode limits for all xattr types

[Vasily Averin]

Currently it's possible to create a huge number of xattr per inode,
and I would like to add USER-like restrcition to all xattr types.

I've prepared RFC patch and would like to discuss it.

This patch moves counters calculations into simple_xattr_set,
under simple_xattrs spinlock. This allows to replace atomic counters
used currently for USER xattr to ints.

To keep current behaviour for USER xattr I keep current limits
and counters and add separated limits and counters for all another
xattr types. However I would like to merge these limits and counters
together, because it simplifies the code even more.
Could someone please clarify if this is acceptable?

Signed-off-by: Vasily Averin <vvs@openvz.org>
---
 fs/kernfs/inode.c           | 67 ++-----------------------------------
 fs/kernfs/kernfs-internal.h |  2 --
 fs/xattr.c                  | 56 +++++++++++++++++++------------
 include/linux/kernfs.h      |  2 ++
 include/linux/xattr.h       | 11 ++++--
 mm/shmem.c                  | 29 +++++++---------
 6 files changed, 61 insertions(+), 106 deletions(-)

diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 3d783d80f5da..7cfdda41fc89 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -47,8 +47,6 @@ static struct kernfs_iattrs *__kernfs_iattrs(struct kernfs_node *kn, int alloc)
 	kn->iattr->ia_ctime = kn->iattr->ia_atime;
 
 	simple_xattrs_init(&kn->iattr->xattrs);
-	atomic_set(&kn->iattr->nr_user_xattrs, 0);
-	atomic_set(&kn->iattr->user_xattr_size, 0);
 out_unlock:
 	ret = kn->iattr;
 	mutex_unlock(&iattr_mutex);
@@ -314,7 +312,7 @@ int kernfs_xattr_set(struct kernfs_node *kn, const char *name,
 	if (!attrs)
 		return -ENOMEM;
 
-	return simple_xattr_set(&attrs->xattrs, name, value, size, flags, NULL);
+	return simple_xattr_set(&attrs->xattrs, name, value, size, flags);
 }
 
 static int kernfs_vfs_xattr_get(const struct xattr_handler *handler,
@@ -339,61 +337,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)
-{
-	atomic_t *sz = &kn->iattr->user_xattr_size;
-	atomic_t *nr = &kn->iattr->nr_user_xattrs;
-	ssize_t removed_size;
-	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;
-	}
-
-	ret = simple_xattr_set(xattrs, full_name, value, size, flags,
-			       &removed_size);
-
-	if (!ret && removed_size >= 0)
-		size = removed_size;
-	else if (!ret)
-		return 0;
-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)
-{
-	atomic_t *sz = &kn->iattr->user_xattr_size;
-	atomic_t *nr = &kn->iattr->nr_user_xattrs;
-	ssize_t removed_size;
-	int ret;
-
-	ret = simple_xattr_set(xattrs, full_name, value, size, flags,
-			       &removed_size);
-
-	if (removed_size >= 0) {
-		atomic_sub(removed_size, sz);
-		atomic_dec(nr);
-	}
-
-	return ret;
-}
-
 static int kernfs_vfs_user_xattr_set(const struct xattr_handler *handler,
 				     struct user_namespace *mnt_userns,
 				     struct dentry *unused, struct inode *inode,
@@ -411,13 +354,7 @@ static int kernfs_vfs_user_xattr_set(const struct xattr_handler *handler,
 	if (!attrs)
 		return -ENOMEM;
 
-	if (value)
-		return kernfs_vfs_user_xattr_add(kn, full_name, &attrs->xattrs,
-						 value, size, flags);
-	else
-		return kernfs_vfs_user_xattr_rm(kn, full_name, &attrs->xattrs,
-						value, size, flags);
-
+	return simple_xattr_set(&attrs->xattrs, 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 eeaa779b929c..a2b89bd48c9d 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -27,8 +27,6 @@ struct kernfs_iattrs {
 	struct timespec64	ia_ctime;
 
 	struct simple_xattrs	xattrs;
-	atomic_t		nr_user_xattrs;
-	atomic_t		user_xattr_size;
 };
 
 struct kernfs_root {
diff --git a/fs/xattr.c b/fs/xattr.c
index b4875514a3ee..de4a2efc7fa4 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -1037,6 +1037,11 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
 	return ret;
 }
 
+static bool xattr_is_user(const char *name)
+{
+	return !strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN);
+}
+
 /**
  * simple_xattr_set - xattr SET operation for in-memory/pseudo filesystems
  * @xattrs: target simple_xattr list
@@ -1053,16 +1058,17 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
  * Returns 0 on success, -errno on failure.
  */
 int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
-		     const void *value, size_t size, int flags,
-		     ssize_t *removed_size)
+		     const void *value, size_t size, int flags)
 {
 	struct simple_xattr *xattr;
 	struct simple_xattr *new_xattr = NULL;
+	bool is_user_xattr = false;
+	int *sz = &xattrs->xattr_size;
+	int *nr = &xattrs->nr_xattrs;
+	int sz_limit = KERNFS_XATTR_SIZE_LIMIT;
+	int nr_limit = KERNFS_MAX_XATTRS;
 	int err = 0;
 
-	if (removed_size)
-		*removed_size = -1;
-
 	/* value == NULL means remove */
 	if (value) {
 		new_xattr = simple_xattr_alloc(value, size);
@@ -1076,6 +1082,14 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
 		}
 	}
 
+	is_user_xattr = xattr_is_user(name);
+	if (is_user_xattr) {
+		sz = &xattrs->user_xattr_size;
+		nr = &xattrs->nr_user_xattrs;
+		sz_limit = KERNFS_USER_XATTR_SIZE_LIMIT;
+		nr_limit = KERNFS_MAX_USER_XATTRS;
+	}
+
 	spin_lock(&xattrs->lock);
 	list_for_each_entry(xattr, &xattrs->head, list) {
 		if (!strcmp(name, xattr->name)) {
@@ -1083,13 +1097,19 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
 				xattr = new_xattr;
 				err = -EEXIST;
 			} else if (new_xattr) {
-				list_replace(&xattr->list, &new_xattr->list);
-				if (removed_size)
-					*removed_size = xattr->size;
+				int delta = new_xattr->size - xattr->size;
+
+				if (*sz + delta > sz_limit) {
+					xattr = new_xattr;
+					err = -ENOSPC;
+				} else {
+					*sz += delta;
+					list_replace(&xattr->list, &new_xattr->list);
+				}
 			} else {
+				*sz -= xattr->size;
+				(*nr)--;
 				list_del(&xattr->list);
-				if (removed_size)
-					*removed_size = xattr->size;
 			}
 			goto out;
 		}
@@ -1097,7 +1117,12 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
 	if (flags & XATTR_REPLACE) {
 		xattr = new_xattr;
 		err = -ENODATA;
+	} else if ((*sz + new_xattr->size > sz_limit) || (*nr == nr_limit)) {
+		xattr = new_xattr;
+		err = -ENOSPC;
 	} else {
+		*sz += new_xattr->size;
+		(*nr)++;
 		list_add(&new_xattr->list, &xattrs->head);
 		xattr = NULL;
 	}
@@ -1172,14 +1197,3 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
 
 	return err ? err : size - remaining_size;
 }
-
-/*
- * Adds an extended attribute to the list
- */
-void simple_xattr_list_add(struct simple_xattrs *xattrs,
-			   struct simple_xattr *new_xattr)
-{
-	spin_lock(&xattrs->lock);
-	list_add(&new_xattr->list, &xattrs->head);
-	spin_unlock(&xattrs->lock);
-}
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index e2ae15a6225e..1972beb0d7b9 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -44,6 +44,8 @@ enum kernfs_node_type {
 #define KERNFS_FLAG_MASK		~KERNFS_TYPE_MASK
 #define KERNFS_MAX_USER_XATTRS		128
 #define KERNFS_USER_XATTR_SIZE_LIMIT	(128 << 10)
+#define KERNFS_MAX_XATTRS		128
+#define KERNFS_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 4c379d23ec6e..c6b9258958d5 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -81,6 +81,10 @@ static inline const char *xattr_prefix(const struct xattr_handler *handler)
 
 struct simple_xattrs {
 	struct list_head head;
+	int	nr_xattrs;
+	int	nr_user_xattrs;
+	int	xattr_size;
+	int	user_xattr_size;
 	spinlock_t lock;
 };
 
@@ -98,6 +102,10 @@ static inline void simple_xattrs_init(struct simple_xattrs *xattrs)
 {
 	INIT_LIST_HEAD(&xattrs->head);
 	spin_lock_init(&xattrs->lock);
+	xattrs->nr_xattrs = 0;
+	xattrs->nr_user_xattrs = 0;
+	xattrs->xattr_size = 0;
+	xattrs->user_xattr_size = 0;
 }
 
 /*
@@ -117,8 +125,7 @@ struct simple_xattr *simple_xattr_alloc(const void *value, size_t size);
 int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
 		     void *buffer, size_t size);
 int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
-		     const void *value, size_t size, int flags,
-		     ssize_t *removed_size);
+		     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);
 void simple_xattr_list_add(struct simple_xattrs *xattrs,
diff --git a/mm/shmem.c b/mm/shmem.c
index 66eed363e5c2..0215c16a2643 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3155,30 +3155,27 @@ static int shmem_initxattrs(struct inode *inode,
 	struct shmem_inode_info *info = SHMEM_I(inode);
 	const struct xattr *xattr;
 	struct simple_xattr *new_xattr;
+	char *name;
 	size_t len;
+	int ret = 0;
 
 	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
-		new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
-		if (!new_xattr)
-			return -ENOMEM;
-
 		len = strlen(xattr->name) + 1;
-		new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len,
-					  GFP_KERNEL_ACCOUNT);
-		if (!new_xattr->name) {
-			kvfree(new_xattr);
+		name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len, GFP_KERNEL);
+		if (!name)
 			return -ENOMEM;
-		}
 
-		memcpy(new_xattr->name, XATTR_SECURITY_PREFIX,
-		       XATTR_SECURITY_PREFIX_LEN);
-		memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN,
-		       xattr->name, len);
+		memcpy(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN);
+		memcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name, len);
 
-		simple_xattr_list_add(&info->xattrs, new_xattr);
+		ret = simple_xattr_set(&info->xattrs, name, xattr->value,
+				       xattr->value_len, XATTR_CREATE);
+		kfree(name);
+		if (ret)
+			break;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int shmem_xattr_handler_get(const struct xattr_handler *handler,
@@ -3200,7 +3197,7 @@ static int shmem_xattr_handler_set(const struct xattr_handler *handler,
 	struct shmem_inode_info *info = SHMEM_I(inode);
 
 	name = xattr_full_name(handler, name);
-	return simple_xattr_set(&info->xattrs, name, value, size, flags, NULL);
+	return simple_xattr_set(&info->xattrs, name, value, size, flags);
 }
 
 static const struct xattr_handler shmem_security_xattr_handler = {
-- 
2.25.1

Re: [RFC PATCH] kernfs: enable per-inode limits for all xattr types

[Christian Brauner]

On Thu, Aug 11, 2022 at 07:58:46AM +0300, Vasily Averin wrote:
> Currently it's possible to create a huge number of xattr per inode,
> and I would like to add USER-like restrcition to all xattr types.
> 
> I've prepared RFC patch and would like to discuss it.
> 
> This patch moves counters calculations into simple_xattr_set,
> under simple_xattrs spinlock. This allows to replace atomic counters
> used currently for USER xattr to ints.
> 
> To keep current behaviour for USER xattr I keep current limits
> and counters and add separated limits and counters for all another
> xattr types. However I would like to merge these limits and counters
> together, because it simplifies the code even more.
> Could someone please clarify if this is acceptable?
> 
> Signed-off-by: Vasily Averin <vvs@openvz.org>
> ---

Hey Vasily,

Fyi, this patch doesn't seem to apply cleanly to anything from v5.17 up
until current master. It's a bit unfortunate that it's the middle of the
merge window so ideally you'd resend once the merge window closes on top
of v5.20-rc1.

A few questions below.

>  fs/kernfs/inode.c           | 67 ++-----------------------------------
>  fs/kernfs/kernfs-internal.h |  2 --
>  fs/xattr.c                  | 56 +++++++++++++++++++------------
>  include/linux/kernfs.h      |  2 ++
>  include/linux/xattr.h       | 11 ++++--
>  mm/shmem.c                  | 29 +++++++---------
>  6 files changed, 61 insertions(+), 106 deletions(-)
> 
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 3d783d80f5da..7cfdda41fc89 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -47,8 +47,6 @@ static struct kernfs_iattrs *__kernfs_iattrs(struct kernfs_node *kn, int alloc)
>  	kn->iattr->ia_ctime = kn->iattr->ia_atime;
>  
>  	simple_xattrs_init(&kn->iattr->xattrs);
> -	atomic_set(&kn->iattr->nr_user_xattrs, 0);
> -	atomic_set(&kn->iattr->user_xattr_size, 0);
>  out_unlock:
>  	ret = kn->iattr;
>  	mutex_unlock(&iattr_mutex);
> @@ -314,7 +312,7 @@ int kernfs_xattr_set(struct kernfs_node *kn, const char *name,
>  	if (!attrs)
>  		return -ENOMEM;
>  
> -	return simple_xattr_set(&attrs->xattrs, name, value, size, flags, NULL);
> +	return simple_xattr_set(&attrs->xattrs, name, value, size, flags);
>  }
>  
>  static int kernfs_vfs_xattr_get(const struct xattr_handler *handler,
> @@ -339,61 +337,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)
> -{
> -	atomic_t *sz = &kn->iattr->user_xattr_size;
> -	atomic_t *nr = &kn->iattr->nr_user_xattrs;
> -	ssize_t removed_size;
> -	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;
> -	}
> -
> -	ret = simple_xattr_set(xattrs, full_name, value, size, flags,
> -			       &removed_size);
> -
> -	if (!ret && removed_size >= 0)
> -		size = removed_size;
> -	else if (!ret)
> -		return 0;
> -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)
> -{
> -	atomic_t *sz = &kn->iattr->user_xattr_size;
> -	atomic_t *nr = &kn->iattr->nr_user_xattrs;
> -	ssize_t removed_size;
> -	int ret;
> -
> -	ret = simple_xattr_set(xattrs, full_name, value, size, flags,
> -			       &removed_size);
> -
> -	if (removed_size >= 0) {
> -		atomic_sub(removed_size, sz);
> -		atomic_dec(nr);
> -	}
> -
> -	return ret;
> -}
> -
>  static int kernfs_vfs_user_xattr_set(const struct xattr_handler *handler,
>  				     struct user_namespace *mnt_userns,
>  				     struct dentry *unused, struct inode *inode,
> @@ -411,13 +354,7 @@ static int kernfs_vfs_user_xattr_set(const struct xattr_handler *handler,
>  	if (!attrs)
>  		return -ENOMEM;
>  
> -	if (value)
> -		return kernfs_vfs_user_xattr_add(kn, full_name, &attrs->xattrs,
> -						 value, size, flags);
> -	else
> -		return kernfs_vfs_user_xattr_rm(kn, full_name, &attrs->xattrs,
> -						value, size, flags);
> -
> +	return simple_xattr_set(&attrs->xattrs, 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 eeaa779b929c..a2b89bd48c9d 100644
> --- a/fs/kernfs/kernfs-internal.h
> +++ b/fs/kernfs/kernfs-internal.h
> @@ -27,8 +27,6 @@ struct kernfs_iattrs {
>  	struct timespec64	ia_ctime;
>  
>  	struct simple_xattrs	xattrs;
> -	atomic_t		nr_user_xattrs;
> -	atomic_t		user_xattr_size;
>  };
>  
>  struct kernfs_root {
> diff --git a/fs/xattr.c b/fs/xattr.c
> index b4875514a3ee..de4a2efc7fa4 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -1037,6 +1037,11 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
>  	return ret;
>  }
>  
> +static bool xattr_is_user(const char *name)
> +{
> +	return !strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN);
> +}
> +
>  /**
>   * simple_xattr_set - xattr SET operation for in-memory/pseudo filesystems
>   * @xattrs: target simple_xattr list
> @@ -1053,16 +1058,17 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
>   * Returns 0 on success, -errno on failure.
>   */
>  int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
> -		     const void *value, size_t size, int flags,
> -		     ssize_t *removed_size)
> +		     const void *value, size_t size, int flags)
>  {
>  	struct simple_xattr *xattr;
>  	struct simple_xattr *new_xattr = NULL;
> +	bool is_user_xattr = false;
> +	int *sz = &xattrs->xattr_size;
> +	int *nr = &xattrs->nr_xattrs;
> +	int sz_limit = KERNFS_XATTR_SIZE_LIMIT;
> +	int nr_limit = KERNFS_MAX_XATTRS;
>  	int err = 0;
>  
> -	if (removed_size)
> -		*removed_size = -1;
> -
>  	/* value == NULL means remove */
>  	if (value) {
>  		new_xattr = simple_xattr_alloc(value, size);
> @@ -1076,6 +1082,14 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
>  		}
>  	}
>  
> +	is_user_xattr = xattr_is_user(name);
> +	if (is_user_xattr) {
> +		sz = &xattrs->user_xattr_size;
> +		nr = &xattrs->nr_user_xattrs;
> +		sz_limit = KERNFS_USER_XATTR_SIZE_LIMIT;
> +		nr_limit = KERNFS_MAX_USER_XATTRS;
> +	}
> +
>  	spin_lock(&xattrs->lock);
>  	list_for_each_entry(xattr, &xattrs->head, list) {
>  		if (!strcmp(name, xattr->name)) {
> @@ -1083,13 +1097,19 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
>  				xattr = new_xattr;
>  				err = -EEXIST;
>  			} else if (new_xattr) {
> -				list_replace(&xattr->list, &new_xattr->list);
> -				if (removed_size)
> -					*removed_size = xattr->size;
> +				int delta = new_xattr->size - xattr->size;
> +
> +				if (*sz + delta > sz_limit) {
> +					xattr = new_xattr;
> +					err = -ENOSPC;
> +				} else {
> +					*sz += delta;
> +					list_replace(&xattr->list, &new_xattr->list);
> +				}
>  			} else {
> +				*sz -= xattr->size;
> +				(*nr)--;
>  				list_del(&xattr->list);
> -				if (removed_size)
> -					*removed_size = xattr->size;
>  			}
>  			goto out;
>  		}
> @@ -1096,7 +1117,12 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
>  	if (flags & XATTR_REPLACE) {
>  		xattr = new_xattr;
>  		err = -ENODATA;
> +	} else if ((*sz + new_xattr->size > sz_limit) || (*nr == nr_limit)) {
> +		xattr = new_xattr;
> +		err = -ENOSPC;
>  	} else {
> +		*sz += new_xattr->size;
> +		(*nr)++;
>  		list_add(&new_xattr->list, &xattrs->head);
>  		xattr = NULL;
>  	}
> @@ -1172,14 +1197,3 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
>  
>  	return err ? err : size - remaining_size;
>  }
> -
> -/*
> - * Adds an extended attribute to the list
> - */
> -void simple_xattr_list_add(struct simple_xattrs *xattrs,
> -			   struct simple_xattr *new_xattr)

You should also remove the function from the header.

> -{
> -	spin_lock(&xattrs->lock);
> -	list_add(&new_xattr->list, &xattrs->head);
> -	spin_unlock(&xattrs->lock);
> -}
> diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
> index e2ae15a6225e..1972beb0d7b9 100644
> --- a/include/linux/kernfs.h
> +++ b/include/linux/kernfs.h
> @@ -44,6 +44,8 @@ enum kernfs_node_type {
>  #define KERNFS_FLAG_MASK		~KERNFS_TYPE_MASK
>  #define KERNFS_MAX_USER_XATTRS		128
>  #define KERNFS_USER_XATTR_SIZE_LIMIT	(128 << 10)
> +#define KERNFS_MAX_XATTRS		128
> +#define KERNFS_XATTR_SIZE_LIMIT		(128 << 10)

So iiuc, for tmpfs this is effectively a per-inode limit of 128 xattrs
and 128 user xattrs; nr_inodes * 256. I honestly have no idea if there
are legimitate use-cases to want more. But there's at least a remote
chance that this might break someone.

Apart from

> Currently it's possible to create a huge number of xattr per inode,

what exactly is this limit protecting against? In other words, the
commit message misses the motivation for the patch.
(The original thread it was spun out of was so scattered I honestly
don't have time to dig through it currently. So it'd be great if this
patch expanded on that a bit.)

I'd also prefer to see a summary of what filesystems are affected by
this change. Afaict, the patchset doesn't change anything for kernfs
users such as cgroup{1,2} so it should only be tmpfs and potential
future users of the simple_xattr_* api.

Sidenote: While looking at this I found out that cgroup{1,2} supports
e.g. security.capability to be set on say cpu.stat or similar files.
That seems very strange to me.

Re: [RFC PATCH] kernfs: enable per-inode limits for all xattr types

[Vasily Averin]

On 8/12/22 13:20, Christian Brauner wrote:
> So iiuc, for tmpfs this is effectively a per-inode limit of 128 xattrs
> and 128 user xattrs; nr_inodes * 256. I honestly have no idea if there
> are legimitate use-cases to want more. But there's at least a remote
> chance that this might break someone.
> 
> Apart from
> 
>> Currently it's possible to create a huge number of xattr per inode,
> 
> what exactly is this limit protecting against? In other words, the
> commit message misses the motivation for the patch.

This should prevent softlockup and hung_task_panic caused by slow search
in xattrs->list in simple_xattr_set() and _get()

Adding new xattr checks all present entries in the list,
so execution time linearly depends on the number of such entries.

To avoid this problem I decided to limit somehow the number of entries in the list.
As an alternative Tejun advises to switch this list to something like rb-tree,
now I think he is right.

> I'd also prefer to see a summary of what filesystems are affected by
> this change. Afaict, the patchset doesn't change anything for kernfs
> users such as cgroup{1,2} so it should only be tmpfs and potential
> future users of the simple_xattr_* api.

This affect all file systems used  simple_xattr_* API: sysfs and tmpfs.

Now I'll try to follow Tejun's advice and switch the xatrrs list to rb-tree.
Unfortunately, I doubt that I will have enough time to finish before the merge
window closes.

Thank you,
	Vasily Averin