Skip to contentSkip to conversation

md:When opened /proc/mdstat, increase the refcount of

PATCH majianpeng 2 participants 7 messages Feb 26, 2012 – Feb 27, 2012

Subsystems (2)
MD RAID Proc filesystem
lore
Share & tools
lore

majianpeng

author
#0 lore raw

From 07a0eeb62a7bedec3f3312aff24ef62fbb36d820 Mon Sep 17 00:00:00 2001
From: majianpeng <[email protected]>
Date: Sun, 26 Feb 2012 20:43:05 +0800
Subject: [PATCH] md:When opened /proc/mdstat, increase the refcount of
md-mod.ko

If md configured module, polled /proc/mdstat and removed the md-mod can incur oops problem.

Reproduce this bug by following step:
1:configure md is module and insmod
2:poll(/proc/mdstat,timeout)
3:remove md-mod.ko
4:oops occur

[  343.764057] BUG: unable to handle kernel paging request at
ffffffffa007f9e0
[  343.764101] IP: [<ffffffff8155578c>] _raw_spin_lock_irqsave+0xc/0x30
[  343.764133] PGD 1807067 PUD 180b063 PMD b7b72067 PTE 0
[  343.764167] Oops: 0002 [#1] SMP
[  343.764190] CPU 3
[  343.764205] Modules linked in: ext4 jbd2 crc16 async_pq async_xor xor
async_memcpy async_raid6_recov raid6_pq async_tx sata_sil e1000e mvsas
libsas scsi_transport_sas [last unloaded: md_mod]
[  343.764304]
[  343.764312] Pid: 5454, comm: udisks-daemon Not tainted 3.3.0-rc4+ #13
To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M.
[  343.764366] RIP: 0010:[<ffffffff8155578c>]  [<ffffffff8155578c>]
_raw_spin_lock_irqsave+0xc/0x30
[  343.764403] RSP: 0018:ffff8800a514bab8  EFLAGS: 00010082
[  343.764423] RAX: 0000000000000282 RBX: ffffffffa007f9e0 RCX:
ffff8800ba0051a8
[  343.764448] RDX: 0000000000000100 RSI: ffff8800a514bc48 RDI:
ffffffffa007f9e0
[  343.764473] RBP: ffff8800a514bab8 R08: ffff8800a9bdb630 R09:
0000000000000001
[  343.764499] R10: 0000000000000000 R11: 0000000000000000 R12:
ffff8800a514bc48
[  343.764524] R13: ffff8800a514bb88 R14: ffff8800a514be2c R15:
0000000000000000
[  343.764551] FS:  00007fec6a4567c0(0000) GS:ffff8800bdb80000(0000)
knlGS:0000000000000000
[  343.764582] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  343.764603] CR2: ffffffffa007f9e0 CR3: 00000000a50f5000 CR4:
00000000000406e0
[  343.764628] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[  343.764653] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
0000000000000400
[  343.764679] Process udisks-daemon (pid: 5454, threadinfo
ffff8800a514a000, task ffff8800b7c41d80)
[  343.764710] Stack:
[  343.764719]  ffff8800a514bad8 ffffffff81054ceb 0000000000000003
ffff8800a514bc38
[  343.764756]  ffff8800a514bb28 ffffffff8112e216 ffff8800a514baf8
0000000000000000
[  343.764794]  ffff8800a514bb28 000000000128ed20 ffff8800a514bef8
0000000000000001
[  343.764830] Call Trace:
[  343.764842]  [<ffffffff81054ceb>] remove_wait_queue+0x1b/0x60
[  343.764864]  [<ffffffff8112e216>] poll_freewait+0x46/0xd0
[  343.764884]  [<ffffffff8112f6e6>] do_sys_poll+0x416/0x500
[  343.764905]  [<ffffffff8112e2a0>] ? poll_freewait+0xd0/0xd0
[  343.764926]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.764953]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.764979]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8143aec6>] ? verify_iovec+0x56/0xd0
[  343.765002]  [<ffffffff8142eb76>] ? __sys_sendmsg+0x376/0x380
[  343.765156]  [<ffffffff810f7459>] ? handle_mm_fault+0x139/0x240
[  343.765156]  [<ffffffff81156b0a>] ? fsnotify+0x1ca/0x2b0
[  343.765156]  [<ffffffff811d0388>] ?
selinux_file_permission+0xe8/0x130
[  343.765156]  [<ffffffff81077408>] ? ktime_get_ts+0xa8/0xe0
[  343.765156]  [<ffffffff8112e58a>] ? poll_select_set_timeout+0x7a/0x90
[  343.765156]  [<ffffffff8112f896>] sys_poll+0x66/0x100
[  343.765156]  [<ffffffff8155d162>] system_call_fastpath+0x16/0x1b
[  343.765156] Code: 8a 00 01 00 00 89 d0 f0 66 0f b1 0f 66 39 d0 0f 94
c0 0f b6 c0 5d c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 9c 58 fa ba 00 01
00 00 <f0> 66 0f c1 17 0f b6 ce 38 d1 74 11 0f 1f 84 00 00 00 00 00 f3
[  343.765156] RIP  [<ffffffff8155578c>] _raw_spin_lock_irqsave+0xc/0x30
[  343.765156]  RSP <ffff8800a514bab8>
[  343.765156] CR2: ffffffffa007f9e0
[  343.791890] ---[ end trace 10ed0211ce0a5736 ]---
Signed-off-by majianpeng <[email protected]>
---
 drivers/md/md.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
