[PATCH] s390/dasd: fix double module refcount decrement

[PATCH] s390/dasd: fix double module refcount decrement

[Miroslav Franc]

Once the discipline is associated with the device, deleting the device
takes care of decrementing the module's refcount.  Doing it manually on
this error path causes refcount to artificially decrease on each error
while it should just stay the same.

Fixes: c020d722b110 ("s390/dasd: fix panic during offline processing")
Signed-off-by: Miroslav Franc <mfranc@suse.cz>
---
 drivers/s390/block/dasd.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 833cfab7d877..739da1c2b71f 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -3546,8 +3546,6 @@ int dasd_generic_set_online(struct ccw_device *cdev,
 	if (rc) {
 		pr_warn("%s Setting the DASD online with discipline %s failed with rc=%i\n",
 			dev_name(&cdev->dev), discipline->name, rc);
-		module_put(discipline->owner);
-		module_put(base_discipline->owner);
 		dasd_delete_device(device);
 		return rc;
 	}

Re: [PATCH] s390/dasd: fix double module refcount decrement

[Jan Höppner]

On 10/01/2024 17:01, Miroslav Franc wrote:
> Once the discipline is associated with the device, deleting the device
> takes care of decrementing the module's refcount.  Doing it manually on
> this error path causes refcount to artificially decrease on each error
> while it should just stay the same.
> 
> Fixes: c020d722b110 ("s390/dasd: fix panic during offline processing")
> Signed-off-by: Miroslav Franc <mfranc@suse.cz>
> ---
>  drivers/s390/block/dasd.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 833cfab7d877..739da1c2b71f 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -3546,8 +3546,6 @@ int dasd_generic_set_online(struct ccw_device *cdev,
>  	if (rc) {
>  		pr_warn("%s Setting the DASD online with discipline %s failed with rc=%i\n",
>  			dev_name(&cdev->dev), discipline->name, rc);
> -		module_put(discipline->owner);
> -		module_put(base_discipline->owner);

Good catch. I think there is one more line above this part that should
also be removed:

if (!try_module_get(discipline->owner)) {
        module_put(base_discipline->owner); <---
        dasd_delete_device(device);
        return -EINVAL;
}

Can you add it to the patch? Thanks!

>  		dasd_delete_device(device);
>  		return rc;
>  	}
> 

Re: [PATCH] s390/dasd: fix double module refcount decrement

[Miroslav Franc]

Jan Höppner <hoeppner@linux.ibm.com> writes:

> On 10/01/2024 17:01, Miroslav Franc wrote:
>> Once the discipline is associated with the device, deleting the device
>> takes care of decrementing the module's refcount.  Doing it manually on
>> this error path causes refcount to artificially decrease on each error
>> while it should just stay the same.
>> 
>> Fixes: c020d722b110 ("s390/dasd: fix panic during offline processing")
>> Signed-off-by: Miroslav Franc <mfranc@suse.cz>
>> ---
>>  drivers/s390/block/dasd.c | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
>> index 833cfab7d877..739da1c2b71f 100644
>> --- a/drivers/s390/block/dasd.c
>> +++ b/drivers/s390/block/dasd.c
>> @@ -3546,8 +3546,6 @@ int dasd_generic_set_online(struct ccw_device *cdev,
>>  	if (rc) {
>>  		pr_warn("%s Setting the DASD online with discipline %s failed with rc=%i\n",
>>  			dev_name(&cdev->dev), discipline->name, rc);
>> -		module_put(discipline->owner);
>> -		module_put(base_discipline->owner);
>
> Good catch. I think there is one more line above this part that should
> also be removed:
>
> if (!try_module_get(discipline->owner)) {
>         module_put(base_discipline->owner); <---
>         dasd_delete_device(device);
>         return -EINVAL;
> }

Oh, I was under impression that the following line is necessary for
dasd_delete_device to work that way.

device->base_discipline = base_discipline;

I could move it before the if statement before removing module_put from
it.  Does it make sense?

>
> Can you add it to the patch? Thanks!
>
>>  		dasd_delete_device(device);
>>  		return rc;
>>  	}
>> 

Once the discipline is associated with the device, deleting the device
takes care of decrementing the module's refcount.  Doing it manually on
this error path causes refcount to artificially decrease on each error
while it should just stay the same.

Fixes: c020d722b110 ("s390/dasd: fix panic during offline processing")
Signed-off-by: Miroslav Franc <mfranc@suse.cz>
---
 drivers/s390/block/dasd.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 833cfab7d877..8e453454c271 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -3533,12 +3533,11 @@ int dasd_generic_set_online(struct ccw_device *cdev,
 		dasd_delete_device(device);
 		return -EINVAL;
 	}
+	device->base_discipline = base_discipline;
 	if (!try_module_get(discipline->owner)) {
-		module_put(base_discipline->owner);
 		dasd_delete_device(device);
 		return -EINVAL;
 	}
-	device->base_discipline = base_discipline;
 	device->discipline = discipline;
 
 	/* check_device will allocate block device if necessary */
@@ -3546,8 +3545,6 @@ int dasd_generic_set_online(struct ccw_device *cdev,
 	if (rc) {
 		pr_warn("%s Setting the DASD online with discipline %s failed with rc=%i\n",
 			dev_name(&cdev->dev), discipline->name, rc);
-		module_put(discipline->owner);
-		module_put(base_discipline->owner);
 		dasd_delete_device(device);
 		return rc;
 	}

Re: [PATCH] s390/dasd: fix double module refcount decrement

[Jan Höppner]

On 11/01/2024 15:54, Miroslav Franc wrote:
> Jan Höppner <hoeppner@linux.ibm.com> writes:
> 
>> On 10/01/2024 17:01, Miroslav Franc wrote:
>>> Once the discipline is associated with the device, deleting the device
>>> takes care of decrementing the module's refcount.  Doing it manually on
>>> this error path causes refcount to artificially decrease on each error
>>> while it should just stay the same.
>>>
>>> Fixes: c020d722b110 ("s390/dasd: fix panic during offline processing")
>>> Signed-off-by: Miroslav Franc <mfranc@suse.cz>
>>> ---
>>>  drivers/s390/block/dasd.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
>>> index 833cfab7d877..739da1c2b71f 100644
>>> --- a/drivers/s390/block/dasd.c
>>> +++ b/drivers/s390/block/dasd.c
>>> @@ -3546,8 +3546,6 @@ int dasd_generic_set_online(struct ccw_device *cdev,
>>>  	if (rc) {
>>>  		pr_warn("%s Setting the DASD online with discipline %s failed with rc=%i\n",
>>>  			dev_name(&cdev->dev), discipline->name, rc);
>>> -		module_put(discipline->owner);
>>> -		module_put(base_discipline->owner);
>>
>> Good catch. I think there is one more line above this part that should
>> also be removed:
>>
>> if (!try_module_get(discipline->owner)) {
>>         module_put(base_discipline->owner); <---
>>         dasd_delete_device(device);
>>         return -EINVAL;
>> }
> 
> Oh, I was under impression that the following line is necessary for
> dasd_delete_device to work that way.

You're absolutely right, I've missed that part, sorry.

> 
> device->base_discipline = base_discipline;
> 
> I could move it before the if statement before removing module_put from
> it.  Does it make sense?

Yes that makes sense. That way the (decrement) refcounting is entirely done
via the dasd_delete_device() function. I'll take your patch as suggested
below. Thanks a lot!

> 
>>
>> Can you add it to the patch? Thanks!
>>
>>>  		dasd_delete_device(device);
>>>  		return rc;
>>>  	}
>>>
> 
> Once the discipline is associated with the device, deleting the device
> takes care of decrementing the module's refcount.  Doing it manually on
> this error path causes refcount to artificially decrease on each error
> while it should just stay the same.
> 
> Fixes: c020d722b110 ("s390/dasd: fix panic during offline processing")
> Signed-off-by: Miroslav Franc <mfranc@suse.cz>
> ---
>  drivers/s390/block/dasd.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 833cfab7d877..8e453454c271 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -3533,12 +3533,11 @@ int dasd_generic_set_online(struct ccw_device *cdev,
>  		dasd_delete_device(device);
>  		return -EINVAL;
>  	}
> +	device->base_discipline = base_discipline;
>  	if (!try_module_get(discipline->owner)) {
> -		module_put(base_discipline->owner);
>  		dasd_delete_device(device);
>  		return -EINVAL;
>  	}
> -	device->base_discipline = base_discipline;
>  	device->discipline = discipline;
>  
>  	/* check_device will allocate block device if necessary */
> @@ -3546,8 +3545,6 @@ int dasd_generic_set_online(struct ccw_device *cdev,
>  	if (rc) {
>  		pr_warn("%s Setting the DASD online with discipline %s failed with rc=%i\n",
>  			dev_name(&cdev->dev), discipline->name, rc);
> -		module_put(discipline->owner);
> -		module_put(base_discipline->owner);
>  		dasd_delete_device(device);
>  		return rc;
>  	}