Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 25 May 2023 06:36:17 +0900
From:   Dominique Martinet <asmadeus@codewreck.org>
To:     Christian Brauner <brauner@kernel.org>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Jens Axboe <axboe@kernel.dk>,
        Pavel Begunkov <asml.silence@gmail.com>,
        Stefan Roesch <shr@fb.com>, Clay Harris <bugs@claycon.org>,
        Dave Chinner <david@fromorbit.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        io-uring@vger.kernel.org
Subject: Re: [PATCH v2 1/6] fs: split off vfs_getdents function of getdents64
 syscall
Message-ID: <ZG6DUfdbTHS-e5P7@codewreck.org>
References: <ZG0slV2BhSZkRL_y@codewreck.org>
 <ZG0qgniV1DzIbbzi@codewreck.org>
 <20230524-monolog-punkband-4ed95d8ea852@brauner>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230524-monolog-punkband-4ed95d8ea852@brauner>
Precedence: bulk

Christian Brauner wrote on Wed, May 24, 2023 at 03:52:45PM +0200:
> The main objection in [1] was to allow specifying an arbitrary offset
> from userspace. What [3] did was to implement a pread() variant for
> directories, i.e., pgetdents(). That can't work in principle/is
> prohibitively complex. Which is what your series avoids by not allowing
> any offsets to be specified.

Yes.

> However, there's still a problem here. Updates to f_pos happen under an
> approriate lock to guarantee consistency of the position between calls
> that move the cursor position. In the normal read-write path io_uring
> doesn't concern itself with f_pos as it keeps local state in
> kiocb->ki_pos.
> 
> But then it still does end up running into f_pos consistency problems
> for read-write because it does allow operating on the current f_pos if
> the offset if struct io_rw is set to -1.
> 
> In that case it does retrieve and update f_pos which should take
> f_pos_lock and a patchset for this was posted but it didn't go anywhere.
> It should probably hold that lock. See Jann's comments in the other
> thread how that currently can lead to issues.

Assuming that is this mail:
https://lore.kernel.org/io-uring/CAG48ez1O9VxSuWuLXBjke23YxUA8EhMP+6RCHo5PNQBf3B0pDQ@mail.gmail.com/

So, ok, I didn't realize fdget_pos() actually acted as a lock on the
file's f_pos_lock (apparently only if FMODE_ATMOIC_POS is set? but it
looks set on most if not all directories through finish_open(), that
looks called consistently enough)

What was confusing is that default_llseek updates f_pos under the
inode_lock (write), and getdents also takes that lock (for read only in
shared implem), so I assumed getdents also was just protected by this
read lock, but I guess that was a bad assumption (as I kept pointing
out, a shared read lock isn't good enough, we definitely agree there)


In practice, in the non-registered file case io_uring is also calling
fdget, so the lock is held exactly the same as the syscall and I wasn't
so far off -- but we need to figure something for the registered file
case.
openat variants don't allow working with the registered variant at all
on the parent fd, so as far as I'm concerned I'd be happy setting the
same limitation for getdents: just say it acts on fd and not files, and
call it a day...
It'd also be possible to check if REQ_F_FIXED_FILE flag was set and
manually take the lock somehow but we don't have any primitive that
takes f_pos_lock from a file (the only place that takes it is fdget
which requires a fd), so I'd rather not add such a new exception.
I assume the other patch you mentioned about adding that lock was this
one:
https://lore.kernel.org/all/20220222105504.3331010-1-dylany@fb.com/T/#m3609dc8057d0bc8e41ceab643e4d630f7b91bde6
and it just atkes the lock, but __fdget_pos also checks for
FMODE_ATOMIC_OPS and file_count and I'm not sure I understand how it
sets (f.flags & FDPUT_POS_UNLOCK) (for fdput_pos) so I'd rather not add
such a code path at this point..


So, ok, what do you think about just forbidding registered files?
I can't see where that wouldn't suffice but I might be missing something
else.

> For getdents() not protecting f_pos is equally bad or worse. The patch
> doesn't hold f_pos_lock and just updates f_pos prior _and_ post
> iterate_dir() arguing that this race is fine. But again, f_version and
> f_pos are consistent after each system call invocation.
> 
> But without that you can have a concurrent seek running and can end up
> with an inconsistent f_pos position within the same system call. IOW,
> you're breaking f_pos being in a well-known state. And you're not doing
> that just for io_uring you're doing it for the regular system call
> interface as well as both can be used on the same fd simultaneously.
> So that's a no go imho.

(so seek is fine, but I agree two concurrent getdents on registered
files won't have the required lock)

-- 
Dominique Martinet | Asmadeus