[PATCH 1/1] staging: lustre/lustre/llite: get rid of incorrect type warning

[PATCH 1/1] staging: lustre/lustre/llite: get rid of incorrect type warning

[Tolga Ceylan]

In dir.c and llite_lib.c, sparse reports multiple warnings messages
due to different address spaces. This patch resolves these warnings
by adding the tag __user for username addresses.

Signed-off-by: Tolga Ceylan <tolga.ceylan@gmail.com>
---
 drivers/staging/lustre/lustre/llite/dir.c       | 78 +++++++++++++------------
 drivers/staging/lustre/lustre/llite/llite_lib.c |  8 +--
 2 files changed, 45 insertions(+), 41 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
index 4b0de8d..0441c20 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -1258,7 +1258,7 @@ static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		return ll_iocontrol(inode, file, cmd, arg);
 	case FSFILT_IOC_GETVERSION_OLD:
 	case FSFILT_IOC_GETVERSION:
-		return put_user(inode->i_generation, (int *)arg);
+		return put_user(inode->i_generation, (int __user *)arg);
 	/* We need to special case any other ioctls we want to handle,
 	 * to send them to the MDS/OST as appropriate and to properly
 	 * network encode the arg field.
@@ -1272,7 +1272,7 @@ static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (mdtidx < 0)
 			return mdtidx;
 
-		if (put_user((int)mdtidx, (int *)arg))
+		if (put_user((int)mdtidx, (int __user *)arg))
 			return -EFAULT;
 
 		return 0;
@@ -1369,8 +1369,10 @@ lmv_out_free:
 	case LL_IOC_LOV_SETSTRIPE: {
 		struct lov_user_md_v3 lumv3;
 		struct lov_user_md_v1 *lumv1 = (struct lov_user_md_v1 *)&lumv3;
-		struct lov_user_md_v1 *lumv1p = (struct lov_user_md_v1 *)arg;
-		struct lov_user_md_v3 *lumv3p = (struct lov_user_md_v3 *)arg;
+		struct lov_user_md_v1 __user *lumv1p =
+			(struct lov_user_md_v1 __user *)arg;
+		struct lov_user_md_v3 __user *lumv3p =
+			(struct lov_user_md_v3 __user *)arg;
 
 		int set_default = 0;
 
@@ -1395,7 +1397,8 @@ lmv_out_free:
 		return rc;
 	}
 	case LL_IOC_LMV_GETSTRIPE: {
-		struct lmv_user_md *lump = (struct lmv_user_md *)arg;
+		struct lmv_user_md __user *lump =
+			(struct lmv_user_md __user *)arg;
 		struct lmv_user_md lum;
 		struct lmv_user_md *tmp;
 		int lum_size;
@@ -1428,7 +1431,7 @@ lmv_out_free:
 		tmp->lum_objects[0].lum_mds = mdtindex;
 		memcpy(&tmp->lum_objects[0].lum_fid, ll_inode2fid(inode),
 		       sizeof(struct lu_fid));
-		if (copy_to_user((void *)arg, tmp, lum_size)) {
+		if (copy_to_user((void __user *)arg, tmp, lum_size)) {
 			rc = -EFAULT;
 			goto free_lmv;
 		}
@@ -1449,7 +1452,7 @@ free_lmv:
 		if (!(exp_connect_flags(sbi->ll_md_exp) & OBD_CONNECT_LVB_TYPE))
 			return -ENOTSUPP;
 
-		filename = ll_getname((const char *)arg);
+		filename = ll_getname((const char __user *)arg);
 		if (IS_ERR(filename))
 			return PTR_ERR(filename);
 
@@ -1474,7 +1477,7 @@ out_rmdir:
 	case IOC_MDC_GETFILEINFO:
 	case IOC_MDC_GETFILESTRIPE: {
 		struct ptlrpc_request *request = NULL;
-		struct lov_user_md *lump;
+		struct lov_user_md __user *lump;
 		struct lov_mds_md *lmm = NULL;
 		struct mdt_body *body;
 		char *filename = NULL;
@@ -1482,7 +1485,7 @@ out_rmdir:
 
 		if (cmd == IOC_MDC_GETFILEINFO ||
 		    cmd == IOC_MDC_GETFILESTRIPE) {
-			filename = ll_getname((const char *)arg);
+			filename = ll_getname((const char __user *)arg);
 			if (IS_ERR(filename))
 				return PTR_ERR(filename);
 
@@ -1511,11 +1514,11 @@ out_rmdir:
 
 		if (cmd == IOC_MDC_GETFILESTRIPE ||
 		    cmd == LL_IOC_LOV_GETSTRIPE) {
-			lump = (struct lov_user_md *)arg;
+			lump = (struct lov_user_md __user *)arg;
 		} else {
-			struct lov_user_mds_data *lmdp;
+			struct lov_user_mds_data __user *lmdp;
 
-			lmdp = (struct lov_user_mds_data *)arg;
+			lmdp = (struct lov_user_mds_data __user *)arg;
 			lump = &lmdp->lmd_lmm;
 		}
 		if (copy_to_user(lump, lmm, lmmsize)) {
@@ -1527,7 +1530,7 @@ out_rmdir:
 		}
 skip_lmm:
 		if (cmd == IOC_MDC_GETFILEINFO || cmd == LL_IOC_MDC_GETINFO) {
-			struct lov_user_mds_data *lmdp;
+			struct lov_user_mds_data __user *lmdp;
 			lstat_t st = { 0 };
 
 			st.st_dev     = inode->i_sb->s_dev;
@@ -1544,7 +1547,7 @@ skip_lmm:
 			st.st_ctime   = body->ctime;
 			st.st_ino     = inode->i_ino;
 
-			lmdp = (struct lov_user_mds_data *)arg;
+			lmdp = (struct lov_user_mds_data __user *)arg;
 			if (copy_to_user(&lmdp->lmd_st, &st, sizeof(st))) {
 				rc = -EFAULT;
 				goto out_req;
@@ -1558,14 +1561,14 @@ out_req:
 		return rc;
 	}
 	case IOC_LOV_GETINFO: {
-		struct lov_user_mds_data *lumd;
+		struct lov_user_mds_data __user *lumd;
 		struct lov_stripe_md *lsm;
-		struct lov_user_md *lum;
+		struct lov_user_md __user *lum;
 		struct lov_mds_md *lmm;
 		int lmmsize;
 		lstat_t st;
 
-		lumd = (struct lov_user_mds_data *)arg;
+		lumd = (struct lov_user_mds_data __user *)arg;
 		lum = &lumd->lmd_lmm;
 
 		rc = ll_get_max_mdsize(sbi, &lmmsize);
@@ -1671,8 +1674,8 @@ free_lmm:
 				   NULL);
 		if (rc) {
 			CDEBUG(D_QUOTA, "mdc ioctl %d failed: %d\n", cmd, rc);
-			if (copy_to_user((void *)arg, check,
-					     sizeof(*check)))
+			if (copy_to_user((void __user *)arg, check,
+					 sizeof(*check)))
 				CDEBUG(D_QUOTA, "copy_to_user failed\n");
 			goto out_poll;
 		}
@@ -1681,8 +1684,8 @@ free_lmm:
 				   NULL);
 		if (rc) {
 			CDEBUG(D_QUOTA, "osc ioctl %d failed: %d\n", cmd, rc);
-			if (copy_to_user((void *)arg, check,
-					     sizeof(*check)))
+			if (copy_to_user((void __user *)arg, check,
+					 sizeof(*check)))
 				CDEBUG(D_QUOTA, "copy_to_user failed\n");
 			goto out_poll;
 		}
@@ -1697,14 +1700,15 @@ out_poll:
 		if (!qctl)
 			return -ENOMEM;
 
-		if (copy_from_user(qctl, (void *)arg, sizeof(*qctl))) {
+		if (copy_from_user(qctl, (void __user *)arg, sizeof(*qctl))) {
 			rc = -EFAULT;
 			goto out_quotactl;
 		}
 
 		rc = quotactl_ioctl(sbi, qctl);
 
-		if (rc == 0 && copy_to_user((void *)arg, qctl, sizeof(*qctl)))
+		if (rc == 0 && copy_to_user((void __user *)arg, qctl,
+					    sizeof(*qctl)))
 			rc = -EFAULT;
 
 out_quotactl:
@@ -1734,7 +1738,7 @@ out_quotactl:
 		int count, vallen;
 		struct obd_export *exp;
 
-		if (copy_from_user(&count, (int *)arg, sizeof(int)))
+		if (copy_from_user(&count, (int __user *)arg, sizeof(int)))
 			return -EFAULT;
 
 		/* get ost count when count is zero, get mdt count otherwise */
