| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 13 Jul 2015 17:28:31 +0200
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: KY Srinivasan <kys@...rosoft.com>
Cc: "devel\@linuxdriverproject.org" <devel@...uxdriverproject.org>,
Haiyang Zhang <haiyangz@...rosoft.com>,
Dexuan Cui <decui@...rosoft.com>,
"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on device shutdown
KY Srinivasan <kys@...rosoft.com> writes:
>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@...hat.com]
>> Sent: Monday, July 13, 2015 5:19 AM
>> To: devel@...uxdriverproject.org
>> Cc: KY Srinivasan; Haiyang Zhang; Dexuan Cui; linux-kernel@...r.kernel.org
>> Subject: [PATCH] Drivers: hv: vmbus: prevent new subchannel creation on
>> device shutdown
>>
>> When a new subchannel offer from host comes during device shutdown
>> (e.g.
>> when a netvsc/storvsc module is unloadedshortly after it was loaded) a
>> crash can happen as vmbus_process_offer() is not anyhow serialized with
>> vmbus_remove(). As an example we can try calling subchannel create
>> callback when the module is already unloaded.
>> The following crash was observed while keeping loading/unloading
>> hv_netvsc
>> module on 64-CPU guest:
>>
>> hv_netvsc vmbus_14: net device safe to remove
>> BUG: unable to handle kernel paging request at 000000000000a118
>> IP: [<ffffffffa01c1b21>] netvsc_sc_open+0x31/0xb0 [hv_netvsc]
>> PGD 1f3946a067 PUD 1f38a5f067 PMD 0
>> Oops: 0000 [#1] SMP
>> ...
>> Call Trace:
>> [<ffffffffa0055cd7>] vmbus_onoffer+0x477/0x530 [hv_vmbus]
>> [<ffffffff81092e1f>] ? move_linked_works+0x5f/0x80
>> [<ffffffffa0055fd3>] vmbus_onmessage+0x33/0xa0 [hv_vmbus]
>> [<ffffffffa0052f81>] vmbus_onmessage_work+0x21/0x30 [hv_vmbus]
>> [<ffffffff81095cde>] process_one_work+0x18e/0x4e0
>> ...
>>
>> The issue cannot be solved by just resetting sc_creation_callback on
>> driver removal as while we search for the parent channel with channel_lock
>> held we release it after the channel was found and it can disapper beneath
>> our feet while we're still in vmbus_process_offer();
>>
>> Introduce new sc_create_lock mutex and take it in vmbus_remove() to
>> ensure
>> no new subchannels are created after we started the removal procedure.
>> Check its state with mutex_trylock in vmbus_process_offer().
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
>
> Thanks Vitaly; strangely enough, I too am looking at this exact problem. I was planning to
> address this problem a little differently: I was planning to hold the context that performed
> the initial (primary) channel open until all of the "open activity" was completed and this would include
> opening of the sub-channels for multi-channel devices.
(not sure it can actually happen with current implementation of Hyper-V
host but) in case a subchannel is rescinded and reopened again later on
such 'full open lock' won't probably help but in other cases both
approaches should give us equal results. This is not a hotpath, any
syncronization should do the job.
>
> Regards,
>
> K. Y
>
>> ---
>> drivers/hv/channel.c | 3 +++
>> drivers/hv/channel_mgmt.c | 20 ++++++++++++++++++--
>> drivers/hv/vmbus_drv.c | 6 ++++++
>> include/linux/hyperv.h | 4 ++++
>> 4 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
>> index 603ce97..faa91fe 100644
>> --- a/drivers/hv/channel.c
>> +++ b/drivers/hv/channel.c
>> @@ -555,6 +555,9 @@ static int vmbus_close_internal(struct vmbus_channel
>> *channel)
>> if (channel->rescind)
>> hv_process_channel_removal(channel,
>> channel->offermsg.child_relid);
>> + else if (mutex_is_locked(&channel->sc_create_lock))
>> + mutex_unlock(&channel->sc_create_lock);
>> +
>> return ret;
>> }
>>
>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>> index 4506a66..96f8b96 100644
>> --- a/drivers/hv/channel_mgmt.c
>> +++ b/drivers/hv/channel_mgmt.c
>> @@ -150,6 +150,8 @@ static struct vmbus_channel *alloc_channel(void)
>> INIT_LIST_HEAD(&channel->sc_list);
>> INIT_LIST_HEAD(&channel->percpu_list);
>>
>> + mutex_init(&channel->sc_create_lock);
>> +
>> return channel;
>> }
>>
>> @@ -158,6 +160,7 @@ static struct vmbus_channel *alloc_channel(void)
>> */
>> static void free_channel(struct vmbus_channel *channel)
>> {
>> + mutex_destroy(&channel->sc_create_lock);
>> kfree(channel);
>> }
>>
>> @@ -247,8 +250,18 @@ static void vmbus_process_offer(struct
>> vmbus_channel *newchannel)
>> newchannel->offermsg.offer.if_type) &&
>> !uuid_le_cmp(channel->offermsg.offer.if_instance,
>> newchannel->offermsg.offer.if_instance)) {
>> - fnew = false;
>> - break;
>> + if (mutex_trylock(&channel->sc_create_lock)) {
>> + fnew = false;
>> + break;
>> + }
>> + /*
>> + * If we fail to acquire mutex here it means we're
>> + * closing parent channel at this moment. We can't
>> + * create new subchannel in this case.
>> + */
>> +
>> spin_unlock_irqrestore(&vmbus_connection.channel_lock,
>> + flags);
>> + goto err_free_chan;
>> }
>> }
>>
>> @@ -297,6 +310,7 @@ static void vmbus_process_offer(struct
>> vmbus_channel *newchannel)
>> if (!fnew) {
>> if (channel->sc_creation_callback != NULL)
>> channel->sc_creation_callback(newchannel);
>> + mutex_unlock(&channel->sc_create_lock);
>> return;
>> }
>>
>> @@ -340,6 +354,8 @@ err_deq_chan:
>> }
>>
>> err_free_chan:
>> + if (!fnew)
>> + mutex_unlock(&channel->sc_create_lock);
>> free_channel(newchannel);
>> }
>>
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index cf20400..7ad7fcc 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -539,6 +539,12 @@ static int vmbus_remove(struct device
>> *child_device)
>> struct hv_device *dev = device_to_hv_device(child_device);
>> u32 relid = dev->channel->offermsg.child_relid;
>>
>> + /*
>> + * Device is being removed, prevent creating new subchannels.
>> Mutex
>> + * will be released in vmbus_close_internal().
>> + */
>> + mutex_lock(&dev->channel->sc_create_lock);
>> +
>> if (child_device->driver) {
>> drv = drv_to_hv_drv(child_device->driver);
>> if (drv->remove)
>> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
>> index 30d3a1f..f1af05c 100644
>> --- a/include/linux/hyperv.h
>> +++ b/include/linux/hyperv.h
>> @@ -748,6 +748,10 @@ struct vmbus_channel {
>> */
>> struct vmbus_channel *primary_channel;
>> /*
>> + * Protects sub-channel create path.
>> + */
>> + struct mutex sc_create_lock;
>> + /*
>> * Support per-channel state for use by vmbus drivers.
>> */
>> void *per_channel_state;
>> --
>> 2.4.3
--
Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists