Re: [PATCH 06/31] xfs: factor out a helper for a single XFS_IOC_ATTRMULTI_BY_HANDLE op

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Feb 21, 2020 at 06:11:29AM -0800, Christoph Hellwig wrote:
> Add a new helper to handle a single attr multi ioctl operation that
> can be shared between the native and compat ioctl implementation.
> 
> There is a slight change in heavior in that we don't break out of the
> loop when copying in the attribute name fails.  The previous behavior
> was rather inconsistent here as it continued for any other kind of
> error, and that we don't clear the flags in the structure returned
> to userspace, a behavior only introduced as a bug fix in the last
> merge window.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Chandan Rajendra <chandanrlinux@xxxxxxxxx>

Seems pretty straightforward,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>

--D

> ---
>  fs/xfs/xfs_ioctl.c   | 97 +++++++++++++++++++++++---------------------
>  fs/xfs/xfs_ioctl.h   | 18 ++------
>  fs/xfs/xfs_ioctl32.c | 50 +++--------------------
>  3 files changed, 59 insertions(+), 106 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index bb490a954c0b..b17458c8947e 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -349,7 +349,7 @@ xfs_attrlist_by_handle(
>  	return error;
>  }
>  
> -int
> +static int
>  xfs_attrmulti_attr_get(
>  	struct inode		*inode,
>  	unsigned char		*name,
> @@ -381,7 +381,7 @@ xfs_attrmulti_attr_get(
>  	return error;
>  }
>  
> -int
> +static int
>  xfs_attrmulti_attr_set(
>  	struct inode		*inode,
>  	unsigned char		*name,
> @@ -412,6 +412,51 @@ xfs_attrmulti_attr_set(
>  	return error;
>  }
>  
> +int
> +xfs_ioc_attrmulti_one(
> +	struct file		*parfilp,
> +	struct inode		*inode,
> +	uint32_t		opcode,
> +	void __user		*uname,
> +	void __user		*value,
> +	uint32_t		*len,
> +	uint32_t		flags)
> +{
> +	unsigned char		*name;
> +	int			error;
> +
> +	if ((flags & ATTR_ROOT) && (flags & ATTR_SECURE))
> +		return -EINVAL;
> +	flags &= ~ATTR_KERNEL_FLAGS;
> +
> +	name = strndup_user(uname, MAXNAMELEN);
> +	if (IS_ERR(name))
> +		return PTR_ERR(name);
> +
> +	switch (opcode) {
> +	case ATTR_OP_GET:
> +		error = xfs_attrmulti_attr_get(inode, name, value, len, flags);
> +		break;
> +	case ATTR_OP_REMOVE:
> +		value = NULL;
> +		*len = 0;
> +		/* fall through */
> +	case ATTR_OP_SET:
> +		error = mnt_want_write_file(parfilp);
> +		if (error)
> +			break;
> +		error = xfs_attrmulti_attr_set(inode, name, value, *len, flags);
> +		mnt_drop_write_file(parfilp);
> +		break;
> +	default:
> +		error = -EINVAL;
> +		break;
> +	}
> +
> +	kfree(name);
> +	return error;
> +}
> +
>  STATIC int
>  xfs_attrmulti_by_handle(
>  	struct file		*parfilp,
> @@ -422,7 +467,6 @@ xfs_attrmulti_by_handle(
>  	xfs_fsop_attrmulti_handlereq_t am_hreq;
>  	struct dentry		*dentry;
>  	unsigned int		i, size;
> -	unsigned char		*attr_name;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> @@ -450,49 +494,10 @@ xfs_attrmulti_by_handle(
>  
>  	error = 0;
>  	for (i = 0; i < am_hreq.opcount; i++) {
> -		if ((ops[i].am_flags & ATTR_ROOT) &&
> -		    (ops[i].am_flags & ATTR_SECURE)) {
> -			ops[i].am_error = -EINVAL;
> -			continue;
> -		}
> -		ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
> -
> -		attr_name = strndup_user(ops[i].am_attrname, MAXNAMELEN);
> -		if (IS_ERR(attr_name)) {
> -			ops[i].am_error = PTR_ERR(attr_name);
> -			break;
> -		}
> -
> -		switch (ops[i].am_opcode) {
> -		case ATTR_OP_GET:
> -			ops[i].am_error = xfs_attrmulti_attr_get(
> -					d_inode(dentry), attr_name,
> -					ops[i].am_attrvalue, &ops[i].am_length,
> -					ops[i].am_flags);
> -			break;
> -		case ATTR_OP_SET:
> -			ops[i].am_error = mnt_want_write_file(parfilp);
> -			if (ops[i].am_error)
> -				break;
> -			ops[i].am_error = xfs_attrmulti_attr_set(
> -					d_inode(dentry), attr_name,
> -					ops[i].am_attrvalue, ops[i].am_length,
> -					ops[i].am_flags);
> -			mnt_drop_write_file(parfilp);
> -			break;
> -		case ATTR_OP_REMOVE:
> -			ops[i].am_error = mnt_want_write_file(parfilp);
> -			if (ops[i].am_error)
> -				break;
> -			ops[i].am_error = xfs_attrmulti_attr_set(
> -					d_inode(dentry), attr_name, NULL, 0,
> -					ops[i].am_flags);
> -			mnt_drop_write_file(parfilp);
> -			break;
> -		default:
> -			ops[i].am_error = -EINVAL;
> -		}
> -		kfree(attr_name);
> +		ops[i].am_error = xfs_ioc_attrmulti_one(parfilp,
> +				d_inode(dentry), ops[i].am_opcode,
> +				ops[i].am_attrname, ops[i].am_attrvalue,
> +				&ops[i].am_length, ops[i].am_flags);
>  	}
>  
>  	if (copy_to_user(am_hreq.ops, ops, size))
> diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
> index 819504df00ae..bb50cb3dc61f 100644
> --- a/fs/xfs/xfs_ioctl.h
> +++ b/fs/xfs/xfs_ioctl.h
> @@ -30,21 +30,9 @@ xfs_readlink_by_handle(
>  	struct file		*parfilp,
>  	xfs_fsop_handlereq_t	*hreq);
>  
> -extern int
> -xfs_attrmulti_attr_get(
> -	struct inode		*inode,
> -	unsigned char		*name,
> -	unsigned char		__user *ubuf,
> -	uint32_t		*len,
> -	uint32_t		flags);
> -
> -extern int
> -xfs_attrmulti_attr_set(
> -	struct inode		*inode,
> -	unsigned char		*name,
> -	const unsigned char	__user *ubuf,
> -	uint32_t		len,
> -	uint32_t		flags);
> +int xfs_ioc_attrmulti_one(struct file *parfilp, struct inode *inode,
> +		uint32_t opcode, void __user *uname, void __user *value,
> +		uint32_t *len, uint32_t flags);
>  
>  extern struct dentry *
>  xfs_handle_to_dentry(
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index 936c2f62fb6c..e1daf095c585 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -418,7 +418,6 @@ xfs_compat_attrmulti_by_handle(
>  	compat_xfs_fsop_attrmulti_handlereq_t	am_hreq;
>  	struct dentry				*dentry;
>  	unsigned int				i, size;
> -	unsigned char				*attr_name;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> @@ -447,50 +446,11 @@ xfs_compat_attrmulti_by_handle(
>  
>  	error = 0;
>  	for (i = 0; i < am_hreq.opcount; i++) {
> -		if ((ops[i].am_flags & ATTR_ROOT) &&
> -		    (ops[i].am_flags & ATTR_SECURE)) {
> -			ops[i].am_error = -EINVAL;
> -			continue;
> -		}
> -		ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
> -
> -		attr_name = strndup_user(compat_ptr(ops[i].am_attrname),
> -				MAXNAMELEN);
> -		if (IS_ERR(attr_name)) {
> -			ops[i].am_error = PTR_ERR(attr_name);
> -			break;
> -		}
> -
> -		switch (ops[i].am_opcode) {
> -		case ATTR_OP_GET:
> -			ops[i].am_error = xfs_attrmulti_attr_get(
> -					d_inode(dentry), attr_name,
> -					compat_ptr(ops[i].am_attrvalue),
> -					&ops[i].am_length, ops[i].am_flags);
> -			break;
> -		case ATTR_OP_SET:
> -			ops[i].am_error = mnt_want_write_file(parfilp);
> -			if (ops[i].am_error)
> -				break;
> -			ops[i].am_error = xfs_attrmulti_attr_set(
> -					d_inode(dentry), attr_name,
> -					compat_ptr(ops[i].am_attrvalue),
> -					ops[i].am_length, ops[i].am_flags);
> -			mnt_drop_write_file(parfilp);
> -			break;
> -		case ATTR_OP_REMOVE:
> -			ops[i].am_error = mnt_want_write_file(parfilp);
> -			if (ops[i].am_error)
> -				break;
> -			ops[i].am_error = xfs_attrmulti_attr_set(
> -					d_inode(dentry), attr_name, NULL, 0,
> -					ops[i].am_flags);
> -			mnt_drop_write_file(parfilp);
> -			break;
> -		default:
> -			ops[i].am_error = -EINVAL;
> -		}
> -		kfree(attr_name);
> +		ops[i].am_error = xfs_ioc_attrmulti_one(parfilp,
> +				d_inode(dentry), ops[i].am_opcode,
> +				compat_ptr(ops[i].am_attrname),
> +				compat_ptr(ops[i].am_attrvalue),
> +				&ops[i].am_length, ops[i].am_flags);
>  	}
>  
>  	if (copy_to_user(compat_ptr(am_hreq.ops), ops, size))
> -- 
> 2.24.1
> 



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux