| Message ID | 20201103030758.317781-2-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: fix use-after-freed and double lock bugs | expand |
On Mon, Nov 02, 2020 at 10:07:57PM -0500, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > The crash call trace: > > <6>[ 1012.319386] block nbd1: NBD_DISCONNECT > <1>[ 1012.319437] BUG: kernel NULL pointer dereference, address: 0000000000000020 > <1>[ 1012.319439] #PF: supervisor write access in kernel mode > <1>[ 1012.319441] #PF: error_code(0x0002) - not-present page > <6>[ 1012.319442] PGD 0 P4D 0 > <4>[ 1012.319448] Oops: 0002 [#1] SMP NOPTI > <4>[ 1012.319454] CPU: 9 PID: 25111 Comm: rbd-nbd Tainted: G E 5.9.0+ #6 > [...] > <4>[ 1012.319505] PKRU: 55555554 > <4>[ 1012.319506] Call Trace: > <4>[ 1012.319560] flush_workqueue+0x81/0x440 > <4>[ 1012.319598] nbd_disconnect_and_put+0x50/0x70 [nbd] > <4>[ 1012.319607] nbd_genl_disconnect+0xc7/0x170 [nbd] > <4>[ 1012.319627] genl_rcv_msg+0x1dd/0x2f9 > <4>[ 1012.319642] ? genl_start+0x140/0x140 > <4>[ 1012.319644] netlink_rcv_skb+0x49/0x110 > <4>[ 1012.319649] genl_rcv+0x24/0x40 > <4>[ 1012.319651] netlink_unicast+0x1a5/0x280 > <4>[ 1012.319653] netlink_sendmsg+0x23d/0x470 > <4>[ 1012.319667] sock_sendmsg+0x5b/0x60 > <4>[ 1012.319676] ____sys_sendmsg+0x1ef/0x260 > <4>[ 1012.319679] ? copy_msghdr_from_user+0x5c/0x90 > <4>[ 1012.319680] ? ____sys_recvmsg+0xa5/0x180 > <4>[ 1012.319682] ___sys_sendmsg+0x7c/0xc0 > <4>[ 1012.319683] ? copy_msghdr_from_user+0x5c/0x90 > <4>[ 1012.319685] ? ___sys_recvmsg+0x89/0xc0 > <4>[ 1012.319692] ? __wake_up_common_lock+0x87/0xc0 > <4>[ 1012.319715] ? __check_object_size+0x46/0x173 > <4>[ 1012.319727] ? _copy_to_user+0x22/0x30 > <4>[ 1012.319729] ? move_addr_to_user+0xc3/0x100 > <4>[ 1012.319731] __sys_sendmsg+0x57/0xa0 > <4>[ 1012.319744] do_syscall_64+0x33/0x40 > <4>[ 1012.319760] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > <4>[ 1012.319780] RIP: 0033:0x7f5baa8e3ad5 > > In case the reference of nbd->config has reached zero and the > related resource has been released, if another process tries to > send the DISCONENCT cmd by using the netlink, it will potentially > crash like this. > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > drivers/block/nbd.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > index f46e26c9d9b3..3bb8281bb753 100644 > --- a/drivers/block/nbd.c > +++ b/drivers/block/nbd.c > @@ -2003,16 +2003,31 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info) > > static void nbd_disconnect_and_put(struct nbd_device *nbd) > { > + bool flush = true; > + > mutex_lock(&nbd->config_lock); > nbd_disconnect(nbd); > nbd_clear_sock(nbd); > - mutex_unlock(&nbd->config_lock); > /* > - * Make sure recv thread has finished, so it does not drop the last > - * config ref and try to destroy the workqueue from inside the work > - * queue. > + * In very rare case that the nbd->recv_workq may already have been > + * released by the recv_work(). > */ > - flush_workqueue(nbd->recv_workq); > + if (likely(!nbd->recv_workq)) It is actually unlikely, but doesn't make any sense to annotate the check since it isn't fast path, so please remove it. > + refcount_inc(&nbd->config_refs); > + else > + flush = false; > + mutex_unlock(&nbd->config_lock); > + > + if (flush) { > + /* > + * Make sure recv thread has finished, so it does not drop > + * the last config ref and try to destroy the workqueue > + * from inside the work queue. > + */ > + flush_workqueue(nbd->recv_workq); > + nbd_config_put(nbd); > + } > + > if (test_and_clear_bit(NBD_RT_HAS_CONFIG_REF, > &nbd->config->runtime_flags)) > nbd_config_put(nbd); The approach is fine, and the reason is that nbd_disconnect_and_put() still can be called via netlink when the nbd device has been closed: Once the above likely() is removed, feel free to add: Reviewed-by: Ming Lei <ming.lei@redhat.com> Thanks, Ming
On 2020/11/3 12:12, Ming Lei wrote: > On Mon, Nov 02, 2020 at 10:07:57PM -0500, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> The crash call trace: >> >> <6>[ 1012.319386] block nbd1: NBD_DISCONNECT >> <1>[ 1012.319437] BUG: kernel NULL pointer dereference, address: 0000000000000020 >> <1>[ 1012.319439] #PF: supervisor write access in kernel mode >> <1>[ 1012.319441] #PF: error_code(0x0002) - not-present page >> <6>[ 1012.319442] PGD 0 P4D 0 >> <4>[ 1012.319448] Oops: 0002 [#1] SMP NOPTI >> <4>[ 1012.319454] CPU: 9 PID: 25111 Comm: rbd-nbd Tainted: G E 5.9.0+ #6 >> [...] >> <4>[ 1012.319505] PKRU: 55555554 >> <4>[ 1012.319506] Call Trace: >> <4>[ 1012.319560] flush_workqueue+0x81/0x440 >> <4>[ 1012.319598] nbd_disconnect_and_put+0x50/0x70 [nbd] >> <4>[ 1012.319607] nbd_genl_disconnect+0xc7/0x170 [nbd] >> <4>[ 1012.319627] genl_rcv_msg+0x1dd/0x2f9 >> <4>[ 1012.319642] ? genl_start+0x140/0x140 >> <4>[ 1012.319644] netlink_rcv_skb+0x49/0x110 >> <4>[ 1012.319649] genl_rcv+0x24/0x40 >> <4>[ 1012.319651] netlink_unicast+0x1a5/0x280 >> <4>[ 1012.319653] netlink_sendmsg+0x23d/0x470 >> <4>[ 1012.319667] sock_sendmsg+0x5b/0x60 >> <4>[ 1012.319676] ____sys_sendmsg+0x1ef/0x260 >> <4>[ 1012.319679] ? copy_msghdr_from_user+0x5c/0x90 >> <4>[ 1012.319680] ? ____sys_recvmsg+0xa5/0x180 >> <4>[ 1012.319682] ___sys_sendmsg+0x7c/0xc0 >> <4>[ 1012.319683] ? copy_msghdr_from_user+0x5c/0x90 >> <4>[ 1012.319685] ? ___sys_recvmsg+0x89/0xc0 >> <4>[ 1012.319692] ? __wake_up_common_lock+0x87/0xc0 >> <4>[ 1012.319715] ? __check_object_size+0x46/0x173 >> <4>[ 1012.319727] ? _copy_to_user+0x22/0x30 >> <4>[ 1012.319729] ? move_addr_to_user+0xc3/0x100 >> <4>[ 1012.319731] __sys_sendmsg+0x57/0xa0 >> <4>[ 1012.319744] do_syscall_64+0x33/0x40 >> <4>[ 1012.319760] entry_SYSCALL_64_after_hwframe+0x44/0xa9 >> <4>[ 1012.319780] RIP: 0033:0x7f5baa8e3ad5 >> >> In case the reference of nbd->config has reached zero and the >> related resource has been released, if another process tries to >> send the DISCONENCT cmd by using the netlink, it will potentially >> crash like this. >> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> drivers/block/nbd.c | 25 ++++++++++++++++++++----- >> 1 file changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c >> index f46e26c9d9b3..3bb8281bb753 100644 >> --- a/drivers/block/nbd.c >> +++ b/drivers/block/nbd.c >> @@ -2003,16 +2003,31 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info) >> >> static void nbd_disconnect_and_put(struct nbd_device *nbd) >> { >> + bool flush = true; >> + >> mutex_lock(&nbd->config_lock); >> nbd_disconnect(nbd); >> nbd_clear_sock(nbd); >> - mutex_unlock(&nbd->config_lock); >> /* >> - * Make sure recv thread has finished, so it does not drop the last >> - * config ref and try to destroy the workqueue from inside the work >> - * queue. >> + * In very rare case that the nbd->recv_workq may already have been >> + * released by the recv_work(). >> */ >> - flush_workqueue(nbd->recv_workq); >> + if (likely(!nbd->recv_workq)) > It is actually unlikely, but doesn't make any sense to annotate the check > since it isn't fast path, so please remove it. Sure, will do it. Thanks. > >> + refcount_inc(&nbd->config_refs); >> + else >> + flush = false; >> + mutex_unlock(&nbd->config_lock); >> + >> + if (flush) { >> + /* >> + * Make sure recv thread has finished, so it does not drop >> + * the last config ref and try to destroy the workqueue >> + * from inside the work queue. >> + */ >> + flush_workqueue(nbd->recv_workq); >> + nbd_config_put(nbd); >> + } >> + >> if (test_and_clear_bit(NBD_RT_HAS_CONFIG_REF, >> &nbd->config->runtime_flags)) >> nbd_config_put(nbd); > The approach is fine, and the reason is that nbd_disconnect_and_put() still > can be called via netlink when the nbd device has been closed: > > Once the above likely() is removed, feel free to add: > > Reviewed-by: Ming Lei <ming.lei@redhat.com> > > > Thanks, > Ming >
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index f46e26c9d9b3..3bb8281bb753 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -2003,16 +2003,31 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info) static void nbd_disconnect_and_put(struct nbd_device *nbd) { + bool flush = true; + mutex_lock(&nbd->config_lock); nbd_disconnect(nbd); nbd_clear_sock(nbd); - mutex_unlock(&nbd->config_lock); /* - * Make sure recv thread has finished, so it does not drop the last - * config ref and try to destroy the workqueue from inside the work - * queue. + * In very rare case that the nbd->recv_workq may already have been + * released by the recv_work(). */ - flush_workqueue(nbd->recv_workq); + if (likely(!nbd->recv_workq)) + refcount_inc(&nbd->config_refs); + else + flush = false; + mutex_unlock(&nbd->config_lock); + + if (flush) { + /* + * Make sure recv thread has finished, so it does not drop + * the last config ref and try to destroy the workqueue + * from inside the work queue. + */ + flush_workqueue(nbd->recv_workq); + nbd_config_put(nbd); + } + if (test_and_clear_bit(NBD_RT_HAS_CONFIG_REF, &nbd->config->runtime_flags)) nbd_config_put(nbd);