[patch] NULL pointer check for vma->vm_mm

[patch] NULL pointer check for vma->vm_mm

[Kenichi Okuyama]

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]

Dear all,

I was looking at the ./mm/rmap.c .. I found that, in function
"page_referenced_one()",
   struct mm_struct *mm = vma->vm_mm;
was being refererred without NULL check.

Though I do agree that this works for most of the cases, I thought it
is better to add
BUG_ON() for case of mm being NULL.

attached is the patch for this

thank you in advance for taking your time.
best regards,
-- 
(Kenichi Okuyama)
URL: http://www.dd.iij4u.or.jp/~okuyamak/

[-- Attachment #2: patch.mm --]
[-- Type: application/octet-stream, Size: 316 bytes --]

--- ./mm/rmap.c.orig	2008-02-01 15:36:50.000000000 +0900
+++ ./mm/rmap.c	2008-02-01 15:42:43.000000000 +0900
@@ -276,6 +276,8 @@ static int page_referenced_one(struct pa
 	spinlock_t *ptl;
 	int referenced = 0;
 
+	BUG_ON(( mm == NULL ));
+
 	address = vma_address(page, vma);
 	if (address == -EFAULT)
 		goto out;

Re: [patch] NULL pointer check for vma->vm_mm

[Andrew Morton]

On Fri, 1 Feb 2008 16:39:07 +0900 "Kenichi Okuyama" <kenichi.okuyama@gmail.com> wrote:

> Dear all,
> 
> I was looking at the ./mm/rmap.c .. I found that, in function
> "page_referenced_one()",
>    struct mm_struct *mm = vma->vm_mm;
> was being refererred without NULL check.
> 
> Though I do agree that this works for most of the cases, I thought it
> is better to add
> BUG_ON() for case of mm being NULL.
> 
> attached is the patch for this

If we dereference NULL then the kernel will display basically the same
information as would a BUG, and it takes the same action.  So adding a
BUG_ON here really doesn't gain us anything.

Also, I think vma->vm_mm == 0 is not a valid state, so this just shouldn't
happen - the code is OK to assume that a particular invariant is being
honoured.

Re: [patch] NULL pointer check for vma->vm_mm

[Kenichi Okuyama]

Dear Andrew, all,

First of all, thank you for looking at the patch.

I do agree that if mm is NULL, system will call Oops anyway.
However, since it's oops, it does not stop the system, nor call kdump.

By calling BUG_ON(), it'll gives us chance of calling kdump at the first chance.

Since this is very rare to happen, I thought we should capture the incident
whenever possible. On other hand, because BUG_ON macro is very light,
I thought this will not harm any performance...

Forgive me in advance if I was wrong.
I still think checking mm with BUG_ON here is better than counting on Oops.

best regards,



2008/2/1, Andrew Morton <akpm@linux-foundation.org>:
> On Fri, 1 Feb 2008 16:39:07 +0900 "Kenichi Okuyama" <kenichi.okuyama@gmail.com> wrote:
>
> > Dear all,
> >
> > I was looking at the ./mm/rmap.c .. I found that, in function
> > "page_referenced_one()",
> >    struct mm_struct *mm = vma->vm_mm;
> > was being refererred without NULL check.
> >
> > Though I do agree that this works for most of the cases, I thought it
> > is better to add
> > BUG_ON() for case of mm being NULL.
> >
> > attached is the patch for this
>
> If we dereference NULL then the kernel will display basically the same
> information as would a BUG, and it takes the same action.  So adding a
> BUG_ON here really doesn't gain us anything.
>
> Also, I think vma->vm_mm == 0 is not a valid state, so this just shouldn't
> happen - the code is OK to assume that a particular invariant is being
> honoured.
>
>


-- 
奥山　健一(Kenichi Okuyama) [煤背会: No. 0x00000001]
URL: http://www.dd.iij4u.or.jp/~okuyamak/
     http://developer.osdl.jp/projects/doubt/

Re: [patch] NULL pointer check for vma->vm_mm

[Andrew Morton]

On Fri, 1 Feb 2008 17:24:17 +0900 "Kenichi Okuyama" <kenichi.okuyama@gmail.com> wrote:

> First of all, thank you for looking at the patch.
> 
> I do agree that if mm is NULL, system will call Oops anyway.
> However, since it's oops, it does not stop the system, nor call kdump.

That would be a huge bug in kdump?  Surely it dumps when the kernel oopses?

> By calling BUG_ON(), it'll gives us chance of calling kdump at the first chance.
> 
> Since this is very rare to happen, I thought we should capture the incident
> whenever possible. On other hand, because BUG_ON macro is very light,
> I thought this will not harm any performance...
> 
> Forgive me in advance if I was wrong.
> I still think checking mm with BUG_ON here is better than counting on Oops.

But there are probably a million potential NULL-pointer dereferences in the 
kernel.  Why single out this one?

Re: [patch] NULL pointer check for vma->vm_mm

[Kenichi Okuyama]

Dear Andrew,
Sorry that it took very long before I could reply.

2008/2/1, Andrew Morton <akpm@linux-foundation.org>:
> On Fri, 1 Feb 2008 17:24:17 +0900 "Kenichi Okuyama" <kenichi.okuyama@gmail.com> wrote:
>
> > First of all, thank you for looking at the patch.
> >
> > I do agree that if mm is NULL, system will call Oops anyway.
> However, since it's oops, it does not stop the system, nor call kdump.
>
> That would be a huge bug in kdump?  Surely it dumps when the kernel oopses?

I'm sorry.
Oops did dump on my home pc. But it didn't on my office pc.
I'll take back the patch, and check what I've done wrong at office.


> But there are probably a million potential NULL-pointer dereferences in the
> kernel.  Why single out this one?

I was interested in "Bad swap file entry" problem.

I expereiced this myself. After (quite a lot of ) "Bad swap file
entry" error log from kernel, it Oopsed three times, then kernel was
dead ( It's almost three years from now, so this was without kdump ).

I did find that three Oops happened inside page_referenced() function,
and that it was due to NULL pointer. In 2.6.24, it was only this "mm"
and one more in page_referenced_file() that did not have NULL pointer
check.

So I was really thinking about two more patches. One for "mappers"
NULL pointer check, and other one is to add msr printout when Oops or
Pank happens , to make sure that when Oops or Paniced, still my PC is
not broken.


I needed the evidence so that I don't have to worry about
broken Memory, nor broken Cache.
and I think we still do not have MSRs dumped out as
part of kdump..
# Am I wrong again??
-- 
Kenichi Okuyama
URL: http://www.dd.iij4u.or.jp/~okuyamak/