Re: [PATCH] nbd: Fix NULL pointer in flush_workqueue

2021-01-31 Thread Sun Ke 

hi，Markus

在 2021/1/29 3:42, Markus Elfring 写道:

…

+++ b/drivers/block/nbd.c
@@ -2011,12 +2011,20 @@ static int nbd_genl_disconnect(struct sk_buff *skb, 
struct genl_info *info)
   index);
return -EINVAL;
}
+   mutex_lock(>config_lock);
if (!refcount_inc_not_zero(>refs)) {
mutex_unlock(_index_mutex);
+   mutex_unlock(>config_lock);

Can an other function call order become relevant for the unlocking of these 
mutexes?

Do you think the nbd->config_lock  mutex here is useless?




printk(KERN_ERR "nbd: device at index %d is going down\n",
   index);

May such an error message be moved into the lock scope?

Sure.




return -EINVAL;
}
+   if (!nbd->recv_workq) {
+   mutex_unlock(>config_lock);
+   mutex_unlock(_index_mutex);
+   return -EINVAL;
+   }

How do you think about to connect the code from this if branch
with a jump target like “unlock” so that such statements would be shareable
for the desired exception handling?

OK, I will improve it in V2 patch.




+   mutex_unlock(>config_lock);
mutex_unlock(_index_mutex);
if (!refcount_inc_not_zero(>config_refs)) {
nbd_put(nbd);


Regards,
Markus
.

[PATCH] nbd: Fix NULL pointer in flush_workqueue

2021-01-27 Thread Sun Ke 
Open /dev/nbdX first, the config_refs will be 1 and
the pointers in nbd_device are still null. Disconnect
/dev/nbdX, then reference a null recv_workq. The
protection by config_refs in nbd_genl_disconnect is useless.

[  656.366194] BUG: kernel NULL pointer dereference, address: 0020
[  656.368943] #PF: supervisor write access in kernel mode
[  656.369844] #PF: error_code(0x0002) - not-present page
[  656.370717] PGD 10cc87067 P4D 10cc87067 PUD 1074b4067 PMD 0
[  656.371693] Oops: 0002 [#1] SMP
[  656.372242] CPU: 5 PID: 7977 Comm: nbd-client Not tainted 
5.11.0-rc5-00040-g76c057c84d28 #1
[  656.373661] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
[  656.375904] RIP: 0010:mutex_lock+0x29/0x60
[  656.376627] Code: 00 0f 1f 44 00 00 55 48 89 fd 48 83 05 6f d7 fe 08 01 e8 
7a c3 ff ff 48 83 05 6a d7 fe 08 01 31 c0 65 48 8b 14 25 00 6d 01 00  48 0f 
b1 55 d
[  656.378934] RSP: 0018:c95eb9b0 EFLAGS: 00010246
[  656.379350] RAX:  RBX:  RCX: 
[  656.379915] RDX: 888104cf2600 RSI: aae8f452 RDI: 0020
[  656.380473] RBP: 0020 R08:  R09: 88813bd6b318
[  656.381039] R10: 00c7 R11: fefefefefefefeff R12: 888102710b40
[  656.381599] R13: c95eb9e0 R14: b2930680 R15: 88810770ef00
[  656.382166] FS:  7fdf117ebb40() GS:88813bd4() 
knlGS:
[  656.382806] CS:  0010 DS:  ES:  CR0: 80050033
[  656.383261] CR2: 0020 CR3: 000100c84000 CR4: 06e0
[  656.383819] DR0:  DR1:  DR2: 
[  656.384370] DR3:  DR6: fffe0ff0 DR7: 0400
[  656.384927] Call Trace:
[  656.385111]  flush_workqueue+0x92/0x6c0
[  656.385395]  nbd_disconnect_and_put+0x81/0xd0
[  656.385716]  nbd_genl_disconnect+0x125/0x2a0
[  656.386034]  genl_family_rcv_msg_doit.isra.0+0x102/0x1b0
[  656.386422]  genl_rcv_msg+0xfc/0x2b0
[  656.386685]  ? nbd_ioctl+0x490/0x490
[  656.386954]  ? genl_family_rcv_msg_doit.isra.0+0x1b0/0x1b0
[  656.387354]  netlink_rcv_skb+0x62/0x180
[  656.387638]  genl_rcv+0x34/0x60
[  656.387874]  netlink_unicast+0x26d/0x590
[  656.388162]  netlink_sendmsg+0x398/0x6c0
[  656.388451]  ? netlink_rcv_skb+0x180/0x180
[  656.388750]  sys_sendmsg+0x1da/0x320
[  656.389038]  ? sys_recvmsg+0x130/0x220
[  656.389334]  ___sys_sendmsg+0x8e/0xf0
[  656.389605]  ? ___sys_recvmsg+0xa2/0xf0
[  656.389889]  ? handle_mm_fault+0x1671/0x21d0
[  656.390201]  __sys_sendmsg+0x6d/0xe0
[  656.390464]  __x64_sys_sendmsg+0x23/0x30
[  656.390751]  do_syscall_64+0x45/0x70
[  656.391017]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

To fix it, just add a check for a non null task_recv in
nbd_genl_disconnect.

Fixes: e9e006f5fcf2 ("nbd: fix max number of supported devs")
Signed-off-by: Sun Ke 
---
 drivers/block/nbd.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 6727358e147d..4f7885966d32 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -2011,12 +2011,20 @@ static int nbd_genl_disconnect(struct sk_buff *skb, 
struct genl_info *info)
   index);
return -EINVAL;
}
+   mutex_lock(>config_lock);
if (!refcount_inc_not_zero(>refs)) {
mutex_unlock(_index_mutex);
+   mutex_unlock(>config_lock);
printk(KERN_ERR "nbd: device at index %d is going down\n",
   index);
return -EINVAL;
}
+   if (!nbd->recv_workq) {
+   mutex_unlock(>config_lock);
+   mutex_unlock(_index_mutex);
+   return -EINVAL;
+   }
+   mutex_unlock(>config_lock);
mutex_unlock(_index_mutex);
if (!refcount_inc_not_zero(>config_refs)) {
nbd_put(nbd);
-- 
2.25.4