public inbox for [email protected]
 help / color / mirror / Atom feed
From: Christian Brauner <[email protected]>
To: Linus Torvalds <[email protected]>
Cc: Al Viro <[email protected]>,
	[email protected],  [email protected],
	[email protected], [email protected],
	 [email protected], [email protected], [email protected],
	[email protected],  [email protected],
	[email protected], [email protected],
	 [email protected], [email protected],
	 [email protected],
	[email protected]
Subject: Re: [PATCH] epoll: try to be a _bit_ better about file lifetimes
Date: Sat, 4 May 2024 11:37:31 +0200	[thread overview]
Message-ID: <20240504-wohngebiet-restwert-6c3c94fddbdd@brauner> (raw)
In-Reply-To: <CAHk-=wjpsTEkHgo1uev3xGJ2bQXYShaRf3GPEqDWNgUuKx0JFw@mail.gmail.com>

On Fri, May 03, 2024 at 02:33:37PM -0700, Linus Torvalds wrote:
> On Fri, 3 May 2024 at 14:24, Al Viro <[email protected]> wrote:
> >
> > Can we get to ep_item_poll(epi, ...) after eventpoll_release_file()
> > got past __ep_remove()?  Because if we can, we have a worse problem -
> > epi freed under us.
> 
> Look at the hack in __ep_remove(): if it is concurrent with
> eventpoll_release_file(), it will hit this code
> 
>         spin_lock(&file->f_lock);
>         if (epi->dying && !force) {
>                 spin_unlock(&file->f_lock);
>                 return false;
>         }
> 
> and not free the epi.
> 
> But as far as I can tell, almost nothing else cares about the f_lock
> and dying logic.
> 
> And in fact, I don't think doing
> 
>         spin_lock(&file->f_lock);
> 
> is even valid in the places that look up file through "epi->ffd.file",
> because the lock itself is inside the thing that you can't trust until
> you've taken the lock...
> 
> So I agree with Kees about the use of "atomic_dec_not_zero()" kind of
> logic - but it also needs to be in an RCU-readlocked region, I think.

Why isn't it enough to just force dma_buf_poll() to use
get_file_active()? Then that whole problem goes away afaict.

So the fix I had yesterday before I had to step away from the computer
was literally just that [1]. It currently uses two atomic incs
potentially but that can probably be fixed by the dma folks to be
smarter about when they actually need to take a file reference.

> 
> I wish epoll() just took the damn file ref itself. But since it relies
> on the file refcount to release the data structure, that obviously
> can't work.
> 
>                 Linus

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 8fe5aa67b167..7149c45976e1 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -244,13 +244,18 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
        if (!dmabuf || !dmabuf->resv)
                return EPOLLERR;

+       if (!get_file_active(&dmabuf->file))
+               return EPOLLERR;
+
        resv = dmabuf->resv;

        poll_wait(file, &dmabuf->poll, poll);

        events = poll_requested_events(poll) & (EPOLLIN | EPOLLOUT);
-       if (!events)
+       if (!events) {
+               fput(file);
                return 0;
+       }

        dma_resv_lock(resv, NULL);

@@ -268,7 +273,6 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
                if (events & EPOLLOUT) {
                        /* Paired with fput in dma_buf_poll_cb */
                        get_file(dmabuf->file);
-
                        if (!dma_buf_poll_add_cb(resv, true, dcb))
                                /* No callback queued, wake up any other waiters */
                                dma_buf_poll_cb(NULL, &dcb->cb);
@@ -301,6 +305,7 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll)
        }

        dma_resv_unlock(resv);
+       fput(file);
        return events;
 }

  parent reply	other threads:[~2024-05-04  9:37 UTC|newest]

