Skip to contentSkip to conversation

kcore: fix test for end of list

PATCH In review Dan Carpenter 4 participants 6 messages Mar 22, 2010 – Mar 23, 2010

Subsystems (3)
Kernel janitors Proc filesystem Kcore
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 kcore: fix test for end of list

"m" is never NULL here. We need a different test for the end of list condition.

Signed-off-by Dan Carpenter <[email protected]>

---
Found with a static checker and compile tested only. Please review carefully.

fs/proc/kcore.c +1 -1
index a44a789..b442dac 100644
@@ -490,7 +490,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
 		}
 		read_unlock(&kclist_lock);
 
-		if (m = NULL) {
+		if (&m->list = &kclist_head) {
 			if (clear_user(buffer, tsz))
 				return -EFAULT;
 		} else if (is_vmalloc_or_module_addr((void *)start)) {
Acked-by on v1 1/1
#1 lore raw
Quoted 4 lines · On Mon, 22 Mar 2010 15:05:09 +0300 Dan Carpenter <[email protected]> wrote:

"m" is never NULL here. We need a different test for the end of list condition.

Signed-off-by Dan Carpenter <[email protected]>

yes, thank you.

Acked-by KAMEZAWA Hiroyuki <[email protected]>
Quoted 23 lines

---
Found with a static checker and compile tested only. Please review carefully.

fs/proc/kcore.c +1 -2
index a44a789..b442dac 100644
@@ -490,7 +490,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
 		}
 		read_unlock(&kclist_lock);
 
-		if (m = NULL) {
+		if (&m->list = &kclist_head) {
 			if (clear_user(buffer, tsz))
 				return -EFAULT;
 		} else if (is_vmalloc_or_module_addr((void *)start)) {
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Acked-by
#2 lore raw
Quoted 10 lines · KAMEZAWA Hiroyuki
Quoted 4 lines · On Mon, 22 Mar 2010 15:05:09 +0300 Dan Carpenter <[email protected]> wrote:

"m" is never NULL here.  We need a different test for the end of list condition.

Signed-off-by Dan Carpenter <[email protected]>

yes, thank you.

Acked-by KAMEZAWA Hiroyuki <[email protected]>

Yeah, this is a good catch for me too!

Acked-by WANG Cong <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
In reply to Dan Carpenter
Quoted 13 lines · On Mon, 22 Mar 2010 15:05:09 +0300 Dan Carpenter <[email protected]> wrote:

"m" is never NULL here. We need a different test for the end of list condition.

Signed-off-by Dan Carpenter <[email protected]>

---
Found with a static checker and compile tested only. Please review carefully.

fs/proc/kcore.c +0 -0
index a44a789..b442dac 100644

fs/proc/kcore.c read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) · line 496

Quoted 8 lines
fs/proc/kcore.c +1 -1
@@ -490,7 +490,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
 		}
 		read_unlock(&kclist_lock);
 
-		if (m = NULL) {
+		if (&m->list = &kclist_head) {
 			if (clear_user(buffer, tsz))
 				return -EFAULT;
 		} else if (is_vmalloc_or_module_addr((void *)start)) {

hm, that code's been there for five years. I wonder if it's actually necessary.

#4 lore raw
Quoted 18 lines · On Mon, 22 Mar 2010 23:31:13 -0400 Andrew Morton <[email protected]>…
Quoted 16 lines · On Mon, 22 Mar 2010 15:05:09 +0300 Dan Carpenter <[email protected]> wrote:

"m" is never NULL here. We need a different test for the end of list condition.

Signed-off-by Dan Carpenter <[email protected]>

---
Found with a static checker and compile tested only. Please review carefully.

fs/proc/kcore.c +0 -0
index a44a789..b442dac 100644
@@ -490,7 +490,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
 		}
 		read_unlock(&kclist_lock);
Quoted 8 lines

fs/proc/kcore.c read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) · line 496

Quoted 5 lines
fs/proc/kcore.c +1 -1
-		if (m = NULL) {
+		if (&m->list = &kclist_head) {
 			if (clear_user(buffer, tsz))
 				return -EFAULT;
 		} else if (is_vmalloc_or_module_addr((void *)start)) {

hm, that code's been there for five years. I wonder if it's actually necessary.

Hmm..on x86 and others, finally,
(kern_addr_valid(start))
will catch bad access. But some architectures seems to define kern_addr_valid() as

#define kern_addr_valid(addr) (1) (from arch/ia64/include/asm/pgtable.h)

Then, we should check "start" is valid address by ourselves (for now). We have a complete list of registered address in kclist_head.

Regards,
-Kame

#5 lore raw
In reply to Andrew Morton
Quoted 18 lines · Andrew Morton
Quoted 16 lines · On Mon, 22 Mar 2010 15:05:09 +0300 Dan Carpenter <[email protected]> wrote:

"m" is never NULL here.  We need a different test for the end of list condition.

Signed-off-by Dan Carpenter <[email protected]>

---
Found with a static checker and compile tested only.  Please review carefully.

fs/proc/kcore.c +0 -0
index a44a789..b442dac 100644
@@ -490,7 +490,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
               }
               read_unlock(&kclist_lock);
Quoted 7 lines

fs/proc/kcore.c read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) · line 496

Quoted 5 lines
fs/proc/kcore.c +1 -1
-             if (m = NULL) {
+             if (&m->list = &kclist_head) {
                       if (clear_user(buffer, tsz))
                               return -EFAULT;
               } else if (is_vmalloc_or_module_addr((void *)start)) {

hm, that code's been there for five years.  I wonder if it's actually necessary.

Nope, not that long, please see commit 2ef43ec7. :)