Re: [PATCH v2 2/2] s390/sclp: Ensure no callback gets called after sclp_unregister() returns

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing synchronization in sclp_register() error path leads to Use-After-Free.
- [Medium] Concurrent execution of state_change_fn and receiver_fn corrupts the completion mechanism.
--

--- Patch [2]: [PATCH v2 2/2] s390/sclp: Ensure no callback gets called after sclp_unregister() returns ---
commit 56ff5dbe692c5051e8907f572849ceb9402fdda1
Author: Alexander Egorenkov <[email protected]>

s390/sclp: Ensure no callback gets called after sclp_unregister() returns

This commit adds a completion mechanism to prevent callbacks from being 
executed after a driver unregisters from the SCLP subsystem. It blocks 
sclp_unregister() until any inflight state change or receive callbacks 
have finished executing.

> It is sufficient to use only one completion per struct sclp_register
> because the callbacks state_change_fn() and receiver_fn() never get called
> in parallel.

[Severity: Medium]
Is this assumption accurate?

Looking at sclp_dispatch_state_change() and sclp_dispatch_evbufs(), both 
functions drop sclp_lock before invoking their respective callbacks. If a 
driver registers both callbacks (such as sclp_input_event in sclp_tty.c), 
could both dispatchers execute concurrently?

If they run in parallel, it seems they would both call reinit_completion() 
and complete() on the same reg->done completion, potentially corrupting it 
and causing a premature wakeup in sclp_unregister().

> diff --git a/drivers/s390/char/sclp.c b/drivers/s390/char/sclp.c
> index 98e334724a627..8954f4260401b 100644
> --- a/drivers/s390/char/sclp.c
> +++ b/drivers/s390/char/sclp.c

[ ... ]

> @@ -885,6 +890,8 @@ sclp_register(struct sclp_register *reg)
>  	/* Trigger initial state change callback */
>  	reg->sclp_receive_mask = 0;
>  	reg->sclp_send_mask = 0;
> +	init_completion(&reg->done);
> +	complete(&reg->done);
>  	list_add(&reg->list, &sclp_reg_list);
>  	spin_unlock_irqrestore(&sclp_lock, flags);
>  	rc = sclp_init_mask(1);

[Severity: High]
If sclp_init_mask(1) fails here and returns an error, the listener is removed
from sclp_reg_list and sclp_register() returns the error to the caller.

Since the listener was briefly in the active list, could a hardware interrupt
have started executing the callback concurrently? 

If so, does this error path also need to call wait_for_completion(&reg->done)
before returning? Otherwise, it looks like a concurrently executing callback
could call complete(&reg->done) on a freed structure if the caller frees the
listener after the registration failure.

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