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

["Jan Höppner" <hoeppner@linux.ibm.com>]

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;
>  	}