Re: [PATCH] ocfs2: retain all security xattrs during inode creation

[Heming Zhao <heming.zhao@suse.com>]

On Wed, Sep 16, 2026 at 08:02:58PM +0800, Joseph Qi wrote:
> When creating a new inode, ocfs2_init_security_get() stores the
> security xattr returned by the LSM framework in
> ocfs2_security_xattr_info so that it can be written later within the
> same transaction.  But ocfs2_initxattrs() only ever looks at the
> first element of the xattr array, and it keeps the name as a bare
> pointer into memory owned by the security layer instead of copying
> it.  The value, by contrast, is already duplicated with kmemdup().
> 
> With stacked LSMs, security_inode_init_security() provides one xattr
> per LSM, so all labels but the first are silently dropped and never
> written to disk, leaving newly created files without the security
> xattrs the other LSMs rely on.
> 
> The borrowed name is not an outright bug with the LSMs in tree
> today: security_inode_init_security() frees only ->value once the
> callback returns, and documents ->name as the attribute name
> suffix, so the pointer stays valid for as long as ocfs2 uses it.
> It is fragile though, as an LSM that allocates the name together
> with the value would turn it into a use-after-free, so the rework
> below copies the names as well.
> 
> Fix this by storing copies of all the security xattrs: allocate an
> array in ocfs2_initxattrs(), copy the names and values, account for
> all of them in the credit calculations, and write each one in
> ocfs2_init_security_set().
> 
> While at it, switch the allocations from GFP_KERNEL to GFP_NOFS.
> The callback runs in the inode creation path with the parent
> directory locked, and the security layer allocates the very same
> xattr array with GFP_NOFS, so direct reclaim must not be allowed to
> recurse into the filesystem from here.
> 
> Fixes: de3004c874e7 ("ocfs2: Switch to security_inode_init_security()")
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>

The code looks good to me.
Reviewed-by: Heming Zhao <heming.zhao@suse.com>
> ---
>  fs/ocfs2/namei.c |   6 +--
>  fs/ocfs2/xattr.c | 122 ++++++++++++++++++++++++++++++++++++-----------
>  fs/ocfs2/xattr.h |  12 +++--
>  3 files changed, 105 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
> index 58c6061ed983..ef03f90e7265 100644
> --- a/fs/ocfs2/namei.c
> +++ b/fs/ocfs2/namei.c
> @@ -248,7 +248,6 @@ static int ocfs2_mknod(struct mnt_idmap *idmap,
>  	int want_meta = 0;
>  	int xattr_credits = 0;
>  	struct ocfs2_security_xattr_info si = {
> -		.name = NULL,
>  		.enable = 1,
>  	};
>  	int did_quota_inode = 0;
> @@ -475,7 +474,7 @@ static int ocfs2_mknod(struct mnt_idmap *idmap,
>  
>  	brelse(new_fe_bh);
>  	brelse(parent_fe_bh);
> -	kfree(si.value);
> +	ocfs2_free_security_xattrs(&si);
>  
>  	ocfs2_acl_init_release(&acl_state);
>  
> @@ -1831,7 +1830,6 @@ static int ocfs2_symlink(struct mnt_idmap *idmap,
>  	int want_clusters = 0;
>  	int xattr_credits = 0;
>  	struct ocfs2_security_xattr_info si = {
> -		.name = NULL,
>  		.enable = 1,
>  	};
>  	int did_quota = 0, did_quota_inode = 0;
> @@ -2063,7 +2061,7 @@ static int ocfs2_symlink(struct mnt_idmap *idmap,
>  
>  	brelse(new_fe_bh);
>  	brelse(parent_fe_bh);
> -	kfree(si.value);
> +	ocfs2_free_security_xattrs(&si);
>  	ocfs2_free_dir_lookup_result(&lookup);
>  	if (inode_ac)
>  		ocfs2_free_alloc_context(inode_ac);
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index a2d6c3d0f2e8..f2b7a53ed969 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -643,10 +643,15 @@ int ocfs2_calc_security_init(struct inode *dir,
>  			     int *xattr_credits,
>  			     struct ocfs2_alloc_context **xattr_ac)
>  {
> +	int i;
>  	int ret = 0;
>  	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);
> -	int s_size = ocfs2_xattr_entry_real_size(strlen(si->name),
> -						 si->value_len);
> +	int s_size = 0;
> +
> +	for (i = 0; i < si->count; i++)
> +		s_size += ocfs2_xattr_entry_real_size(
> +				strlen(si->xattrs[i].name),
> +				si->xattrs[i].value_len);
>  
>  	/*
>  	 * The max space of security xattr taken inline is
> @@ -664,13 +669,15 @@ int ocfs2_calc_security_init(struct inode *dir,
>  	}
>  
>  	/* reserve clusters for xattr value which will be set in B tree*/
> -	if (si->value_len > OCFS2_XATTR_INLINE_SIZE) {
> -		int new_clusters = ocfs2_clusters_for_bytes(dir->i_sb,
> -							    si->value_len);
> -
> -		*xattr_credits += ocfs2_clusters_to_blocks(dir->i_sb,
> -							   new_clusters);
> -		*want_clusters += new_clusters;
> +	for (i = 0; i < si->count; i++) {
> +		if (si->xattrs[i].value_len > OCFS2_XATTR_INLINE_SIZE) {
> +			int new_clusters = ocfs2_clusters_for_bytes(dir->i_sb,
> +						si->xattrs[i].value_len);
> +
> +			*xattr_credits += ocfs2_clusters_to_blocks(dir->i_sb,
> +								   new_clusters);
> +			*want_clusters += new_clusters;
> +		}
>  	}
>  	return ret;
>  }
> @@ -680,12 +687,16 @@ void ocfs2_calc_xattr_init(struct inode *dir, umode_t mode,
>  			   int *want_clusters, int *xattr_credits,
>  			   int *want_meta, struct ocfs2_acl_state *acl_state)
>  {
> +	int i;
>  	struct ocfs2_super *osb = OCFS2_SB(dir->i_sb);
>  	int s_size = 0, a_size = 0, acl_len = 0, new_clusters;
>  
> -	if (si->enable)
> -		s_size = ocfs2_xattr_entry_real_size(strlen(si->name),
> -						     si->value_len);
> +	if (si->enable) {
> +		for (i = 0; i < si->count; i++)
> +			s_size += ocfs2_xattr_entry_real_size(
> +					strlen(si->xattrs[i].name),
> +					si->xattrs[i].value_len);
> +	}
>  
>  	if (osb->s_mount_opt & OCFS2_MOUNT_POSIX_ACL) {
>  		if (acl_state->default_acl && S_ISDIR(mode)) {
> @@ -732,12 +743,14 @@ void ocfs2_calc_xattr_init(struct inode *dir, umode_t mode,
>  	 * reserve credits and clusters for xattrs which has large value
>  	 * and have to be set outside
>  	 */
> -	if (si->enable && si->value_len > OCFS2_XATTR_INLINE_SIZE) {
> -		new_clusters = ocfs2_clusters_for_bytes(dir->i_sb,
> -							si->value_len);
> -		*xattr_credits += ocfs2_clusters_to_blocks(dir->i_sb,
> -							   new_clusters);
> -		*want_clusters += new_clusters;
> +	for (i = 0; si->enable && i < si->count; i++) {
> +		if (si->xattrs[i].value_len > OCFS2_XATTR_INLINE_SIZE) {
> +			new_clusters = ocfs2_clusters_for_bytes(dir->i_sb,
> +						si->xattrs[i].value_len);
> +			*xattr_credits += ocfs2_clusters_to_blocks(dir->i_sb,
> +								   new_clusters);
> +			*want_clusters += new_clusters;
> +		}
>  	}
>  	if (osb->s_mount_opt & OCFS2_MOUNT_POSIX_ACL) {
>  		if (acl_state->default_acl && S_ISDIR(mode)) {
> @@ -7630,17 +7643,46 @@ static int ocfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array
>  {
>  	struct ocfs2_security_xattr_info *si = fs_info;
>  	const struct xattr *xattr;
> +	struct ocfs2_security_xattr *xattrs;
> +	int count = 0, i;
>  	int err = 0;
>  
>  	if (si) {
> -		si->value = kmemdup(xattr_array->value, xattr_array->value_len,
> -				    GFP_KERNEL);
> -		if (!si->value)
> +		for (xattr = xattr_array; xattr->name != NULL; xattr++)
> +			count++;
> +
> +		xattrs = kcalloc(count, sizeof(*xattrs), GFP_NOFS);
> +		if (!xattrs)
>  			return -ENOMEM;
>  
> -		si->name = xattr_array->name;
> -		si->value_len = xattr_array->value_len;
> +		for (i = 0; i < count; i++) {
> +			xattrs[i].name = kstrdup(xattr_array[i].name, GFP_NOFS);
> +			if (!xattrs[i].name) {
> +				err = -ENOMEM;
> +				goto out_err;
> +			}
> +
> +			xattrs[i].value = kmemdup(xattr_array[i].value,
> +						  xattr_array[i].value_len,
> +						  GFP_NOFS);
> +			if (!xattrs[i].value) {
> +				err = -ENOMEM;
> +				goto out_err;
> +			}
> +			xattrs[i].value_len = xattr_array[i].value_len;
> +		}
> +
> +		si->xattrs = xattrs;
> +		si->count = count;
>  		return 0;
> +
> +out_err:
> +		for (; i >= 0; i--) {
> +			kfree(xattrs[i].name);
> +			kfree(xattrs[i].value);
> +		}
> +		kfree(xattrs);
> +		return err;
>  	}
>  
>  	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> @@ -7653,6 +7695,19 @@ static int ocfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array
>  	return err;
>  }
>  
> +void ocfs2_free_security_xattrs(struct ocfs2_security_xattr_info *si)
> +{
> +	int i;
> +
> +	for (i = 0; i < si->count; i++) {
> +		kfree(si->xattrs[i].name);
> +		kfree(si->xattrs[i].value);
> +	}
> +	kfree(si->xattrs);
> +	si->xattrs = NULL;
> +	si->count = 0;
> +}
> +
>  int ocfs2_init_security_get(struct inode *inode,
>  			    struct inode *dir,
>  			    const struct qstr *qstr,
> @@ -7670,7 +7725,7 @@ int ocfs2_init_security_get(struct inode *inode,
>  		 * security_inode_init_security() does not return -EOPNOTSUPP,
>  		 * we have to check the xattr ourselves.
>  		 */
> -		if (!ret && !si->name)
> +		if (!ret && !si->count)
>  			si->enable = 0;
>  
>  		return ret;
> @@ -7687,10 +7742,21 @@ int ocfs2_init_security_set(handle_t *handle,
>  			    struct ocfs2_alloc_context *xattr_ac,
>  			    struct ocfs2_alloc_context *data_ac)
>  {
> -	return ocfs2_xattr_set_handle(handle, inode, di_bh,
> -				     OCFS2_XATTR_INDEX_SECURITY,
> -				     si->name, si->value, si->value_len, 0,
> -				     xattr_ac, data_ac);
> +	int i;
> +	int ret = 0;
> +
> +	for (i = 0; i < si->count; i++) {
> +		ret = ocfs2_xattr_set_handle(handle, inode, di_bh,
> +					     OCFS2_XATTR_INDEX_SECURITY,
> +					     si->xattrs[i].name,
> +					     si->xattrs[i].value,
> +					     si->xattrs[i].value_len, 0,
> +					     xattr_ac, data_ac);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
>  }
>  
>  const struct xattr_handler ocfs2_xattr_security_handler = {
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index 887cc1a18b1a..1dd08f979e3c 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -20,13 +20,18 @@ enum ocfs2_xattr_type {
>  	OCFS2_XATTR_MAX
>  };
>  
> -struct ocfs2_security_xattr_info {
> -	int enable;
> -	const char *name;
> +struct ocfs2_security_xattr {
> +	char *name;
>  	void *value;
>  	size_t value_len;
>  };
>  
> +struct ocfs2_security_xattr_info {
> +	int enable;
> +	struct ocfs2_security_xattr *xattrs;
> +	int count;
> +};
> +
>  extern const struct xattr_handler ocfs2_xattr_user_handler;
>  extern const struct xattr_handler ocfs2_xattr_trusted_handler;
>  extern const struct xattr_handler ocfs2_xattr_security_handler;
> @@ -49,6 +54,7 @@ int ocfs2_xattr_remove(struct inode *, struct buffer_head *);
>  int ocfs2_init_security_get(struct inode *, struct inode *,
>  			    const struct qstr *,
>  			    struct ocfs2_security_xattr_info *);
> +void ocfs2_free_security_xattrs(struct ocfs2_security_xattr_info *);
>  int ocfs2_init_security_set(handle_t *, struct inode *,
>  			    struct buffer_head *,
>  			    struct ocfs2_security_xattr_info *,
> -- 
> 2.39.3
>