fs: use debug-only asserts around fd allocation and install

[PATCH] fs: use debug-only asserts around fd allocation and install

Posted by Mateusz Guzik 9 months, 1 week ago

This also restores the check which got removed in 52732bb9abc9ee5b
("fs/file.c: remove sanity_check and add likely/unlikely in alloc_fd()")
for performance reasons -- they no longer apply with a debug-only
variant.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---

I have about 0 opinion whether this should be BUG or WARN, the code was
already inconsistent on this front. If you want the latter, I'll have 0
complaints if you just sed it and commit as yours.

This reminded me to sort out that litmus test for smp_rmb, hopefully
soon(tm) as it is now nagging me.

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

diff --git a/fs/file.c b/fs/file.c
index 6c159ede55f1..09460ec74ef8 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -582,6 +582,7 @@ static int alloc_fd(unsigned start, unsigned end, unsigned flags)
 
 	__set_open_fd(fd, fdt, flags & O_CLOEXEC);
 	error = fd;
+	VFS_BUG_ON(rcu_access_pointer(fdt->fd[fd]) != NULL);
 
 out:
 	spin_unlock(&files->file_lock);
@@ -647,7 +648,7 @@ void fd_install(unsigned int fd, struct file *file)
 		rcu_read_unlock_sched();
 		spin_lock(&files->file_lock);
 		fdt = files_fdtable(files);
-		WARN_ON(fdt->fd[fd] != NULL);
+		VFS_BUG_ON(fdt->fd[fd] != NULL);
 		rcu_assign_pointer(fdt->fd[fd], file);
 		spin_unlock(&files->file_lock);
 		return;
@@ -655,7 +656,7 @@ void fd_install(unsigned int fd, struct file *file)
 	/* coupled with smp_wmb() in expand_fdtable() */
 	smp_rmb();
 	fdt = rcu_dereference_sched(files->fdt);
-	BUG_ON(fdt->fd[fd] != NULL);
+	VFS_BUG_ON(fdt->fd[fd] != NULL);
 	rcu_assign_pointer(fdt->fd[fd], file);
 	rcu_read_unlock_sched();
 }
-- 
2.43.0

Re: [PATCH] fs: use debug-only asserts around fd allocation and install

Posted by Christian Brauner 9 months, 1 week ago

On Wed, 12 Mar 2025 17:19:41 +0100, Mateusz Guzik wrote:
> This also restores the check which got removed in 52732bb9abc9ee5b
> ("fs/file.c: remove sanity_check and add likely/unlikely in alloc_fd()")
> for performance reasons -- they no longer apply with a debug-only
> variant.
> 
> 

Applied to the vfs-6.15.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.15.misc 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-6.15.misc

[1/1] fs: use debug-only asserts around fd allocation and install
      https://git.kernel.org/vfs/vfs/c/dc530c44cd64

Re: [PATCH] fs: use debug-only asserts around fd allocation and install

Posted by Mateusz Guzik 9 months, 1 week ago

On Wed, Mar 12, 2025 at 5:19 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> This also restores the check which got removed in 52732bb9abc9ee5b
> ("fs/file.c: remove sanity_check and add likely/unlikely in alloc_fd()")
> for performance reasons -- they no longer apply with a debug-only
> variant.
>
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
>
> I have about 0 opinion whether this should be BUG or WARN, the code was
> already inconsistent on this front. If you want the latter, I'll have 0
> complaints if you just sed it and commit as yours.
>
> This reminded me to sort out that litmus test for smp_rmb, hopefully
> soon(tm) as it is now nagging me.
>
>  fs/file.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/file.c b/fs/file.c
> index 6c159ede55f1..09460ec74ef8 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -582,6 +582,7 @@ static int alloc_fd(unsigned start, unsigned end, unsigned flags)
>
>         __set_open_fd(fd, fdt, flags & O_CLOEXEC);
>         error = fd;
> +       VFS_BUG_ON(rcu_access_pointer(fdt->fd[fd]) != NULL);
>

when restoring this check i dutifully copy-pasted the original. I only
now mentally registered it uses a rcu primitive to do the load, while
the others do a plain load. arguably the former is closer to being
correct and it definitely does not hurt

so this line should replace the other 2 lines below. i can send a v2
to that effect, but given the triviality of the edit, perhaps you will
be happy to sort it out

>  out:
>         spin_unlock(&files->file_lock);
> @@ -647,7 +648,7 @@ void fd_install(unsigned int fd, struct file *file)
>                 rcu_read_unlock_sched();
>                 spin_lock(&files->file_lock);
>                 fdt = files_fdtable(files);
> -               WARN_ON(fdt->fd[fd] != NULL);
> +               VFS_BUG_ON(fdt->fd[fd] != NULL);
>                 rcu_assign_pointer(fdt->fd[fd], file);
>                 spin_unlock(&files->file_lock);
>                 return;
> @@ -655,7 +656,7 @@ void fd_install(unsigned int fd, struct file *file)
>         /* coupled with smp_wmb() in expand_fdtable() */
>         smp_rmb();
>         fdt = rcu_dereference_sched(files->fdt);
> -       BUG_ON(fdt->fd[fd] != NULL);
> +       VFS_BUG_ON(fdt->fd[fd] != NULL);
>         rcu_assign_pointer(fdt->fd[fd], file);
>         rcu_read_unlock_sched();
>  }
> --
> 2.43.0
>


-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] fs: use debug-only asserts around fd allocation and install

Posted by Christian Brauner 9 months, 1 week ago

On Wed, Mar 12, 2025 at 06:21:01PM +0100, Mateusz Guzik wrote:
> On Wed, Mar 12, 2025 at 5:19 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> >
> > This also restores the check which got removed in 52732bb9abc9ee5b
> > ("fs/file.c: remove sanity_check and add likely/unlikely in alloc_fd()")
> > for performance reasons -- they no longer apply with a debug-only
> > variant.
> >
> > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> > ---
> >
> > I have about 0 opinion whether this should be BUG or WARN, the code was
> > already inconsistent on this front. If you want the latter, I'll have 0
> > complaints if you just sed it and commit as yours.
> >
> > This reminded me to sort out that litmus test for smp_rmb, hopefully
> > soon(tm) as it is now nagging me.
> >
> >  fs/file.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/file.c b/fs/file.c
> > index 6c159ede55f1..09460ec74ef8 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -582,6 +582,7 @@ static int alloc_fd(unsigned start, unsigned end, unsigned flags)
> >
> >         __set_open_fd(fd, fdt, flags & O_CLOEXEC);
> >         error = fd;
> > +       VFS_BUG_ON(rcu_access_pointer(fdt->fd[fd]) != NULL);
> >
> 
> when restoring this check i dutifully copy-pasted the original. I only
> now mentally registered it uses a rcu primitive to do the load, while
> the others do a plain load. arguably the former is closer to being
> correct and it definitely does not hurt
> 
> so this line should replace the other 2 lines below. i can send a v2
> to that effect, but given the triviality of the edit, perhaps you will
> be happy to sort it out

Yes, sure. Done!

Re: [PATCH] fs: use debug-only asserts around fd allocation and install

Posted by Jan Kara 9 months, 1 week ago

On Wed 12-03-25 17:19:41, Mateusz Guzik wrote:
> This also restores the check which got removed in 52732bb9abc9ee5b
> ("fs/file.c: remove sanity_check and add likely/unlikely in alloc_fd()")
> for performance reasons -- they no longer apply with a debug-only
> variant.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Looks good. Feel free to add:

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

								Honza

> ---
> 
> I have about 0 opinion whether this should be BUG or WARN, the code was
> already inconsistent on this front. If you want the latter, I'll have 0
> complaints if you just sed it and commit as yours.
> 
> This reminded me to sort out that litmus test for smp_rmb, hopefully
> soon(tm) as it is now nagging me.
> 
>  fs/file.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 6c159ede55f1..09460ec74ef8 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -582,6 +582,7 @@ static int alloc_fd(unsigned start, unsigned end, unsigned flags)
>  
>  	__set_open_fd(fd, fdt, flags & O_CLOEXEC);
>  	error = fd;
> +	VFS_BUG_ON(rcu_access_pointer(fdt->fd[fd]) != NULL);
>  
>  out:
>  	spin_unlock(&files->file_lock);
> @@ -647,7 +648,7 @@ void fd_install(unsigned int fd, struct file *file)
>  		rcu_read_unlock_sched();
>  		spin_lock(&files->file_lock);
>  		fdt = files_fdtable(files);
> -		WARN_ON(fdt->fd[fd] != NULL);
> +		VFS_BUG_ON(fdt->fd[fd] != NULL);
>  		rcu_assign_pointer(fdt->fd[fd], file);
>  		spin_unlock(&files->file_lock);
>  		return;
> @@ -655,7 +656,7 @@ void fd_install(unsigned int fd, struct file *file)
>  	/* coupled with smp_wmb() in expand_fdtable() */
>  	smp_rmb();
>  	fdt = rcu_dereference_sched(files->fdt);
> -	BUG_ON(fdt->fd[fd] != NULL);
> +	VFS_BUG_ON(fdt->fd[fd] != NULL);
>  	rcu_assign_pointer(fdt->fd[fd], file);
>  	rcu_read_unlock_sched();
>  }
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR