[PATCH] fs: relax assertions on failure to encode file handles

[PATCH] fs: relax assertions on failure to encode file handles

[Amir Goldstein]

Encoding file handles is usually performed by a filesystem >encode_fh()
method that may fail for various reasons.

The legacy users of exportfs_encode_fh(), namely, nfsd and
name_to_handle_at(2) syscall are ready to cope with the possibility
of failure to encode a file handle.

There are a few other users of exportfs_encode_{fh,fid}() that
currently have a WARN_ON() assertion when ->encode_fh() fails.
Relax those assertions because they are wrong.

The second linked bug report states commit 16aac5ad1fa9 ("ovl: support
encoding non-decodable file handles") in v6.6 as the regressing commit,
but this is not accurate.

The aforementioned commit only increases the chances of the assertion
and allows triggering the assertion with the reproducer using overlayfs,
inotify and drop_caches.

Triggering this assertion was always possible with other filesystems and
other reasons of ->encode_fh() failures and more particularly, it was
also possible with the exact same reproducer using overlayfs that is
mounted with options index=on,nfs_export=on also on kernels < v6.6.
Therefore, I am not listing the aforementioned commit as a Fixes commit.

Backport hint: this patch will have a trivial conflict applying to
v6.6.y, and other trivial conflicts applying to stable kernels < v6.6.

Reported-by: syzbot+ec07f6f5ce62b858579f@syzkaller.appspotmail.com
Tested-by: syzbot+ec07f6f5ce62b858579f@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-unionfs/671fd40c.050a0220.4735a.024f.GAE@google.com/
Reported-by: Dmitry Safonov <dima@arista.com>
Closes: https://lore.kernel.org/linux-fsdevel/CAGrbwDTLt6drB9eaUagnQVgdPBmhLfqqxAf3F+Juqy_o6oP8uw@mail.gmail.com/
Cc: stable@vger.kernel.org
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Christian,

I could have sumbitted two independant patches to relax the assertion
in fsnotify and overlayfs via fsnotify and overlayfs trees, but the
nature of the problem is the same and in both cases, the problem became
worse with the introduction of non-decodable file handles support,
so decided to fix them together and ask you to take the fix via the
vfs tree.

Please let you if you think it should be done differently.

Thanks,
Amir.

 fs/notify/fdinfo.c     | 4 +---
 fs/overlayfs/copy_up.c | 5 ++---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index dec553034027e..e933f9c65d904 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -47,10 +47,8 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
 	size = f->handle_bytes >> 2;
 
 	ret = exportfs_encode_fid(inode, (struct fid *)f->f_handle, &size);
-	if ((ret == FILEID_INVALID) || (ret < 0)) {
-		WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
+	if ((ret == FILEID_INVALID) || (ret < 0))
 		return;
-	}
 
 	f->handle_type = ret;
 	f->handle_bytes = size * sizeof(u32);
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 3601ddfeddc2e..56eee9f23ea9a 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -442,9 +442,8 @@ struct ovl_fh *ovl_encode_real_fh(struct ovl_fs *ofs, struct dentry *real,
 	buflen = (dwords << 2);
 
 	err = -EIO;
-	if (WARN_ON(fh_type < 0) ||
-	    WARN_ON(buflen > MAX_HANDLE_SZ) ||
-	    WARN_ON(fh_type == FILEID_INVALID))
+	if (fh_type < 0 || fh_type == FILEID_INVALID ||
+	    WARN_ON(buflen > MAX_HANDLE_SZ))
 		goto out_err;
 
 	fh->fb.version = OVL_FH_VERSION;
-- 
2.34.1

Re: [PATCH] fs: relax assertions on failure to encode file handles

[Amir Goldstein]

On Thu, Dec 19, 2024 at 12:53 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Encoding file handles is usually performed by a filesystem >encode_fh()
> method that may fail for various reasons.
>
> The legacy users of exportfs_encode_fh(), namely, nfsd and
> name_to_handle_at(2) syscall are ready to cope with the possibility
> of failure to encode a file handle.
>
> There are a few other users of exportfs_encode_{fh,fid}() that
> currently have a WARN_ON() assertion when ->encode_fh() fails.
> Relax those assertions because they are wrong.
>
> The second linked bug report states commit 16aac5ad1fa9 ("ovl: support
> encoding non-decodable file handles") in v6.6 as the regressing commit,
> but this is not accurate.
>
> The aforementioned commit only increases the chances of the assertion
> and allows triggering the assertion with the reproducer using overlayfs,
> inotify and drop_caches.
>
> Triggering this assertion was always possible with other filesystems and
> other reasons of ->encode_fh() failures and more particularly, it was
> also possible with the exact same reproducer using overlayfs that is
> mounted with options index=on,nfs_export=on also on kernels < v6.6.
> Therefore, I am not listing the aforementioned commit as a Fixes commit.
>
> Backport hint: this patch will have a trivial conflict applying to
> v6.6.y, and other trivial conflicts applying to stable kernels < v6.6.
>
> Reported-by: syzbot+ec07f6f5ce62b858579f@syzkaller.appspotmail.com
> Tested-by: syzbot+ec07f6f5ce62b858579f@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-unionfs/671fd40c.050a0220.4735a.024f.GAE@google.com/
> Reported-by: Dmitry Safonov <dima@arista.com>
> Closes: https://lore.kernel.org/linux-fsdevel/CAGrbwDTLt6drB9eaUagnQVgdPBmhLfqqxAf3F+Juqy_o6oP8uw@mail.gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Christian,
>
> I could have sumbitted two independant patches to relax the assertion
> in fsnotify and overlayfs via fsnotify and overlayfs trees, but the
> nature of the problem is the same and in both cases, the problem became
> worse with the introduction of non-decodable file handles support,
> so decided to fix them together and ask you to take the fix via the
> vfs tree.
>
> Please let you if you think it should be done differently.

FWIW, pushed two separate patches to branch fsnotify-fixes
on my github.

I guess it is nicer this way and will help automatic backporting.

Thanks,
Amir.

>
> Thanks,
> Amir.
>
>  fs/notify/fdinfo.c     | 4 +---
>  fs/overlayfs/copy_up.c | 5 ++---
>  2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
> index dec553034027e..e933f9c65d904 100644
> --- a/fs/notify/fdinfo.c
> +++ b/fs/notify/fdinfo.c
> @@ -47,10 +47,8 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
>         size = f->handle_bytes >> 2;
>
>         ret = exportfs_encode_fid(inode, (struct fid *)f->f_handle, &size);
> -       if ((ret == FILEID_INVALID) || (ret < 0)) {
> -               WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
> +       if ((ret == FILEID_INVALID) || (ret < 0))
>                 return;
> -       }
>
>         f->handle_type = ret;
>         f->handle_bytes = size * sizeof(u32);
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 3601ddfeddc2e..56eee9f23ea9a 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -442,9 +442,8 @@ struct ovl_fh *ovl_encode_real_fh(struct ovl_fs *ofs, struct dentry *real,
>         buflen = (dwords << 2);
>
>         err = -EIO;
> -       if (WARN_ON(fh_type < 0) ||
> -           WARN_ON(buflen > MAX_HANDLE_SZ) ||
> -           WARN_ON(fh_type == FILEID_INVALID))
> +       if (fh_type < 0 || fh_type == FILEID_INVALID ||
> +           WARN_ON(buflen > MAX_HANDLE_SZ))
>                 goto out_err;
>
>         fh->fb.version = OVL_FH_VERSION;
> --
> 2.34.1
>

Re: [PATCH] fs: relax assertions on failure to encode file handles

[Christian Brauner]

On Thu, 19 Dec 2024 12:53:01 +0100, Amir Goldstein wrote:
> Encoding file handles is usually performed by a filesystem >encode_fh()
> method that may fail for various reasons.
> 
> The legacy users of exportfs_encode_fh(), namely, nfsd and
> name_to_handle_at(2) syscall are ready to cope with the possibility
> of failure to encode a file handle.
> 
> [...]

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: relax assertions on failure to encode file handles
      https://git.kernel.org/vfs/vfs/c/974e3fe0ac61

Re: [PATCH] fs: relax assertions on failure to encode file handles

[Jan Kara]

On Thu 19-12-24 12:53:01, Amir Goldstein wrote:
> Encoding file handles is usually performed by a filesystem >encode_fh()
> method that may fail for various reasons.
> 
> The legacy users of exportfs_encode_fh(), namely, nfsd and
> name_to_handle_at(2) syscall are ready to cope with the possibility
> of failure to encode a file handle.
> 
> There are a few other users of exportfs_encode_{fh,fid}() that
> currently have a WARN_ON() assertion when ->encode_fh() fails.
> Relax those assertions because they are wrong.
> 
> The second linked bug report states commit 16aac5ad1fa9 ("ovl: support
> encoding non-decodable file handles") in v6.6 as the regressing commit,
> but this is not accurate.
> 
> The aforementioned commit only increases the chances of the assertion
> and allows triggering the assertion with the reproducer using overlayfs,
> inotify and drop_caches.
> 
> Triggering this assertion was always possible with other filesystems and
> other reasons of ->encode_fh() failures and more particularly, it was
> also possible with the exact same reproducer using overlayfs that is
> mounted with options index=on,nfs_export=on also on kernels < v6.6.
> Therefore, I am not listing the aforementioned commit as a Fixes commit.
> 
> Backport hint: this patch will have a trivial conflict applying to
> v6.6.y, and other trivial conflicts applying to stable kernels < v6.6.
> 
> Reported-by: syzbot+ec07f6f5ce62b858579f@syzkaller.appspotmail.com
> Tested-by: syzbot+ec07f6f5ce62b858579f@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-unionfs/671fd40c.050a0220.4735a.024f.GAE@google.com/
> Reported-by: Dmitry Safonov <dima@arista.com>
> Closes: https://lore.kernel.org/linux-fsdevel/CAGrbwDTLt6drB9eaUagnQVgdPBmhLfqqxAf3F+Juqy_o6oP8uw@mail.gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Yeah, looks sensible. Feel free to add:

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

								Honza

> ---
> 
> Christian,
> 
> I could have sumbitted two independant patches to relax the assertion
> in fsnotify and overlayfs via fsnotify and overlayfs trees, but the
> nature of the problem is the same and in both cases, the problem became
> worse with the introduction of non-decodable file handles support,
> so decided to fix them together and ask you to take the fix via the
> vfs tree.
> 
> Please let you if you think it should be done differently.
> 
> Thanks,
> Amir.
> 
>  fs/notify/fdinfo.c     | 4 +---
>  fs/overlayfs/copy_up.c | 5 ++---
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
> index dec553034027e..e933f9c65d904 100644
> --- a/fs/notify/fdinfo.c
> +++ b/fs/notify/fdinfo.c
> @@ -47,10 +47,8 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
>  	size = f->handle_bytes >> 2;
>  
>  	ret = exportfs_encode_fid(inode, (struct fid *)f->f_handle, &size);
> -	if ((ret == FILEID_INVALID) || (ret < 0)) {
> -		WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
> +	if ((ret == FILEID_INVALID) || (ret < 0))
>  		return;
> -	}
>  
>  	f->handle_type = ret;
>  	f->handle_bytes = size * sizeof(u32);
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 3601ddfeddc2e..56eee9f23ea9a 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -442,9 +442,8 @@ struct ovl_fh *ovl_encode_real_fh(struct ovl_fs *ofs, struct dentry *real,
>  	buflen = (dwords << 2);
>  
>  	err = -EIO;
> -	if (WARN_ON(fh_type < 0) ||
> -	    WARN_ON(buflen > MAX_HANDLE_SZ) ||
> -	    WARN_ON(fh_type == FILEID_INVALID))
> +	if (fh_type < 0 || fh_type == FILEID_INVALID ||
> +	    WARN_ON(buflen > MAX_HANDLE_SZ))
>  		goto out_err;
>  
>  	fh->fb.version = OVL_FH_VERSION;
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR