linux-kernel - RE: [PATCH] scsi: storvsc: Prevent running tasklet for long

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 5 Jul 2022 17:39:12 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Saurabh Sengar <ssengar@...ux.microsoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Saurabh Singh Sengar <ssengar@...rosoft.com>
Subject: RE: [PATCH] scsi: storvsc: Prevent running tasklet for long

From: Saurabh Sengar <ssengar@...ux.microsoft.com> Sent: Tuesday, July 5, 2022 8:32 AM
> 
> There can be scenarios where packets in ring buffer are continuously
> getting queued from upper layer and dequeued from storvsc interrupt
> handler, such scenarios can hold the foreach_vmbus_pkt loop (which is
> executing as a tasklet) for a long duration. Theoretically its possible
> that this loop executes forever. 

Actually, the "forever" part isn't possible.  The way foreach_vmbus_pkt()
works, it only runs until the ring buffer is empty, and it does not update
the ring buffer read index until the empty condition is reached.  So the
Hyper-V host is prevented from continuously feeding new packets into the
ring buffer while storvsc is reading them.  But ring buffer is pretty big, so
storvsc could still end up processing a lot of completion packets and take
an undesirable amount of time in the loop.  Hence the scenario is valid.

> Add a condition to limit execution of
> this tasklet for finite amount of time to avoid such hazardous scenarios.
> 
> Signed-off-by: Saurabh Sengar <ssengar@...ux.microsoft.com>
> ---
>  drivers/scsi/storvsc_drv.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index fe000da..0c428cb 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -60,6 +60,9 @@
>  #define VMSTOR_PROTO_VERSION_WIN8_1	VMSTOR_PROTO_VERSION(6, 0)
>  #define VMSTOR_PROTO_VERSION_WIN10	VMSTOR_PROTO_VERSION(6, 2)
> 
> +/* channel callback timeout in ms */
> +#define CALLBACK_TIMEOUT		5
> +
>  /*  Packet structure describing virtual storage requests. */
>  enum vstor_packet_operation {
>  	VSTOR_OPERATION_COMPLETE_IO		= 1,
> @@ -1204,6 +1207,7 @@ static void storvsc_on_channel_callback(void *context)
>  	struct hv_device *device;
>  	struct storvsc_device *stor_device;
>  	struct Scsi_Host *shost;
> +	unsigned long expire = jiffies + msecs_to_jiffies(CALLBACK_TIMEOUT);
> 
>  	if (channel->primary_channel != NULL)
>  		device = channel->primary_channel->device_obj;
> @@ -1224,6 +1228,9 @@ static void storvsc_on_channel_callback(void *context)
>  		u32 minlen = rqst_id ? sizeof(struct vstor_packet) :
>  			sizeof(enum vstor_packet_operation);
> 
> +		if (time_after(jiffies, expire))
> +			break;
> +

Per my comment on the commit message, breaking out of the
foreach_vmbus_pkt() loop leaves the ring indices unchanged.  I *think* we
want to close out the iteration via hv_pkt_iter_close() in the case where we
exit the loop early, so that Hyper-V can re-use the ring buffer space from
completions we've already processed.

Also when we re-enter storvsc_on_channel_callback() and again do
hv_pkt_iter_first(), that should be only after hv_pkt_iter_close() has been
called; i.e., the hv_pkt_iter_* functions should be matched up correctly.
The current implementation of these functions doesn't go awry
if they aren't matched up correctly, but we should nonetheless do them
correctly in case the implementation changes in the future.

Similar code in netvsc_poll() has the same behavior of not closing out
the hv_pkt_iter when the poll budget is exhausted, and we've had a recent
internal discussion about changing that.

Michael

>  		if (pktlen < minlen) {
>  			dev_err(&device->device,
>  				"Invalid pkt: id=%llu, len=%u, minlen=%u\n",
> --
> 1.8.3.1