From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755347AbcEQIO5 (ORCPT ); Tue, 17 May 2016 04:14:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40096 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754977AbcEQIOx (ORCPT ); Tue, 17 May 2016 04:14:53 -0400 From: Vitaly Kuznetsov To: Dexuan Cui Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, driverdev-devel@linuxdriverproject.org, olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com, kys@microsoft.com, haiyangz@microsoft.com Subject: Re: [PATCH] Drivers: hv: vmbus: fix the race when querying & updating the percpu list References: <1463461980-13401-1-git-send-email-decui@microsoft.com> Date: Tue, 17 May 2016 10:14:47 +0200 In-Reply-To: <1463461980-13401-1-git-send-email-decui@microsoft.com> (Dexuan Cui's message of "Mon, 16 May 2016 22:13:00 -0700") Message-ID: <87r3d1drt4.fsf@vitty.brq.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 17 May 2016 08:14:51 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Dexuan Cui writes: > There is a rare race when we remove an entry from the global list > hv_context.percpu_list[cpu] in hv_process_channel_removal() -> > percpu_channel_deq() -> list_del(): at this time, if vmbus_on_event() -> > process_chn_event() -> pcpu_relid2channel() is trying to query the list, > we can get the general protection fault: > > general protection fault: 0000 [#1] SMP > ... > RIP: 0010:[] [] vmbus_on_event+0xc4/0x149 > > Similarly, we also have the issue in the code path: vmbus_process_offer() -> > percpu_channel_enq(). > > We can resolve the issue by disabling the tasklet when updating the list. > > Reported-by: Rolf Neugebauer > Signed-off-by: Dexuan Cui > --- > drivers/hv/channel.c | 3 +++ > drivers/hv/channel_mgmt.c | 20 +++++++++----------- > include/linux/hyperv.h | 3 +++ > 3 files changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > index 56dd261..7811cf9 100644 > --- a/drivers/hv/channel.c > +++ b/drivers/hv/channel.c > @@ -546,8 +546,11 @@ static int vmbus_close_internal(struct vmbus_channel *channel) > put_cpu(); > smp_call_function_single(channel->target_cpu, reset_channel_cb, > channel, true); > + smp_call_function_single(channel->target_cpu, > + percpu_channel_deq, channel, true); > } else { > reset_channel_cb(channel); > + percpu_channel_deq(channel); > put_cpu(); > } > > diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c > index 38b682ba..c4b5c07 100644 > --- a/drivers/hv/channel_mgmt.c > +++ b/drivers/hv/channel_mgmt.c > @@ -21,6 +21,7 @@ > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > #include > +#include > #include > #include > #include > @@ -277,7 +278,7 @@ static void free_channel(struct vmbus_channel *channel) > kfree(channel); > } > > -static void percpu_channel_enq(void *arg) > +void percpu_channel_enq(void *arg) > { > struct vmbus_channel *channel = arg; > int cpu = smp_processor_id(); > @@ -285,7 +286,7 @@ static void percpu_channel_enq(void *arg) > list_add_tail(&channel->percpu_list, &hv_context.percpu_list[cpu]); > } > > -static void percpu_channel_deq(void *arg) > +void percpu_channel_deq(void *arg) > { > struct vmbus_channel *channel = arg; > > @@ -313,15 +314,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); > > @@ -363,6 +355,7 @@ void vmbus_free_channels(void) > */ > static void vmbus_process_offer(struct vmbus_channel *newchannel) > { > + struct tasklet_struct *tasklet; > struct vmbus_channel *channel; > bool fnew = true; > unsigned long flags; > @@ -409,6 +402,8 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) > > init_vp_index(newchannel, dev_type); > > + tasklet = hv_context.event_dpc[newchannel->target_cpu]; > + tasklet_disable(tasklet); > if (newchannel->target_cpu != get_cpu()) { > put_cpu(); > smp_call_function_single(newchannel->target_cpu, > @@ -418,6 +413,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) > percpu_channel_enq(newchannel); > put_cpu(); > } > + tasklet_enable(tasklet); Do we need to do tasklet_schedule() to make sure there are no events pending? This is probably not a big issue as some other event will trigger scheduling but in some corner cases it may bite. Same question applies to the code below and to vmbus_close_internal(). > > /* > * This state is used to indicate a successful open > @@ -469,6 +465,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) > list_del(&newchannel->listentry); > mutex_unlock(&vmbus_connection.channel_mutex); > > + tasklet_disable(tasklet); > if (newchannel->target_cpu != get_cpu()) { > put_cpu(); > smp_call_function_single(newchannel->target_cpu, > @@ -477,6 +474,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) > percpu_channel_deq(newchannel); > put_cpu(); > } > + tasklet_enable(tasklet); > > err_free_chan: > free_channel(newchannel); > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index 7be7237..95aea09 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -1328,6 +1328,9 @@ extern bool vmbus_prep_negotiate_resp(struct icmsg_hdr *, > struct icmsg_negotiate *, u8 *, int, > int); > > +void percpu_channel_enq(void *arg); > +void percpu_channel_deq(void *arg); > + > void hv_process_channel_removal(struct vmbus_channel *channel, u32 relid); > > /* -- Vitaly