[X] UBUNTU: SAUCE: (no-up) virtio-scsi: Fix race in target free

BugLink: http://bugs.launchpad.net/bugs/1765241

	A race condition exists in virtio_scsi between the completion of
a request and the freeing of the target structure.  The race is between
(a) virtscsi_complete_cmd that, first, wakes up a task waiting for a
completion, then, second, releases a reference in the target structure
and (b) the woken up task freeing that target structure.

	The race appears to exist in all verisons of virtio_scsi, but
most kernels are not impacted due to a coincidental RCU sync in the
"(b)" path above that will effectively wait for the "(a)" path to
complete.  The Ubuntu Xenial 4.4 kernel since commit be2a20802abbde
lacks any RCU sync in the "(b)" code path, thus opening the race window.

	The fix is to wait for any outstanding requests to release their
references prior to freeing the target structure.

Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>

---
 drivers/scsi/virtio_scsi.c | 4 ++++
 1 file changed, 4 insertions(+)

On 19.04.2018 21:40, Jay Vosburgh wrote:
> BugLink: http://bugs.launchpad.net/bugs/1765241
> 
> 	A race condition exists in virtio_scsi between the completion of
> a request and the freeing of the target structure.  The race is between
> (a) virtscsi_complete_cmd that, first, wakes up a task waiting for a
> completion, then, second, releases a reference in the target structure
> and (b) the woken up task freeing that target structure.
> 
> 	The race appears to exist in all verisons of virtio_scsi, but
> most kernels are not impacted due to a coincidental RCU sync in the
> "(b)" path above that will effectively wait for the "(a)" path to
> complete.  The Ubuntu Xenial 4.4 kernel since commit be2a20802abbde
> lacks any RCU sync in the "(b)" code path, thus opening the race window.
> 
> 	The fix is to wait for any outstanding requests to release their
> references prior to freeing the target structure.
> 
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>

> 
> ---

Sounds reasonable and safe enough.

-Stefan

>  drivers/scsi/virtio_scsi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 8ef905cbfc9c..e2da31286793 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -785,6 +785,10 @@ static int virtscsi_target_alloc(struct scsi_target *starget)
>  static void virtscsi_target_destroy(struct scsi_target *starget)
>  {
>  	struct virtio_scsi_target_state *tgt = starget->hostdata;
> +
> +	/* we can race with concurrent virtscsi_complete_cmd */
> +	while (atomic_read(&tgt->reqs))
> +		cpu_relax();
>  	kfree(tgt);
>  }
>  
>

On 04/19/18 21:40, Jay Vosburgh wrote:
> BugLink: http://bugs.launchpad.net/bugs/1765241
> 
> 	A race condition exists in virtio_scsi between the completion of
> a request and the freeing of the target structure.  The race is between
> (a) virtscsi_complete_cmd that, first, wakes up a task waiting for a
> completion, then, second, releases a reference in the target structure
> and (b) the woken up task freeing that target structure.
> 
> 	The race appears to exist in all verisons of virtio_scsi, but
> most kernels are not impacted due to a coincidental RCU sync in the
> "(b)" path above that will effectively wait for the "(a)" path to
> complete.  The Ubuntu Xenial 4.4 kernel since commit be2a20802abbde
> lacks any RCU sync in the "(b)" code path, thus opening the race window.
> 
> 	The fix is to wait for any outstanding requests to release their
> references prior to freeing the target structure.
> 
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

Very well documented and tested fix.

Thanks,
Kleber


> 
> ---
>  drivers/scsi/virtio_scsi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 8ef905cbfc9c..e2da31286793 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -785,6 +785,10 @@ static int virtscsi_target_alloc(struct scsi_target *starget)
>  static void virtscsi_target_destroy(struct scsi_target *starget)
>  {
>  	struct virtio_scsi_target_state *tgt = starget->hostdata;
> +
> +	/* we can race with concurrent virtscsi_complete_cmd */
> +	while (atomic_read(&tgt->reqs))
> +		cpu_relax();
>  	kfree(tgt);
>  }
>  
>

On 19.04.2018 21:40, Jay Vosburgh wrote:
> BugLink: http://bugs.launchpad.net/bugs/1765241
> 
> 	A race condition exists in virtio_scsi between the completion of
> a request and the freeing of the target structure.  The race is between
> (a) virtscsi_complete_cmd that, first, wakes up a task waiting for a
> completion, then, second, releases a reference in the target structure
> and (b) the woken up task freeing that target structure.
> 
> 	The race appears to exist in all verisons of virtio_scsi, but
> most kernels are not impacted due to a coincidental RCU sync in the
> "(b)" path above that will effectively wait for the "(a)" path to
> complete.  The Ubuntu Xenial 4.4 kernel since commit be2a20802abbde
> lacks any RCU sync in the "(b)" code path, thus opening the race window.
> 
> 	The fix is to wait for any outstanding requests to release their
> references prior to freeing the target structure.
> 
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> 
> ---

Applied to xenial/master-next

>  drivers/scsi/virtio_scsi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 8ef905cbfc9c..e2da31286793 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -785,6 +785,10 @@ static int virtscsi_target_alloc(struct scsi_target *starget)
>  static void virtscsi_target_destroy(struct scsi_target *starget)
>  {
>  	struct virtio_scsi_target_state *tgt = starget->hostdata;
> +
> +	/* we can race with concurrent virtscsi_complete_cmd */
> +	while (atomic_read(&tgt->reqs))
> +		cpu_relax();
>  	kfree(tgt);
>  }
>  
>