On Mon 10-02-25 13:01:38, Christian Brauner wrote:
> On Fri, Feb 07, 2025 at 05:42:17PM +0100, Mateusz Guzik wrote:
> > On Fri, Feb 7, 2025 at 4:50 PM Jan Kara <jack@xxxxxxx> wrote:
> > >
> > > On Fri 07-02-25 15:10:33, Christian Brauner wrote:
> > > > Before 2011 there was no meaningful synchronization between
> > > > read/readdir/write/seek. Only in commit
> > > > ef3d0fd27e90 ("vfs: do (nearly) lockless generic_file_llseek")
> > > > synchronization was added for SEEK_CUR by taking f_lock around
> > > > vfs_setpos().
> > > >
> > > > Then in 2014 full synchronization between read/readdir/write/seek was
> > > > added in commit 9c225f2655e3 ("vfs: atomic f_pos accesses as per POSIX")
> > > > by introducing f_pos_lock for regular files with FMODE_ATOMIC_POS and
> > > > for directories. At that point taking f_lock became unnecessary for such
> > > > files.
> > > >
> > > > So only acquire f_lock for SEEK_CUR if this isn't a file that would have
> > > > acquired f_pos_lock if necessary.
> > > >
> > > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> > >
> > > ...
> > >
> > > > if (whence == SEEK_CUR) {
> > > > + bool locked;
> > > > +
> > > > /*
> > > > - * f_lock protects against read/modify/write race with
> > > > - * other SEEK_CURs. Note that parallel writes and reads
> > > > - * behave like SEEK_SET.
> > > > + * If the file requires locking via f_pos_lock we know
> > > > + * that mutual exclusion for SEEK_CUR on the same file
> > > > + * is guaranteed. If the file isn't locked, we take
> > > > + * f_lock to protect against f_pos races with other
> > > > + * SEEK_CURs.
> > > > */
> > > > - guard(spinlock)(&file->f_lock);
> > > > - return vfs_setpos(file, file->f_pos + offset, maxsize);
> > > > + locked = (file->f_mode & FMODE_ATOMIC_POS) ||
> > > > + file->f_op->iterate_shared;
> > >
> > > As far as I understand the rationale this should match to
> > > file_needs_f_pos_lock() (or it can possibly be weaker) but it isn't obvious
> > > to me that's the case. After thinking about possibilities, I could convince
> > > myself that what you suggest is indeed safe but the condition being in two
> > > completely independent places and leading to subtle bugs if it gets out of
> > > sync seems a bit fragile to me.
> > >
> >
> > A debug-only assert that the lock is held when expected should sort it out?
>
> Good idea. Let me reuse the newly added infra:
> VFS_WARN_ON_ONCE(locked && !mutex_is_locked(&file->f_pos_lock));
Fine, but won't this actually trigger? file_needs_f_pos_lock() is:
return (file->f_mode & FMODE_ATOMIC_POS) &&
(file_count(file) > 1 || file->f_op->iterate_shared);
and here you have:
locked = (file->f_mode & FMODE_ATOMIC_POS) ||
file->f_op->iterate_shared;
So if (file->f_mode & FMODE_ATOMIC_POS) and (file_count(file) == 1),
file_needs_f_pos_lock() returns false but locked is true?
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
