Re: [PATCH 1/2] s390/con3215: Add __must_hold() attribute to raw3215_make_room()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Use-After-Free during device removal due to concurrent lock dropping and missing console unregistration.
- [High] Sleeping in atomic context during panic/reboot on PREEMPT_RT.
--

--- Patch [1]: [PATCH 1/2] s390/con3215: Add __must_hold() attribute to raw3215_make_room() ---
commit bdab109aec8f8a62fc73654df5f2f42b168ab432
Author: Heiko Carstens <[email protected]>

s390/con3215: Add __must_hold() attribute to raw3215_make_room()
    
Add __must_hold() attribute to raw3215_make_room() in order to let
clang's context analysis know that "get_ccwdev_lock(raw->cdev)" must be
held on function entry.

> diff --git a/drivers/s390/char/con3215.c b/drivers/s390/char/con3215.c
> index 46265f4fbfd80..57fefa8986d6b 100644
> --- a/drivers/s390/char/con3215.c
> +++ b/drivers/s390/char/con3215.c
> @@ -481,6 +481,7 @@ static unsigned int raw3215_drop(struct raw3215_info *raw)
>   */
>  static unsigned int raw3215_make_room(struct raw3215_info *raw,
>  				      unsigned int length, bool drop)
> +	__must_hold(get_ccwdev_lock(raw->cdev))
>  {
>  	while (RAW3215_BUFFER_SIZE - raw->count < length) {
>  		if (drop)

[Severity: Critical]
This is a pre-existing issue, but can this sequence lead to a use-after-free
if a concurrent device unbind occurs?

When raw3215_make_room() yields the lock and sleeps, it does so without any
reference counting:

drivers/s390/char/con3215.c:raw3215_make_room() {
    ...
		/* there might be another cpu waiting for the lock */
		spin_unlock(get_ccwdev_lock(raw->cdev));
		udelay(100);
		spin_lock(get_ccwdev_lock(raw->cdev));
    ...
}

If con3215_write() calls raw3215_make_room() and drops the lock here,
could a concurrent sysfs unbind invoke raw3215_remove(), which nullifies
raw3215[0] and calls kfree(raw) without unregistering the console or
synchronizing with the ccwlock?

When raw3215_make_room() wakes up from the udelay, it appears it would
directly dereference the freed raw pointer when calling spin_lock().

[Severity: High]
This is also a pre-existing issue, but does this spin_lock() introduce a
sleepable lock acquisition into an atomic panic context on PREEMPT_RT?

When the system panics or reboots, con3215_notify() uses
spin_trylock_irqsave() to avoid sleeping, as local IRQs are disabled.

If it then calls raw3215_make_room() and the buffer is full, the code drops
the lock and unconditionally calls spin_lock(). On PREEMPT_RT kernels,
spin_lock() maps to a sleepable rt_mutex.

Could calling this with hardware interrupts disabled violate wait-context
rules and trigger a sleeping-in-atomic bug?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1