Re: [PATCH] s390/tape: cancel delayed work before freeing device

[sashiko-bot@kernel.org]

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