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

[Christian König]

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+5d4cb6b4409edfd18...@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=5d4cb6b4409edfd18646
Signed-off-by: Dmitry Antipov <dmanti...@yandex.ru>
---
  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;
  }