Re: [PATCH v3] scsi: sg: Avoid race in error handling & drop bogus warn

[Martin K. Petersen]

On Mon, 01 Apr 2024 21:10:38 +0200, Alexander Wetzel wrote:

> commit 27f58c04a8f4 ("scsi: sg: Avoid sg device teardown race")
> introduced an incorrect WARN_ON_ONCE() and missed a sequence where
> sg_device_destroy() was used after scsi_device_put().
> 
> sg_device_destroy() is accessing the parent scsi_device request_queue which
> will already be set to NULL when the preceding call to scsi_device_put()
> removed the last reference to the parent scsi_device.
> 
> [...]

Applied to 6.9/scsi-fixes, thanks!

[1/1] scsi: sg: Avoid race in error handling & drop bogus warn
  https://git.kernel.org/mkp/scsi/c/d4e655c49f47

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v3] scsi: sg: Avoid race in error handling & drop bogus warn

[Martin K. Petersen]

Alexander,

> commit 27f58c04a8f4 ("scsi: sg: Avoid sg device teardown race")
> introduced an incorrect WARN_ON_ONCE() and missed a sequence where
> sg_device_destroy() was used after scsi_device_put().

Applied to 6.9/scsi-fixes, thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v3] scsi: sg: Avoid race in error handling & drop bogus warn

[Bart Van Assche]

On 4/1/24 12:10, Alexander Wetzel wrote:

commit 27f58c04a8f4 ("scsi: sg: Avoid sg device teardown race")
introduced an incorrect WARN_ON_ONCE() and missed a sequence where
sg_device_destroy() was used after scsi_device_put().

sg_device_destroy() is accessing the parent scsi_device request_queue which
will already be set to NULL when the preceding call to scsi_device_put()
removed the last reference to the parent scsi_device.

Drop the incorrect WARN_ON_ONCE() - allowing more than one concurrent
access to the sg device - and make sure sg_device_destroy() is not used
after scsi_device_put() in the error handling.


Reviewed-by: Bart Van Assche 

Re: [PATCH v3] scsi: sg: Avoid race in error handling & drop bogus warn

[Alexander Wetzel]

On 04.04.24 01:24, Bart Van Assche wrote:

On 4/1/24 12:10 PM, Alexander Wetzel wrote:

@@ -301,11 +302,12 @@ sg_open(struct inode *inode, struct file *filp)
  /* This driver's module count bumped by fops_get in  */
  /* Prevent the device driver from vanishing while we sleep */
-    retval = scsi_device_get(sdp->device);
+    device = sdp->device;
+    retval = scsi_device_get(device);
  if (retval)
  goto sg_put;


Are all the sdp->device -> device changes essential? Isn't there a
preference to minimize patches that will end up in the stable trees?



Only the very last change is essential:
-   scsi_device_put(sdp->device);
-   goto sg_put;
+   kref_put(>d_ref, sg_device_destroy);
+   scsi_device_put(device);
+   return retval;

Not using a (required) local variable and de-referencing it again and 
looks strange for anyone reading the code. While the additional lines in 
the patch are trivial to review...


Alexander

Re: [PATCH v3] scsi: sg: Avoid race in error handling & drop bogus warn

[Bart Van Assche]

On 4/1/24 12:10 PM, Alexander Wetzel wrote:

@@ -301,11 +302,12 @@ sg_open(struct inode *inode, struct file *filp)
  
  	/* This driver's module count bumped by fops_get in  */

/* Prevent the device driver from vanishing while we sleep */
-   retval = scsi_device_get(sdp->device);
+   device = sdp->device;
+   retval = scsi_device_get(device);
if (retval)
goto sg_put;


Are all the sdp->device -> device changes essential? Isn't there a
preference to minimize patches that will end up in the stable trees?

Thanks,

Bart.

Re: [PATCH v3] scsi: sg: Avoid race in error handling & drop bogus warn

[Sachin Sant]

> On 2 Apr 2024, at 12:40 AM, Alexander Wetzel  wrote:
> 
> commit 27f58c04a8f4 ("scsi: sg: Avoid sg device teardown race")
> introduced an incorrect WARN_ON_ONCE() and missed a sequence where
> sg_device_destroy() was used after scsi_device_put().
> 
> sg_device_destroy() is accessing the parent scsi_device request_queue which
> will already be set to NULL when the preceding call to scsi_device_put()
> removed the last reference to the parent scsi_device.
> 
> Drop the incorrect WARN_ON_ONCE() - allowing more than one concurrent
> access to the sg device - and make sure sg_device_destroy() is not used
> after scsi_device_put() in the error handling.
> 
> Link: 
> https://lore.kernel.org/all/5375b275-d137-4d5f-be25-6af8acae4...@linux.ibm.com
> Fixes: 27f58c04a8f4 ("scsi: sg: Avoid sg device teardown race")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Alexander Wetzel 
> ---

Thanks for the fix. I tested this patch and confirm it fixes the reported 
problem.

Tested-by: Sachin Sant 


— Sachin