drivers/md/md.c +7 -1
index ce88755..105c7f9 100644
@@ -6859,9 +6859,15 @@ static int md_seq_open(struct inode *inode, struct file *file)
 
 	seq = file->private_data;
 	seq->poll_event = atomic_read(&md_event_count);
+	__module_get(THIS_MODULE);
 	return error;
 }
 
+static int md_seq_release(struct inode *inode, struct file *file)
+{
+	module_put(THIS_MODULE);
+	return seq_release_private(inode, file);
+}
 static unsigned int mdstat_poll(struct file *filp, poll_table *wait)
 {
 	struct seq_file *seq = filp->private_data;

@@ -6882,7 +6888,7 @@ static const struct file_operations md_seq_fops = {
 	.open           = md_seq_open,
 	.read           = seq_read,
 	.llseek         = seq_lseek,
-	.release	= seq_release_private,
+	.release	= md_seq_release,
 	.poll		= mdstat_poll,
 };
Signature · 6 lines
1.7.5.4

 				
--------------
majianpeng
2012-02-26
#1 lore raw
Quoted 112 lines · On Sun, 26 Feb 2012 12:55:03 +0800 "majianpeng" <[email protected]> wrote:
Quoted 1 line

From 07a0eeb62a7bedec3f3312aff24ef62fbb36d820 Mon Sep 17 00:00:00 2001

From: majianpeng <[email protected]>
Date: Sun, 26 Feb 2012 20:43:05 +0800
Subject: [PATCH] md:When opened /proc/mdstat, increase the refcount of
md-mod.ko

If md configured module, polled /proc/mdstat and removed the md-mod can incur oops problem.

Reproduce this bug by following step:
1:configure md is module and insmod
2:poll(/proc/mdstat,timeout)
3:remove md-mod.ko
4:oops occur

[  343.764057] BUG: unable to handle kernel paging request at
ffffffffa007f9e0
[  343.764101] IP: [<ffffffff8155578c>] _raw_spin_lock_irqsave+0xc/0x30
[  343.764133] PGD 1807067 PUD 180b063 PMD b7b72067 PTE 0
[  343.764167] Oops: 0002 [#1] SMP
[  343.764190] CPU 3
[  343.764205] Modules linked in: ext4 jbd2 crc16 async_pq async_xor xor
async_memcpy async_raid6_recov raid6_pq async_tx sata_sil e1000e mvsas
libsas scsi_transport_sas [last unloaded: md_mod]
[  343.764304]
[  343.764312] Pid: 5454, comm: udisks-daemon Not tainted 3.3.0-rc4+ #13
To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M.
[  343.764366] RIP: 0010:[<ffffffff8155578c>]  [<ffffffff8155578c>]
_raw_spin_lock_irqsave+0xc/0x30
[  343.764403] RSP: 0018:ffff8800a514bab8  EFLAGS: 00010082
[  343.764423] RAX: 0000000000000282 RBX: ffffffffa007f9e0 RCX:
ffff8800ba0051a8
[  343.764448] RDX: 0000000000000100 RSI: ffff8800a514bc48 RDI:
ffffffffa007f9e0
[  343.764473] RBP: ffff8800a514bab8 R08: ffff8800a9bdb630 R09:
0000000000000001
[  343.764499] R10: 0000000000000000 R11: 0000000000000000 R12:
ffff8800a514bc48
[  343.764524] R13: ffff8800a514bb88 R14: ffff8800a514be2c R15:
0000000000000000
[  343.764551] FS:  00007fec6a4567c0(0000) GS:ffff8800bdb80000(0000)
knlGS:0000000000000000
[  343.764582] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  343.764603] CR2: ffffffffa007f9e0 CR3: 00000000a50f5000 CR4:
00000000000406e0
[  343.764628] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[  343.764653] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
0000000000000400
[  343.764679] Process udisks-daemon (pid: 5454, threadinfo
ffff8800a514a000, task ffff8800b7c41d80)
[  343.764710] Stack:
[  343.764719]  ffff8800a514bad8 ffffffff81054ceb 0000000000000003
ffff8800a514bc38
[  343.764756]  ffff8800a514bb28 ffffffff8112e216 ffff8800a514baf8
0000000000000000
[  343.764794]  ffff8800a514bb28 000000000128ed20 ffff8800a514bef8
0000000000000001
[  343.764830] Call Trace:
[  343.764842]  [<ffffffff81054ceb>] remove_wait_queue+0x1b/0x60
[  343.764864]  [<ffffffff8112e216>] poll_freewait+0x46/0xd0
[  343.764884]  [<ffffffff8112f6e6>] do_sys_poll+0x416/0x500
[  343.764905]  [<ffffffff8112e2a0>] ? poll_freewait+0xd0/0xd0
[  343.764926]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.764953]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.764979]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8143aec6>] ? verify_iovec+0x56/0xd0
[  343.765002]  [<ffffffff8142eb76>] ? __sys_sendmsg+0x376/0x380
[  343.765156]  [<ffffffff810f7459>] ? handle_mm_fault+0x139/0x240
[  343.765156]  [<ffffffff81156b0a>] ? fsnotify+0x1ca/0x2b0
[  343.765156]  [<ffffffff811d0388>] ?
selinux_file_permission+0xe8/0x130
[  343.765156]  [<ffffffff81077408>] ? ktime_get_ts+0xa8/0xe0
[  343.765156]  [<ffffffff8112e58a>] ? poll_select_set_timeout+0x7a/0x90
[  343.765156]  [<ffffffff8112f896>] sys_poll+0x66/0x100
[  343.765156]  [<ffffffff8155d162>] system_call_fastpath+0x16/0x1b
[  343.765156] Code: 8a 00 01 00 00 89 d0 f0 66 0f b1 0f 66 39 d0 0f 94
c0 0f b6 c0 5d c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 9c 58 fa ba 00 01
00 00 <f0> 66 0f c1 17 0f b6 ce 38 d1 74 11 0f 1f 84 00 00 00 00 00 f3
[  343.765156] RIP  [<ffffffff8155578c>] _raw_spin_lock_irqsave+0xc/0x30
[  343.765156]  RSP <ffff8800a514bab8>
[  343.765156] CR2: ffffffffa007f9e0
[  343.791890] ---[ end trace 10ed0211ce0a5736 ]---
Signed-off-by majianpeng <[email protected]>
---
 drivers/md/md.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
