Hi all,
This series is to address the problems mentioned in:
4da3a54f5a0258 ("Revert "scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()"")
(cf., in particular, patch 2/3) and to re-introduce the validation in
question (patch 3/3); patch 1/3 emerged from internal review of these
two patches and is a related fix.
Thanks,
Andrea
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: [email protected]
Andrea Parri (Microsoft) (3):
scsi: storvsc: Fix max_outstanding_req_per_channel for Win8 and newer
scsi: storvsc: Resolve data race in storvsc_probe()
scsi: storvsc: Validate length of incoming packet in
storvsc_on_channel_callback()
drivers/scsi/storvsc_drv.c | 58 +++++++++++++++++++++++---------------
1 file changed, 36 insertions(+), 22 deletions(-)
--
2.25.1
Current code overestimates the value of max_outstanding_req_per_channel
for Win8 and newer hosts, since vmscsi_size_delta is set to the initial
value of sizeof(vmscsi_win8_extension) rather than zero. This may lead
to wrong decisions when using ring_avail_percent_lowater equals to zero.
The estimate of max_outstanding_req_per_channel is 'exact' for Win7 and
older hosts. A better choice, keeping the algorithm for the estimation
simple, is to err the other way around, i.e., to underestimate for Win7
and older but to use the exact value for Win8 and newer.
Suggested-by: Dexuan Cui <[email protected]>
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: [email protected]
---
drivers/scsi/storvsc_drv.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index ded00a89bfc4e..64298aa2f151e 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -2141,12 +2141,15 @@ static int __init storvsc_drv_init(void)
* than the ring buffer size since that page is reserved for
* the ring buffer indices) by the max request size (which is
* vmbus_channel_packet_multipage_buffer + struct vstor_packet + u64)
+ *
+ * The computation underestimates max_outstanding_req_per_channel
+ * for Win7 and older hosts because it does not take into account
+ * the vmscsi_size_delta correction to the max request size.
*/
max_outstanding_req_per_channel =
((storvsc_ringbuffer_size - PAGE_SIZE) /
ALIGN(MAX_MULTIPAGE_BUFFER_PACKET +
- sizeof(struct vstor_packet) + sizeof(u64) -
- vmscsi_size_delta,
+ sizeof(struct vstor_packet) + sizeof(u64),
sizeof(u64)));
#if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
--
2.25.1
vmscsi_size_delta can be written concurrently by multiple instances of
storvsc_probe(), corresponding to multiple synthetic IDE/SCSI devices;
cf. storvsc_drv's probe_type == PROBE_PREFER_ASYNCHRONOUS. Change the
global variable vmscsi_size_delta to per-synthetic-IDE/SCSI-device.
Suggested-by: Dexuan Cui <[email protected]>
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: [email protected]
---
drivers/scsi/storvsc_drv.c | 45 +++++++++++++++++++++-----------------
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 64298aa2f151e..8714355cb63e7 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -216,18 +216,6 @@ struct vmscsi_request {
} __attribute((packed));
-
-/*
- * The size of the vmscsi_request has changed in win8. The
- * additional size is because of new elements added to the
- * structure. These elements are valid only when we are talking
- * to a win8 host.
- * Track the correction to size we need to apply. This value
- * will likely change during protocol negotiation but it is
- * valid to start by assuming pre-Win8.
- */
-static int vmscsi_size_delta = sizeof(struct vmscsi_win8_extension);
-
/*
* The list of storage protocols in order of preference.
*/
@@ -449,6 +437,17 @@ struct storvsc_device {
unsigned char path_id;
unsigned char target_id;
+ /*
+ * The size of the vmscsi_request has changed in win8. The
+ * additional size is because of new elements added to the
+ * structure. These elements are valid only when we are talking
+ * to a win8 host.
+ * Track the correction to size we need to apply. This value
+ * will likely change during protocol negotiation but it is
+ * valid to start by assuming pre-Win8.
+ */
+ int vmscsi_size_delta;
+
/*
* Max I/O, the device can support.
*/
@@ -762,7 +761,7 @@ static void handle_multichannel_storage(struct hv_device *device, int max_chns)
ret = vmbus_sendpacket(device->channel, vstor_packet,
(sizeof(struct vstor_packet) -
- vmscsi_size_delta),
+ stor_device->vmscsi_size_delta),
(unsigned long)request,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
@@ -816,9 +815,14 @@ static int storvsc_execute_vstor_op(struct hv_device *device,
struct storvsc_cmd_request *request,
bool status_check)
{
+ struct storvsc_device *stor_device;
struct vstor_packet *vstor_packet;
int ret, t;
+ stor_device = get_out_stor_device(device);
+ if (!stor_device)
+ return -ENODEV;
+
vstor_packet = &request->vstor_packet;
init_completion(&request->wait_event);
@@ -826,7 +830,7 @@ static int storvsc_execute_vstor_op(struct hv_device *device,
ret = vmbus_sendpacket(device->channel, vstor_packet,
(sizeof(struct vstor_packet) -
- vmscsi_size_delta),
+ stor_device->vmscsi_size_delta),
(unsigned long)request,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
@@ -903,7 +907,7 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
sense_buffer_size =
vmstor_protocols[i].sense_buffer_size;
- vmscsi_size_delta =
+ stor_device->vmscsi_size_delta =
vmstor_protocols[i].vmscsi_size_delta;
break;
@@ -1249,7 +1253,7 @@ static void storvsc_on_channel_callback(void *context)
if (request == &stor_device->init_request ||
request == &stor_device->reset_request) {
memcpy(&request->vstor_packet, packet,
- (sizeof(struct vstor_packet) - vmscsi_size_delta));
+ (sizeof(struct vstor_packet) - stor_device->vmscsi_size_delta));
complete(&request->wait_event);
} else {
storvsc_on_receive(stor_device, packet, request);
@@ -1461,7 +1465,7 @@ static int storvsc_do_io(struct hv_device *device,
vstor_packet->flags |= REQUEST_COMPLETION_FLAG;
vstor_packet->vm_srb.length = (sizeof(struct vmscsi_request) -
- vmscsi_size_delta);
+ stor_device->vmscsi_size_delta);
vstor_packet->vm_srb.sense_info_length = sense_buffer_size;
@@ -1478,12 +1482,12 @@ static int storvsc_do_io(struct hv_device *device,
request->payload, request->payload_sz,
vstor_packet,
(sizeof(struct vstor_packet) -
- vmscsi_size_delta),
+ stor_device->vmscsi_size_delta),
(unsigned long)request);
} else {
ret = vmbus_sendpacket(outgoing_channel, vstor_packet,
(sizeof(struct vstor_packet) -
- vmscsi_size_delta),
+ stor_device->vmscsi_size_delta),
(unsigned long)request,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
@@ -1589,7 +1593,7 @@ static int storvsc_host_reset_handler(struct scsi_cmnd *scmnd)
ret = vmbus_sendpacket(device->channel, vstor_packet,
(sizeof(struct vstor_packet) -
- vmscsi_size_delta),
+ stor_device->vmscsi_size_delta),
(unsigned long)&stor_device->reset_request,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
@@ -1939,6 +1943,7 @@ static int storvsc_probe(struct hv_device *device,
init_waitqueue_head(&stor_device->waiting_to_drain);
stor_device->device = device;
stor_device->host = host;
+ stor_device->vmscsi_size_delta = sizeof(struct vmscsi_win8_extension);
spin_lock_init(&stor_device->lock);
hv_set_drvdata(device, stor_device);
--
2.25.1
> From: Andrea Parri (Microsoft) <[email protected]>
> Sent: Thursday, December 17, 2020 12:33 PM
>
> Current code overestimates the value of max_outstanding_req_per_channel
> for Win8 and newer hosts, since vmscsi_size_delta is set to the initial
> value of sizeof(vmscsi_win8_extension) rather than zero. This may lead
> to wrong decisions when using ring_avail_percent_lowater equals to zero.
> The estimate of max_outstanding_req_per_channel is 'exact' for Win7 and
> older hosts. A better choice, keeping the algorithm for the estimation
> simple, is to err the other way around, i.e., to underestimate for Win7
> and older but to use the exact value for Win8 and newer.
>
> Suggested-by: Dexuan Cui <[email protected]>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> Cc: "James E.J. Bottomley" <[email protected]>
> Cc: "Martin K. Petersen" <[email protected]>
> Cc: [email protected]
Reviewed-by: Dexuan Cui <[email protected]>
> From: Andrea Parri (Microsoft) <[email protected]>
> Sent: Thursday, December 17, 2020 12:33 PM
>
> vmscsi_size_delta can be written concurrently by multiple instances of
> storvsc_probe(), corresponding to multiple synthetic IDE/SCSI devices;
> cf. storvsc_drv's probe_type == PROBE_PREFER_ASYNCHRONOUS. Change
> the
> global variable vmscsi_size_delta to per-synthetic-IDE/SCSI-device.
>
> Suggested-by: Dexuan Cui <[email protected]>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> Cc: "James E.J. Bottomley" <[email protected]>
> Cc: "Martin K. Petersen" <[email protected]>
> Cc: [email protected]
Reviewed-by: Dexuan Cui <[email protected]>
From: Andrea Parri (Microsoft) <[email protected]> Sent: Thursday, December 17, 2020 12:33 PM
>
> Current code overestimates the value of max_outstanding_req_per_channel
> for Win8 and newer hosts, since vmscsi_size_delta is set to the initial
> value of sizeof(vmscsi_win8_extension) rather than zero. This may lead
> to wrong decisions when using ring_avail_percent_lowater equals to zero.
> The estimate of max_outstanding_req_per_channel is 'exact' for Win7 and
> older hosts. A better choice, keeping the algorithm for the estimation
> simple, is to err the other way around, i.e., to underestimate for Win7
> and older but to use the exact value for Win8 and newer.
>
> Suggested-by: Dexuan Cui <[email protected]>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> Cc: "James E.J. Bottomley" <[email protected]>
> Cc: "Martin K. Petersen" <[email protected]>
> Cc: [email protected]
> ---
> drivers/scsi/storvsc_drv.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index ded00a89bfc4e..64298aa2f151e 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -2141,12 +2141,15 @@ static int __init storvsc_drv_init(void)
> * than the ring buffer size since that page is reserved for
> * the ring buffer indices) by the max request size (which is
> * vmbus_channel_packet_multipage_buffer + struct vstor_packet + u64)
> + *
> + * The computation underestimates max_outstanding_req_per_channel
> + * for Win7 and older hosts because it does not take into account
> + * the vmscsi_size_delta correction to the max request size.
> */
> max_outstanding_req_per_channel =
> ((storvsc_ringbuffer_size - PAGE_SIZE) /
> ALIGN(MAX_MULTIPAGE_BUFFER_PACKET +
> - sizeof(struct vstor_packet) + sizeof(u64) -
> - vmscsi_size_delta,
> + sizeof(struct vstor_packet) + sizeof(u64),
> sizeof(u64)));
>
> #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
> --
> 2.25.1
Reviewed-by: Michael Kelley <[email protected]>
From: Andrea Parri (Microsoft) <[email protected]> Sent: Thursday, December 17, 2020 12:33 PM
>
> vmscsi_size_delta can be written concurrently by multiple instances of
> storvsc_probe(), corresponding to multiple synthetic IDE/SCSI devices;
> cf. storvsc_drv's probe_type == PROBE_PREFER_ASYNCHRONOUS. Change the
> global variable vmscsi_size_delta to per-synthetic-IDE/SCSI-device.
>
> Suggested-by: Dexuan Cui <[email protected]>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> Cc: "James E.J. Bottomley" <[email protected]>
> Cc: "Martin K. Petersen" <[email protected]>
> Cc: [email protected]
> ---
> drivers/scsi/storvsc_drv.c | 45 +++++++++++++++++++++-----------------
> 1 file changed, 25 insertions(+), 20 deletions(-)
>
Reviewed-by: Michael Kelley <[email protected]>
Andrea,
> This series is to address the problems mentioned in:
>
> 4da3a54f5a0258 ("Revert "scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()"")
>
> (cf., in particular, patch 2/3) and to re-introduce the validation in
> question (patch 3/3); patch 1/3 emerged from internal review of these
> two patches and is a related fix.
Applied to 5.12/scsi-staging, thanks!
--
Martin K. Petersen Oracle Linux Engineering
On Thu, 17 Dec 2020 21:33:18 +0100, Andrea Parri (Microsoft) wrote:
> This series is to address the problems mentioned in:
>
> 4da3a54f5a0258 ("Revert "scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()"")
>
> (cf., in particular, patch 2/3) and to re-introduce the validation in
> question (patch 3/3); patch 1/3 emerged from internal review of these
> two patches and is a related fix.
>
> [...]
Applied to 5.12/scsi-queue, thanks!
[1/3] scsi: storvsc: Fix max_outstanding_req_per_channel for Win8 and newer
https://git.kernel.org/mkp/scsi/c/ab548fd21e1c
[2/3] scsi: storvsc: Resolve data race in storvsc_probe()
https://git.kernel.org/mkp/scsi/c/244808e03029
[3/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()
https://git.kernel.org/mkp/scsi/c/91b1b640b834
--
Martin K. Petersen Oracle Linux Engineering