The assert in function file_seek_cur_needs_f_lock() can be triggered very
easily because, as Jan Kara suggested, the file reference may get
incremented after checking it with fdget_pos().

Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
Signed-off-by: Luis Henriques <luis@igalia.com>
---
Hi Christian,

It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
see it on vfs.git, I decided to explicitly send the patch so that it doesn't
slip through the cracks.

Cheers,
-- 
Luis

 fs/file.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 3a3146664cf3..075f07bdc977 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
 	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
 		return false;
 
-	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
-			 !mutex_is_locked(&file->f_pos_lock));
 	return true;
 }

On Thu, Jun 12, 2025 at 10:41:01AM +0100, Luis Henriques wrote:
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because, as Jan Kara suggested, the file reference may get
> incremented after checking it with fdget_pos().
> 
> Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
> Hi Christian,
> 
> It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
> see it on vfs.git, I decided to explicitly send the patch so that it doesn't
> slip through the cracks.
> 
> Cheers,
> -- 
> Luis
> 
>  fs/file.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 3a3146664cf3..075f07bdc977 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
>  	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
>  		return false;
>  
> -	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> -			 !mutex_is_locked(&file->f_pos_lock));
>  	return true;
>  }
>  

There this justifies the change.

fdget_pos() can only legally skip locking if it determines to be in
position where nobody else can operate on the same file obj, meaning
file_count(file) == 1 and it can't go up. Otherwise the lock is taken.

Or to put it differently, fdget_pos() NOT taking the lock and new refs
showing up later is a bug.

I don't believe anything of the sort is happening here.

Instead, overlayfs is playing games and *NOT* going through fdget_pos():

	ovl_inode_lock(inode);
        realfile = ovl_real_file(file);
	[..]
        ret = vfs_llseek(realfile, offset, whence);

Given the custom inode locking around the call, it may be any other
locking is unnecessary and the code happens to be correct despite the
splat.

I think the safest way out with some future-proofing is to in fact *add*
the locking in ovl_llseek() to shut up the assert -- personally I find
it uneasy there is some underlying file obj flying around.

Even if ultimately the assert has to go, the proposed commit message
does not justify it.

On Thu 12-06-25 15:55:40, Mateusz Guzik wrote:
> On Thu, Jun 12, 2025 at 10:41:01AM +0100, Luis Henriques wrote:
> > The assert in function file_seek_cur_needs_f_lock() can be triggered very
> > easily because, as Jan Kara suggested, the file reference may get
> > incremented after checking it with fdget_pos().
> > 
> > Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
> > Signed-off-by: Luis Henriques <luis@igalia.com>
> > ---
> > Hi Christian,
> > 
> > It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
> > see it on vfs.git, I decided to explicitly send the patch so that it doesn't
> > slip through the cracks.
> > 
> > Cheers,
> > -- 
> > Luis
> > 
> >  fs/file.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/fs/file.c b/fs/file.c
> > index 3a3146664cf3..075f07bdc977 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
> >  	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
> >  		return false;
> >  
> > -	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> > -			 !mutex_is_locked(&file->f_pos_lock));
> >  	return true;
> >  }
> 
> fdget_pos() can only legally skip locking if it determines to be in
> position where nobody else can operate on the same file obj, meaning
> file_count(file) == 1 and it can't go up. Otherwise the lock is taken.
> 
> Or to put it differently, fdget_pos() NOT taking the lock and new refs
> showing up later is a bug.

I mostly agree and as I've checked again, this indeed seems to be the case
as fdget() will increment f_ref if file table is shared with another thread
and thus file_needs_f_pos_lock() returns true whenever there are more
threads sharing the file table or if the struct file is dupped to another
fd. That being said I find the assertion in file_seek_cur_needs_f_lock()
misplaced - it just doesn't make sense in that place to me.
 
> I don't believe anything of the sort is happening here.
> 
> Instead, overlayfs is playing games and *NOT* going through fdget_pos():
> 
> 	ovl_inode_lock(inode);
>         realfile = ovl_real_file(file);
> 	[..]
>         ret = vfs_llseek(realfile, offset, whence);
> 
> Given the custom inode locking around the call, it may be any other
> locking is unnecessary and the code happens to be correct despite the
> splat.

Right and good spotting. That's indeed more likely explanation than mine.
Actually custom locking around llseek isn't all that uncommon (mostly for
historical reasons AFAIK but that's another story).

