[PATCH 0/5] ceph: cleanup ceph page vector functions

[PATCH 0/5] ceph: cleanup ceph page vector functions

[Alex Elder]

This series does some cleanup related to some functions that
implement ceph page vectors.  Together, these resolve:
    http://tracker.ceph.com/issues/4053

					-Alex

[PATCH 1/5] ceph: remove a few bogus declarations
[PATCH 2/5] libceph: use void pointers in page vector functions
[PATCH 3/5] rbd: prevent bytes transferred overflow
[PATCH 4/5] rbd: ignore result of ceph_copy_from_page_vector()
[PATCH 5/5] libceph: drop return value from page vector copy

[PATCH 1/5] ceph: remove a few bogus declarations

[Alex Elder]

There are three ceph page vector functions declared in
"fs/ceph/super.h" that don't belong there.  They're
probably left over from some long-ago code reorganization.

They're properly declared in "include/linux/ceph/libceph.h"
so just delete the ones in "super.h".

Signed-off-by: Alex Elder <elder@inktank.com>
---
 fs/ceph/super.h |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 66ebe72..9861cce 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -798,13 +798,7 @@ extern int ceph_mmap(struct file *file, struct
vm_area_struct *vma);
 /* file.c */
 extern const struct file_operations ceph_file_fops;
 extern const struct address_space_operations ceph_aops;
-extern int ceph_copy_to_page_vector(struct page **pages,
-				    const char *data,
-				    loff_t off, size_t len);
-extern int ceph_copy_from_page_vector(struct page **pages,
-				    char *data,
-				    loff_t off, size_t len);
-extern struct page **ceph_alloc_page_vector(int num_pages, gfp_t flags);
+
 extern int ceph_open(struct inode *inode, struct file *file);
 extern int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
 			    struct file *file, unsigned flags, umode_t mode,
-- 
1.7.9.5

[PATCH 2/5] libceph: use void pointers in page vector functions

[Alex Elder]

The functions used for working with ceph page vectors are defined
with char pointers, but they're really intended to operate on
untyped data.  Change the types of these function parameters
to (void *) to reflect this.

(Note that the functions now assume void pointer arithmetic works
like arithmetic on char pointers.)

Signed-off-by: Alex Elder <elder@inktank.com>
---
 include/linux/ceph/libceph.h |   10 +++++-----
 net/ceph/pagevec.c           |   10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index c44275a..2250f8b 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -222,7 +222,7 @@ extern int ceph_open_session(struct ceph_client
*client);
 /* pagevec.c */
 extern void ceph_release_page_vector(struct page **pages, int num_pages);

-extern struct page **ceph_get_direct_page_vector(const char __user *data,
+extern struct page **ceph_get_direct_page_vector(const void __user *data,
 						 int num_pages,
 						 bool write_page);
 extern void ceph_put_page_vector(struct page **pages, int num_pages,
@@ -230,15 +230,15 @@ extern void ceph_put_page_vector(struct page
**pages, int num_pages,
 extern void ceph_release_page_vector(struct page **pages, int num_pages);
 extern struct page **ceph_alloc_page_vector(int num_pages, gfp_t flags);
 extern int ceph_copy_user_to_page_vector(struct page **pages,
-					 const char __user *data,
+					 const void __user *data,
 					 loff_t off, size_t len);
 extern int ceph_copy_to_page_vector(struct page **pages,
-				    const char *data,
+				    const void *data,
 				    loff_t off, size_t len);
 extern int ceph_copy_from_page_vector(struct page **pages,
-				    char *data,
+				    void *data,
 				    loff_t off, size_t len);
-extern int ceph_copy_page_vector_to_user(struct page **pages, char
__user *data,
+extern int ceph_copy_page_vector_to_user(struct page **pages, void
__user *data,
 				    loff_t off, size_t len);
 extern void ceph_zero_page_vector_range(int off, int len, struct page
**pages);

diff --git a/net/ceph/pagevec.c b/net/ceph/pagevec.c
index cd9c21d..5b20be9 100644
--- a/net/ceph/pagevec.c
+++ b/net/ceph/pagevec.c
@@ -12,7 +12,7 @@
 /*
  * build a vector of user pages
  */
-struct page **ceph_get_direct_page_vector(const char __user *data,
+struct page **ceph_get_direct_page_vector(const void __user *data,
 					  int num_pages, bool write_page)
 {
 	struct page **pages;
@@ -93,7 +93,7 @@ EXPORT_SYMBOL(ceph_alloc_page_vector);
  * copy user data into a page vector
  */
 int ceph_copy_user_to_page_vector(struct page **pages,
-					 const char __user *data,
+					 const void __user *data,
 					 loff_t off, size_t len)
 {
 	int i = 0;
@@ -119,7 +119,7 @@ int ceph_copy_user_to_page_vector(struct page **pages,
 EXPORT_SYMBOL(ceph_copy_user_to_page_vector);

 int ceph_copy_to_page_vector(struct page **pages,
-				    const char *data,
+				    const void *data,
 				    loff_t off, size_t len)
 {
 	int i = 0;
@@ -143,7 +143,7 @@ int ceph_copy_to_page_vector(struct page **pages,
 EXPORT_SYMBOL(ceph_copy_to_page_vector);

 int ceph_copy_from_page_vector(struct page **pages,
-				    char *data,
+				    void *data,
 				    loff_t off, size_t len)
 {
 	int i = 0;
@@ -170,7 +170,7 @@ EXPORT_SYMBOL(ceph_copy_from_page_vector);
  * copy user data from a page vector into a user pointer
  */
 int ceph_copy_page_vector_to_user(struct page **pages,
-					 char __user *data,
+					 void __user *data,
 					 loff_t off, size_t len)
 {
 	int i = 0;
-- 
1.7.9.5

[PATCH 3/5] rbd: prevent bytes transferred overflow

[Alex Elder]

In rbd_obj_read_sync(), verify the number of bytes transferred won't
exceed what can be represented by a size_t before using it to
indicate the number of bytes to copy to the result buffer.

(The real motivation for this is to prepare for the next patch.)

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 37361bd..99f1a29 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2048,6 +2048,7 @@ static int rbd_obj_read_sync(struct rbd_device
*rbd_dev,
 	struct ceph_osd_client *osdc;
 	struct page **pages = NULL;
 	u32 page_count;
+	size_t size;
 	int ret;

 	page_count = (u32) calc_pages_for(offset, length);
@@ -2084,7 +2085,10 @@ static int rbd_obj_read_sync(struct rbd_device
*rbd_dev,
 	ret = obj_request->result;
 	if (ret < 0)
 		goto out;
-	ret = ceph_copy_from_page_vector(pages, buf, 0, obj_request->xferred);
+
+	rbd_assert(obj_request->xferred <= (u64) SIZE_MAX);
+	size = (size_t) obj_request->xferred;
+	ret = ceph_copy_from_page_vector(pages, buf, 0, size);
 	if (version)
 		*version = obj_request->version;
 out:
-- 
1.7.9.5

[PATCH 4/5] rbd: ignore result of ceph_copy_from_page_vector()

[Alex Elder]

The result of ceph_copy_from_page_vector() is simply the length
argument it is provided.

This is called by rbd_obj_method_sync(), which returns the result if
it's non-negative.  But we always either ignore or overwrite that
return value.  So explicitly ignore what's returned by the copy
function, and have rbd_obj_method_sync() always return either a
negative errno or 0.

We also return the result of ceph_copy_from_page_vector() in
rbd_obj_read_sync().  There we still want to return the number of
bytes transferred, but we can use the value we already have in hand
rather than what ceph_copy_from_page_vector() provides.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 99f1a29..958e733 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1889,7 +1889,8 @@ static int rbd_obj_method_sync(struct rbd_device
*rbd_dev,
 	ret = obj_request->result;
 	if (ret < 0)
 		goto out;
-	ret = ceph_copy_from_page_vector(pages, inbound, 0,
+	ret = 0;
+	(void) ceph_copy_from_page_vector(pages, inbound, 0,
 					obj_request->xferred);
 	if (version)
 		*version = obj_request->version;
@@ -2088,7 +2089,9 @@ static int rbd_obj_read_sync(struct rbd_device
*rbd_dev,

 	rbd_assert(obj_request->xferred <= (u64) SIZE_MAX);
 	size = (size_t) obj_request->xferred;
-	ret = ceph_copy_from_page_vector(pages, buf, 0, size);
+	(void) ceph_copy_from_page_vector(pages, buf, 0, size);
+	rbd_assert(size <= (size_t) INT_MAX);
+	ret = (int) size;
 	if (version)
 		*version = obj_request->version;
 out:
@@ -2141,7 +2144,6 @@ rbd_dev_v1_header_read(struct rbd_device *rbd_dev,
u64 *version)
 		ret = rbd_obj_read_sync(rbd_dev, rbd_dev->header_name,
 				       0, size,
 				       (char *) ondisk, version);
-
 		if (ret < 0)
 			goto out_err;
 		if (WARN_ON((size_t) ret < size)) {
@@ -2803,7 +2805,6 @@ static int rbd_dev_v2_object_prefix(struct
rbd_device *rbd_dev)
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret < 0)
 		goto out;
-	ret = 0;    /* rbd_obj_method_sync() can return positive */

 	p = reply_buf;
 	rbd_dev->header.object_prefix = ceph_extract_encoded_string(&p,
@@ -3742,7 +3743,6 @@ static int rbd_dev_image_id(struct rbd_device
*rbd_dev)
 	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
 	if (ret < 0)
 		goto out;
-	ret = 0;    /* rbd_obj_method_sync() can return positive */

 	p = response;
 	rbd_dev->spec->image_id = ceph_extract_encoded_string(&p,
-- 
1.7.9.5

[PATCH 5/5] libceph: drop return value from page vector copy routines

[Alex Elder]

The return values provided for ceph_copy_to_page_vector() and
ceph_copy_from_page_vector() serve no purpose, so get rid of them.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c          |    5 ++---
 include/linux/ceph/libceph.h |    4 ++--
 net/ceph/pagevec.c           |   14 ++++++--------
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 958e733..4242cb8 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1890,8 +1890,7 @@ static int rbd_obj_method_sync(struct rbd_device
*rbd_dev,
 	if (ret < 0)
 		goto out;
 	ret = 0;
-	(void) ceph_copy_from_page_vector(pages, inbound, 0,
-					obj_request->xferred);
+	ceph_copy_from_page_vector(pages, inbound, 0, obj_request->xferred);
 	if (version)
 		*version = obj_request->version;
 out:
@@ -2089,7 +2088,7 @@ static int rbd_obj_read_sync(struct rbd_device
*rbd_dev,

 	rbd_assert(obj_request->xferred <= (u64) SIZE_MAX);
 	size = (size_t) obj_request->xferred;
-	(void) ceph_copy_from_page_vector(pages, buf, 0, size);
+	ceph_copy_from_page_vector(pages, buf, 0, size);
 	rbd_assert(size <= (size_t) INT_MAX);
 	ret = (int) size;
 	if (version)
diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index 2250f8b..29818fc 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -232,10 +232,10 @@ extern struct page **ceph_alloc_page_vector(int
num_pages, gfp_t flags);
 extern int ceph_copy_user_to_page_vector(struct page **pages,
 					 const void __user *data,
 					 loff_t off, size_t len);
-extern int ceph_copy_to_page_vector(struct page **pages,
+extern void ceph_copy_to_page_vector(struct page **pages,
 				    const void *data,
 				    loff_t off, size_t len);
-extern int ceph_copy_from_page_vector(struct page **pages,
+extern void ceph_copy_from_page_vector(struct page **pages,
 				    void *data,
 				    loff_t off, size_t len);
 extern int ceph_copy_page_vector_to_user(struct page **pages, void
__user *data,
diff --git a/net/ceph/pagevec.c b/net/ceph/pagevec.c
index 5b20be9..815a224 100644
--- a/net/ceph/pagevec.c
+++ b/net/ceph/pagevec.c
@@ -118,17 +118,17 @@ int ceph_copy_user_to_page_vector(struct page **pages,
 }
 EXPORT_SYMBOL(ceph_copy_user_to_page_vector);

-int ceph_copy_to_page_vector(struct page **pages,
+void ceph_copy_to_page_vector(struct page **pages,
 				    const void *data,
 				    loff_t off, size_t len)
 {
 	int i = 0;
 	size_t po = off & ~PAGE_CACHE_MASK;
 	size_t left = len;
-	size_t l;

 	while (left > 0) {
-		l = min_t(size_t, PAGE_CACHE_SIZE-po, left);
+		size_t l = min_t(size_t, PAGE_CACHE_SIZE-po, left);
+
 		memcpy(page_address(pages[i]) + po, data, l);
 		data += l;
 		left -= l;
@@ -138,21 +138,20 @@ int ceph_copy_to_page_vector(struct page **pages,
 			i++;
 		}
 	}
-	return len;
 }
 EXPORT_SYMBOL(ceph_copy_to_page_vector);

-int ceph_copy_from_page_vector(struct page **pages,
+void ceph_copy_from_page_vector(struct page **pages,
 				    void *data,
 				    loff_t off, size_t len)
 {
 	int i = 0;
 	size_t po = off & ~PAGE_CACHE_MASK;
 	size_t left = len;
-	size_t l;

 	while (left > 0) {
-		l = min_t(size_t, PAGE_CACHE_SIZE-po, left);
+		size_t l = min_t(size_t, PAGE_CACHE_SIZE-po, left);
+
 		memcpy(data, page_address(pages[i]) + po, l);
 		data += l;
 		left -= l;
@@ -162,7 +161,6 @@ int ceph_copy_from_page_vector(struct page **pages,
 			i++;
 		}
 	}
-	return len;
 }
 EXPORT_SYMBOL(ceph_copy_from_page_vector);

-- 
1.7.9.5

Re: [PATCH 0/5] ceph: cleanup ceph page vector functions

[Josh Durgin]

On 02/08/2013 08:31 AM, Alex Elder wrote:
> This series does some cleanup related to some functions that
> implement ceph page vectors.  Together, these resolve:
>      http://tracker.ceph.com/issues/4053
>
> 					-Alex
>
> [PATCH 1/5] ceph: remove a few bogus declarations
> [PATCH 2/5] libceph: use void pointers in page vector functions
> [PATCH 3/5] rbd: prevent bytes transferred overflow
> [PATCH 4/5] rbd: ignore result of ceph_copy_from_page_vector()
> [PATCH 5/5] libceph: drop return value from page vector copy

These all seem fine to me.
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>