On Fri, Sep 11, 2020 at 11:28 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
> The current scheme stashes away ->ring_fd and ->ring_file, and uses
> that to check against whether or not ->files could have changed. This
> works, but doesn't work so well for SQPOLL. If the application does
> close the ring_fd, then we require that applications enter the kernel
> to refresh our state.
I don't understand the intent; please describe the scenario this is
trying to fix. Is this something about applications that call dup()
and close() on the uring fd, or something like that?
> Add an atomic sequence for the ->flush() count on the ring fd, and if
> we get a mismatch between checking this sequence before and after
> grabbing the ->files, then we fail the request.
Is this expected to actually be possible during benign usage?
> This should offer the same protection that we currently have, with the
> added benefit of being able to update the ->files automatically.
Please clarify what "update the ->files" is about.
> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
> ---
> fs/io_uring.c | 137 ++++++++++++++++++++++++++++++--------------------
> 1 file changed, 83 insertions(+), 54 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 4958a9dca51a..49be5e21f166 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -308,8 +308,11 @@ struct io_ring_ctx {
> */
> struct fixed_file_data *file_data;
> unsigned nr_user_files;
> - int ring_fd;
> - struct file *ring_file;
> +
> + /* incremented when ->flush() is called */
> + atomic_t files_seq;
If this ends up landing, all of this should probably use 64-bit types
(atomic64_t and s64). 32-bit counters in fast syscalls can typically
be wrapped around in a reasonable amount of time. (For example, the
VMA cache sequence number wraparound issue
<https://googleprojectzero.blogspot.com/2018/09/a-cache-invalidation-bug-in-linux.html>
could be triggered in about an hour according to my blogpost from back
then. For this sequence number, it should be significantly faster, I
think.)
(I haven't properly looked at the rest of this patch so far - I stared
at it for a bit, but wasn't able to immediately figure out what's
actually going on. So I figured I'd ask the more fundamental questions
first.)
