* Re: [PATCH] s390/tape: cancel delayed work before freeing device
2026-10-19 20:34 [PATCH] s390/tape: cancel delayed work before freeing device Tristan Madani
@ 2026-10-19 20:53 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-10-19 20:53 UTC (permalink / raw)
To: Tristan Madani
Cc: linux-s390, Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] s390/tape: sleep in atomic and deadlock in tape_put_device()
- [High] s390/tape: unsafe and incomplete delayed work cancellation in tape_put_device()
--
commit e3b5b4c025b8afebd3737fb5bf4782522c4d545f
Author: Tristan Madani <tristan@talencesecurity.com>
s390/tape: cancel delayed work before freeing device
This commit adds a call to cancel_delayed_work_sync() in tape_put_device()
when the reference count reaches zero to ensure the delayed work tape_dnr
has finished before freeing the tape_device structure.
> diff --git a/drivers/s390/char/tape_core.c b/drivers/s390/char/tape_core.c
> index bd8e3deb11993..e3aacfab2cc7a 100644
> --- a/drivers/s390/char/tape_core.c
> +++ b/drivers/s390/char/tape_core.c
> @@ -532,6 +532,7 @@ tape_put_device(struct tape_device *device)
> DBF_EVENT(4, "tape_put_device(%p) -> %i\n", device, count);
> BUG_ON(count < 0);
> if (count == 0) {
> + cancel_delayed_work_sync(&device->tape_dnr);
> kfree(device->modeset_byte);
> kfree(device);
> }
[Severity: High]
Can calling cancel_delayed_work_sync() in tape_put_device() sleep in atomic
context and self-deadlock?
tape_delayed_next_request() holds get_ccwdev_lock(device->cdev) with
interrupts disabled while calling __tape_start_next_request():
drivers/s390/char/tape_core.c:tape_delayed_next_request()
spin_lock_irq(get_ccwdev_lock(device->cdev));
__tape_start_next_request(device);
spin_unlock_irq(get_ccwdev_lock(device->cdev));
When starting or cancelling a queued request fails,
__tape_start_next_request() invokes request->callback while still holding
get_ccwdev_lock(device->cdev):
drivers/s390/char/tape_core.c:__tape_start_next_request()
if (request->callback != NULL)
request->callback(request, request->callback_data);
For asynchronous requests such as medium sense queued via
tape_3490_medium_sense_async() (which __tape_start_request() permits in
TS_UNUSED and TS_INIT states), the callback __tape_3490_medium_sense() calls
tape_free_request() -> tape_put_device(request->device).
If tape_generic_remove() runs while the device is in TS_UNUSED or TS_INIT, it
does not call __tape_discard_requests(device) before dropping its reference:
drivers/s390/char/tape_core.c:tape_generic_remove()
case TS_UNUSED:
tape_state_set(device, TS_NOT_OPER);
spin_unlock_irq(get_ccwdev_lock(device->cdev));
tape_cleanup_device(device);
break;
This leaves request->device as the final reference on struct tape_device while
device->tape_dnr is still pending. When tape_delayed_next_request() runs and
__tape_start_io() fails on the non-operational device,
__tape_3490_medium_sense() -> tape_free_request() -> tape_put_device() drops
ref_count to 0 inside tape_delayed_next_request() under spin_lock_irq().
At that point, cancel_delayed_work_sync(&device->tape_dnr) sleeps with
interrupts disabled and deadlocks waiting for the currently executing
tape_delayed_next_request() work item to finish (and if it returned,
kfree(device) would free device before tape_delayed_next_request()
dereferences device->cdev in spin_unlock_irq()).
Similarly, __tape_do_irq() -> __tape_end_request(),
tape_long_busy_timeout(), and __tape_discard_requests() also call
tape_put_device() while holding get_ccwdev_lock(device->cdev) in hardirq or
softirq context. If count == 0 is reached there, cancel_delayed_work_sync()
will sleep in atomic context and deadlock if tape_delayed_next_request() is
waiting on get_ccwdev_lock(device->cdev).
[Severity: High]
Also, is cancelling device->tape_dnr only when device->ref_count reaches 0
sufficient to prevent tape_delayed_next_request() from accessing a freed
struct ccw_device?
In tapechar_open(), an open file descriptor holds a reference to struct
tape_device in filp->private_data:
drivers/s390/char/tape_char.c:tapechar_open()
rc = tape_open(device);
if (rc == 0) {
filp->private_data = device;
stream_open(inode, filp);
} else
tape_put_device(device);
If the ccw_device is removed via tape_generic_remove() while the character
device is open and device->tape_dnr is scheduled:
drivers/s390/char/tape_core.c:tape_generic_remove()
device = dev_get_drvdata(&cdev->dev);
if (device) {
sysfs_remove_group(&cdev->dev.kobj, &tape_attr_group);
dev_set_drvdata(&cdev->dev, NULL);
tape_put_device(device);
}
tape_put_device(device) decrements device->ref_count to a non-zero value
because filp->private_data still holds a reference, so
cancel_delayed_work_sync(&device->tape_dnr) is not called during removal.
Once tape_generic_remove() returns, the driver core can release and free
cdev while device->tape_dnr remains pending. When tape_delayed_next_request()
subsequently runs before the user closes the file descriptor, won't
spin_lock_irq(get_ccwdev_lock(device->cdev)) dereference the freed
device->cdev?
Would it make more sense to cancel device->tape_dnr (and discard any queued
requests across all states) during device teardown in tape_generic_remove()
after dropping get_ccwdev_lock(device->cdev), rather than inside
tape_put_device()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019203458.1365046-1-tristmd@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread