Message-ID: <ZG/ZOP0qtG4lVrNY@ip-172-31-38-16.us-west-2.compute.internal>
Date:   Thu, 25 May 2023 21:55:04 +0000
From:   Alok Tiagi <aloktiagi@...il.com>
To:     Christian Brauner <brauner@...nel.org>
Cc:     viro@...iv.linux.org.uk, willy@...radead.org,
        David.Laight@...lab.com, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, keescook@...omium.org,
        hch@...radead.org, tycho@...ho.pizza
Subject: Re: [RFC v7 1/2] epoll: Implement eventpoll_replace_file()

On Wed, May 24, 2023 at 07:26:24PM +0200, Christian Brauner wrote:
> On Wed, May 24, 2023 at 06:39:32AM +0000, aloktiagi wrote:
> > Introduce a mechanism to replace a file linked in the epoll interface with a new
> > file.
> > 
> > eventpoll_replace() finds all instances of the file to be replaced and replaces
> > them with the new file and the interested events.h
> 
> I've spent a bit more time on this and I have a few more
> questions/thoughts.
> 
> * What if the seccomp notifier replaces a pollable file with a
>   non-pollable file? Right now you check that the file is pollable and
>   if it isn't you're not updating the file references in the epoll
>   instance for the file descriptor you updated. Why these semantics and
>   not e.g., removing all instances of that file referring to the updated
>   fd?
>

good question. the current implementation relies on __fput() calling
eventpoll_release() to ultimately release the file. eventpoll_replace_file()
only removes the file if it can successfully install the new file in epoll.
 
> * What if the seccomp notifier replaces the file of a file descriptor
>   with an epoll file descriptor? If the fd and original file are present
>   in an epoll instance does that mean you add the epoll file into all
>   epoll instances? That also means you create nested epoll instances
>   which are supported but are subject to various limitations. What's the
>   plan?
>

My plan was to allow these cases since there is support for nested epoll
instances. But thinking more on this, since seccomp subsystem is the only
caller of eventpoll_replace_file(), I am not sure whether there is a valid
use case where seccomp is used to intercept a system call that uses an
epoll fd as a parameter. Maybe its ok to not do the replacement for such
cases. Thoughts?
 
> * What if you have two threads in the same threadgroup that each have a
>   seccomp listener profile attached to them. Both have the same fd open.
> 
>   Now both replace the same fd concurrently. Both threads concurrently
>   update references in the epoll instances now since the spinlock and
>   mutex are acquired and reacquired again. Afaict, you can end up with
>   some instances of the fd temporarily generating events for file1 and
>   other instances generating events for file2 while the replace is in
>   progress. Thus generating spurious events and userspace might be
>   acting on a file descriptor that doesn't yet refer to the new file?
>   That's possibly dangerous.
> 
>   Maybe I'm mistaken but if so I'd like to hear the details why that
>   can't happen.
>

Considering file1 is the original file and file2 is the new file. First
the eventpoll_replace_file() is called before receive_fd_replace(), so
the file1 is still active and file2 would not receive any events. Within
receive_fd_replace() the install phase first installs file2 alongside file1
without removing file1. So during this phase the userspace can continue to
receive events on file1. In the remove phase within eventpoll_replace_file()
the epi for file1 is set to dying and replaced with file2. At this point the
fd should see no new events, since receive_fd_replace() is yet to be called.
 
>   Thinking about it what if the same file is registered via multiple fds
>   at the same time? Can't you end up in a scenario where you have the
>   same fd referring to different files in one or multiple epoll
>   instance?
> 
>   I mean, you can get into that situation via dup2() where you change
>   the file descriptor to refer to a different file but the fd might
>   still be registered in the epoll instance referring to the old file
>   provided there's another fd open holding the old file alive.
> 

The current implementation scopes the replacement to the fd being replaced
in the call to receive_fd_replace() since thats what the userspace intends
to do. In case there are multiple fds pointing to the same file, and 
receive_fd_replace() replaces only one of them, we would end up updating
the file for only one of the fds. The other fd will see the same result as
seen today without this patch, where the replaced file doesn't exist in 
epoll since it got cleared due to __fput().

>   The difference though is that userspace must've been dumb enough to
>   actually do that whereas now this can just happen behind their back
>   misleading them.
>

Since this is mainly serving the seccomp add fd usecase today, do you think
its something that can be documented as a limitation? I am not aware of the
interesting ways users are using seccomp add fd to think of all the possible
scenarios, so I am open to suggestions.

>   Honestly, the kernel can't give you any atomicity in replacing these
>   references and if so it would require new possibly invasive locking
>   that would very likely not be acceptable upstream just for the sake of
>   this feature. I still have a very hard time seeing any of this
>   happening.
> 
> * I haven't looked at the codepath that tries to restore the old file on
>   failure. That might introduce even more weirdness.

Powered by blists - more mailing lists