* [PATCH] s390/dasd: fix double module refcount decrement
@ 2024-01-10 16:01 Miroslav Franc
2024-01-11 12:56 ` Jan Höppner
0 siblings, 1 reply; 4+ messages in thread
From: Miroslav Franc @ 2024-01-10 16:01 UTC (permalink / raw)
To: linux-s390; +Cc: Stefan Haberland, Jan Hoeppner
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;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] s390/dasd: fix double module refcount decrement 2024-01-10 16:01 [PATCH] s390/dasd: fix double module refcount decrement Miroslav Franc @ 2024-01-11 12:56 ` Jan Höppner 2024-01-11 14:54 ` Miroslav Franc 0 siblings, 1 reply; 4+ messages in thread From: Jan Höppner @ 2024-01-11 12:56 UTC (permalink / raw) To: Miroslav Franc, linux-s390; +Cc: Stefan Haberland 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; > } > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] s390/dasd: fix double module refcount decrement 2024-01-11 12:56 ` Jan Höppner @ 2024-01-11 14:54 ` Miroslav Franc 2024-01-12 10:22 ` Jan Höppner 0 siblings, 1 reply; 4+ messages in thread From: Miroslav Franc @ 2024-01-11 14:54 UTC (permalink / raw) To: Jan Höppner, linux-s390; +Cc: Stefan Haberland 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; } ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] s390/dasd: fix double module refcount decrement 2024-01-11 14:54 ` Miroslav Franc @ 2024-01-12 10:22 ` Jan Höppner 0 siblings, 0 replies; 4+ messages in thread From: Jan Höppner @ 2024-01-12 10:22 UTC (permalink / raw) To: Miroslav Franc, linux-s390; +Cc: Stefan Haberland 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; > } ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-01-12 10:22 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-10 16:01 [PATCH] s390/dasd: fix double module refcount decrement Miroslav Franc 2024-01-11 12:56 ` Jan Höppner 2024-01-11 14:54 ` Miroslav Franc 2024-01-12 10:22 ` Jan Höppner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.