From: Christian Brauner
> Sent: 10 May 2024 11:55
>
> > For the uapi issue you describe below my take would be that we should just
> > try, and hope that everyone's been dutifully using O_CLOEXEC. But maybe
> > I'm biased from the gpu world, where we've been hammering it in that
> > "O_CLOEXEC or
> For the uapi issue you describe below my take would be that we should just
> try, and hope that everyone's been dutifully using O_CLOEXEC. But maybe
> I'm biased from the gpu world, where we've been hammering it in that
> "O_CLOEXEC or bust" mantra since well over a decade. Really the only valid
On Thu, May 09, 2024 at 08:48:20AM -0700, Linus Torvalds wrote:
> On Thu, 9 May 2024 at 04:39, Christian Brauner wrote:
> >
> > Not worth it without someone explaining in detail why imho. First pass
> > should be to try and replace kcmp() in scenarios where it's obviously
> > not needed or
On Thu, 9 May 2024 at 04:39, Christian Brauner wrote:
>
> Not worth it without someone explaining in detail why imho. First pass
> should be to try and replace kcmp() in scenarios where it's obviously
> not needed or overkill.
Ack.
> I've added a CLASS(fd_raw) in a preliminary patch since we'll
On Wed, May 08, 2024 at 10:14:44AM -0700, Linus Torvalds wrote:
> On Wed, 8 May 2024 at 09:19, Linus Torvalds
> wrote:
> >
> > So since we already have two versions of F_DUPFD (the other being
> > F_DUPFD_CLOEXEC) I decided that the best thing to do is to just extend
> > on that existing naming
On Wed, 8 May 2024 at 09:19, Linus Torvalds
wrote:
>
> So since we already have two versions of F_DUPFD (the other being
> F_DUPFD_CLOEXEC) I decided that the best thing to do is to just extend
> on that existing naming pattern, and called it F_DUPFD_QUERY instead.
>
> I'm not married to the
On Tue, 7 May 2024 at 12:07, Linus Torvalds
wrote:
>
> That example thing shows that we shouldn't make it a FISAME ioctl - we
> should make it a fcntl() instead, and it would just be a companion to
> F_DUPFD.
>
> Doesn't that strike everybody as a *much* cleaner interface? I think
> F_ISDUP would
On Wed, May 08, 2024 at 12:08:57PM +0200, Christian Brauner wrote:
> On Mon, May 06, 2024 at 04:29:44PM +0200, Christian König wrote:
> > Am 04.05.24 um 20:20 schrieb Linus Torvalds:
> > > On Sat, 4 May 2024 at 08:32, Linus Torvalds
> > > wrote:
> > > > Lookie here, the fundamental issue is that
On Wed, May 08, 2024 at 10:32:08AM +0200, Daniel Vetter wrote:
> On Wed, May 08, 2024 at 07:55:08AM +0200, Christian König wrote:
> > Am 07.05.24 um 21:07 schrieb Linus Torvalds:
> > > On Tue, 7 May 2024 at 11:04, Daniel Vetter wrote:
> > > > On Tue, May 07, 2024 at 09:46:31AM -0700, Linus
On Mon, May 06, 2024 at 04:29:44PM +0200, Christian König wrote:
> Am 04.05.24 um 20:20 schrieb Linus Torvalds:
> > On Sat, 4 May 2024 at 08:32, Linus Torvalds
> > wrote:
> > > Lookie here, the fundamental issue is that epoll can call '->poll()'
> > > on a file descriptor that is being closed
Am 08.05.24 um 10:23 schrieb Christian Brauner:
On Tue, May 07, 2024 at 07:45:02PM +0200, Christian König wrote:
Am 07.05.24 um 18:46 schrieb Linus Torvalds:
On Tue, 7 May 2024 at 04:03, Daniel Vetter wrote:
It's really annoying that on some distros/builds we don't have that, and
for gpu
On Wed, May 08, 2024 at 07:55:08AM +0200, Christian König wrote:
> Am 07.05.24 um 21:07 schrieb Linus Torvalds:
> > On Tue, 7 May 2024 at 11:04, Daniel Vetter wrote:
> > > On Tue, May 07, 2024 at 09:46:31AM -0700, Linus Torvalds wrote:
> > >
> > > > I'd be perfectly ok with adding a generic
On Tue, May 07, 2024 at 07:45:02PM +0200, Christian König wrote:
> Am 07.05.24 um 18:46 schrieb Linus Torvalds:
> > On Tue, 7 May 2024 at 04:03, Daniel Vetter wrote:
> > > It's really annoying that on some distros/builds we don't have that, and
> > > for gpu driver stack reasons we _really_ need
On Tue, May 07, 2024 at 12:07:10PM -0700, Linus Torvalds wrote:
> On Tue, 7 May 2024 at 11:04, Daniel Vetter wrote:
> >
> > On Tue, May 07, 2024 at 09:46:31AM -0700, Linus Torvalds wrote:
> >
> > > I'd be perfectly ok with adding a generic "FISAME" VFS level ioctl
> > > too, if this is possibly a
Am 08.05.24 um 09:51 schrieb Michel Dänzer:
On 2024-05-07 19:45, Christian König wrote:
Am 07.05.24 um 18:46 schrieb Linus Torvalds:
Just what are the requirements for the GPU stack? Is one of the file
descriptors "trusted", IOW, you know what kind it is?
Because dammit, it's *so* easy to do.
On 2024-05-07 19:45, Christian König wrote:
> Am 07.05.24 um 18:46 schrieb Linus Torvalds:
>>
>> Just what are the requirements for the GPU stack? Is one of the file
>> descriptors "trusted", IOW, you know what kind it is?
>>
>> Because dammit, it's *so* easy to do. You could just add a core DRM
Am 07.05.24 um 21:07 schrieb Linus Torvalds:
On Tue, 7 May 2024 at 11:04, Daniel Vetter wrote:
On Tue, May 07, 2024 at 09:46:31AM -0700, Linus Torvalds wrote:
I'd be perfectly ok with adding a generic "FISAME" VFS level ioctl
too, if this is possibly a more common thing. and not just DRM
From: Christian Brauner
> Sent: 06 May 2024 09:45
>
> > The fact is, it's not dma-buf that is violating any rules. It's epoll.
>
> I agree that epoll() not taking a reference on the file is at least
> unexpected and contradicts the usual code patterns for the sake of
> performance and that it
On Tue, 7 May 2024 at 11:04, Daniel Vetter wrote:
>
> On Tue, May 07, 2024 at 09:46:31AM -0700, Linus Torvalds wrote:
>
> > I'd be perfectly ok with adding a generic "FISAME" VFS level ioctl
> > too, if this is possibly a more common thing. and not just DRM wants
> > it.
> >
> > Would something
On Tue, May 07, 2024 at 09:46:31AM -0700, Linus Torvalds wrote:
> On Tue, 7 May 2024 at 04:03, Daniel Vetter wrote:
> >
> > It's really annoying that on some distros/builds we don't have that, and
> > for gpu driver stack reasons we _really_ need to know whether a fd is the
> > same as another,
Am 07.05.24 um 18:46 schrieb Linus Torvalds:
On Tue, 7 May 2024 at 04:03, Daniel Vetter wrote:
It's really annoying that on some distros/builds we don't have that, and
for gpu driver stack reasons we _really_ need to know whether a fd is the
same as another, due to some messy uniqueness
On Tue, 7 May 2024 at 04:03, Daniel Vetter wrote:
>
> It's really annoying that on some distros/builds we don't have that, and
> for gpu driver stack reasons we _really_ need to know whether a fd is the
> same as another, due to some messy uniqueness requirements on buffer
> objects various
On Mon, May 06, 2024 at 04:29:44PM +0200, Christian König wrote:
> Am 04.05.24 um 20:20 schrieb Linus Torvalds:
> > On Sat, 4 May 2024 at 08:32, Linus Torvalds
> > wrote:
> > > Lookie here, the fundamental issue is that epoll can call '->poll()'
> > > on a file descriptor that is being closed
On Mon, May 06, 2024 at 04:46:54PM +0200, Christian Brauner wrote:
> On Mon, May 06, 2024 at 02:47:23PM +0200, Daniel Vetter wrote:
> > On Sun, May 05, 2024 at 01:53:48PM -0700, Linus Torvalds wrote:
> > > On Sun, 5 May 2024 at 13:30, Al Viro wrote:
> > > >
> > > > 0. special-cased ->f_count
Am 05.05.24 um 22:53 schrieb Linus Torvalds:
On Sun, 5 May 2024 at 13:30, Al Viro wrote:
0. special-cased ->f_count rule for ->poll() is a wart and it's
better to get rid of it.
1. fs/eventpoll.c is a steaming pile of shit and I'd be glad to see
git rm taken to it. Short of that,
On Mon, May 06, 2024 at 02:47:23PM +0200, Daniel Vetter wrote:
> On Sun, May 05, 2024 at 01:53:48PM -0700, Linus Torvalds wrote:
> > On Sun, 5 May 2024 at 13:30, Al Viro wrote:
> > >
> > > 0. special-cased ->f_count rule for ->poll() is a wart and it's
> > > better to get rid of it.
> > >
>
Am 04.05.24 um 20:20 schrieb Linus Torvalds:
On Sat, 4 May 2024 at 08:32, Linus Torvalds
wrote:
Lookie here, the fundamental issue is that epoll can call '->poll()'
on a file descriptor that is being closed concurrently.
Thinking some more about this, and replying to myself...
Actually, I
On Mon, May 06, 2024 at 11:27:04AM +0200, Christian Brauner wrote:
> On Mon, May 06, 2024 at 10:45:35AM +0200, Christian Brauner wrote:
> > > The fact is, it's not dma-buf that is violating any rules. It's epoll.
> >
> > I agree that epoll() not taking a reference on the file is at least
> >
On Sun, May 05, 2024 at 01:53:48PM -0700, Linus Torvalds wrote:
> On Sun, 5 May 2024 at 13:30, Al Viro wrote:
> >
> > 0. special-cased ->f_count rule for ->poll() is a wart and it's
> > better to get rid of it.
> >
> > 1. fs/eventpoll.c is a steaming pile of shit and I'd be glad to see
On Fri, May 03, 2024 at 04:41:19PM -0700, Linus Torvalds wrote:
> On Fri, 3 May 2024 at 16:23, Kees Cook wrote:
> >
> > static bool __must_check get_dma_buf_unless_doomed(struct dma_buf *dmabuf)
> > {
> > return atomic_long_inc_not_zero(>file->f_count) != 0L;
> > }
> >
> > If we end up
On Mon, May 06, 2024 at 10:45:35AM +0200, Christian Brauner wrote:
> > The fact is, it's not dma-buf that is violating any rules. It's epoll.
>
> I agree that epoll() not taking a reference on the file is at least
> unexpected and contradicts the usual code patterns for the sake of
> performance
> The fact is, it's not dma-buf that is violating any rules. It's epoll.
I agree that epoll() not taking a reference on the file is at least
unexpected and contradicts the usual code patterns for the sake of
performance and that it very likely is the case that most callers of
f_op->poll() don't
On Sun, 5 May 2024 at 13:30, Al Viro wrote:
>
> 0. special-cased ->f_count rule for ->poll() is a wart and it's
> better to get rid of it.
>
> 1. fs/eventpoll.c is a steaming pile of shit and I'd be glad to see
> git rm taken to it. Short of that, by all means, let's grab reference
>
On Sun, May 05, 2024 at 01:03:07PM -0700, Linus Torvalds wrote:
> On Sun, 5 May 2024 at 12:46, Al Viro wrote:
> >
> > I've no problem with having epoll grab a reference, but if we make that
> > a universal requirement ->poll() instances can rely upon,
>
> Al, we're note "making that a
On Sun, May 05, 2024 at 09:46:05AM -0700, Linus Torvalds wrote:
> WHY?
>
> Why cannot you and Al just admit that the problem is in epoll. Always
> has been, always will be.
Nobody (well, nobody who'd ever read epoll) argues that epoll is not
a problem.
> The fact is, it's not dma-buf that is
On Sun, 5 May 2024 at 12:46, Al Viro wrote:
>
> I've no problem with having epoll grab a reference, but if we make that
> a universal requirement ->poll() instances can rely upon,
Al, we're note "making that a requirement".
It always has been.
Otgherwise, the docs should have shouted out DAMN
On Sat, May 04, 2024 at 08:53:47AM -0700, Linus Torvalds wrote:
> poll_wait
> -> __pollwait
> -> get_file (*boom*)
>
> but the boom is very small because the poll_wait() will be undone by
> poll_freewait(), and normally poll/select has held the file count
> elevated.
Not quite. It's
On 5/3/24 3:11 PM, Linus Torvalds wrote:
> epoll is a mess, and does various invalid things in the name of
> performance.
>
> Let's try to rein it in a bit. Something like this, perhaps?
>
> Not-yet-signed-off-by: Linus Torvalds
> ---
>
> This is entirely untested, thus the
On Sun, 5 May 2024 at 03:50, Christian Brauner wrote:
>
> And I agree with you that for some instances it's valid to take another
> reference to a file from f_op->poll() but then they need to use
> get_file_active() imho and simply handle the case where f_count is zero.
I think this is
(a)
On Sat, May 04, 2024 at 08:40:25AM -0700, Linus Torvalds wrote:
> On Sat, 4 May 2024 at 08:32, Linus Torvalds
> wrote:
> >
> > Now, during this TOTALLY INNOCENT sock_poll(), in another thread, the
> > file closing completes, eventpoll_release() finishes [..]
>
> Actually, Al is right that
On Sat, 4 May 2024 at 08:32, Linus Torvalds
wrote:
>
> Lookie here, the fundamental issue is that epoll can call '->poll()'
> on a file descriptor that is being closed concurrently.
Thinking some more about this, and replying to myself...
Actually, I wonder if we could *really* fix this by
On Sat, 4 May 2024 at 08:40, Linus Torvalds
wrote:
>
> And maybe it's even *only* dma-buf that does that fget() in its
> ->poll() function. Even *then* it's not a dma-buf.c bug.
They all do in the sense that they do
poll_wait
-> __pollwait
-> get_file (*boom*)
but the boom is very
On Sat, 4 May 2024 at 08:32, Linus Torvalds
wrote:
>
> Now, during this TOTALLY INNOCENT sock_poll(), in another thread, the
> file closing completes, eventpoll_release() finishes [..]
Actually, Al is right that ep_item_poll() should be holding the
ep->mtx, so eventpoll_release() ->
On Sat, 4 May 2024 at 02:37, Christian Brauner wrote:
>
> --- 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;
>
> +
On Sat, May 04, 2024 at 12:39:00AM +0100, Al Viro wrote:
> On Fri, May 03, 2024 at 04:16:15PM -0700, Linus Torvalds wrote:
> > On Fri, 3 May 2024 at 15:07, Al Viro wrote:
> > >
> > > Suppose your program calls select() on a pipe and dmabuf, sees data to be
> > > read
> > > from pipe, reads it,
On Fri, May 03, 2024 at 02:33:37PM -0700, Linus Torvalds wrote:
> On Fri, 3 May 2024 at 14:24, Al Viro 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
On Fri, May 03, 2024 at 04:41:19PM -0700, Linus Torvalds wrote:
> On Fri, 3 May 2024 at 16:23, Kees Cook wrote:
> >
> > static bool __must_check get_dma_buf_unless_doomed(struct dma_buf *dmabuf)
> > {
> > return atomic_long_inc_not_zero(>file->f_count) != 0L;
> > }
> >
> > If we end up
On Fri, 3 May 2024 at 16:39, Al Viro wrote:
>
> *IF* those files are on purely internal filesystem, that's probably
> OK; do that with something on something mountable (char device,
> sysfs file, etc.) and you have a problem with filesystem staying
> busy.
Yeah, I agree, it's a bit annoying in
On Fri, 3 May 2024 at 16:23, Kees Cook wrote:
>
> static bool __must_check get_dma_buf_unless_doomed(struct dma_buf *dmabuf)
> {
> return atomic_long_inc_not_zero(>file->f_count) != 0L;
> }
>
> If we end up adding epi_fget(), we'll have 2 cases of using
> "atomic_long_inc_not_zero" for
On Fri, May 03, 2024 at 04:16:15PM -0700, Linus Torvalds wrote:
> On Fri, 3 May 2024 at 15:07, Al Viro wrote:
> >
> > Suppose your program calls select() on a pipe and dmabuf, sees data to be
> > read
> > from pipe, reads it, closes both pipe and dmabuf and exits.
> >
> > Would you expect that
On Sat, May 04, 2024 at 12:03:18AM +0100, Al Viro wrote:
> On Fri, May 03, 2024 at 03:46:25PM -0700, Kees Cook wrote:
> > On Fri, May 03, 2024 at 02:52:38PM -0700, Linus Torvalds wrote:
> > > That means that the file will be released - and it means that you have
> > > violated all the refcounting
On Fri, 3 May 2024 at 15:07, Al Viro wrote:
>
> Suppose your program calls select() on a pipe and dmabuf, sees data to be read
> from pipe, reads it, closes both pipe and dmabuf and exits.
>
> Would you expect that dmabuf file would stick around for hell knows how long
> after that? I would
On Fri, May 03, 2024 at 03:46:25PM -0700, Kees Cook wrote:
> On Fri, May 03, 2024 at 02:52:38PM -0700, Linus Torvalds wrote:
> > That means that the file will be released - and it means that you have
> > violated all the refcounting rules for poll().
>
> I feel like I've been looking at this too
On Fri, May 03, 2024 at 02:52:38PM -0700, Linus Torvalds wrote:
> That means that the file will be released - and it means that you have
> violated all the refcounting rules for poll().
I feel like I've been looking at this too long. I think I see another
problem here, but with dmabuf even when
On Fri, May 03, 2024 at 11:01:45PM +0100, Al Viro wrote:
> Having ->poll() instance itself grab reference is really asking for problem,
> even on the boxen that have CONFIG_EPOLL turned off. select(2) is enough;
> it will take care of references grabbed by __pollwait(), but it doesn't
> know
On Fri, May 03, 2024 at 02:52:38PM -0700, Linus Torvalds wrote:
> On Fri, 3 May 2024 at 14:45, Al Viro wrote:
> >
> > How do you get through eventpoll_release_file() while someone
> > has entered ep_item_poll()?
>
> Doesn't matter.
>
> Look, it's enough that the file count has gone down to
On Fri, 3 May 2024 at 14:45, Al Viro wrote:
>
> How do you get through eventpoll_release_file() while someone
> has entered ep_item_poll()?
Doesn't matter.
Look, it's enough that the file count has gone down to zero. You may
not even have gotten to eventpoll_release_file() yet - the important
On Fri, May 03, 2024 at 02:33:37PM -0700, Linus Torvalds wrote:
> Look at the hack in __ep_remove(): if it is concurrent with
> eventpoll_release_file(), it will hit this code
>
> spin_lock(>f_lock);
> if (epi->dying && !force) {
> spin_unlock(>f_lock);
>
On Fri, 3 May 2024 at 14:24, Al Viro 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(),
On Fri, May 03, 2024 at 02:11:30PM -0700, Linus Torvalds wrote:
> epoll is a mess, and does various invalid things in the name of
> performance.
>
> Let's try to rein it in a bit. Something like this, perhaps?
> +/*
> + * The ffd.file pointer may be in the process of
> + * being torn down due to
epoll is a mess, and does various invalid things in the name of
performance.
Let's try to rein it in a bit. Something like this, perhaps?
Not-yet-signed-off-by: Linus Torvalds
---
This is entirely untested, thus the "Not-yet-signed-off-by". But I
think this may be kind of the right path
61 matches
Mail list logo