drivers/md/md.c +6 -0
index ce88755..105c7f9 100644
@@ -6859,9 +6859,15 @@ static int md_seq_open(struct inode *inode, struct file *file)
 
 	seq = file->private_data;
 	seq->poll_event = atomic_read(&md_event_count);
+	__module_get(THIS_MODULE);
 	return error;
 }
 
+static int md_seq_release(struct inode *inode, struct file *file)
+{
+	module_put(THIS_MODULE);
+	return seq_release_private(inode, file);
+}
 static unsigned int mdstat_poll(struct file *filp, poll_table *wait)
 {
 	struct seq_file *seq = filp->private_data;

drivers/md/md.c static const struct file_operations md_seq_fops = { · line 6894

Quoted 8 lines
drivers/md/md.c +1 -1
@@ -6882,7 +6888,7 @@ static const struct file_operations md_seq_fops = {
 	.open           = md_seq_open,
 	.read           = seq_read,
 	.llseek         = seq_lseek,
-	.release	= seq_release_private,
+	.release	= md_seq_release,
 	.poll		= mdstat_poll,
 };

Thanks for the report and the patch.
However I think the problem should be fixed in procfs. md_set_fops contains

	.owner		= THIS_MODULE,

and procfs should be able to use this to keep a reference on the module whenever the file is open. It seems that it doesn't.
Fixing this in procfs is more general than just fixing it in md, and also it is likely to be safe. Having a module take a reference to itself is usually racy and should be avoided.

Could you see if it can be fixed in procfs?

Thanks,
NeilBrown

author
#2 lore raw

Yes,I know this.But I can't to fix procfs,so using this patch.

2012-02-26

majianpeng

发件人： NeilBrown
发送时间： 2012-02-26 13:27:15
收件人： majianpeng
抄送： linux-raid
主题： Re: md:When opened /proc/mdstat, increase the refcount of

Quoted 112 lines · On Sun, 26 Feb 2012 12:55:03 +0800 "majianpeng" <[email protected]> wrote:
Quoted 1 line

From 07a0eeb62a7bedec3f3312aff24ef62fbb36d820 Mon Sep 17 00:00:00 2001

From: majianpeng <[email protected]>
Date: Sun, 26 Feb 2012 20:43:05 +0800
Subject: [PATCH] md:When opened /proc/mdstat, increase the refcount of
md-mod.ko

If md configured module, polled /proc/mdstat and removed the md-mod can incur oops problem.

Reproduce this bug by following step:
1:configure md is module and insmod
2:poll(/proc/mdstat,timeout)
3:remove md-mod.ko
4:oops occur

[  343.764057] BUG: unable to handle kernel paging request at
ffffffffa007f9e0
[  343.764101] IP: [<ffffffff8155578c>] _raw_spin_lock_irqsave+0xc/0x30
[  343.764133] PGD 1807067 PUD 180b063 PMD b7b72067 PTE 0
[  343.764167] Oops: 0002 [#1] SMP
[  343.764190] CPU 3
[  343.764205] Modules linked in: ext4 jbd2 crc16 async_pq async_xor xor
async_memcpy async_raid6_recov raid6_pq async_tx sata_sil e1000e mvsas
libsas scsi_transport_sas [last unloaded: md_mod]
[  343.764304]
[  343.764312] Pid: 5454, comm: udisks-daemon Not tainted 3.3.0-rc4+ #13
To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M.
[  343.764366] RIP: 0010:[<ffffffff8155578c>]  [<ffffffff8155578c>]
_raw_spin_lock_irqsave+0xc/0x30
[  343.764403] RSP: 0018:ffff8800a514bab8  EFLAGS: 00010082
[  343.764423] RAX: 0000000000000282 RBX: ffffffffa007f9e0 RCX:
ffff8800ba0051a8
[  343.764448] RDX: 0000000000000100 RSI: ffff8800a514bc48 RDI:
ffffffffa007f9e0
[  343.764473] RBP: ffff8800a514bab8 R08: ffff8800a9bdb630 R09:
0000000000000001
[  343.764499] R10: 0000000000000000 R11: 0000000000000000 R12:
ffff8800a514bc48
[  343.764524] R13: ffff8800a514bb88 R14: ffff8800a514be2c R15:
0000000000000000
[  343.764551] FS:  00007fec6a4567c0(0000) GS:ffff8800bdb80000(0000)
knlGS:0000000000000000
[  343.764582] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  343.764603] CR2: ffffffffa007f9e0 CR3: 00000000a50f5000 CR4:
00000000000406e0
[  343.764628] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[  343.764653] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
0000000000000400
[  343.764679] Process udisks-daemon (pid: 5454, threadinfo
ffff8800a514a000, task ffff8800b7c41d80)
[  343.764710] Stack:
[  343.764719]  ffff8800a514bad8 ffffffff81054ceb 0000000000000003
ffff8800a514bc38
[  343.764756]  ffff8800a514bb28 ffffffff8112e216 ffff8800a514baf8
0000000000000000
[  343.764794]  ffff8800a514bb28 000000000128ed20 ffff8800a514bef8
0000000000000001
[  343.764830] Call Trace:
[  343.764842]  [<ffffffff81054ceb>] remove_wait_queue+0x1b/0x60
[  343.764864]  [<ffffffff8112e216>] poll_freewait+0x46/0xd0
[  343.764884]  [<ffffffff8112f6e6>] do_sys_poll+0x416/0x500
[  343.764905]  [<ffffffff8112e2a0>] ? poll_freewait+0xd0/0xd0
[  343.764926]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.764953]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.764979]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8143aec6>] ? verify_iovec+0x56/0xd0
[  343.765002]  [<ffffffff8142eb76>] ? __sys_sendmsg+0x376/0x380
[  343.765156]  [<ffffffff810f7459>] ? handle_mm_fault+0x139/0x240
[  343.765156]  [<ffffffff81156b0a>] ? fsnotify+0x1ca/0x2b0
[  343.765156]  [<ffffffff811d0388>] ?
selinux_file_permission+0xe8/0x130
[  343.765156]  [<ffffffff81077408>] ? ktime_get_ts+0xa8/0xe0
[  343.765156]  [<ffffffff8112e58a>] ? poll_select_set_timeout+0x7a/0x90
[  343.765156]  [<ffffffff8112f896>] sys_poll+0x66/0x100
[  343.765156]  [<ffffffff8155d162>] system_call_fastpath+0x16/0x1b
[  343.765156] Code: 8a 00 01 00 00 89 d0 f0 66 0f b1 0f 66 39 d0 0f 94
c0 0f b6 c0 5d c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 9c 58 fa ba 00 01
00 00 <f0> 66 0f c1 17 0f b6 ce 38 d1 74 11 0f 1f 84 00 00 00 00 00 f3
[  343.765156] RIP  [<ffffffff8155578c>] _raw_spin_lock_irqsave+0xc/0x30
[  343.765156]  RSP <ffff8800a514bab8>
[  343.765156] CR2: ffffffffa007f9e0
[  343.791890] ---[ end trace 10ed0211ce0a5736 ]---
Signed-off-by majianpeng <[email protected]>
---
 drivers/md/md.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
drivers/md/md.c +6 -0
index ce88755..105c7f9 100644
@@ -6859,9 +6859,15 @@ static int md_seq_open(struct inode *inode, struct file *file)
 
  seq = file->private_data;
  seq->poll_event = atomic_read(&md_event_count);
+ __module_get(THIS_MODULE);
  return error;
 }
 
+static int md_seq_release(struct inode *inode, struct file *file)
+{
+ module_put(THIS_MODULE);
+ return seq_release_private(inode, file);
+}
 static unsigned int mdstat_poll(struct file *filp, poll_table *wait)
 {
  struct seq_file *seq = filp->private_data;

drivers/md/md.c static const struct file_operations md_seq_fops = { · line 6894

Quoted 8 lines
drivers/md/md.c +1 -1
@@ -6882,7 +6888,7 @@ static const struct file_operations md_seq_fops = {
  .open           = md_seq_open,
  .read           = seq_read,
  .llseek         = seq_lseek,
- .release = seq_release_private,
+ .release = md_seq_release,
  .poll = mdstat_poll,
 };

Thanks for the report and the patch.
However I think the problem should be fixed in procfs. md_set_fops contains
.owner = THIS_MODULE,
and procfs should be able to use this to keep a reference on the module whenever the file is open. It seems that it doesn't.
Fixing this in procfs is more general than just fixing it in md, and also it is likely to be safe. Having a module take a reference to itself is usually racy and should be avoided.
Could you see if it can be fixed in procfs?
Thanks,
NeilBrown

#3 lore raw
Quoted 2 lines · On Sun, 26 Feb 2012 13:39:57 +0800 "majianpeng" <[email protected]> wrote:

Yes,I know this.But I can't to fix procfs,so using this patch.

I wonder if this will fix it.

fs/proc/inode.c +2 -1
index 7737c54..fc89842 100644
@@ -449,7 +449,8 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
 		if (de->proc_iops)
 			inode->i_op = de->proc_iops;
 		if (de->proc_fops) {
-			if (S_ISREG(inode->i_mode)) {
+			if (S_ISREG(inode->i_mode) &&
+			    !de->proc_fops->owner) {
 #ifdef CONFIG_COMPAT
 				if (!de->proc_fops->compat_ioctl)
 					inode->i_fop =

however I don't really understand the point of proc_ref_file_ops, so maybe I'm missing something.

NeilBrown

author
#4 lore raw
In reply to majianpeng

I wrote a letter to maintainer of procfs. He's answer is follow:

Quoted 6 lines

No. Read remove_proc_entry(), especially the part under ->pde_unload_lock. The whole damn point of that stuff is that opened file on procfs does *not* pin the module down; IO in progress does, but that's it.
You can't deadlock rmmod foobar </proc/crap/foobar; with your patch we'll be back to a pile of deadlocks in there.
IOW, NAK.

------------------
majianpeng
2012-02-26

-------------------------------------------------------------
发件人：NeilBrown
发送日期：2012-02-26 13:27:15
收件人：majianpeng
抄送：linux-raid
主题：Re: md:When opened /proc/mdstat, increase the refcount of

Quoted 112 lines · On Sun, 26 Feb 2012 12:55:03 +0800 "majianpeng" <[email protected]> wrote:
Quoted 1 line

From 07a0eeb62a7bedec3f3312aff24ef62fbb36d820 Mon Sep 17 00:00:00 2001

From: majianpeng <[email protected]>
Date: Sun, 26 Feb 2012 20:43:05 +0800
Subject: [PATCH] md:When opened /proc/mdstat, increase the refcount of
md-mod.ko

If md configured module, polled /proc/mdstat and removed the md-mod can incur oops problem.

Reproduce this bug by following step:
1:configure md is module and insmod
2:poll(/proc/mdstat,timeout)
3:remove md-mod.ko
4:oops occur

[  343.764057] BUG: unable to handle kernel paging request at
ffffffffa007f9e0
[  343.764101] IP: [<ffffffff8155578c>] _raw_spin_lock_irqsave+0xc/0x30
[  343.764133] PGD 1807067 PUD 180b063 PMD b7b72067 PTE 0
[  343.764167] Oops: 0002 [#1] SMP
[  343.764190] CPU 3
[  343.764205] Modules linked in: ext4 jbd2 crc16 async_pq async_xor xor
async_memcpy async_raid6_recov raid6_pq async_tx sata_sil e1000e mvsas
libsas scsi_transport_sas [last unloaded: md_mod]
[  343.764304]
[  343.764312] Pid: 5454, comm: udisks-daemon Not tainted 3.3.0-rc4+ #13
To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M.
[  343.764366] RIP: 0010:[<ffffffff8155578c>]  [<ffffffff8155578c>]
_raw_spin_lock_irqsave+0xc/0x30
[  343.764403] RSP: 0018:ffff8800a514bab8  EFLAGS: 00010082
[  343.764423] RAX: 0000000000000282 RBX: ffffffffa007f9e0 RCX:
ffff8800ba0051a8
[  343.764448] RDX: 0000000000000100 RSI: ffff8800a514bc48 RDI:
ffffffffa007f9e0
[  343.764473] RBP: ffff8800a514bab8 R08: ffff8800a9bdb630 R09:
0000000000000001
[  343.764499] R10: 0000000000000000 R11: 0000000000000000 R12:
ffff8800a514bc48
[  343.764524] R13: ffff8800a514bb88 R14: ffff8800a514be2c R15:
0000000000000000
[  343.764551] FS:  00007fec6a4567c0(0000) GS:ffff8800bdb80000(0000)
knlGS:0000000000000000
[  343.764582] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  343.764603] CR2: ffffffffa007f9e0 CR3: 00000000a50f5000 CR4:
00000000000406e0
[  343.764628] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[  343.764653] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
0000000000000400
[  343.764679] Process udisks-daemon (pid: 5454, threadinfo
ffff8800a514a000, task ffff8800b7c41d80)
[  343.764710] Stack:
[  343.764719]  ffff8800a514bad8 ffffffff81054ceb 0000000000000003
ffff8800a514bc38
[  343.764756]  ffff8800a514bb28 ffffffff8112e216 ffff8800a514baf8
0000000000000000
[  343.764794]  ffff8800a514bb28 000000000128ed20 ffff8800a514bef8
0000000000000001
[  343.764830] Call Trace:
[  343.764842]  [<ffffffff81054ceb>] remove_wait_queue+0x1b/0x60
[  343.764864]  [<ffffffff8112e216>] poll_freewait+0x46/0xd0
[  343.764884]  [<ffffffff8112f6e6>] do_sys_poll+0x416/0x500
[  343.764905]  [<ffffffff8112e2a0>] ? poll_freewait+0xd0/0xd0
[  343.764926]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.764953]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.764979]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8112e390>] ? __pollwait+0xf0/0xf0
[  343.765002]  [<ffffffff8143aec6>] ? verify_iovec+0x56/0xd0
[  343.765002]  [<ffffffff8142eb76>] ? __sys_sendmsg+0x376/0x380
[  343.765156]  [<ffffffff810f7459>] ? handle_mm_fault+0x139/0x240
[  343.765156]  [<ffffffff81156b0a>] ? fsnotify+0x1ca/0x2b0
[  343.765156]  [<ffffffff811d0388>] ?
selinux_file_permission+0xe8/0x130
[  343.765156]  [<ffffffff81077408>] ? ktime_get_ts+0xa8/0xe0
[  343.765156]  [<ffffffff8112e58a>] ? poll_select_set_timeout+0x7a/0x90
[  343.765156]  [<ffffffff8112f896>] sys_poll+0x66/0x100
[  343.765156]  [<ffffffff8155d162>] system_call_fastpath+0x16/0x1b
[  343.765156] Code: 8a 00 01 00 00 89 d0 f0 66 0f b1 0f 66 39 d0 0f 94
c0 0f b6 c0 5d c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 9c 58 fa ba 00 01
00 00 <f0> 66 0f c1 17 0f b6 ce 38 d1 74 11 0f 1f 84 00 00 00 00 00 f3
[  343.765156] RIP  [<ffffffff8155578c>] _raw_spin_lock_irqsave+0xc/0x30
[  343.765156]  RSP <ffff8800a514bab8>
[  343.765156] CR2: ffffffffa007f9e0
[  343.791890] ---[ end trace 10ed0211ce0a5736 ]---
Signed-off-by majianpeng <[email protected]>
---
 drivers/md/md.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
drivers/md/md.c +6 -0
index ce88755..105c7f9 100644
@@ -6859,9 +6859,15 @@ static int md_seq_open(struct inode *inode, struct file *file)
 
 	seq = file->private_data;
 	seq->poll_event = atomic_read(&md_event_count);
+	__module_get(THIS_MODULE);
 	return error;
 }
 
+static int md_seq_release(struct inode *inode, struct file *file)
+{
+	module_put(THIS_MODULE);
+	return seq_release_private(inode, file);
+}
 static unsigned int mdstat_poll(struct file *filp, poll_table *wait)
 {
 	struct seq_file *seq = filp->private_data;

drivers/md/md.c static const struct file_operations md_seq_fops = { · line 6894

Quoted 8 lines
drivers/md/md.c +1 -1
@@ -6882,7 +6888,7 @@ static const struct file_operations md_seq_fops = {
 	.open           = md_seq_open,
 	.read           = seq_read,
 	.llseek         = seq_lseek,
-	.release	= seq_release_private,
+	.release	= md_seq_release,
 	.poll		= mdstat_poll,
 };

Thanks for the report and the patch.
However I think the problem should be fixed in procfs. md_set_fops contains

	.owner		= THIS_MODULE,

and procfs should be able to use this to keep a reference on the module whenever the file is open. It seems that it doesn't.
Fixing this in procfs is more general than just fixing it in md, and also it is likely to be safe. Having a module take a reference to itself is usually racy and should be avoided.

Could you see if it can be fixed in procfs?

Thanks,
NeilBrown

#5 lore raw
Quoted 7 lines · On Sun, 26 Feb 2012 16:33:25 +0800 "majianpeng" <[email protected]> wrote:

I wrote a letter to maintainer of procfs. He's answer is follow:

Quoted 6 lines

No. Read remove_proc_entry(), especially the part under ->pde_unload_lock. The whole damn point of that stuff is that opened file on procfs does *not* pin the module down; IO in progress does, but that's it.
You can't deadlock rmmod foobar </proc/crap/foobar; with your patch we'll be back to a pile of deadlocks in there.
IOW, NAK.

Hmmm... that sounds like Al Viro.

I see the point of that code now. And your patch would be open to the same problem wouldn't it. i.e.
rmmod md_mod < /proc/mdstat

would deadlock??

So we still need to find a fix that actually works correctly.

That probably means moving the call to 'poll_wait' into procfs code, and using a wait_queue_head which is managed by procfs.

So maybe:
- put a wait_queue_head in 'struct proc_dir_entry'
- have proc_reg_poll call poll_wait passing that wait_queue_head
- write a function e.g. proc_poll_wake which calls wake_up on that wait_queue_head
- have md.c save the return value from proc_create, and call the above function instead of calling "wake_up(&md_event_waiters)"

That should work.

NeilBrown

author
#6 lore raw

my patch is:

---
 fs/proc/inode.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)
fs/proc/inode.c +11 -2
index 84fd323..78dbb03 100644
@@ -317,6 +317,7 @@ static int proc_reg_open(struct inode *inode, struct file *file)
 	int (*open)(struct inode *, struct file *);
 	int (*release)(struct inode *, struct file *);
 	struct pde_opener *pdeo;
+	const struct file_operations *fops;
 
 	/*
 	 * What for, you ask? Well, we can have open, rmmod, remove_proc_entry

@@ -339,7 +340,12 @@ static int proc_reg_open(struct inode *inode, struct file *file)
 		return -ENOENT;
 	}
 	pde->pde_users++;
-	open = pde->proc_fops->open;
+	/*
+	 *If proc_fops defined in module and used .owner = THIS_MODULE,so open must
+	 *get a reference of module.
+	 */
+	fops = fops_get(pde->proc_fops);
+	open = fops->open;
 	release = pde->proc_fops->release;
 	spin_unlock(&pde->pde_unload_lock);
 

@@ -354,8 +360,10 @@ static int proc_reg_open(struct inode *inode, struct file *file)
 		/* Strictly for "too late" ->release in proc_reg_release(). */
 		pdeo->release = release;
 		list_add(&pdeo->lh, &pde->pde_openers);
-	} else
+	} else	{
+		fops_put(pde->proc_fops);
 		kfree(pdeo);
+	}
 	__pde_users_dec(pde);
 	spin_unlock(&pde->pde_unload_lock);
 	return rv;

