Re: [PATCH] [RFC] dma-buf: fix race condition between poll and close

[Christian König <christian.koenig@xxxxxxx>]

Am 23.04.24 um 21:13 schrieb Dmitry Antipov:
> Syzbot has found the race condition where 'fput()' is in progress
> when 'dma_buf_poll()' makes an attempt to hold the 'struct file'
> with zero 'f_count'. So use explicit 'atomic_long_inc_not_zero()'
> to detect such a case and cancel an undergoing poll activity with
> EPOLLERR.

Well this is really interesting, you are the second person which comes 
up with this nonsense.

To repeat what I already said on the other thread: Calling 
dma_buf_poll() while fput() is in progress is illegal in the first place.

So there is nothing to fix in dma_buf_poll(), but rather to figure out 
who is incorrectly calling fput().

Regards,
Christian.

> Reported-by: syzbot+5d4cb6b4409edfd18646@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=5d4cb6b4409edfd18646
> Signed-off-by: Dmitry Antipov <dmantipov@xxxxxxxxx>
> ---
>   drivers/dma-buf/dma-buf.c | 23 ++++++++++++++++++-----
>   1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 8fe5aa67b167..39eb75d23219 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -266,8 +266,17 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
>   		spin_unlock_irq(&dmabuf->poll.lock);
>   
>   		if (events & EPOLLOUT) {
> -			/* Paired with fput in dma_buf_poll_cb */
> -			get_file(dmabuf->file);
> +			/*
> +			 * Catch the case when fput() is in progress
> +			 * (e.g. due to close() from another thread).
> +			 * Otherwise the paired fput() will be issued
> +			 * from dma_buf_poll_cb().
> +			 */
> +			if (unlikely(!atomic_long_inc_not_zero(&file->f_count))) {
> +				events = EPOLLERR;
> +				dcb->active = 0;
> +				goto out;
> +			}
>   
>   			if (!dma_buf_poll_add_cb(resv, true, dcb))
>   				/* No callback queued, wake up any other waiters */
> @@ -289,8 +298,12 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
>   		spin_unlock_irq(&dmabuf->poll.lock);
>   
>   		if (events & EPOLLIN) {
> -			/* Paired with fput in dma_buf_poll_cb */
> -			get_file(dmabuf->file);
> +			/* See above */
> +			if (unlikely(!atomic_long_inc_not_zero(&file->f_count))) {
> +				events = EPOLLERR;
> +				dcb->active = 0;
> +				goto out;
> +			}
>   
>   			if (!dma_buf_poll_add_cb(resv, false, dcb))
>   				/* No callback queued, wake up any other waiters */
> @@ -299,7 +312,7 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
>   				events &= ~EPOLLIN;
>   		}
>   	}
> -
> +out:
>   	dma_resv_unlock(resv);
>   	return events;
>   }