@@ -1747,14 +1751,14 @@ out_quotactl:
 			return rc;
 		}
 
-		if (copy_to_user((int *)arg, &count, sizeof(int)))
+		if (copy_to_user((int __user *)arg, &count, sizeof(int)))
 			return -EFAULT;
 
 		return 0;
 	}
 	case LL_IOC_PATH2FID:
-		if (copy_to_user((void *)arg, ll_inode2fid(inode),
-				     sizeof(struct lu_fid)))
+		if (copy_to_user((void __user *)arg, ll_inode2fid(inode),
+				 sizeof(struct lu_fid)))
 			return -EFAULT;
 		return 0;
 	case LL_IOC_GET_CONNECT_FLAGS: {
@@ -1762,11 +1766,11 @@ out_quotactl:
 	}
 	case OBD_IOC_CHANGELOG_SEND:
 	case OBD_IOC_CHANGELOG_CLEAR:
-		rc = copy_and_ioctl(cmd, sbi->ll_md_exp, (void *)arg,
+		rc = copy_and_ioctl(cmd, sbi->ll_md_exp, (void __user *)arg,
 				    sizeof(struct ioc_changelog));
 		return rc;
 	case OBD_IOC_FID2PATH:
-		return ll_fid2path(inode, (void *)arg);
+		return ll_fid2path(inode, (void __user *)arg);
 	case LL_IOC_HSM_REQUEST: {
 		struct hsm_user_request	*hur;
 		ssize_t			 totalsize;
@@ -1776,7 +1780,7 @@ out_quotactl:
 			return -ENOMEM;
 
 		/* We don't know the true size yet; copy the fixed-size part */
-		if (copy_from_user(hur, (void *)arg, sizeof(*hur))) {
+		if (copy_from_user(hur, (void __user *)arg, sizeof(*hur))) {
 			kfree(hur);
 			return -EFAULT;
 		}
@@ -1796,7 +1800,7 @@ out_quotactl:
 			return -ENOMEM;
 
 		/* Copy the whole struct */
-		if (copy_from_user(hur, (void *)arg, totalsize)) {
+		if (copy_from_user(hur, (void __user *)arg, totalsize)) {
 			OBD_FREE_LARGE(hur, totalsize);
 			return -EFAULT;
 		}
@@ -1832,7 +1836,7 @@ out_quotactl:
 		struct hsm_progress_kernel	hpk;
 		struct hsm_progress		hp;
 
-		if (copy_from_user(&hp, (void *)arg, sizeof(hp)))
+		if (copy_from_user(&hp, (void __user *)arg, sizeof(hp)))
 			return -EFAULT;
 
 		hpk.hpk_fid = hp.hp_fid;
@@ -1849,7 +1853,7 @@ out_quotactl:
 		return rc;
 	}
 	case LL_IOC_HSM_CT_START:
-		rc = copy_and_ioctl(cmd, sbi->ll_md_exp, (void *)arg,
+		rc = copy_and_ioctl(cmd, sbi->ll_md_exp, (void __user *)arg,
 				    sizeof(struct lustre_kernelcomm));
 		return rc;
 
@@ -1860,13 +1864,13 @@ out_quotactl:
 		copy = kzalloc(sizeof(*copy), GFP_NOFS);
 		if (!copy)
 			return -ENOMEM;
-		if (copy_from_user(copy, (char *)arg, sizeof(*copy))) {
+		if (copy_from_user(copy, (char __user *)arg, sizeof(*copy))) {
 			kfree(copy);
 			return -EFAULT;
 		}
 
 		rc = ll_ioc_copy_start(inode->i_sb, copy);
-		if (copy_to_user((char *)arg, copy, sizeof(*copy)))
+		if (copy_to_user((char __user *)arg, copy, sizeof(*copy)))
 			rc = -EFAULT;
 
 		kfree(copy);
@@ -1879,13 +1883,13 @@ out_quotactl:
 		copy = kzalloc(sizeof(*copy), GFP_NOFS);
 		if (!copy)
 			return -ENOMEM;
-		if (copy_from_user(copy, (char *)arg, sizeof(*copy))) {
+		if (copy_from_user(copy, (char __user *)arg, sizeof(*copy))) {
 			kfree(copy);
 			return -EFAULT;
 		}
 
 		rc = ll_ioc_copy_end(inode->i_sb, copy);
-		if (copy_to_user((char *)arg, copy, sizeof(*copy)))
+		if (copy_to_user((char __user *)arg, copy, sizeof(*copy)))
 			rc = -EFAULT;
 
 		kfree(copy);
diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
index 2513988..4302041 100644
--- a/drivers/staging/lustre/lustre/llite/llite_lib.c
+++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
@@ -1834,14 +1834,14 @@ int ll_iocontrol(struct inode *inode, struct file *file,
 
 		ptlrpc_req_finished(req);
 
-		return put_user(flags, (int *)arg);
+		return put_user(flags, (int __user *)arg);
 	}
 	case FSFILT_IOC_SETFLAGS: {
 		struct lov_stripe_md *lsm;
 		struct obd_info oinfo = { { { 0 } } };
 		struct md_op_data *op_data;
 
-		if (get_user(flags, (int *)arg))
+		if (get_user(flags, (int __user *)arg))
 			return -EFAULT;
 
 		op_data = ll_prep_md_op_data(NULL, inode, NULL, NULL, 0, 0,
@@ -2273,8 +2273,8 @@ int ll_get_obd_name(struct inode *inode, unsigned int cmd, unsigned long arg)
 	if (!obd)
 		return -ENOENT;
 
-	if (copy_to_user((void *)arg, obd->obd_name,
-			     strlen(obd->obd_name) + 1))
+	if (copy_to_user((void __user *)arg, obd->obd_name,
+			 strlen(obd->obd_name) + 1))
 		return -EFAULT;
 
 	return 0;
-- 
2.4.2

Re: [PATCH 1/1] staging: lustre/lustre/llite: get rid of incorrect type warning

[Greg Kroah-Hartman]

On Mon, Jun 08, 2015 at 04:01:57PM -0700, Tolga Ceylan wrote:
> In dir.c and llite_lib.c, sparse reports multiple warnings messages
> due to different address spaces. This patch resolves these warnings
> by adding the tag __user for username addresses.
> 
> Signed-off-by: Tolga Ceylan <tolga.ceylan@gmail.com>

Are you sure all of these are correct?  The kernel/user api for lustre
is a complex beast, and just casting away the pointer types isn't
usually the proper thing to do in order to resolve the issues here.

thanks,

greg k-h

Re: [PATCH 1/1] staging: lustre/lustre/llite: get rid of incorrect type warning

[Tolga Ceylan]

On Wed, Jun 10, 2015 at 5:48 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Are you sure all of these are correct?  The kernel/user api for lustre
> is a complex beast, and just casting away the pointer types isn't
> usually the proper thing to do in order to resolve the issues here.
>
> thanks,
>
> greg k-h

I'm not 100% sure, but the pointers that I added the annotation to end
up being used as user memory. (eg. passed to copy_to_user, etc.)
Sometimes these pointers are passed to functions that already have
__user annotation in their signatures (eg. ll_getname, copy_and_ioctl,
ll_fid2path, etc.).

Re: [PATCH 1/1] staging: lustre/lustre/llite: get rid of incorrect type warning

[Dan Carpenter]

On Mon, Jun 08, 2015 at 04:01:57PM -0700, Tolga Ceylan wrote:
> In dir.c and llite_lib.c, sparse reports multiple warnings messages
> due to different address spaces. This patch resolves these warnings
> by adding the tag __user for username addresses.
> 
> Signed-off-by: Tolga Ceylan <tolga.ceylan@gmail.com>
> ---
>  drivers/staging/lustre/lustre/llite/dir.c       | 78 +++++++++++++------------
>  drivers/staging/lustre/lustre/llite/llite_lib.c |  8 +--
>  2 files changed, 45 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
> index 4b0de8d..0441c20 100644
> --- a/drivers/staging/lustre/lustre/llite/dir.c
> +++ b/drivers/staging/lustre/lustre/llite/dir.c
> @@ -1258,7 +1258,7 @@ static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		return ll_iocontrol(inode, file, cmd, arg);
>  	case FSFILT_IOC_GETVERSION_OLD:
>  	case FSFILT_IOC_GETVERSION:
> -		return put_user(inode->i_generation, (int *)arg);
> +		return put_user(inode->i_generation, (int __user *)arg);

I have looked at this briefly and I think the correct fix is to make
ll_dir_ioctl() take a void __user *arg instead of an unsigned long arg.

Of course, doing that will introduce more sparse warnings, but those are
correct warnings and we should be warned about them.

Maybe start at the lower level functions like obd_ioctl_getdata() and
obd_ioctl_popdata() and add annotations then work up to the big
functions like ll_dir_ioctl().

We shouldn't be introducing these little casts throughout.

regards,
dan carpenter

RE: [HPDD-discuss] [PATCH 1/1] staging: lustre/lustre/llite: get rid of incorrect type warning

[Simmons, James A.]

>>On Wed, Jun 10, 2015 at 5:48 PM, Greg Kroah-Hartman
><gregkh@linuxfoundation.org> wrote:
>>
>> Are you sure all of these are correct?  The kernel/user api for lustre
>> is a complex beast, and just casting away the pointer types isn't
>> usually the proper thing to do in order to resolve the issues here.
>>
>> thanks,
>>
>> greg k-h
>
>I'm not 100% sure, but the pointers that I added the annotation to end
>up being used as user memory. (eg. passed to copy_to_user, etc.)
>Sometimes these pointers are passed to functions that already have
>__user annotation in their signatures (eg. ll_getname, copy_and_ioctl,
>ll_fid2path, etc.).

Using these simple cast are not the proper fix. We had a lot of issues with user
land tools breaking due to leakage of kernel space stuff and other problems.
Some work went into cleaning that up in the OpenSFS branch but it is not totally
complete yet. Evans you wanted something challenging to work on well this is
up your alley. I would recommend looking at JIRA ticket LU-6401 and all its sub
tickets.  You could start the port of those to the upstream client. At the same
time we can finish the cleanup in the OpenSFS branch as well.