hugetlb: unsigned ret cannot be negative.

PATCH Superseded roel kluin 2 participants 4 messages Nov 29, 2008 – Dec 3, 2008

Subsystems (2)

Memory management HugeTLB

lore

Share & tools

lore

roel kluin

author

2008-11-29 11:37 UTC #0 lore raw

PATCH 1/1 hugetlb: unsigned ret cannot be negative.

unsigned long ret cannot be negative, but ret can get -EFAULT.

Signed-off-by Roel Kluin <[email protected]>

---
hugetlbfs_read_actor() returns int,
see
vi fs/hugetlbfs/inode.c +187

fs/hugetlbfs/inode.c +5 -3

index 61edc70..0af64e4 100644
@@ -252,6 +252,7 @@ static ssize_t hugetlbfs_read(struct file *filp, char __user *buf,
 	for (;;) {
 		struct page *page;
 		unsigned long nr, ret;
+		int ra;
 
 		/* nr is the maximum number of bytes to copy from this page */
 		nr = huge_page_size(h);

@@ -279,15 +280,16 @@ static ssize_t hugetlbfs_read(struct file *filp, char __user *buf,
 			/*
 			 * We have the page, copy it to user space buffer.
 			 */
-			ret = hugetlbfs_read_actor(page, offset, buf, len, nr);
+			ra = hugetlbfs_read_actor(page, offset, buf, len, nr);
 		}
-		if (ret < 0) {
+		if (ra < 0) {
 			if (retval == 0)
-				retval = ret;
+				retval = ra;
 			if (page)
 				page_cache_release(page);
 			goto out;
 		}
+		ret = ra;
 
 		offset += ret;
 		retval += ret;

Andrew Morton

maintainer

2008-12-02 22:02 UTC #1 lore raw

Quoted 30 lines · On Sat, 29 Nov 2008 06:36:59 -0500 roel kluin <[email protected]> wrote:

unsigned long ret cannot be negative, but ret can get -EFAULT.

Signed-off-by Roel Kluin <[email protected]>

---
hugetlbfs_read_actor() returns int,
see
vi fs/hugetlbfs/inode.c +187

fs/hugetlbfs/inode.c +3 -2

index 61edc70..0af64e4 100644
@@ -252,6 +252,7 @@ static ssize_t hugetlbfs_read(struct file *filp, char __user *buf,
 	for (;;) {
 		struct page *page;
 		unsigned long nr, ret;
+		int ra;
 
 		/* nr is the maximum number of bytes to copy from this page */
 		nr = huge_page_size(h);

@@ -279,15 +280,16 @@ static ssize_t hugetlbfs_read(struct file *filp, char __user *buf,
 			/*
 			 * We have the page, copy it to user space buffer.
 			 */
-			ret = hugetlbfs_read_actor(page, offset, buf, len, nr);
+			ra = hugetlbfs_read_actor(page, offset, buf, len, nr);
 		}
-		if (ret < 0) {
+		if (ra < 0) {
 			if (retval == 0)

fs/hugetlbfs/inode.c static ssize_t hugetlbfs_read(struct file *filp, char __user *buf, · line 295

Quoted 10 lines

fs/hugetlbfs/inode.c +2 -1

-				retval = ret;
+				retval = ra;
 			if (page)
 				page_cache_release(page);
 			goto out;
 		}
+		ret = ra;
 
 		offset += ret;
 		retval += ret;

`ra' can obviously be used uninitialised here. The compiler reports this, too.

Roel Kluin

author

2008-12-02 22:51 UTC #2 lore raw

PATCH v2 1/1 hugetlb: unsigned ret cannot be negative.

Quoted 21 lines · Andrew Morton

Quoted 18 lines · On Sat, 29 Nov 2008 06:36:59 -0500 roel kluin <[email protected]> wrote:
unsigned long ret cannot be negative, but ret can get -EFAULT.
Signed-off-by Roel Kluin <[email protected]>
---
hugetlbfs_read_actor() returns int,
see
vi fs/hugetlbfs/inode.c +187
fs/hugetlbfs/inode.c +1 -0
index 61edc70..0af64e4 100644
@@ -252,6 +252,7 @@ static ssize_t hugetlbfs_read(struct file *filp, char __user *buf,
 	for (;;) {
 		struct page *page;
 		unsigned long nr, ret;
+		int ra;
 

Quoted 14 lines

fs/hugetlbfs/inode.c static ssize_t hugetlbfs_read(struct file *filp, char __user *buf, · line 285

Quoted 11 lines

fs/hugetlbfs/inode.c +2 -2

 		/* nr is the maximum number of bytes to copy from this page */
 		nr = huge_page_size(h);

@@ -279,15 +280,16 @@ static ssize_t hugetlbfs_read(struct file *filp, char __user *buf,
 			/*
 			 * We have the page, copy it to user space buffer.
 			 */
-			ret = hugetlbfs_read_actor(page, offset, buf, len, nr);
+			ra = hugetlbfs_read_actor(page, offset, buf, len, nr);
 		}
-		if (ret < 0) {
+		if (ra < 0) {

`ra' can obviously be used uninitialised here. The compiler reports this, too.

Yes, it was incomplete as well, sorry. This should be OK. (checkpatch tested) --------------->8----------------8<--------------------- unsigned long ret cannot be negative, but ret can get -EFAULT.

Signed-off-by Roel Kluin <[email protected]>

---

fs/hugetlbfs/inode.c +8 -4

index 61edc70..07fa7e3 100644
@@ -252,6 +252,7 @@ static ssize_t hugetlbfs_read(struct file *filp, char __user *buf,
 	for (;;) {
 		struct page *page;
 		unsigned long nr, ret;
+		int ra;
 
 		/* nr is the maximum number of bytes to copy from this page */
 		nr = huge_page_size(h);

@@ -274,16 +275,19 @@ static ssize_t hugetlbfs_read(struct file *filp, char __user *buf,
 			 */
 			ret = len < nr ? len : nr;
 			if (clear_user(buf, ret))
-				ret = -EFAULT;
+				ra = -EFAULT;
+			else
+				ra = 0;
 		} else {
 			/*
 			 * We have the page, copy it to user space buffer.
 			 */
-			ret = hugetlbfs_read_actor(page, offset, buf, len, nr);
+			ra = hugetlbfs_read_actor(page, offset, buf, len, nr);
+			ret = ra;
 		}
-		if (ret < 0) {
+		if (ra < 0) {
 			if (retval == 0)
-				retval = ret;
+				retval = ra;
 			if (page)
 				page_cache_release(page);
 			goto out;

That function is pretty sad-looking now. It has ra', ret' and retval', all rather confusingly. After being renamed to something useful, ret' should have type size_t, hugetlbfs_read_actor() should return size_t and the whole logic around these three things needs a can of drano tipped down it.

Oh well.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>