> I think the safest way out with some future-proofing is to in fact *add*
> the locking in ovl_llseek() to shut up the assert -- personally I find
> it uneasy there is some underlying file obj flying around.

Well, if you grep for vfs_llseek(), you'll see there are much more calls to
it in the kernel than overlayfs. These callers outside of fs/read_write.c
are responsible for their locking. So I don't think keeping the assert in
file_seek_cur_needs_f_lock() makes any sense. If anything I'd be open to
putting it in fdput_pos() or something like that.

> Even if ultimately the assert has to go, the proposed commit message
> does not justify it.

I guess the commit message could be improved. Something like:

The assert in function file_seek_cur_needs_f_lock() can be triggered very
easily because there are many users of vfs_llseek() (such as overlayfs)
that do their custom locking around llseek instead of relying on
fdget_pos(). Just drop the overzealous assertion.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

On Thu, Jun 12 2025, Jan Kara wrote:

> On Thu 12-06-25 15:55:40, Mateusz Guzik wrote:
>> On Thu, Jun 12, 2025 at 10:41:01AM +0100, Luis Henriques wrote:
>> > The assert in function file_seek_cur_needs_f_lock() can be triggered very
>> > easily because, as Jan Kara suggested, the file reference may get
>> > incremented after checking it with fdget_pos().
>> > 
>> > Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
>> > Signed-off-by: Luis Henriques <luis@igalia.com>
>> > ---
>> > Hi Christian,
>> > 
>> > It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
>> > see it on vfs.git, I decided to explicitly send the patch so that it doesn't
>> > slip through the cracks.
>> > 
>> > Cheers,
>> > -- 
>> > Luis
>> > 
>> >  fs/file.c | 2 --
>> >  1 file changed, 2 deletions(-)
>> > 
>> > diff --git a/fs/file.c b/fs/file.c
>> > index 3a3146664cf3..075f07bdc977 100644
>> > --- a/fs/file.c
>> > +++ b/fs/file.c
>> > @@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
>> >  	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
>> >  		return false;
>> >  
>> > -	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
>> > -			 !mutex_is_locked(&file->f_pos_lock));
>> >  	return true;
>> >  }
>> 
>> fdget_pos() can only legally skip locking if it determines to be in
>> position where nobody else can operate on the same file obj, meaning
>> file_count(file) == 1 and it can't go up. Otherwise the lock is taken.
>> 
>> Or to put it differently, fdget_pos() NOT taking the lock and new refs
>> showing up later is a bug.
>
> I mostly agree and as I've checked again, this indeed seems to be the case
> as fdget() will increment f_ref if file table is shared with another thread
> and thus file_needs_f_pos_lock() returns true whenever there are more
> threads sharing the file table or if the struct file is dupped to another
> fd. That being said I find the assertion in file_seek_cur_needs_f_lock()
> misplaced - it just doesn't make sense in that place to me.
>  
>> I don't believe anything of the sort is happening here.
>> 
>> Instead, overlayfs is playing games and *NOT* going through fdget_pos():
>> 
>> 	ovl_inode_lock(inode);
>>         realfile = ovl_real_file(file);
>> 	[..]
>>         ret = vfs_llseek(realfile, offset, whence);
>> 
>> Given the custom inode locking around the call, it may be any other
>> locking is unnecessary and the code happens to be correct despite the
>> splat.
>
> Right and good spotting. That's indeed more likely explanation than mine.
> Actually custom locking around llseek isn't all that uncommon (mostly for
> historical reasons AFAIK but that's another story).
>
>> I think the safest way out with some future-proofing is to in fact *add*
>> the locking in ovl_llseek() to shut up the assert -- personally I find
>> it uneasy there is some underlying file obj flying around.
>
> Well, if you grep for vfs_llseek(), you'll see there are much more calls to
> it in the kernel than overlayfs. These callers outside of fs/read_write.c
> are responsible for their locking. So I don't think keeping the assert in
> file_seek_cur_needs_f_lock() makes any sense. If anything I'd be open to
> putting it in fdput_pos() or something like that.

Thank you Mateusz and Honza for looking into this.  Overlayfs was indeed
my initial suspect, but I had two reasons for thinking that the assert was
the problem: 1) that code was there for quite some time and 2) nobody else
was reporting this issue.

>> Even if ultimately the assert has to go, the proposed commit message
>> does not justify it.
>
> I guess the commit message could be improved. Something like:
>
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because there are many users of vfs_llseek() (such as overlayfs)
> that do their custom locking around llseek instead of relying on
> fdget_pos(). Just drop the overzealous assertion.

Thanks, makes more sense.

Christian, do you prefer me to resend the patch or is it easier for you to
just amend the commit?  (Though, to be fair, the authorship could also be
changed as I mostly reported the issue and tested!)

Cheers,
-- 
Luís

On Thu, Jun 12, 2025 at 8:07 PM Luis Henriques <luis@igalia.com> wrote:
> > I guess the commit message could be improved. Something like:
> >
> > The assert in function file_seek_cur_needs_f_lock() can be triggered very
> > easily because there are many users of vfs_llseek() (such as overlayfs)
> > that do their custom locking around llseek instead of relying on
> > fdget_pos(). Just drop the overzealous assertion.
>
> Thanks, makes more sense.
>
> Christian, do you prefer me to resend the patch or is it easier for you to
> just amend the commit?  (Though, to be fair, the authorship could also be
> changed as I mostly reported the issue and tested!)
>

How about leaving a trace in the code.

For example a comment of this sort in place of the assert:
Note that we are not guaranteed to be called after fdget_pos() on this
file obj, in which case the caller is expected to provide the
appropriate locking.

I find it fishy af that a rando fs is playing games with the file obj
*and* the fact that games are played is not assertable, but at least
people can be warned.
-- 
Mateusz Guzik <mjguzik gmail.com>

On Thu 12-06-25 23:35:09, Mateusz Guzik wrote:
> On Thu, Jun 12, 2025 at 8:07 PM Luis Henriques <luis@igalia.com> wrote:
> > > I guess the commit message could be improved. Something like:
> > >
> > > The assert in function file_seek_cur_needs_f_lock() can be triggered very
> > > easily because there are many users of vfs_llseek() (such as overlayfs)
> > > that do their custom locking around llseek instead of relying on
> > > fdget_pos(). Just drop the overzealous assertion.
> >
> > Thanks, makes more sense.
> >
> > Christian, do you prefer me to resend the patch or is it easier for you to
> > just amend the commit?  (Though, to be fair, the authorship could also be
> > changed as I mostly reported the issue and tested!)
> >
> 
> How about leaving a trace in the code.
> 
> For example a comment of this sort in place of the assert:
> Note that we are not guaranteed to be called after fdget_pos() on this
> file obj, in which case the caller is expected to provide the
> appropriate locking.

I'm not opposed to that.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

The assert in function file_seek_cur_needs_f_lock() can be triggered very
easily because there are many users of vfs_llseek() (such as overlayfs)
that do their custom locking around llseek instead of relying on
fdget_pos(). Just drop the overzealous assertion.

Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
Suggested-by: Jan Kara <jack@suse.cz>
Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
Signed-off-by: Luis Henriques <luis@igalia.com>
---
Hi!

As suggested by Mateusz, I'm adding a comment (also suggested by him!) to
replace the assertion.  I'm also adding the 'Suggested-by:' tags, although
I'm not sure it's the correct tag to use -- the authorship of this patch
isn't really clear at this point :-)

 fs/file.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 3a3146664cf3..b6db031545e6 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1198,8 +1198,12 @@ bool file_seek_cur_needs_f_lock(struct file *file)
 	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
 		return false;
 
-	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
-			 !mutex_is_locked(&file->f_pos_lock));
+	/*
+	 * Note that we are not guaranteed to be called after fdget_pos() on
+	 * this file obj, in which case the caller is expected to provide the
+	 * appropriate locking.
+	 */
+
 	return true;
 }

On Fri, 13 Jun 2025 11:11:11 +0100, Luis Henriques wrote:
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because there are many users of vfs_llseek() (such as overlayfs)
> that do their custom locking around llseek instead of relying on
> fdget_pos(). Just drop the overzealous assertion.
> 
> 

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fs: drop assert in file_seek_cur_needs_f_lock
      https://git.kernel.org/vfs/vfs/c/dd2d6b7f6f51

On Fri, Jun 13, 2025 at 12:11 PM Luis Henriques <luis@igalia.com> wrote:
>
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because there are many users of vfs_llseek() (such as overlayfs)
> that do their custom locking around llseek instead of relying on
> fdget_pos(). Just drop the overzealous assertion.
>
> Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
> Suggested-by: Jan Kara <jack@suse.cz>
> Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
> Hi!
>
> As suggested by Mateusz, I'm adding a comment (also suggested by him!) to
> replace the assertion.  I'm also adding the 'Suggested-by:' tags, although
> I'm not sure it's the correct tag to use -- the authorship of this patch
> isn't really clear at this point :-)
>
>  fs/file.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/file.c b/fs/file.c
> index 3a3146664cf3..b6db031545e6 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1198,8 +1198,12 @@ bool file_seek_cur_needs_f_lock(struct file *file)
>         if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
>                 return false;
>
> -       VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> -                        !mutex_is_locked(&file->f_pos_lock));
> +       /*
> +        * Note that we are not guaranteed to be called after fdget_pos() on
> +        * this file obj, in which case the caller is expected to provide the
> +        * appropriate locking.
> +        */
> +
>         return true;
>  }
>

well i think this is fine, obviously ;-)

I was hoping a native speaker would do some touch ups on the comment though.
-- 
Mateusz Guzik <mjguzik gmail.com>

On Thu, Jun 12, 2025 at 3:55 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Thu, Jun 12, 2025 at 10:41:01AM +0100, Luis Henriques wrote:
> > The assert in function file_seek_cur_needs_f_lock() can be triggered very
> > easily because, as Jan Kara suggested, the file reference may get
> > incremented after checking it with fdget_pos().
> >
> > Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
> > Signed-off-by: Luis Henriques <luis@igalia.com>
> > ---
> > Hi Christian,
> >
> > It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
> > see it on vfs.git, I decided to explicitly send the patch so that it doesn't
> > slip through the cracks.
> >
> > Cheers,
> > --
> > Luis
> >
> >  fs/file.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/fs/file.c b/fs/file.c
> > index 3a3146664cf3..075f07bdc977 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
> >       if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
> >               return false;
> >
> > -     VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> > -                      !mutex_is_locked(&file->f_pos_lock));
> >       return true;
> >  }
> >
>
> There this justifies the change.
>

Huh. scratch this sentence. I stand by the rest. :)

> fdget_pos() can only legally skip locking if it determines to be in
> position where nobody else can operate on the same file obj, meaning
> file_count(file) == 1 and it can't go up. Otherwise the lock is taken.
>
> Or to put it differently, fdget_pos() NOT taking the lock and new refs
> showing up later is a bug.
>
> I don't believe anything of the sort is happening here.
>
> Instead, overlayfs is playing games and *NOT* going through fdget_pos():
>
>         ovl_inode_lock(inode);
>         realfile = ovl_real_file(file);
>         [..]
>         ret = vfs_llseek(realfile, offset, whence);
>
> Given the custom inode locking around the call, it may be any other
> locking is unnecessary and the code happens to be correct despite the
> splat.
>
> I think the safest way out with some future-proofing is to in fact *add*
> the locking in ovl_llseek() to shut up the assert -- personally I find
> it uneasy there is some underlying file obj flying around.
>
> Even if ultimately the assert has to go, the proposed commit message
> does not justify it.



-- 
Mateusz Guzik <mjguzik gmail.com>

On Thu, 12 Jun 2025 10:41:01 +0100, Luis Henriques wrote:
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because, as Jan Kara suggested, the file reference may get
> incremented after checking it with fdget_pos().
> 
> 

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fs: drop assert in file_seek_cur_needs_f_lock
      https://git.kernel.org/vfs/vfs/c/ec86bba684b1

On Thu 12-06-25 10:41:01, Luis Henriques wrote:
> The assert in function file_seek_cur_needs_f_lock() can be triggered very
> easily because, as Jan Kara suggested, the file reference may get
> incremented after checking it with fdget_pos().
> 
> Fixes: da06e3c51794 ("fs: don't needlessly acquire f_lock")
> Signed-off-by: Luis Henriques <luis@igalia.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
> Hi Christian,
> 
> It wasn't clear whether you'd be queueing this fix yourself.  Since I don't
> see it on vfs.git, I decided to explicitly send the patch so that it doesn't
> slip through the cracks.
> 
> Cheers,
> -- 
> Luis
> 
>  fs/file.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 3a3146664cf3..075f07bdc977 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1198,8 +1198,6 @@ bool file_seek_cur_needs_f_lock(struct file *file)
>  	if (!(file->f_mode & FMODE_ATOMIC_POS) && !file->f_op->iterate_shared)
>  		return false;
>  
> -	VFS_WARN_ON_ONCE((file_count(file) > 1) &&
> -			 !mutex_is_locked(&file->f_pos_lock));
>  	return true;
>  }
>  
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR