2011-03-27 09:16:09

by Anca Emanuel

[permalink] [raw]
Subject: BUG: unable to handle kernel paging request

Hi, I'm using latest kernel git.


[15117.080119] BUG: unable to handle kernel paging request at fffffffffffffff3
[15117.080152] IP: [<ffffffff811b4989>] vma_stop+0x19/0x40
[15117.080172] PGD 1a05067 PUD 1a06067 PMD 0
[15117.080191] Oops: 0000 [#1] SMP
[15117.080204] last sysfs file:
/sys/devices/pci0000:00/0000:00:1f.2/host0/target0:0:0/0:0:0:0/block/sda/uevent
[15117.080222] CPU 1
[15117.080229] Modules linked in: binfmt_misc parport_pc ppdev
snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm
snd_seq_midi snd_rawmidi adt7475 hwmon_vid snd_seq_midi_event snd_seq
nouveau snd_timer snd_seq_device ttm snd drm_kms_helper soundcore drm
snd_page_alloc i2c_algo_bit psmouse serio_raw video r8169 intel_agp
intel_gtt lp parport mii pata_marvell ahci libahci
[15117.080379]
[15117.080387] Pid: 1446, comm: chromium-browse Not tainted
2.6.38-git18+ #3 MICRO-STAR INTERNATIONAL CO.,LTD MS-7360/MS-7360
[15117.080412] RIP: 0010:[<ffffffff811b4989>] [<ffffffff811b4989>]
vma_stop+0x19/0x40
[15117.080430] RSP: 0018:ffff8800654a3e48 EFLAGS: 00010213
[15117.080440] RAX: 00000000fffffff3 RBX: ffff88006c6ae7e0 RCX: 0000000000000013
[15117.080453] RDX: ffffffff81619200 RSI: fffffffffffffff3 RDI: ffff88006c6ae7e0
[15117.080465] RBP: ffff8800654a3e58 R08: 0000000000000001 R09: 0000000000000000
[15117.080478] R10: 0000000000000022 R11: 0000000000000293 R12: ffff88002509aa00
[15117.080490] R13: fffffffffffffff3 R14: ffff88005d142480 R15: ffff8800654a3ec0
[15117.080503] FS: 00007ff183615920(0000) GS:ffff88007fc80000(0000)
knlGS:0000000000000000
[15117.080517] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[15117.080529] CR2: fffffffffffffff3 CR3: 00000000654ab000 CR4: 00000000000006e0
[15117.080541] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[15117.080554] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[15117.080567] Process chromium-browse (pid: 1446, threadinfo
ffff8800654a2000, task ffff88006a05db00)
[15117.080581] Stack:
[15117.080588] 0000000000000000 ffff88006c6ae7e0 ffff8800654a3e78
ffffffff811b4a3a
[15117.080613] ffff88005d142480 ffff88005d142480 ffff8800654a3ef8
ffffffff8117d991
[15117.080637] 00000000fffffff3 00007fff1aecb250 ffff88002509aa38
ffff8800654a3f48
[15117.080660] Call Trace:
[15117.080671] [<ffffffff811b4a3a>] m_stop+0x1a/0x40
[15117.080685] [<ffffffff8117d991>] seq_read+0x1e1/0x420
[15117.080698] [<ffffffff8115c685>] vfs_read+0xc5/0x190
[15117.080711] [<ffffffff8115c851>] sys_read+0x51/0x90
[15117.080724] [<ffffffff815b2d02>] system_call_fastpath+0x16/0x1b
[15117.080736] Code: c9 90 c3 eb 0d 90 90 90 90 90 90 90 90 90 90 90
90 90 55 48 89 e5 53 48 83 ec 08 0f 1f 44 00 00 48 85 f6 74 1a 48 39
77 10 74 14
[15117.080872] 8b 1e 48 8d 7b 60 e8 eb 2f ed ff 48 89 df e8 43 9b ea ff 48
[15117.080940] RIP [<ffffffff811b4989>] vma_stop+0x19/0x40
[15117.080955] RSP <ffff8800654a3e48>
[15117.080963] CR2: fffffffffffffff3
[15117.113062] ---[ end trace 4aa56b1e030e2b1e ]---


Attachments:
error.txt (54.62 kB)

2011-03-27 15:37:57

by Linus Torvalds

[permalink] [raw]
Subject: Re: BUG: unable to handle kernel paging request

Al, this smells like your /proc cleanups/fixes...

On Sun, Mar 27, 2011 at 2:16 AM, Anca Emanuel <[email protected]> wrote:
> Hi, I'm using latest kernel git.
>
>
> [15117.080119] BUG: unable to handle kernel paging request at fffffffffffffff3

That's "-13" (possibly -EACCES)

> [15117.080152] IP: [<ffffffff811b4989>] vma_stop+0x19/0x40

.. and the code disassembles to

0: 55 push %rbp
1: 48 89 e5 mov %rsp,%rbp
4: 53 push %rbx
5: 48 83 ec 08 sub $0x8,%rsp
9: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
e: 48 85 f6 test %rsi,%rsi
11: 74 1a je 0x2d
13: 48 39 77 10 cmp %rsi,0x10(%rdi)
17: 74 14 je 0x2d
19: 8b 1e mov (%rsi),%ebx
1b: 48 8d 7b 60 lea 0x60(%rbx),%rdi
1f: e8 eb 2f ed ff callq up_read

where that instruction at 0x19 is the access "mm = vma->vm_mm". So
it's vma that is -EPERM.

I bet it's due to commit ec6fd8a4355c ("report errors in /proc/*/*map*
sanely"), which replaces NULL with various ERR_PTR() cases.

> [15117.080660] Call Trace:
> [15117.080671] ?[<ffffffff811b4a3a>] m_stop+0x1a/0x40
> [15117.080685] ?[<ffffffff8117d991>] seq_read+0x1e1/0x420
> [15117.080698] ?[<ffffffff8115c685>] vfs_read+0xc5/0x190
> [15117.080711] ?[<ffffffff8115c851>] sys_read+0x51/0x90
> [15117.080724] ?[<ffffffff815b2d02>] system_call_fastpath+0x16/0x1b

Al?

Linus

2011-03-27 16:00:46

by Cong Wang

[permalink] [raw]
Subject: Re: BUG: unable to handle kernel paging request

On Sun, Mar 27, 2011 at 11:37 PM, Linus Torvalds
<[email protected]> wrote:
> Al, this smells like your /proc cleanups/fixes...
>
> On Sun, Mar 27, 2011 at 2:16 AM, Anca Emanuel <[email protected]> wrote:
>> Hi, I'm using latest kernel git.
>>
>>
>> [15117.080119] BUG: unable to handle kernel paging request at fffffffffffffff3
>
> That's "-13" (possibly -EACCES)
>
>> [15117.080152] IP: [<ffffffff811b4989>] vma_stop+0x19/0x40
>
> .. and the code disassembles to
>
>   0:   55                      push   %rbp
>   1:   48 89 e5                mov    %rsp,%rbp
>   4:   53                      push   %rbx
>   5:   48 83 ec 08             sub    $0x8,%rsp
>   9:   0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
>   e:   48 85 f6                test   %rsi,%rsi
>  11:   74 1a                   je     0x2d
>  13:   48 39 77 10             cmp    %rsi,0x10(%rdi)
>  17:   74 14                   je     0x2d
>  19:   8b 1e                   mov    (%rsi),%ebx
>  1b:   48 8d 7b 60             lea    0x60(%rbx),%rdi
>  1f:   e8 eb 2f ed ff          callq  up_read
>
> where that instruction at 0x19 is the access "mm = vma->vm_mm". So
> it's vma that is -EPERM.
>
> I bet it's due to commit ec6fd8a4355c ("report errors in /proc/*/*map*
> sanely"), which replaces NULL with various ERR_PTR() cases.
>

Exactly... should be fixed by something like:

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 7c708a4..6b82632 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -90,7 +90,7 @@ static void pad_len_spaces(struct seq_file *m, int len)

static void vma_stop(struct proc_maps_private *priv, struct
vm_area_struct *vma)
{
- if (vma && vma != priv->tail_vma) {
+ if (vma && !IS_ERR(vma) && vma != priv->tail_vma) {
struct mm_struct *mm = vma->vm_mm;
up_read(&mm->mmap_sem);
mmput(mm);

2011-03-27 17:44:17

by Anca Emanuel

[permalink] [raw]
Subject: Re: BUG: unable to handle kernel paging request

On Sun, Mar 27, 2011 at 7:00 PM, Am?rico Wang <[email protected]> wrote:
> Exactly... should be fixed by something like:
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 7c708a4..6b82632 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -90,7 +90,7 @@ static void pad_len_spaces(struct seq_file *m, int len)
>
> ?static void vma_stop(struct proc_maps_private *priv, struct
> vm_area_struct *vma)
> ?{
> - ? ? ? if (vma && vma != priv->tail_vma) {
> + ? ? ? if (vma && !IS_ERR(vma) && vma != priv->tail_vma) {
> ? ? ? ? ? ? ? ?struct mm_struct *mm = vma->vm_mm;
> ? ? ? ? ? ? ? ?up_read(&mm->mmap_sem);
> ? ? ? ? ? ? ? ?mmput(mm);
>

I tested the patch and works ok now. Thanks Am?rico Wang !
Without it, I can reproduce the bug.

2011-03-27 17:52:15

by Stephen Wilson

[permalink] [raw]
Subject: Re: BUG: unable to handle kernel paging request

On Mon, Mar 28, 2011 at 12:00:44AM +0800, Américo Wang wrote:
> On Sun, Mar 27, 2011 at 11:37 PM, Linus Torvalds
> <[email protected]> wrote:
> > Al, this smells like your /proc cleanups/fixes...
> >
> > On Sun, Mar 27, 2011 at 2:16 AM, Anca Emanuel <[email protected]> wrote:
> >> Hi, I'm using latest kernel git.
> >>
> >>
> >> [15117.080119] BUG: unable to handle kernel paging request at fffffffffffffff3
> >
> > That's "-13" (possibly -EACCES)
> >
> >> [15117.080152] IP: [<ffffffff811b4989>] vma_stop+0x19/0x40
> >
> > .. and the code disassembles to
> >
> >   0:   55                      push   %rbp
> >   1:   48 89 e5                mov    %rsp,%rbp
> >   4:   53                      push   %rbx
> >   5:   48 83 ec 08             sub    $0x8,%rsp
> >   9:   0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
> >   e:   48 85 f6                test   %rsi,%rsi
> >  11:   74 1a                   je     0x2d
> >  13:   48 39 77 10             cmp    %rsi,0x10(%rdi)
> >  17:   74 14                   je     0x2d
> >  19:   8b 1e                   mov    (%rsi),%ebx
> >  1b:   48 8d 7b 60             lea    0x60(%rbx),%rdi
> >  1f:   e8 eb 2f ed ff          callq  up_read
> >
> > where that instruction at 0x19 is the access "mm = vma->vm_mm". So
> > it's vma that is -EPERM.
> >
> > I bet it's due to commit ec6fd8a4355c ("report errors in /proc/*/*map*
> > sanely"), which replaces NULL with various ERR_PTR() cases.
> >
>
> Exactly... should be fixed by something like:
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 7c708a4..6b82632 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -90,7 +90,7 @@ static void pad_len_spaces(struct seq_file *m, int len)
>
> static void vma_stop(struct proc_maps_private *priv, struct
> vm_area_struct *vma)
> {
> - if (vma && vma != priv->tail_vma) {
> + if (vma && !IS_ERR(vma) && vma != priv->tail_vma) {
> struct mm_struct *mm = vma->vm_mm;
> up_read(&mm->mmap_sem);
> mmput(mm);

FWIW, that looks like the right fix to me.

Also CC'ing Tony Luck as he reported what appears to be the same issue
on the 25'th.

--
steve

2011-03-28 03:47:37

by Cong Wang

[permalink] [raw]
Subject: [Patch] proc: check error pointer returned by m_start()

Anca reported a bug:

[15117.080119] BUG: unable to handle kernel paging request at fffffffffffffff3
[15117.080152] IP: [<ffffffff811b4989>] vma_stop+0x19/0x40

Linus did the initial analysis, and found this was caused
by commit ec6fd8a4355c ("report errors in /proc/*/*map*
sanely"), which replaces NULL with various ERR_PTR() cases.

This is true, that commit changed the return value of m_start(),
which will return an error pointer on failure, but Al forgot
to check the error pointer in m_stop() which will be called
when m_start() fails. This patches fixes it.

Reported-by: Anca Emanuel <[email protected]>
Tested-by: Anca Emanuel <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Al Viro <[email protected]>
Signed-off-by: WANG Cong <[email protected]>

---
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 7c708a4..6b82632 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -90,7 +90,7 @@ static void pad_len_spaces(struct seq_file *m, int len)

static void vma_stop(struct proc_maps_private *priv, struct vm_area_struct *vma)
{
- if (vma && vma != priv->tail_vma) {
+ if (vma && !IS_ERR(vma) && vma != priv->tail_vma) {
struct mm_struct *mm = vma->vm_mm;
up_read(&mm->mmap_sem);
mmput(mm);

2011-03-28 03:59:21

by Linus Torvalds

[permalink] [raw]
Subject: Re: [Patch] proc: check error pointer returned by m_start()

On Sun, Mar 27, 2011 at 8:46 PM, Amerigo Wang <[email protected]> wrote:
>
> This is true, that commit changed the return value of m_start(),
> which will return an error pointer on failure, but Al forgot
> to check the error pointer in m_stop() which will be called
> when m_start() fails. This patches fixes it.

I did this slightly differently, and put the check in m_stop()
instead, because I felt that matched the logic of m_start, while
vma_stop() is more of an internal helper thing.

I dunno. I don't think it matters. But one thing I reacted to was that
when I was walking through the logic, I really wanted to say "seq_file
is wrong to call m_stop if m_start returned an error code". I really
felt like "hwy, if ->start fails, we damn well shouldn't have called
->stop".

But I guess we're stuck with that particular semantic for seq_files by now.

Linus

2011-03-28 05:03:33

by Cong Wang

[permalink] [raw]
Subject: Re: [Patch] proc: check error pointer returned by m_start()

于 2011年03月28日 11:58, Linus Torvalds 写道:
> On Sun, Mar 27, 2011 at 8:46 PM, Amerigo Wang<[email protected]> wrote:
>>
>> This is true, that commit changed the return value of m_start(),
>> which will return an error pointer on failure, but Al forgot
>> to check the error pointer in m_stop() which will be called
>> when m_start() fails. This patches fixes it.
>
> I did this slightly differently, and put the check in m_stop()
> instead, because I felt that matched the logic of m_start, while
> vma_stop() is more of an internal helper thing.
>

Ok, I am fine with this, will send an updated patch.

> I dunno. I don't think it matters. But one thing I reacted to was that
> when I was walking through the logic, I really wanted to say "seq_file
> is wrong to call m_stop if m_start returned an error code". I really
> felt like "hwy, if ->start fails, we damn well shouldn't have called
> ->stop".


This is a good point and makes prefect sense.

>
> But I guess we're stuck with that particular semantic for seq_files by now.
>

Yup, I guess there are some seq_file users still rely on this behavior,
we can fix them all later.

Thanks.

2011-03-28 05:28:19

by Cong Wang

[permalink] [raw]
Subject: [Patch V2] proc: check error pointer returned by m_start()

From: WANG Cong <[email protected]>

V2: move the check into m_stop() as suggested by Linus,
also, most ->show() implementations assume the second parameter 'v'
is not NULL, this fixes them too.

Anca reported a bug:

[15117.080119] BUG: unable to handle kernel paging request at fffffffffffffff3
[15117.080152] IP: [<ffffffff811b4989>] vma_stop+0x19/0x40

Linus did the initial analysis, and found this was caused
by commit ec6fd8a4355c ("report errors in /proc/*/*map*
sanely"), which replaces NULL with various ERR_PTR() cases.

This is true, that commit changed the return value of m_start(),
which will return an error pointer on failure, but Al forgot
to check the error pointer in m_stop() which will be called
when m_start() fails. This patches fixes it.

Reported-by: Anca Emanuel <[email protected]>
Tested-by: Anca Emanuel <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Al Viro <[email protected]>
Signed-off-by: WANG Cong <[email protected]>
Signed-off-by: WANG Cong <[email protected]>

---

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 7c708a4..8e59169 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -124,8 +124,10 @@ static void *m_start(struct seq_file *m, loff_t *pos)
return ERR_PTR(-ESRCH);

mm = mm_for_maps(priv->task);
- if (!mm || IS_ERR(mm))
+ if (IS_ERR_OR_NULL(mm)) {
+ put_task_struct(priv->task);
return mm;
+ }
down_read(&mm->mmap_sem);

tail_vma = get_gate_vma(priv->task->mm);
@@ -182,6 +184,8 @@ static void m_stop(struct seq_file *m, void *v)
struct proc_maps_private *priv = m->private;
struct vm_area_struct *vma = v;

+ if (IS_ERR_OR_NULL(v))
+ return;
vma_stop(priv, vma);
if (priv->task)
put_task_struct(priv->task);
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 05d6b0e..e17d5e6 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -83,7 +83,7 @@ static int traverse(struct seq_file *m, loff_t offset)
p = m->op->start(m, &index);
while (p) {
error = PTR_ERR(p);
- if (IS_ERR(p))
+ if (IS_ERR_OR_NULL(p))
break;
error = m->op->show(m, p);
if (error < 0)

2011-03-28 05:46:25

by Cong Wang

[permalink] [raw]
Subject: Re: [Patch V2] proc: check error pointer returned by m_start()

于 2011年03月28日 13:26, Amerigo Wang 写道:
> From: WANG Cong<[email protected]>
>
> V2: move the check into m_stop() as suggested by Linus,
> also, most ->show() implementations assume the second parameter 'v'
> is not NULL, this fixes them too.

Linus, forget this one, I saw you already checked in your own
fix, I will make one based on that. :)

Thanks.

2011-03-28 05:46:32

by Anca Emanuel

[permalink] [raw]
Subject: Re: [Patch V2] proc: check error pointer returned by m_start()

On Mon, Mar 28, 2011 at 8:26 AM, Amerigo Wang <[email protected]> wrote:
> From: WANG Cong <[email protected]>
>
> V2: move the check into m_stop() as suggested by Linus,
> also, most ->show() implementations assume the second parameter 'v'
> is not NULL, this fixes them too.
>
> Anca reported a bug:
>
> [15117.080119] BUG: unable to handle kernel paging request at fffffffffffffff3
> [15117.080152] IP: [<ffffffff811b4989>] vma_stop+0x19/0x40
>
> Linus did the initial analysis, and found this was caused
> by commit ec6fd8a4355c ("report errors in /proc/*/*map*
> sanely"), which replaces NULL with various ERR_PTR() cases.
>
> This is true, that commit changed the return value of m_start(),
> which will return an error pointer on failure, but Al forgot
> to check the error pointer in m_stop() which will be called
> when m_start() fails. This patches fixes it.
>
> Reported-by: Anca Emanuel <[email protected]>
> Tested-by: Anca Emanuel <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Cc: Al Viro <[email protected]>
> Signed-off-by: WANG Cong <[email protected]>
> Signed-off-by: WANG Cong <[email protected]>
>
> ---
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 7c708a4..8e59169 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -124,8 +124,10 @@ static void *m_start(struct seq_file *m, loff_t *pos)
> ? ? ? ? ? ? ? ?return ERR_PTR(-ESRCH);
>
> ? ? ? ?mm = mm_for_maps(priv->task);
> - ? ? ? if (!mm || IS_ERR(mm))
> + ? ? ? if (IS_ERR_OR_NULL(mm)) {
> + ? ? ? ? ? ? ? put_task_struct(priv->task);
> ? ? ? ? ? ? ? ?return mm;
> + ? ? ? }
> ? ? ? ?down_read(&mm->mmap_sem);
>
> ? ? ? ?tail_vma = get_gate_vma(priv->task->mm);
> @@ -182,6 +184,8 @@ static void m_stop(struct seq_file *m, void *v)
> ? ? ? ?struct proc_maps_private *priv = m->private;
> ? ? ? ?struct vm_area_struct *vma = v;
>
> + ? ? ? if (IS_ERR_OR_NULL(v))
> + ? ? ? ? ? ? ? return;

Note: this is not functional equivalent with the previous patch.

> ? ? ? ?vma_stop(priv, vma);
> ? ? ? ?if (priv->task)
> ? ? ? ? ? ? ? ?put_task_struct(priv->task);
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 05d6b0e..e17d5e6 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -83,7 +83,7 @@ static int traverse(struct seq_file *m, loff_t offset)
> ? ? ? ?p = m->op->start(m, &index);
> ? ? ? ?while (p) {
> ? ? ? ? ? ? ? ?error = PTR_ERR(p);
> - ? ? ? ? ? ? ? if (IS_ERR(p))
> + ? ? ? ? ? ? ? if (IS_ERR_OR_NULL(p))
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?error = m->op->show(m, p);
> ? ? ? ? ? ? ? ?if (error < 0)
> --

I din't test the above patch.

Linus already have the fix in his tree.

2011-03-28 05:50:16

by Cong Wang

[permalink] [raw]
Subject: Re: [Patch V2] proc: check error pointer returned by m_start()

于 2011年03月28日 13:46, Anca Emanuel 写道:
> On Mon, Mar 28, 2011 at 8:26 AM, Amerigo Wang<[email protected]> wrote:
>> From: WANG Cong<[email protected]>
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 7c708a4..8e59169 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -124,8 +124,10 @@ static void *m_start(struct seq_file *m, loff_t *pos)
>> return ERR_PTR(-ESRCH);
>>
>> mm = mm_for_maps(priv->task);
>> - if (!mm || IS_ERR(mm))
>> + if (IS_ERR_OR_NULL(mm)) {
>> + put_task_struct(priv->task);
>> return mm;
>> + }
>> down_read(&mm->mmap_sem);
>>
>> tail_vma = get_gate_vma(priv->task->mm);
>> @@ -182,6 +184,8 @@ static void m_stop(struct seq_file *m, void *v)
>> struct proc_maps_private *priv = m->private;
>> struct vm_area_struct *vma = v;
>>
>> + if (IS_ERR_OR_NULL(v))
>> + return;
>
> Note: this is not functional equivalent with the previous patch.
>

I moved that put_task_struct() into m_start() itself.

>
> I din't test the above patch.
>
> Linus already have the fix in his tree.

Yes, I really should pull before I made a patch. :-/

Anyway, thanks for reporting and testing.

2011-03-28 07:08:24

by Cong Wang

[permalink] [raw]
Subject: [PATCH 1/2] proc: Use IS_ERR_OR_NULL() helper

Use IS_ERR_OR_NULL() helper

Signed-off-by: Amerigo Wang <[email protected]>
---
fs/proc/task_mmu.c | 10 ++++++----
fs/proc/task_nommu.c | 2 +-
2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 2e7addf..3c06570 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -124,7 +124,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
return ERR_PTR(-ESRCH);

mm = mm_for_maps(priv->task);
- if (!mm || IS_ERR(mm))
+ if (IS_ERR_OR_NULL(mm))
return mm;
down_read(&mm->mmap_sem);

@@ -182,7 +182,7 @@ static void m_stop(struct seq_file *m, void *v)
struct proc_maps_private *priv = m->private;
struct vm_area_struct *vma = v;

- if (!IS_ERR(vma))
+ if (!IS_ERR_OR_NULL(vma))
vma_stop(priv, vma);
if (priv->task)
put_task_struct(priv->task);
@@ -768,9 +768,11 @@ static ssize_t pagemap_read(struct file *file, char __user *buf,
goto out;

mm = mm_for_maps(task);
- ret = PTR_ERR(mm);
- if (!mm || IS_ERR(mm))
+ if (IS_ERR_OR_NULL(mm)) {
+ if (mm)
+ ret = PTR_ERR(mm);
goto out_task;
+ }

ret = -EINVAL;
/* file position must be aligned */
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 980de54..35e8b2a 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -202,7 +202,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
return ERR_PTR(-ESRCH);

mm = mm_for_maps(priv->task);
- if (!mm || IS_ERR(mm)) {
+ if (IS_ERR_OR_NULL(mm)) {
put_task_struct(priv->task);
priv->task = NULL;
return mm;
--
1.7.4

2011-03-28 07:08:29

by Cong Wang

[permalink] [raw]
Subject: [PATCH 2/2] proc: do cleanup in m_start() rather than m_stop()

In the future, we will not call ->stop() anymore when ->start() fails,
so ->start() is responsible to do cleanups within itself.

Signed-off-by: Amerigo Wang <[email protected]>
---
fs/proc/task_mmu.c | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 3c06570..9ebbe20 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -124,8 +124,10 @@ static void *m_start(struct seq_file *m, loff_t *pos)
return ERR_PTR(-ESRCH);

mm = mm_for_maps(priv->task);
- if (IS_ERR_OR_NULL(mm))
+ if (IS_ERR_OR_NULL(mm)) {
+ put_task_struct(priv->task);
return mm;
+ }
down_read(&mm->mmap_sem);

tail_vma = get_gate_vma(priv->task->mm);
@@ -182,8 +184,9 @@ static void m_stop(struct seq_file *m, void *v)
struct proc_maps_private *priv = m->private;
struct vm_area_struct *vma = v;

- if (!IS_ERR_OR_NULL(vma))
- vma_stop(priv, vma);
+ if (IS_ERR_OR_NULL(vma))
+ return;
+ vma_stop(priv, vma);
if (priv->task)
put_task_struct(priv->task);
}
--
1.7.4

2011-03-28 07:15:29

by Mike Frysinger

[permalink] [raw]
Subject: Re: [PATCH 1/2] proc: Use IS_ERR_OR_NULL() helper

On Mon, Mar 28, 2011 at 3:07 AM, Amerigo Wang wrote:
> Use IS_ERR_OR_NULL() helper

copying & pasting the summary as the only changelog is kind of useless

as for the patch, looks pretty straight forward to me
-mike