[PATCH 1/2] scsi: sr: Fix sr_probe() missing mutex_destroy

[PATCH 1/2] scsi: sr: Fix sr_probe() missing mutex_destroy

[Simon Arlott]

If the device minor cannot be allocated or the cdrom fails to be
registered then the mutex should be destroyed.

Signed-off-by: Simon Arlott <simon@octiron.net>
---
 drivers/scsi/sr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index d2fe3fa470f9..8d062d4f3ce0 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -817,6 +817,7 @@ static int sr_probe(struct device *dev)
 
 fail_put:
 	put_disk(disk);
+	mutex_destroy(&cd->lock);
 fail_free:
 	kfree(cd);
 fail:
-- 
2.17.1

-- 
Simon Arlott

[PATCH 2/2] scsi: sr: Fix sr_probe() missing deallocate of device minor

[Simon Arlott]

If the cdrom fails to be registered then the device minor should be
deallocated.

Signed-off-by: Simon Arlott <simon@octiron.net>
---
 drivers/scsi/sr.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 8d062d4f3ce0..1e13c6a0f0ca 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -797,7 +797,7 @@ static int sr_probe(struct device *dev)
 	cd->cdi.disk = disk;
 
 	if (register_cdrom(&cd->cdi))
-		goto fail_put;
+		goto fail_minor;
 
 	/*
 	 * Initialize block layer runtime PM stuffs before the
@@ -815,6 +815,10 @@ static int sr_probe(struct device *dev)
 
 	return 0;
 
+fail_minor:
+	spin_lock(&sr_index_lock);
+	clear_bit(minor, sr_index_bits);
+	spin_unlock(&sr_index_lock);
 fail_put:
 	put_disk(disk);
 	mutex_destroy(&cd->lock);
-- 
2.17.1

-- 
Simon Arlott

Re: [PATCH 2/2] scsi: sr: Fix sr_probe() missing deallocate of device minor

[Bart Van Assche]

On 2020-05-30 02:33, Simon Arlott wrote:
> If the cdrom fails to be registered then the device minor should be
> deallocated.

Also for this patch, please add Fixes: and Cc: stable tags.

Thanks,

Bart.

Re: [PATCH 1/2] scsi: sr: Fix sr_probe() missing mutex_destroy

[Bart Van Assche]

On 2020-05-30 02:32, Simon Arlott wrote:
> If the device minor cannot be allocated or the cdrom fails to be
> registered then the mutex should be destroyed.

Please add Fixes: and Cc: stable tags.

Thanks,

Bart.

Re: [PATCH 1/2] scsi: sr: Fix sr_probe() missing mutex_destroy

[James Bottomley]

On Sat, 2020-05-30 at 09:24 -0700, Bart Van Assche wrote:
> On 2020-05-30 02:32, Simon Arlott wrote:
> > If the device minor cannot be allocated or the cdrom fails to be
> > registered then the mutex should be destroyed.
> 
> Please add Fixes: and Cc: stable tags.

This isn't really a bug, is it?  mutex_destroy is a nop unless lock
debugging is enabled in which case it checks the lock is unlocked and
marks it as unusable to detect a use after destroy.  Since the
structure containing the mutex is kfree'd in the next statement, kasan
would also detect any use after free.  That's not to say we shouldn't
do this to be fully correct ... just that it has no potential ever to
have user visible impact so there doesn't seem to be much point
cluttering up the stable process with it.

James

Re: [PATCH 1/2] scsi: sr: Fix sr_probe() missing mutex_destroy

[Bart Van Assche]

On 2020-05-30 09:41, James Bottomley wrote:
> On Sat, 2020-05-30 at 09:24 -0700, Bart Van Assche wrote:
>> On 2020-05-30 02:32, Simon Arlott wrote:
>>> If the device minor cannot be allocated or the cdrom fails to be
>>> registered then the mutex should be destroyed.
>>
>> Please add Fixes: and Cc: stable tags.
> 
> This isn't really a bug, is it?  mutex_destroy is a nop unless lock
> debugging is enabled in which case it checks the lock is unlocked and
> marks it as unusable to detect a use after destroy.  Since the
> structure containing the mutex is kfree'd in the next statement, kasan
> would also detect any use after free.  That's not to say we shouldn't
> do this to be fully correct ... just that it has no potential ever to
> have user visible impact so there doesn't seem to be much point
> cluttering up the stable process with it.

That makes sense to me. I may have confused Simon with my suggestion.

Bart.