Thread overview: 87+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-08  8:26 [syzbot] [fs?] [io-uring?] general protection fault in __ep_remove syzbot
2024-04-15 14:31 ` Jens Axboe
2024-04-15 14:57   ` Pavel Begunkov
2024-05-03 11:54 ` Bui Quang Minh
2024-05-03 18:26   ` get_file() unsafe under epoll (was Re: [syzbot] [fs?] [io-uring?] general protection fault in __ep_remove) Kees Cook
2024-05-03 18:49     ` Jens Axboe
2024-05-03 19:22       ` Kees Cook
2024-05-03 19:35         ` Jens Axboe
2024-05-03 19:59           ` Kees Cook
2024-05-03 20:28             ` Kees Cook
2024-05-03 21:11               ` Al Viro
2024-05-03 21:24                 ` Linus Torvalds
2024-05-03 21:30                   ` Al Viro
2024-05-06 17:46                   ` Stefan Metzmacher
2024-05-06 18:17                     ` Linus Torvalds
2024-05-08  8:47                       ` David Laight
2024-05-03 21:36                 ` Al Viro
2024-05-03 21:42                   ` Linus Torvalds
2024-05-03 21:53                     ` Al Viro
2024-05-06 12:23                       ` Daniel Vetter
2024-05-04  9:59             ` Christian Brauner
2024-05-03 21:11     ` [PATCH] epoll: try to be a _bit_ better about file lifetimes Linus Torvalds
2024-05-03 21:24       ` Al Viro
2024-05-03 21:33         ` Linus Torvalds
2024-05-03 21:45           ` Al Viro
2024-05-03 21:52             ` Linus Torvalds
2024-05-03 22:01               ` Al Viro
2024-05-03 22:07                 ` Al Viro
2024-05-03 23:16                   ` Linus Torvalds
2024-05-03 23:39                     ` Al Viro
2024-05-03 23:54                       ` Linus Torvalds
2024-05-04 10:44                       ` Christian Brauner
2024-05-03 22:46               ` Kees Cook
2024-05-03 23:03                 ` Al Viro
2024-05-03 23:23                   ` Kees Cook
2024-05-03 23:41                     ` Linus Torvalds
2024-05-04  9:19                       ` Christian Brauner
2024-05-06 12:37                       ` Daniel Vetter
2024-05-04  9:37           ` Christian Brauner [this message]
2024-05-04 15:32             ` Linus Torvalds
2024-05-04 15:40               ` Linus Torvalds
2024-05-04 15:53                 ` Linus Torvalds
2024-05-05 19:46                   ` Al Viro
2024-05-05 20:03                     ` Linus Torvalds
2024-05-05 20:30                       ` Al Viro
2024-05-05 20:53                         ` Linus Torvalds
2024-05-06 12:47                           ` Daniel Vetter
2024-05-06 14:46                             ` Christian Brauner
2024-05-07 10:58                               ` Daniel Vetter
2024-05-06 16:15                           ` Christian König
2024-05-05 10:50                 ` Christian Brauner
2024-05-05 16:46                   ` Linus Torvalds
2024-05-05 17:55                     ` [PATCH v2] epoll: be " Linus Torvalds
2024-05-05 18:04                       ` Jens Axboe
2024-05-05 20:01                       ` David Laight
2024-05-05 20:16                         ` Linus Torvalds
2024-05-05 20:12                     ` [PATCH] epoll: try to be a _bit_ " Al Viro
2024-05-06  8:45                     ` Christian Brauner
2024-05-06  9:26                       ` Christian Brauner
2024-05-06 14:19                         ` Christian Brauner
2024-05-07 21:02                       ` David Laight
2024-05-04 18:20               ` Linus Torvalds
2024-05-06 14:29                 ` [Linaro-mm-sig] " Christian König
2024-05-07 11:02                   ` Daniel Vetter
2024-05-07 16:46                     ` Linus Torvalds
2024-05-07 17:45                       ` Christian König
2024-05-08  7:51                         ` Michel Dänzer
2024-05-08  7:59                           ` Christian König
2024-05-08  8:23                         ` Christian Brauner
2024-05-08  9:10                           ` Christian König
2024-05-07 18:04                       ` Daniel Vetter
2024-05-07 19:07                         ` Linus Torvalds
2024-05-08  5:55                           ` Christian König
2024-05-08  8:32                             ` Daniel Vetter
2024-05-08 10:16                               ` Christian Brauner
2024-05-08  8:05                           ` Christian Brauner
2024-05-08 16:19                           ` Linus Torvalds
2024-05-08 17:14                             ` Linus Torvalds
2024-05-09 11:38                               ` Christian Brauner
2024-05-09 15:48                                 ` Linus Torvalds
2024-05-10  6:33                                   ` Christian Brauner
2024-05-08 10:08                   ` Christian Brauner
2024-05-08 15:45                     ` Daniel Vetter
2024-05-10 10:55                       ` Christian Brauner
2024-05-11 18:25                         ` David Laight
2024-05-05 17:31       ` Jens Axboe
2024-05-04  9:45     ` get_file() unsafe under epoll (was Re: [syzbot] [fs?] [io-uring?] general protection fault in __ep_remove) Christian Brauner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240504-wohngebiet-restwert-6c3c94fddbdd@brauner \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox