From 18f2c9ce4120584b90e5f8d4c1011dbf8fe1c0ee Mon Sep 17 00:00:00 2001 From: Dexuan Cui Date: Fri, 5 May 2017 16:57:23 -0600 Subject: [PATCH 06/15] vmbus: dynamically enqueue/dequeue a channel on vmbus_open/close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A just-closed channel may have a pending interrupt, and later when a new channel with the same channel ID is not being fully initialized, the pending interrupt of the previous channel with the same channel ID can run the channel callback on the new channel data structure, causing a crash of NULL pointer dereferencing. Normally it’s pretty hard to reproduce the race condition, but it can indeed happen with specially-designed hv_sock stress test cases. Signed-off-by: Dexuan Cui Reported-by: Rolf Neugebauer Tested-by: Rolf Neugebauer Cc: K. Y. Srinivasan Cc: Haiyang Zhang Cc: Stephen Hemminger Origin: https://github.com/dcui/linux/commits/decui/hv_sock/v4.11/20170511-debug-0605 (cherry picked from commit 1df677b35ff010d0def33f5420773015815cf843) --- drivers/hv/channel.c | 12 +++++++++--- drivers/hv/channel_mgmt.c | 50 +++++++++++++++++++++-------------------------- include/linux/hyperv.h | 3 +++ 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index db5e6f8730d2..f288e506fba0 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -177,6 +177,8 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size, &vmbus_connection.chn_msg_list); spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags); + hv_percpu_channel_enq(newchannel); + ret = vmbus_post_msg(open_msg, sizeof(struct vmbus_channel_open_channel), true); @@ -189,23 +191,25 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size, if (ret != 0) { err = ret; - goto error_free_gpadl; + goto error_deq_channel; } if (newchannel->rescind) { err = -ENODEV; - goto error_free_gpadl; + goto error_deq_channel; } if (open_info->response.open_result.status) { err = -EAGAIN; - goto error_free_gpadl; + goto error_deq_channel; } newchannel->state = CHANNEL_OPENED_STATE; kfree(open_info); return 0; +error_deq_channel: + hv_percpu_channel_deq(newchannel); error_free_gpadl: vmbus_teardown_gpadl(newchannel, newchannel->ringbuffer_gpadlhandle); kfree(open_info); @@ -551,6 +555,8 @@ static int vmbus_close_internal(struct vmbus_channel *channel) goto out; } + hv_percpu_channel_deq(channel); + channel->state = CHANNEL_OPEN_STATE; channel->sc_creation_callback = NULL; /* Stop callback and cancel the timer asap */ diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c index fbcb06352308..c5a01a4d589e 100644 --- a/drivers/hv/channel_mgmt.c +++ b/drivers/hv/channel_mgmt.c @@ -363,6 +363,17 @@ static void percpu_channel_enq(void *arg) list_add_tail_rcu(&channel->percpu_list, &hv_cpu->chan_list); } +void hv_percpu_channel_enq(struct vmbus_channel *channel) +{ + if (channel->target_cpu != get_cpu()) + smp_call_function_single(channel->target_cpu, + percpu_channel_enq, channel, true); + else + percpu_channel_enq(channel); + + put_cpu(); +} + static void percpu_channel_deq(void *arg) { struct vmbus_channel *channel = arg; @@ -370,6 +381,17 @@ static void percpu_channel_deq(void *arg) list_del_rcu(&channel->percpu_list); } +void hv_percpu_channel_deq(struct vmbus_channel *channel) +{ + if (channel->target_cpu != get_cpu()) + smp_call_function_single(channel->target_cpu, + percpu_channel_deq, channel, true); + else + percpu_channel_deq(channel); + + put_cpu(); +} + static void vmbus_release_relid(u32 relid) { @@ -390,15 +412,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid) BUG_ON(!channel->rescind); BUG_ON(!mutex_is_locked(&vmbus_connection.channel_mutex)); - if (channel->target_cpu != get_cpu()) { - put_cpu(); - smp_call_function_single(channel->target_cpu, - percpu_channel_deq, channel, true); - } else { - percpu_channel_deq(channel); - put_cpu(); - } - if (channel->primary_channel == NULL) { list_del(&channel->listentry); @@ -491,16 +504,6 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) init_vp_index(newchannel, dev_type); - if (newchannel->target_cpu != get_cpu()) { - put_cpu(); - smp_call_function_single(newchannel->target_cpu, - percpu_channel_enq, - newchannel, true); - } else { - percpu_channel_enq(newchannel); - put_cpu(); - } - /* * This state is used to indicate a successful open * so that when we do close the channel normally, we @@ -549,15 +552,6 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) list_del(&newchannel->listentry); mutex_unlock(&vmbus_connection.channel_mutex); - if (newchannel->target_cpu != get_cpu()) { - put_cpu(); - smp_call_function_single(newchannel->target_cpu, - percpu_channel_deq, newchannel, true); - } else { - percpu_channel_deq(newchannel); - put_cpu(); - } - vmbus_release_relid(newchannel->offermsg.child_relid); err_free_chan: diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 0c170a3f0d8b..ba93b7e4a972 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -1437,6 +1437,9 @@ extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, u8 *buf, const int *srv_version, int srv_vercnt, int *nego_fw_version, int *nego_srv_version); +void hv_percpu_channel_enq(struct vmbus_channel *channel); +void hv_percpu_channel_deq(struct vmbus_channel *channel); + void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid); void vmbus_setevent(struct vmbus_channel *channel); -- 2.13.0