Re: [PATCH] sound/core/seq : fix data-race in snd_seq_fifo_cell_out/snd_seq_fifo_poll_wait

[Takashi Iwai <tiwai@suse.de>]

On Tue, 16 Sep 2025 12:45:48 +0200,
hariconscious@gmail.com wrote:
> 
> From: HariKrishna Sagala <hariconscious@gmail.com>
> 
> data race in both the functions, snd_seq_fifo_cell_out &
> snd_seq_fifo_poll_wait is protected with guards

Please don't, this will lead obviously to a deadlock!
fifo_cell_out() is already protected by f->lock spinlock.
You can at most protect the f->cells reference in
snd_seq_fifo_poll_wait().  Or change f->celss to atomic_t.

But, practically seen, it won't fix any real issue.  The evaluation
there can be racy in anyway.

Also the patch doesn't follow the coding style.
I sincerely hope you verify your patch whether it really works before
submitting at the next time.


thanks,

Takashi

> 
> Reported-by: syzbot+c3dbc239259940ededba@syzkaller.appspotmail.com
> Fixes: https://syzkaller.appspot.com/bug?extid=c3dbc239259940ededba
> Signed-off-by: HariKrishna Sagala <hariconscious@gmail.com>
> ---
>  sound/core/seq/seq_fifo.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/core/seq/seq_fifo.c b/sound/core/seq/seq_fifo.c
> index f23c6b7ae240..65e28ebb0eb1 100644
> --- a/sound/core/seq/seq_fifo.c
> +++ b/sound/core/seq/seq_fifo.c
> @@ -138,16 +138,18 @@ static struct snd_seq_event_cell *fifo_cell_out(struct snd_seq_fifo *f)
>  {
>  	struct snd_seq_event_cell *cell;
>  
> -	cell = f->head;
> -	if (cell) {
> -		f->head = cell->next;
> +	scoped_guard(spinlock_irqsave, &f->lock) {
> +		cell = f->head;
> +		if (cell) {
> +			f->head = cell->next;
>  
> -		/* reset tail if this was the last element */
> -		if (f->tail == cell)
> -			f->tail = NULL;
> +			/* reset tail if this was the last element */
> +			if (f->tail == cell)
> +				f->tail = NULL;
>  
> -		cell->next = NULL;
> -		f->cells--;
> +			cell->next = NULL;
> +			f->cells--;
> +		}
>  	}
>  
>  	return cell;
> @@ -210,7 +212,9 @@ int snd_seq_fifo_poll_wait(struct snd_seq_fifo *f, struct file *file,
>  			   poll_table *wait)
>  {
>  	poll_wait(file, &f->input_sleep, wait);
> -	return (f->cells > 0);
> +	guard(spinlock_irqsave)(&f->lock);
> +	int isNonzero = (f->cells > 0);
> +	return isNonzero;
>  }
>  
>  /* change the size of pool; all old events are removed */
> -- 
> 2.43.0
>