[Ocfs2-devel] [PATCH 1/2] Ocfs2: Add new ioctls prototype and corresponding data structure to ocfs2 header.

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Add new ioctls prototype and corresponding data structure to ocfs2 header.

[Tristan Ye]

We use OCFS2_IOC_INFO to manipulate the new ioctl, the requests can
be stored in a array of (struct ocfs2_info_request), which should be
NULL-terminated, kernel therefore can recoginze and fill them one by
one.

The reason why we need these ioctls is to offer the none-privileged
end-user a possibility to get filesys info gathering.

Idea here is to make the spearated request small enough to guarantee
a better backward&forward compatibility since a small piece of request
would be less likely to be broken if filesys on raw disk get changed.

Currently, the first version include following request type:

	OCFS2_INFO_CLUSTERSIZE,
	OCFS2_INFO_BLOCKSIZE,
	OCFS2_INFO_SLOTNUM,
	OCFS2_INFO_LABEL,
	OCFS2_INFO_UUID,
	OCFS2_INFO_FS_FEATURES

It may be grown from time to time:)

Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
 fs/ocfs2/ocfs2_fs.h |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/fs/ocfs2/ocfs2_fs.h b/fs/ocfs2/ocfs2_fs.h
index e9431e4..dac7f4c 100644
--- a/fs/ocfs2/ocfs2_fs.h
+++ b/fs/ocfs2/ocfs2_fs.h
@@ -309,6 +309,75 @@ struct reflink_arguments {
 };
 #define OCFS2_IOC_REFLINK	_IOW('o', 4, struct reflink_arguments)
 
+#define OCFS2_UUID_LEN              16
+#define OCFS2_MAX_LABEL_LEN         64
+
+/*
+ * Used to query fs information for none-privileged 
+ * users by using OCFS2_IOC_INFO ioctols, a series of
+ * following requests need to be NULL-terminated.
+ *
+ * Always try to separate info request into small pieces to
+ * guarantee the backward&forward compatibility.
+ */
+struct ocfs2_info_request {
+	__u32 ir_code;	/* Info request code */
+	__u32 ir_flags; /* Request flags */
+};
+
+struct ocfs2_info_request_clustersize {
+	struct ocfs2_info_request ir_request;
+	__u32 ir_clustersize;
+};
+
+struct ocfs2_info_request_blocksize {
+	struct ocfs2_info_request ir_request;
+	__u32 ir_blocksize;
+};
+
+struct ocfs2_info_request_slotnum {
+	struct ocfs2_info_request ir_request;
+	__u16 ir_slotnum;
+};
+
+struct ocfs2_info_request_label {
+	struct ocfs2_info_request ir_request;
+	__u8	ir_label[OCFS2_MAX_LABEL_LEN];
+};
+
+struct ocfs2_info_request_uuid {
+	struct ocfs2_info_request ir_request;
+	__u8	ir_uuid[OCFS2_UUID_LEN];
+};
+
+struct ocfs2_info_request_fs_features {
+	struct ocfs2_info_request ir_quest;
+	__u32 ir_compat_features;
+	__u32 ir_incompat_features;
+	__u32 ir_ro_compat_features;
+};
+
+/* Codes for cfs2_info_request */
+
+#define OCFS2_INFO_CLUSTERSIZE	0x00000001
+#define OCFS2_INFO_BLOCKSIZE	0x00000002
+#define OCFS2_INFO_SLOTNUM      0x00000004
+#define OCFS2_INFO_LABEL        0x00000008
+#define OCFS2_INFO_UUID         0x00000010
+#define OCFS2_INFO_FS_FEATURES	0x00000020
+
+/* Flags for struct ocfs2_info_request */
+/* Filled by the caller */
+#define OCFS2_INFO_FL_NON_COHERENT  0x00000001  /* Cluster coherency not required.
+						   This is a hint.  It is up to
+						   ocfs2 whether the request can
+						   be fulfilled without locking. */
+/* Filled by ocfs2 */
+#define OCFS2_INFO_FL_FILLED        0x80000000  /* Filesystem understood this
+						   request and filled in the
+						   answer */
+
+#define OCFS2_IOC_INFO		_IOR('o', 5, struct ocfs2_info_request)
 
 /*
  * Journal Flags (ocfs2_dinode.id1.journal1.i_flags)
-- 
1.5.5

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Tristan Ye]

We implement the request as simple as possible to avoid
a possible extending if disk format get changed.

Completenss of a request include the info required and a flag(FL_FILLED)
filled being returned to the end-user.

Note: the requests in a series from userspace should be NULL-terminated.

Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
 fs/ocfs2/ioctl.c |  184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 184 insertions(+), 0 deletions(-)

diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
index 31fbb06..0ac5218 100644
--- a/fs/ocfs2/ioctl.c
+++ b/fs/ocfs2/ioctl.c
@@ -108,6 +108,188 @@ bail:
 	return status;
 }
 
+static int ocfs2_get_request_info(struct inode *inode,
+				  unsigned long arg)
+{
+
+	int ret = 0, i = 0, num_reqs = 0;
+
+	struct ocfs2_info_request req, *preq = NULL;
+	struct ocfs2_info_request **reqs;
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+
+	typedef struct ocfs2_info_request OIR, *POIR;
+	typedef struct ocfs2_info_request_clustersize OIRC, *POIRC;
+	typedef struct ocfs2_info_request_blocksize OIRB, *POIRB;
+	typedef struct ocfs2_info_request_slotnum OIRS, *POIRS;
+	typedef struct ocfs2_info_request_label OIRL, *POIRL;
+	typedef struct ocfs2_info_request_uuid OIRU, *POIRU;
+	typedef struct ocfs2_info_request_fs_features OIRF, *POIRF;
+
+	/*
+	 * The requests series from userspace need to be NULL-terminated.
+	 */
+	do {
+		preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
+		if (!preq)
+			break;
+		i++;
+
+	} while (preq);
+
+	num_reqs = i;
+
+	reqs = kmalloc(sizeof(POIR) * num_reqs, GFP_KERNEL);
+	if (!reqs) {
+		ret = -ENOMEM;
+		goto bail;
+	}
+
+	ret = ocfs2_inode_lock(inode, NULL, 0);
+	if (ret < 0) {
+		mlog_errno(ret);
+		goto bail;
+	}
+
+	for (i = 0; i < num_reqs; i++) {
+
+		reqs[i] = NULL;
+
+		preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
+
+		if (copy_from_user(&req, preq, sizeof(req))) {
+			ret = -EFAULT;
+			goto bail;
+		}
+
+		switch (req.ir_code) {
+		case OCFS2_INFO_CLUSTERSIZE:
+			reqs[i] = kmalloc(sizeof(OIRC), GFP_KERNEL);
+
+			if (copy_from_user(reqs[i], preq, sizeof(OIRC))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			((POIRC)(reqs[i]))->ir_clustersize = osb->s_clustersize;
+			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
+
+			if (copy_to_user(preq, reqs[i], sizeof(OIRC))) {
+				ret = EFAULT;
+				goto bail;
+			}
+
+			break;
+		case OCFS2_INFO_BLOCKSIZE:
+			reqs[i] = kmalloc(sizeof(OIRB), GFP_KERNEL);
+
+			if (copy_from_user(reqs[i], preq, sizeof(OIRB))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			((POIRB)(reqs[i]))->ir_blocksize =
+					inode->i_sb->s_blocksize;
+			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
+
+			if (copy_to_user(preq, reqs[i], sizeof(OIRB))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			break;
+		case OCFS2_INFO_SLOTNUM:
+			reqs[i] = kmalloc(sizeof(OIRS), GFP_KERNEL);
+
+			if (copy_from_user(reqs[i], preq, sizeof(OIRS))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			((POIRS)(reqs[i]))->ir_slotnum = osb->max_slots;
+			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
+
+			if (copy_to_user(preq, reqs[i], sizeof(OIRS))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			break;
+		case OCFS2_INFO_LABEL:
+			reqs[i] = kmalloc(sizeof(OIRL), GFP_KERNEL);
+
+			if (copy_from_user(reqs[i], preq, sizeof(OIRL))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			memmove(((POIRL)(reqs[i]))->ir_label, osb->vol_label,
+				OCFS2_MAX_VOL_LABEL_LEN);
+			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
+
+			if (copy_to_user(preq, reqs[i], sizeof(OIRL))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			break;
+		case OCFS2_INFO_UUID:
+			reqs[i] = kmalloc(sizeof(OIRU), GFP_KERNEL);
+
+			if (copy_from_user(reqs[i], preq, sizeof(OIRU))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			memmove(((POIRU)(reqs[i]))->ir_uuid, osb->uuid,
+				OCFS2_VOL_UUID_LEN);
+			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
+
+			if (copy_to_user(preq, reqs[i], sizeof(OIRU))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			break;
+		case OCFS2_INFO_FS_FEATURES:
+			reqs[i] = kmalloc(sizeof(OIRF), GFP_KERNEL);
+
+			if (copy_from_user(reqs[i], preq, sizeof(OIRF))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			((POIRF)(reqs[i]))->ir_compat_features =
+						osb->s_feature_compat;
+			((POIRF)(reqs[i]))->ir_incompat_features =
+						osb->s_feature_incompat;
+			((POIRF)(reqs[i]))->ir_ro_compat_features =
+						osb->s_feature_ro_compat;
+			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
+
+			if (copy_to_user(preq, reqs[i], sizeof(OIRF))) {
+				ret = -EFAULT;
+				goto bail;
+			}
+
+			break;
+		default:
+			break;
+		}
+
+	}
+bail:
+	for (i = 0; i < num_reqs; i++)
+		kfree(reqs[i]);
+
+	kfree(reqs);
+
+	ocfs2_inode_unlock(inode, 0);
+
+	mlog_exit(ret);
+	return ret;
+}
+
 long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct inode *inode = filp->f_path.dentry->d_inode;
@@ -173,6 +355,8 @@ long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		preserve = (args.preserve != 0);
 
 		return ocfs2_reflink_ioctl(inode, old_path, new_path, preserve);
+	case OCFS2_IOC_INFO:
+		return ocfs2_get_request_info(inode, arg);
 	default:
 		return -ENOTTY;
 	}
-- 
1.5.5

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Andi Kleen]

Tristan Ye <tristan.ye@oracle.com> writes:
> +
> +	/*
> +	 * The requests series from userspace need to be NULL-terminated.
> +	 */
> +	do {
> +		preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
> +		if (!preq)
> +			break;
> +		i++;

That's the first security leak. Can be used to probe arbitary memory.
You always need to use *_user for any user space access.

> +
> +	} while (preq);
> +
> +	num_reqs = i;
> +
> +	reqs = kmalloc(sizeof(POIR) * num_reqs, GFP_KERNEL);

This is next root exploit. Think what happens when the user passes a very 
large number for num_reqs that overflows the multiplication.

If anything use kcalloc(). And limit the maximum size.

It's unclear why you just can't use separate ioctls for each request.

-Andi
-- 
ak at linux.intel.com -- Speaking for myself only.

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Tristan]

Andi Kleen wrote:
> Tristan Ye <tristan.ye@oracle.com> writes:
>   
>> +
>> +	/*
>> +	 * The requests series from userspace need to be NULL-terminated.
>> +	 */
>> +	do {
>> +		preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
>> +		if (!preq)
>> +			break;
>> +		i++;
>>     
>
> That's the first security leak. Can be used to probe arbitary memory.
> You always need to use *_user for any user space access.
>   

Thank you for pointing this out, I'm now still learning how to hack the 
kernel codes:-)

>   
>> +
>> +	} while (preq);
>> +
>> +	num_reqs = i;
>> +
>> +	reqs = kmalloc(sizeof(POIR) * num_reqs, GFP_KERNEL);
>>     
>
> This is next root exploit. Think what happens when the user passes a very 
> large number for num_reqs that overflows the multiplication.
>
> If anything use kcalloc(). And limit the maximum size.
>   

Good point, I've checked the code, there is no need to allocate a 
pointer array for reqs.

> It's unclear why you just can't use separate ioctls for each request.
>   

A simple reason why we used NULL-terminated array to combine request 
pieces is to avoid a mass propagation of these ioctls, you know we may 
add such separated ioctols much frequently, end-user also I guess would 
be glad to tie all their requsets of this kind to one series, and 
deliver them only once.

> -Andi
>   
Tristan.

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Joel Becker]

On Fri, Nov 27, 2009 at 09:23:36PM +0100, Andi Kleen wrote:
> It's unclear why you just can't use separate ioctls for each request.

	Because we're going to be adding many info requests, with more
over time.  Much nicer to pass in a list of requests than have a hundred
or more ioctls.  Plus we can do fun versioning.

Joel

-- 

"Sometimes I think the surest sign intelligent
 life exists elsewhere in the universe is that
 none of it has tried to contact us."
                                -Calvin & Hobbes

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Add new ioctls prototype and corresponding data structure to ocfs2 header.

[Sunil Mushran]

Tristan Ye wrote:
> We use OCFS2_IOC_INFO to manipulate the new ioctl, the requests can
> be stored in a array of (struct ocfs2_info_request), which should be
> NULL-terminated, kernel therefore can recoginze and fill them one by
> one.
>
> The reason why we need these ioctls is to offer the none-privileged
> end-user a possibility to get filesys info gathering.
>
> Idea here is to make the spearated request small enough to guarantee
> a better backward&forward compatibility since a small piece of request
> would be less likely to be broken if filesys on raw disk get changed.
>
> Currently, the first version include following request type:
>
> 	OCFS2_INFO_CLUSTERSIZE,
> 	OCFS2_INFO_BLOCKSIZE,
> 	OCFS2_INFO_SLOTNUM,
> 	OCFS2_INFO_LABEL,
> 	OCFS2_INFO_UUID,
> 	OCFS2_INFO_FS_FEATURES
>
> It may be grown from time to time:)
>
> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
> ---
>  fs/ocfs2/ocfs2_fs.h |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 69 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ocfs2/ocfs2_fs.h b/fs/ocfs2/ocfs2_fs.h
> index e9431e4..dac7f4c 100644
> --- a/fs/ocfs2/ocfs2_fs.h
> +++ b/fs/ocfs2/ocfs2_fs.h
> @@ -309,6 +309,75 @@ struct reflink_arguments {
>  };
>  #define OCFS2_IOC_REFLINK	_IOW('o', 4, struct reflink_arguments)
>  
> +#define OCFS2_UUID_LEN              16
> +#define OCFS2_MAX_LABEL_LEN         64
>   

What's wrong with the existing OCFS2_MAX_VOL_LABEL_LEN?
For UUID, maybe define:
#define OCFS2_VOL_UUIDSTR_LEN (OCFS2_VOL_UUID_LEN * 2 + 1)

> +
> +/*
> + * Used to query fs information for none-privileged 
> + * users by using OCFS2_IOC_INFO ioctols, a series of
> + * following requests need to be NULL-terminated.
> + *
> + * Always try to separate info request into small pieces to
> + * guarantee the backward&forward compatibility.
> + */
> +struct ocfs2_info_request {
> +	__u32 ir_code;	/* Info request code */
> +	__u32 ir_flags; /* Request flags */
> +};
> +

Rename it ocfs2_info. Then you have ocfs2_info_clustersize. In the next
patch you have typedef-ed these structures presumably because the structure
name is long. Instead, shorten the name and use it as is. BTW, typedef-ing
struct is strongly discouraged in the kernel coding standards. For good 
reason.

> +struct ocfs2_info_request_clustersize {
> +	struct ocfs2_info_request ir_request;
> +	__u32 ir_clustersize;
> +};
> +
> +struct ocfs2_info_request_blocksize {
> +	struct ocfs2_info_request ir_request;
> +	__u32 ir_blocksize;
> +};
> +
> +struct ocfs2_info_request_slotnum {
> +	struct ocfs2_info_request ir_request;
> +	__u16 ir_slotnum;
> +};
> +
> +struct ocfs2_info_request_label {
> +	struct ocfs2_info_request ir_request;
> +	__u8	ir_label[OCFS2_MAX_LABEL_LEN];
> +};
> +
> +struct ocfs2_info_request_uuid {
> +	struct ocfs2_info_request ir_request;
> +	__u8	ir_uuid[OCFS2_UUID_LEN];
> +};
>   

Return the uuid string as that is what we use everywhere.

> +
> +struct ocfs2_info_request_fs_features {
> +	struct ocfs2_info_request ir_quest;
> +	__u32 ir_compat_features;
> +	__u32 ir_incompat_features;
> +	__u32 ir_ro_compat_features;
> +};
> +
> +/* Codes for cfs2_info_request */
> +
> +#define OCFS2_INFO_CLUSTERSIZE	0x00000001
> +#define OCFS2_INFO_BLOCKSIZE	0x00000002
> +#define OCFS2_INFO_SLOTNUM      0x00000004
> +#define OCFS2_INFO_LABEL        0x00000008
> +#define OCFS2_INFO_UUID         0x00000010
> +#define OCFS2_INFO_FS_FEATURES	0x00000020
>   

Fix typos, white-spaces, etc. Make use of tabs.

> +
> +/* Flags for struct ocfs2_info_request */
> +/* Filled by the caller */
> +#define OCFS2_INFO_FL_NON_COHERENT  0x00000001  /* Cluster coherency not required.
> +						   This is a hint.  It is up to
> +						   ocfs2 whether the request can
> +						   be fulfilled without locking. */
> +/* Filled by ocfs2 */
> +#define OCFS2_INFO_FL_FILLED        0x80000000  /* Filesystem understood this
> +						   request and filled in the
> +						   answer */
> +
> +#define OCFS2_IOC_INFO		_IOR('o', 5, struct ocfs2_info_request)
>  
>  /*
>   * Journal Flags (ocfs2_dinode.id1.journal1.i_flags)
>   

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Sunil Mushran]

Tristan Ye wrote:
> We implement the request as simple as possible to avoid
> a possible extending if disk format get changed.
>
> Completenss of a request include the info required and a flag(FL_FILLED)
> filled being returned to the end-user.
>
> Note: the requests in a series from userspace should be NULL-terminated.

Pass in the buffer length. Expecting null-termination is asking for trouble.

>
> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
> ---
>  fs/ocfs2/ioctl.c |  184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 184 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
> index 31fbb06..0ac5218 100644
> --- a/fs/ocfs2/ioctl.c
> +++ b/fs/ocfs2/ioctl.c
> @@ -108,6 +108,188 @@ bail:
>  	return status;
>  }
>  
> +static int ocfs2_get_request_info(struct inode *inode,
> +				  unsigned long arg)
> +{
> +
> +	int ret = 0, i = 0, num_reqs = 0;
> +
> +	struct ocfs2_info_request req, *preq = NULL;
> +	struct ocfs2_info_request **reqs;
> +	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
> +
> +	typedef struct ocfs2_info_request OIR, *POIR;
> +	typedef struct ocfs2_info_request_clustersize OIRC, *POIRC;
> +	typedef struct ocfs2_info_request_blocksize OIRB, *POIRB;
> +	typedef struct ocfs2_info_request_slotnum OIRS, *POIRS;
> +	typedef struct ocfs2_info_request_label OIRL, *POIRL;
> +	typedef struct ocfs2_info_request_uuid OIRU, *POIRU;
> +	typedef struct ocfs2_info_request_fs_features OIRF, *POIRF;
> +

Three no-nos.
1. Do not typedef structures.
2. Use all-caps only for #defines.
3. Do not pre-pend type to the variable name.

For more refer to the kernel coding style.
http://lxr.linux.no/source/Documentation/CodingStyle

> +	/*
> +	 * The requests series from userspace need to be NULL-terminated.
> +	 */
> +	do {
> +		preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
> +		if (!preq)
> +			break;
> +		i++;
> +
> +	} while (preq);
> +
> +	num_reqs = i;

If we pass in the length of the buffer, then num_reqs = len / 
sizeof(struct ...);

> +
> +	reqs = kmalloc(sizeof(POIR) * num_reqs, GFP_KERNEL);
> +	if (!reqs) {
> +		ret = -ENOMEM;
> +		goto bail;
> +	}

We can then copy_from_user the buffer in one go. The remaining
code have to change accordingly.

> +
> +	ret = ocfs2_inode_lock(inode, NULL, 0);
> +	if (ret < 0) {
> +		mlog_errno(ret);
> +		goto bail;
> +	}

The superblock lock is special. See ocfs2_super_lock().

You'll need to traverse to the osb to use super_lock.
struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);

Also, make use of OCFS2_INFO_FL_NON_COHERENT.

> +
> +	for (i = 0; i < num_reqs; i++) {
> +
> +		reqs[i] = NULL;
> +
> +		preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
> +
> +		if (copy_from_user(&req, preq, sizeof(req))) {
> +			ret = -EFAULT;
> +			goto bail;
> +		}
> +
> +		switch (req.ir_code) {
> +		case OCFS2_INFO_CLUSTERSIZE:
> +			reqs[i] = kmalloc(sizeof(OIRC), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRC))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			((POIRC)(reqs[i]))->ir_clustersize = osb->s_clustersize;
> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRC))) {
> +				ret = EFAULT;

-EFAULT.

> +				goto bail;
> +			}
> +
> +			break;
> +		case OCFS2_INFO_BLOCKSIZE:
> +			reqs[i] = kmalloc(sizeof(OIRB), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRB))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			((POIRB)(reqs[i]))->ir_blocksize =
> +					inode->i_sb->s_blocksize;
> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRB))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			break;
> +		case OCFS2_INFO_SLOTNUM:
> +			reqs[i] = kmalloc(sizeof(OIRS), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRS))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			((POIRS)(reqs[i]))->ir_slotnum = osb->max_slots;
> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRS))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			break;
> +		case OCFS2_INFO_LABEL:
> +			reqs[i] = kmalloc(sizeof(OIRL), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRL))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			memmove(((POIRL)(reqs[i]))->ir_label, osb->vol_label,
> +				OCFS2_MAX_VOL_LABEL_LEN);

Why memmove() when memcpy() will do?

> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRL))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			break;
> +		case OCFS2_INFO_UUID:
> +			reqs[i] = kmalloc(sizeof(OIRU), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRU))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			memmove(((POIRU)(reqs[i]))->ir_uuid, osb->uuid,
> +				OCFS2_VOL_UUID_LEN);
> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRU))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			break;
> +		case OCFS2_INFO_FS_FEATURES:
> +			reqs[i] = kmalloc(sizeof(OIRF), GFP_KERNEL);
> +
> +			if (copy_from_user(reqs[i], preq, sizeof(OIRF))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			((POIRF)(reqs[i]))->ir_compat_features =
> +						osb->s_feature_compat;
> +			((POIRF)(reqs[i]))->ir_incompat_features =
> +						osb->s_feature_incompat;
> +			((POIRF)(reqs[i]))->ir_ro_compat_features =
> +						osb->s_feature_ro_compat;
> +			reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
> +
> +			if (copy_to_user(preq, reqs[i], sizeof(OIRF))) {
> +				ret = -EFAULT;
> +				goto bail;
> +			}
> +
> +			break;
> +		default:
> +			break;
> +		}
> +
> +	}
> +bail:
> +	for (i = 0; i < num_reqs; i++)
> +		kfree(reqs[i]);
> +
> +	kfree(reqs);
> +
> +	ocfs2_inode_unlock(inode, 0);
> +
> +	mlog_exit(ret);
> +	return ret;
> +}
> +
>  long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
>  	struct inode *inode = filp->f_path.dentry->d_inode;
> @@ -173,6 +355,8 @@ long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		preserve = (args.preserve != 0);
>  
>  		return ocfs2_reflink_ioctl(inode, old_path, new_path, preserve);
> +	case OCFS2_IOC_INFO:
> +		return ocfs2_get_request_info(inode, arg);
>  	default:
>  		return -ENOTTY;
>  	}

[Ocfs2-devel] [PATCH 1/2] Ocfs2: Add new ioctls prototype and corresponding data structure to ocfs2 header.

[Tristan]

Sunil Mushran wrote:
> Tristan Ye wrote:
>> We use OCFS2_IOC_INFO to manipulate the new ioctl, the requests can
>> be stored in a array of (struct ocfs2_info_request), which should be
>> NULL-terminated, kernel therefore can recoginze and fill them one by
>> one.
>>
>> The reason why we need these ioctls is to offer the none-privileged
>> end-user a possibility to get filesys info gathering.
>>
>> Idea here is to make the spearated request small enough to guarantee
>> a better backward&forward compatibility since a small piece of request
>> would be less likely to be broken if filesys on raw disk get changed.
>>
>> Currently, the first version include following request type:
>>
>> OCFS2_INFO_CLUSTERSIZE,
>> OCFS2_INFO_BLOCKSIZE,
>> OCFS2_INFO_SLOTNUM,
>> OCFS2_INFO_LABEL,
>> OCFS2_INFO_UUID,
>> OCFS2_INFO_FS_FEATURES
>>
>> It may be grown from time to time:)
>>
>> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
>> ---
>> fs/ocfs2/ocfs2_fs.h | 69 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 69 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/ocfs2/ocfs2_fs.h b/fs/ocfs2/ocfs2_fs.h
>> index e9431e4..dac7f4c 100644
>> --- a/fs/ocfs2/ocfs2_fs.h
>> +++ b/fs/ocfs2/ocfs2_fs.h
>> @@ -309,6 +309,75 @@ struct reflink_arguments {
>> };
>> #define OCFS2_IOC_REFLINK _IOW('o', 4, struct reflink_arguments)
>>
>> +#define OCFS2_UUID_LEN 16
>> +#define OCFS2_MAX_LABEL_LEN 64
>
> What's wrong with the existing OCFS2_MAX_VOL_LABEL_LEN?
> For UUID, maybe define:
> #define OCFS2_VOL_UUIDSTR_LEN (OCFS2_VOL_UUID_LEN * 2 + 1)

The existing OCFS2_MAX_VOL_LABEL_LEN macro was placed after definitions 
of ioctls, and I just want all of the ioctls to gather in the same place.
Anyway, I'll move these ioctls properly.

>
>> +
>> +/*
>> + * Used to query fs information for none-privileged + * users by 
>> using OCFS2_IOC_INFO ioctols, a series of
>> + * following requests need to be NULL-terminated.
>> + *
>> + * Always try to separate info request into small pieces to
>> + * guarantee the backward&forward compatibility.
>> + */
>> +struct ocfs2_info_request {
>> + __u32 ir_code; /* Info request code */
>> + __u32 ir_flags; /* Request flags */
>> +};
>> +
>
> Rename it ocfs2_info. Then you have ocfs2_info_clustersize. In the next
> patch you have typedef-ed these structures presumably because the 
> structure
> name is long. Instead, shorten the name and use it as is. BTW, 
> typedef-ing
> struct is strongly discouraged in the kernel coding standards. For 
> good reason.
Good idea, I was always being annoyed by the length of these structures.

>
>> +struct ocfs2_info_request_clustersize {
>> + struct ocfs2_info_request ir_request;
>> + __u32 ir_clustersize;
>> +};
>> +
>> +struct ocfs2_info_request_blocksize {
>> + struct ocfs2_info_request ir_request;
>> + __u32 ir_blocksize;
>> +};
>> +
>> +struct ocfs2_info_request_slotnum {
>> + struct ocfs2_info_request ir_request;
>> + __u16 ir_slotnum;
>> +};
>> +
>> +struct ocfs2_info_request_label {
>> + struct ocfs2_info_request ir_request;
>> + __u8 ir_label[OCFS2_MAX_LABEL_LEN];
>> +};
>> +
>> +struct ocfs2_info_request_uuid {
>> + struct ocfs2_info_request ir_request;
>> + __u8 ir_uuid[OCFS2_UUID_LEN];
>> +};
>
> Return the uuid string as that is what we use everywhere.
Not quite sure why we want to keep the 32bytes uuid_str instead of 
16btyes uuid?

>
>> +
>> +struct ocfs2_info_request_fs_features {
>> + struct ocfs2_info_request ir_quest;
>> + __u32 ir_compat_features;
>> + __u32 ir_incompat_features;
>> + __u32 ir_ro_compat_features;
>> +};
>> +
>> +/* Codes for cfs2_info_request */
>> +
>> +#define OCFS2_INFO_CLUSTERSIZE 0x00000001
>> +#define OCFS2_INFO_BLOCKSIZE 0x00000002
>> +#define OCFS2_INFO_SLOTNUM 0x00000004
>> +#define OCFS2_INFO_LABEL 0x00000008
>> +#define OCFS2_INFO_UUID 0x00000010
>> +#define OCFS2_INFO_FS_FEATURES 0x00000020
>
> Fix typos, white-spaces, etc. Make use of tabs.
>
>> +
>> +/* Flags for struct ocfs2_info_request */
>> +/* Filled by the caller */
>> +#define OCFS2_INFO_FL_NON_COHERENT 0x00000001 /* Cluster coherency 
>> not required.
>> + This is a hint. It is up to
>> + ocfs2 whether the request can
>> + be fulfilled without locking. */
>> +/* Filled by ocfs2 */
>> +#define OCFS2_INFO_FL_FILLED 0x80000000 /* Filesystem understood this
>> + request and filled in the
>> + answer */
>> +
>> +#define OCFS2_IOC_INFO _IOR('o', 5, struct ocfs2_info_request)
>>
>> /*
>> * Journal Flags (ocfs2_dinode.id1.journal1.i_flags)
>

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Tristan]

Sunil Mushran wrote:
> Tristan Ye wrote:
>> We implement the request as simple as possible to avoid
>> a possible extending if disk format get changed.
>>
>> Completenss of a request include the info required and a flag(FL_FILLED)
>> filled being returned to the end-user.
>>
>> Note: the requests in a series from userspace should be NULL-terminated.
>
> Pass in the buffer length. Expecting null-termination is asking for 
> trouble.

I agree, may discuss with joel later, a null-terminated flavor may 
become a security leak...

Actually, maybe we could take single request into account, which mean 
end-user deliver one request for one info each time.

For users, just a matter of call ioctl() mutiple times. For kernel, 
means simpler to handle...

>
>>
>> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
>> ---
>> fs/ocfs2/ioctl.c | 184 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 184 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
>> index 31fbb06..0ac5218 100644
>> --- a/fs/ocfs2/ioctl.c
>> +++ b/fs/ocfs2/ioctl.c
>> @@ -108,6 +108,188 @@ bail:
>> return status;
>> }
>>
>> +static int ocfs2_get_request_info(struct inode *inode,
>> + unsigned long arg)
>> +{
>> +
>> + int ret = 0, i = 0, num_reqs = 0;
>> +
>> + struct ocfs2_info_request req, *preq = NULL;
>> + struct ocfs2_info_request **reqs;
>> + struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>> +
>> + typedef struct ocfs2_info_request OIR, *POIR;
>> + typedef struct ocfs2_info_request_clustersize OIRC, *POIRC;
>> + typedef struct ocfs2_info_request_blocksize OIRB, *POIRB;
>> + typedef struct ocfs2_info_request_slotnum OIRS, *POIRS;
>> + typedef struct ocfs2_info_request_label OIRL, *POIRL;
>> + typedef struct ocfs2_info_request_uuid OIRU, *POIRU;
>> + typedef struct ocfs2_info_request_fs_features OIRF, *POIRF;
>> +
>
> Three no-nos.
> 1. Do not typedef structures.
> 2. Use all-caps only for #defines.
> 3. Do not pre-pend type to the variable name.
>
> For more refer to the kernel coding style.
> http://lxr.linux.no/source/Documentation/CodingStyle
Thank you for pointing this out.
>
>> + /*
>> + * The requests series from userspace need to be NULL-terminated.
>> + */
>> + do {
>> + preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
>> + if (!preq)
>> + break;
>> + i++;
>> +
>> + } while (preq);
>> +
>> + num_reqs = i;
>
> If we pass in the length of the buffer, then num_reqs = len / 
> sizeof(struct ...);
>
>> +
>> + reqs = kmalloc(sizeof(POIR) * num_reqs, GFP_KERNEL);
>> + if (!reqs) {
>> + ret = -ENOMEM;
>> + goto bail;
>> + }
>
> We can then copy_from_user the buffer in one go. The remaining
> code have to change accordingly.

I also noticed that, we actually don't need the pointer array at all.

>
>> +
>> + ret = ocfs2_inode_lock(inode, NULL, 0);
>> + if (ret < 0) {
>> + mlog_errno(ret);
>> + goto bail;
>> + }
>
> The superblock lock is special. See ocfs2_super_lock().
>
> You'll need to traverse to the osb to use super_lock.
> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>
> Also, make use of OCFS2_INFO_FL_NON_COHERENT.

Yes, that's my fault.

>
>> +
>> + for (i = 0; i < num_reqs; i++) {
>> +
>> + reqs[i] = NULL;
>> +
>> + preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
>> +
>> + if (copy_from_user(&req, preq, sizeof(req))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + switch (req.ir_code) {
>> + case OCFS2_INFO_CLUSTERSIZE:
>> + reqs[i] = kmalloc(sizeof(OIRC), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRC))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + ((POIRC)(reqs[i]))->ir_clustersize = osb->s_clustersize;
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRC))) {
>> + ret = EFAULT;
>
> -EFAULT.
>
>> + goto bail;
>> + }
>> +
>> + break;
>> + case OCFS2_INFO_BLOCKSIZE:
>> + reqs[i] = kmalloc(sizeof(OIRB), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRB))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + ((POIRB)(reqs[i]))->ir_blocksize =
>> + inode->i_sb->s_blocksize;
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRB))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + break;
>> + case OCFS2_INFO_SLOTNUM:
>> + reqs[i] = kmalloc(sizeof(OIRS), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRS))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + ((POIRS)(reqs[i]))->ir_slotnum = osb->max_slots;
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRS))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + break;
>> + case OCFS2_INFO_LABEL:
>> + reqs[i] = kmalloc(sizeof(OIRL), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRL))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + memmove(((POIRL)(reqs[i]))->ir_label, osb->vol_label,
>> + OCFS2_MAX_VOL_LABEL_LEN);
>
> Why memmove() when memcpy() will do?
I always have a thought that memmove handled thing more gracefully than 
memcpy when overlap happens.

>
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRL))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + break;
>> + case OCFS2_INFO_UUID:
>> + reqs[i] = kmalloc(sizeof(OIRU), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRU))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + memmove(((POIRU)(reqs[i]))->ir_uuid, osb->uuid,
>> + OCFS2_VOL_UUID_LEN);
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRU))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + break;
>> + case OCFS2_INFO_FS_FEATURES:
>> + reqs[i] = kmalloc(sizeof(OIRF), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRF))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + ((POIRF)(reqs[i]))->ir_compat_features =
>> + osb->s_feature_compat;
>> + ((POIRF)(reqs[i]))->ir_incompat_features =
>> + osb->s_feature_incompat;
>> + ((POIRF)(reqs[i]))->ir_ro_compat_features =
>> + osb->s_feature_ro_compat;
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRF))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + }
>> +bail:
>> + for (i = 0; i < num_reqs; i++)
>> + kfree(reqs[i]);
>> +
>> + kfree(reqs);
>> +
>> + ocfs2_inode_unlock(inode, 0);
>> +
>> + mlog_exit(ret);
>> + return ret;
>> +}
>> +
>> long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> {
>> struct inode *inode = filp->f_path.dentry->d_inode;
>> @@ -173,6 +355,8 @@ long ocfs2_ioctl(struct file *filp, unsigned int 
>> cmd, unsigned long arg)
>> preserve = (args.preserve != 0);
>>
>> return ocfs2_reflink_ioctl(inode, old_path, new_path, preserve);
>> + case OCFS2_IOC_INFO:
>> + return ocfs2_get_request_info(inode, arg);
>> default:
>> return -ENOTTY;
>> }
>

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Tristan]

Sunil Mushran wrote:
> Tristan Ye wrote:
>> We implement the request as simple as possible to avoid
>> a possible extending if disk format get changed.
>>
>> Completenss of a request include the info required and a flag(FL_FILLED)
>> filled being returned to the end-user.
>>
>> Note: the requests in a series from userspace should be NULL-terminated.
>
> Pass in the buffer length. Expecting null-termination is asking for 
> trouble.
>
>>
>> Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
>> ---
>> fs/ocfs2/ioctl.c | 184 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 184 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c
>> index 31fbb06..0ac5218 100644
>> --- a/fs/ocfs2/ioctl.c
>> +++ b/fs/ocfs2/ioctl.c
>> @@ -108,6 +108,188 @@ bail:
>> return status;
>> }
>>
>> +static int ocfs2_get_request_info(struct inode *inode,
>> + unsigned long arg)
>> +{
>> +
>> + int ret = 0, i = 0, num_reqs = 0;
>> +
>> + struct ocfs2_info_request req, *preq = NULL;
>> + struct ocfs2_info_request **reqs;
>> + struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>> +
>> + typedef struct ocfs2_info_request OIR, *POIR;
>> + typedef struct ocfs2_info_request_clustersize OIRC, *POIRC;
>> + typedef struct ocfs2_info_request_blocksize OIRB, *POIRB;
>> + typedef struct ocfs2_info_request_slotnum OIRS, *POIRS;
>> + typedef struct ocfs2_info_request_label OIRL, *POIRL;
>> + typedef struct ocfs2_info_request_uuid OIRU, *POIRU;
>> + typedef struct ocfs2_info_request_fs_features OIRF, *POIRF;
>> +
>
> Three no-nos.
> 1. Do not typedef structures.
> 2. Use all-caps only for #defines.
> 3. Do not pre-pend type to the variable name.
>
> For more refer to the kernel coding style.
> http://lxr.linux.no/source/Documentation/CodingStyle
>
>> + /*
>> + * The requests series from userspace need to be NULL-terminated.
>> + */
>> + do {
>> + preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
>> + if (!preq)
>> + break;
>> + i++;
>> +
>> + } while (preq);
>> +
>> + num_reqs = i;
>
> If we pass in the length of the buffer, then num_reqs = len / 
> sizeof(struct ...);

Size of each request may differ from type to type, think about:

struct ocfs2_info_label {
struct ocfs2_info ir_request;
__u8 ir_label[OCFS2_MAX_VOL_LABEL_LEN];
};

struct ocfs2_info_uuid {
struct ocfs2_info ir_request;
__u8 ir_uuid_str[OCFS2_VOL_UUIDSTR_LEN];
};

struct ocfs2_info_fs_features {
struct ocfs2_info ir_quest;
__u32 ir_compat_features;
__u32 ir_incompat_features;
__u32 ir_ro_compat_features;
};

So a len/sizeof() way may not provide the real number of requests, to 
pass a count of request from userspace to kernel may make more sense. 
but it was more like a workaround anyway,

The most graceful way I guess is to treat request one by one, end-users 
deliver just one request each time, it's just a matter of calling 
ioctl() repeatly for users, and we still use just one ioctl type for all 
of the requests(OCFS2_IOC_INFO), different requests can be recognized by 
ir_code as we specified.


Tristan.


>
>> +
>> + reqs = kmalloc(sizeof(POIR) * num_reqs, GFP_KERNEL);
>> + if (!reqs) {
>> + ret = -ENOMEM;
>> + goto bail;
>> + }
>
> We can then copy_from_user the buffer in one go. The remaining
> code have to change accordingly.
>
>> +
>> + ret = ocfs2_inode_lock(inode, NULL, 0);
>> + if (ret < 0) {
>> + mlog_errno(ret);
>> + goto bail;
>> + }
>
> The superblock lock is special. See ocfs2_super_lock().
>
> You'll need to traverse to the osb to use super_lock.
> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>
> Also, make use of OCFS2_INFO_FL_NON_COHERENT.
>
>> +
>> + for (i = 0; i < num_reqs; i++) {
>> +
>> + reqs[i] = NULL;
>> +
>> + preq = *((POIR *)((char *)arg + i * sizeof(POIR)));
>> +
>> + if (copy_from_user(&req, preq, sizeof(req))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + switch (req.ir_code) {
>> + case OCFS2_INFO_CLUSTERSIZE:
>> + reqs[i] = kmalloc(sizeof(OIRC), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRC))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + ((POIRC)(reqs[i]))->ir_clustersize = osb->s_clustersize;
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRC))) {
>> + ret = EFAULT;
>
> -EFAULT.
>
>> + goto bail;
>> + }
>> +
>> + break;
>> + case OCFS2_INFO_BLOCKSIZE:
>> + reqs[i] = kmalloc(sizeof(OIRB), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRB))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + ((POIRB)(reqs[i]))->ir_blocksize =
>> + inode->i_sb->s_blocksize;
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRB))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + break;
>> + case OCFS2_INFO_SLOTNUM:
>> + reqs[i] = kmalloc(sizeof(OIRS), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRS))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + ((POIRS)(reqs[i]))->ir_slotnum = osb->max_slots;
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRS))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + break;
>> + case OCFS2_INFO_LABEL:
>> + reqs[i] = kmalloc(sizeof(OIRL), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRL))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + memmove(((POIRL)(reqs[i]))->ir_label, osb->vol_label,
>> + OCFS2_MAX_VOL_LABEL_LEN);
>
> Why memmove() when memcpy() will do?
>
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRL))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + break;
>> + case OCFS2_INFO_UUID:
>> + reqs[i] = kmalloc(sizeof(OIRU), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRU))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + memmove(((POIRU)(reqs[i]))->ir_uuid, osb->uuid,
>> + OCFS2_VOL_UUID_LEN);
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRU))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + break;
>> + case OCFS2_INFO_FS_FEATURES:
>> + reqs[i] = kmalloc(sizeof(OIRF), GFP_KERNEL);
>> +
>> + if (copy_from_user(reqs[i], preq, sizeof(OIRF))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + ((POIRF)(reqs[i]))->ir_compat_features =
>> + osb->s_feature_compat;
>> + ((POIRF)(reqs[i]))->ir_incompat_features =
>> + osb->s_feature_incompat;
>> + ((POIRF)(reqs[i]))->ir_ro_compat_features =
>> + osb->s_feature_ro_compat;
>> + reqs[i]->ir_flags |= OCFS2_INFO_FL_FILLED;
>> +
>> + if (copy_to_user(preq, reqs[i], sizeof(OIRF))) {
>> + ret = -EFAULT;
>> + goto bail;
>> + }
>> +
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + }
>> +bail:
>> + for (i = 0; i < num_reqs; i++)
>> + kfree(reqs[i]);
>> +
>> + kfree(reqs);
>> +
>> + ocfs2_inode_unlock(inode, 0);
>> +
>> + mlog_exit(ret);
>> + return ret;
>> +}
>> +
>> long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>> {
>> struct inode *inode = filp->f_path.dentry->d_inode;
>> @@ -173,6 +355,8 @@ long ocfs2_ioctl(struct file *filp, unsigned int 
>> cmd, unsigned long arg)
>> preserve = (args.preserve != 0);
>>
>> return ocfs2_reflink_ioctl(inode, old_path, new_path, preserve);
>> + case OCFS2_IOC_INFO:
>> + return ocfs2_get_request_info(inode, arg);
>> default:
>> return -ENOTTY;
>> }
>

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Sunil Mushran]

Tristan wrote:
> I always have a thought that memmove handled thing more gracefully 
> than memcpy when overlap happens.

Yes and hence we should to use it when there is even a small chance
of buffer overlap. But that is not the case here.

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Joel Becker]

On Tue, Dec 01, 2009 at 11:20:25AM +0800, Tristan wrote:
> Size of each request may differ from type to type, think about:

	Yes, but you are not passing an array of structures, you are
passing an array of pointers.  Andi's right, of course, that NULL
termination is rife for abuse.  Let's pass it via a wrapper structure:

---------------------------------------------------------------------------
struct ocfs2_info {
	__u64 info_requests;	/* Array of __u64 pointers to our requests */
	__u32 info_count;	/* Number of requests in info_requests */
};
---------------------------------------------------------------------------

	Why is info_requests a __u64?  Because we have to handle 32bit
binaries on 64bit systems.  In fact, info_requests is an array of 64bit
values.  See the example below.
	I've also modified struct ocfs2_info_request to have a magic
number and a size:

---------------------------------------------------------------------------
/* Magic number of all requests */
#define OCFS2_INFO_MAGIC            0x4F32494E  /* Magic number for requests */

struct ocfs2_info_request {
/*00*/	__u32 ir_magic;	/* Magic number */
	__u32 ir_code;	/* Info request code */
	__u32 ir_size;	/* Size of request */
	__u32 ir_flags;	/* Request flags */
/*10*/	/* Request specific fields */
};
---------------------------------------------------------------------------

	A user now does:

---------------------------------------------------------------------------
get_info(fd)
{
	struct ocfs2_info_request_clustersize crq = {
		.ir_magic = OCFS2_INFO_MAGIC,
		.ir_code = OCFS2_INFO_REQUEST_CLUSTERSIZE,
		.ir_size = sizeof(struct ocfs2Info_request_clustersize),
	};
	struct ocfs2_info_request_blocksize brq = {
		.ir_magic = OCFS2_INFO_MAGIC,
		.ir_code = OCFS2_INFO_REQUEST_BLOCKSIZE,
		.ir_size = sizeof(struct ocfs2Info_request_blocksize),
	};
	uint64_t info_array[2] = {(unsigned long)&crq, (unsigned long)&brq};
	struct ocfs2_info info = {
		.info_requests = info_array,
		.info_count = 2,
	};
	
	rc = ioctl(fd, OCFS2_IOC_INFO, &info);
	...
}
---------------------------------------------------------------------------

	o2info can create convenience wrappers for setting the magic
number, code, and size, of course.  Note that we cast the request
pointers to unsigned longs so that the promotion from ulong to uint64_t
is handled by C.  This code works on 32bit and 64bit.
	How does the kernel handle this?  One by one, of course.  First
we copy_from_user() the ocfs2_info structure.  We check that info_count
is within some sane boundary (let's say 50 requests) so that someone
doesn't pass one million.   We also check that info_requests is not
NULL.  Both allow us to return -EINVAL.
	Then, for each request in the array, we grab the header,
validate it, and call a handler function:

--------------------------------------------------------------------------
	u64 req_addr;
	struct ocfs2_info_request __user *reqp;

	for (i = 0; i < info->info_count; i++) {
		ret = -EFAULT;
		if (get_user(&req_addr,
			     (unsigned long)((u64**)(info->info_requests) + i)))
			break;
		reqp = (struct ocfs2_info_request *)(unsigned long)req_addr;
		ret = ocfs2_info_handle(file, reqp);
		if (ret)
			break;
	}
--------------------------------------------------------------------------

	Now we have a pointer to the *userspace* memory, we need to get
the header into kernelspace:

--------------------------------------------------------------------------
int ocfs2_info_handle(file, struct ocfs2_info_request __user *reqp)
{
	int ret = 0;
	struct ocfs2_info_request req;

	if (copy_from_user(&req, reqp, sizeof(struct ocfs2_info_request)))
		return -EFAULT;

	if (req.ir_magic != OCFS2_INFO_MAGIC)
		return -EINVAL;
	switch (req.ir_code) {
		case OCFS2_INFO_REQUEST_CLUSTERSIZE:
			if (req.ir_size != sizeof(struct ocfs2_info_request_clustersize)) {
				ret = -EINVAL;
				break;
			}
			ret = ocfs2_info_handle_clustersize(file, reqp, &req);
			break;

		...

		default:
			/* Unknown code */
			ret = -EINVAL;
			break;
	}

	return ret;
}
--------------------------------------------------------------------------

	We can probably wrap the switch into a lookup table, but let's
not worry about that for now.
	As you can see, we've validated the info structure, then we
validate each request as we get it.  We check the magic, the code, and
the size to make sure userspace is doing it right.  Only then do we
actually call the handler function.
	Some handler functions will need information from the extra
portion of the request.  Simple ones will not, and need not copy
anything else from userspace.  The clustersize handler is an example.
It needs no more information from the passed request.  It just needs to
fill in the result to userspace.

--------------------------------------------------------------------------
int ocfs2_info_handle_clustersize(struct file *file,
				  struct ocfs2_info_request __user *user_req,
				  struct ocfs2_info_request *req)
{
	struct ocfs2_info_request_clustersize result;

	memcpy(&result, req, sizeof(struct ocfs2_info_request));
	result.ir_request.ir_flags |= OCFS2_INFO_FL_FILLED;
	result.ir_clustersize =
		OCFS2_SB(file->f_dentry->d_inode->i_sb)->s_clutsersize;

        if (copy_to_user((struct ocfs2_info_request_clustersize __user *)user_req,
			 &result,
			 sizeof(struct ocfs2_info_request_clustersize)))
		return -EFAULT;
	return 0;
}
--------------------------------------------------------------------------

	Done right, we can switch between a single ioctl with an array
of requests, a single ioctl with one request per call, and even multiple
ioctls, all without changing the handler functions.  Whee!

Joel

-- 

Life's Little Instruction Book #511

	"Call your mother."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Tristan]

Joel Becker wrote:
> On Tue, Dec 01, 2009 at 11:20:25AM +0800, Tristan wrote:
>   
>> Size of each request may differ from type to type, think about:
>>     
>
> 	Yes, but you are not passing an array of structures, you are
> passing an array of pointers.  Andi's right, of course, that NULL
> termination is rife for abuse.  Let's pass it via a wrapper structure:
>
> ---------------------------------------------------------------------------
> struct ocfs2_info {
> 	__u64 info_requests;	/* Array of __u64 pointers to our requests */
> 	__u32 info_count;	/* Number of requests in info_requests */
> };
> ---------------------------------------------------------------------------
>
> 	Why is info_requests a __u64?  Because we have to handle 32bit
> binaries on 64bit systems.  In fact, info_requests is an array of 64bit
> values.  See the example below.
> 	I've also modified struct ocfs2_info_request to have a magic
> number and a size:
>
> ---------------------------------------------------------------------------
> /* Magic number of all requests */
> #define OCFS2_INFO_MAGIC            0x4F32494E  /* Magic number for requests */
>
> struct ocfs2_info_request {
> /*00*/	__u32 ir_magic;	/* Magic number */
>   

Why we need a magic here, just to improve the security?

> 	__u32 ir_code;	/* Info request code */
> 	__u32 ir_size;	/* Size of request */
> 	__u32 ir_flags;	/* Request flags */
> /*10*/	/* Request specific fields */
> };
> ---------------------------------------------------------------------------
>
> 	A user now does:
>
> ---------------------------------------------------------------------------
> get_info(fd)
> {
> 	struct ocfs2_info_request_clustersize crq = {
> 		.ir_magic = OCFS2_INFO_MAGIC,
> 		.ir_code = OCFS2_INFO_REQUEST_CLUSTERSIZE,
> 		.ir_size = sizeof(struct ocfs2Info_request_clustersize),
> 	};
> 	struct ocfs2_info_request_blocksize brq = {
> 		.ir_magic = OCFS2_INFO_MAGIC,
> 		.ir_code = OCFS2_INFO_REQUEST_BLOCKSIZE,
> 		.ir_size = sizeof(struct ocfs2Info_request_blocksize),
> 	};
> 	uint64_t info_array[2] = {(unsigned long)&crq, (unsigned long)&brq};
> 	struct ocfs2_info info = {
> 		.info_requests = info_array,
> 		.info_count = 2,
> 	};
> 	
> 	rc = ioctl(fd, OCFS2_IOC_INFO, &info);
> 	...
> }
> ---------------------------------------------------------------------------
>
> 	o2info can create convenience wrappers for setting the magic
> number, code, and size, of course.  Note that we cast the request
> pointers to unsigned longs so that the promotion from ulong to uint64_t
> is handled by C.  This code works on 32bit and 64bit.
>   

Just a confusion here, uint64_t should be 'unsigned long long' on 32bits 
arch, and 'unsigned long' on 64bits arch, why we not use:


uint64_t info_array[2] = {(unsigned long long)&crq, (unsigned long long)&brq};


> 	How does the kernel handle this?  One by one, of course.  First
> we copy_from_user() the ocfs2_info structure.  We check that info_count
> is within some sane boundary (let's say 50 requests) so that someone
> doesn't pass one million.   We also check that info_requests is not
> NULL.  Both allow us to return -EINVAL.
> 	Then, for each request in the array, we grab the header,
> validate it, and call a handler function:
>
> --------------------------------------------------------------------------
> 	u64 req_addr;
> 	struct ocfs2_info_request __user *reqp;
>
> 	for (i = 0; i < info->info_count; i++) {
> 		ret = -EFAULT;
> 		if (get_user(&req_addr,
> 			     (unsigned long)((u64**)(info->info_requests) + i)))
> 			break;
> 		reqp = (struct ocfs2_info_request *)(unsigned long)req_addr;
> 		ret = ocfs2_info_handle(file, reqp);
> 		if (ret)
> 			break;
> 	}
> --------------------------------------------------------------------------
>   

How to avoid accessing an arbitrary mem address, such as, userspace 
passing a wrong info_count(correct_info_count + 1), so the last entry 
for pointer should be invalid, how to avoid to access that?

> 	Now we have a pointer to the *userspace* memory, we need to get
> the header into kernelspace:
>
> --------------------------------------------------------------------------
> int ocfs2_info_handle(file, struct ocfs2_info_request __user *reqp)
> {
> 	int ret = 0;
> 	struct ocfs2_info_request req;
>
> 	if (copy_from_user(&req, reqp, sizeof(struct ocfs2_info_request)))
> 		return -EFAULT;
>
> 	if (req.ir_magic != OCFS2_INFO_MAGIC)
> 		return -EINVAL;
> 	switch (req.ir_code) {
> 		case OCFS2_INFO_REQUEST_CLUSTERSIZE:
> 			if (req.ir_size != sizeof(struct ocfs2_info_request_clustersize)) {
> 				ret = -EINVAL;
> 				break;
> 			}
> 			ret = ocfs2_info_handle_clustersize(file, reqp, &req);
> 			break;
>
> 		...
>
> 		default:
> 			/* Unknown code */
> 			ret = -EINVAL;
> 			break;
> 	}
>
> 	return ret;
> }
> --------------------------------------------------------------------------
>
> 	We can probably wrap the switch into a lookup table, but let's
> not worry about that for now.
> 	As you can see, we've validated the info structure, then we
> validate each request as we get it.  We check the magic, the code, and
> the size to make sure userspace is doing it right.  Only then do we
> actually call the handler function.
> 	Some handler functions will need information from the extra
> portion of the request.  Simple ones will not, and need not copy
> anything else from userspace.  The clustersize handler is an example.
> It needs no more information from the passed request.  It just needs to
> fill in the result to userspace.
>
> --------------------------------------------------------------------------
> int ocfs2_info_handle_clustersize(struct file *file,
> 				  struct ocfs2_info_request __user *user_req,
> 				  struct ocfs2_info_request *req)
> {
> 	struct ocfs2_info_request_clustersize result;
>
> 	memcpy(&result, req, sizeof(struct ocfs2_info_request));
> 	result.ir_request.ir_flags |= OCFS2_INFO_FL_FILLED;
> 	result.ir_clustersize =
> 		OCFS2_SB(file->f_dentry->d_inode->i_sb)->s_clutsersize;
>
>         if (copy_to_user((struct ocfs2_info_request_clustersize __user *)user_req,
> 			 &result,
> 			 sizeof(struct ocfs2_info_request_clustersize)))
> 		return -EFAULT;
> 	return 0;
> }
> --------------------------------------------------------------------------
>
> 	Done right, we can switch between a single ioctl with an array
> of requests, a single ioctl with one request per call, and even multiple
> ioctls, all without changing the handler functions.  Whee!
>   

The scheme above was fine to me:-)

> Joel
>
>   

[Ocfs2-devel] [PATCH 2/2] Ocfs2: Implement new OCFS2_IOC_INFO ioctl for ocfs2.

[Joel Becker]

On Wed, Dec 02, 2009 at 10:29:28AM +0800, Tristan wrote:
> Joel Becker wrote:
> > struct ocfs2_info_request {
> > /*00*/	__u32 ir_magic;	/* Magic number */
> >   
> 
> Why we need a magic here, just to improve the security?

	Not for security - a malicious user can always fake the magic.
We need to solve security anyway.  The magic number is for convenience.
It catches stupid errors quickly.

> > ---------------------------------------------------------------------------
> > get_info(fd)
> > {
> > 	struct ocfs2_info_request_clustersize crq = {
> > 		.ir_magic = OCFS2_INFO_MAGIC,
> > 		.ir_code = OCFS2_INFO_REQUEST_CLUSTERSIZE,
> > 		.ir_size = sizeof(struct ocfs2Info_request_clustersize),
> > 	};
> > 	struct ocfs2_info_request_blocksize brq = {
> > 		.ir_magic = OCFS2_INFO_MAGIC,
> > 		.ir_code = OCFS2_INFO_REQUEST_BLOCKSIZE,
> > 		.ir_size = sizeof(struct ocfs2Info_request_blocksize),
> > 	};
> > 	uint64_t info_array[2] = {(unsigned long)&crq, (unsigned long)&brq};
> > 	struct ocfs2_info info = {
> > 		.info_requests = info_array,
> > 		.info_count = 2,
> > 	};
> > 	
> > 	rc = ioctl(fd, OCFS2_IOC_INFO, &info);
> > 	...
> > }
> > ---------------------------------------------------------------------------
> >
> > 	o2info can create convenience wrappers for setting the magic
> > number, code, and size, of course.  Note that we cast the request
> > pointers to unsigned longs so that the promotion from ulong to uint64_t
> > is handled by C.  This code works on 32bit and 64bit.
> >   
> 
> Just a confusion here, uint64_t should be 'unsigned long long' on 32bits 
> arch, and 'unsigned long' on 64bits arch, why we not use:
> 
> 
> uint64_t info_array[2] = {(unsigned long long)&crq, (unsigned long long)&brq};

	We're casting pointers.  A pointer is always the size of an
unsigned long.  By casting the pointers to unsigned long, we change them
from pointer types to integer types.  On 32bit this will be a 32bit
value, on 64bit a 64bit one.  That's all the casting does.  The C
compiler will notice we're assigning to uint64_t and promote any 32bit
value for us.

> > --------------------------------------------------------------------------
> > 	u64 req_addr;
> > 	struct ocfs2_info_request __user *reqp;
> >
> > 	for (i = 0; i < info->info_count; i++) {
> > 		ret = -EFAULT;
> > 		if (get_user(&req_addr,
> > 			     (unsigned long)((u64**)(info->info_requests) + i)))
> > 			break;
> > 		reqp = (struct ocfs2_info_request *)(unsigned long)req_addr;
> > 		ret = ocfs2_info_handle(file, reqp);
> > 		if (ret)
> > 			break;
> > 	}
> > --------------------------------------------------------------------------
> >   
> 
> How to avoid accessing an arbitrary mem address, such as, userspace 
> passing a wrong info_count(correct_info_count + 1), so the last entry 
> for pointer should be invalid, how to avoid to access that?

	get_user() will return non-zero if the address is not part of
the program's valid memory.  We then break and return -EFAULT, causing a
segfault.
	Note that we're checking two things.  In this function, we copy
the userspace pointer into reqp.  We haven't actually accessed the
memory at reqp, we've just copied the pointer from info->info_requests.
So here we've checked that info->info_requests+i is a valid memory
location.  In ocfs2_info_handle(), we then copy the actual request from
reqp.  There copy_from_user() is checking that reqp points to valid
memory before copying it.
	Btw, the above function should be checking that reqp is not NULL
;-)

Joel

-- 

"Vote early and vote often." 
        - Al Capone

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127