@@ -402,6 +410,7 @@ static int proc_reg_release(struct inode *inode, struct file *file)
 	}
 	pde->pde_users++;
 	release = pde->proc_fops->release;
+	fops_put(pde->proc_fops);
 	if (pdeo) {
 		list_del(&pdeo->lh);
 		kfree(pdeo);

I tested ok,but Al Viro said deadlock, I did not tested.

------------------
majianpeng
2012-02-27

-------------------------------------------------------------
发件人：NeilBrown
发送日期：2012-02-26 19:01:32
收件人：majianpeng
抄送：linux-raid
主题：Re: md:When opened /proc/mdstat, increase the refcount of

Quoted 7 lines · On Sun, 26 Feb 2012 16:33:25 +0800 "majianpeng" <[email protected]> wrote:

I wrote a letter to maintainer of procfs. He's answer is follow:

Quoted 6 lines

No. Read remove_proc_entry(), especially the part under ->pde_unload_lock. The whole damn point of that stuff is that opened file on procfs does *not* pin the module down; IO in progress does, but that's it.
You can't deadlock rmmod foobar </proc/crap/foobar; with your patch we'll be back to a pile of deadlocks in there.
IOW, NAK.

Hmmm... that sounds like Al Viro.

I see the point of that code now. And your patch would be open to the same problem wouldn't it. i.e.
rmmod md_mod < /proc/mdstat

would deadlock??

So we still need to find a fix that actually works correctly.

That probably means moving the call to 'poll_wait' into procfs code, and using a wait_queue_head which is managed by procfs.

So maybe:
- put a wait_queue_head in 'struct proc_dir_entry'
- have proc_reg_poll call poll_wait passing that wait_queue_head
- write a function e.g. proc_poll_wake which calls wake_up on that wait_queue_head
- have md.c save the return value from proc_create, and call the above function instead of calling "wake_up(&md_event_waiters)"

That should work.

NeilBrown