Re: [PATCH 6.1.y 1/1] fs: relax assertions on failure to encode file handles

["gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>]

On Tue, Sep 02, 2025 at 09:29:50AM +0200, Amir Goldstein wrote:
> On Tue, Sep 2, 2025 at 9:20 AM Manthey, Norbert <nmanthey@amazon.de> wrote:
> >
> > On Mon, 2025-09-01 at 22:00 +0200, Greg Kroah-Hartman wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open
> > > attachments unless you can confirm the sender and know the content is safe.
> > >
> > >
> > >
> > > On Mon, Sep 01, 2025 at 09:54:03PM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, Sep 01, 2025 at 03:35:59PM +0000, Norbert Manthey wrote:
> > > > > From: Amir Goldstein <amir73il@gmail.com>
> > > > >
> > > > > commit 974e3fe0ac61de85015bbe5a4990cf4127b304b2 upstream.
> > > > >
> > > > > 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>
> > > > > Link: https://lore.kernel.org/r/20241219115301.465396-1-amir73il@gmail.com
> > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > >
> > > > I never signed off on the original commit, so why was this added?
> >
> > This cherry-pick is not for the upstream commit, but for the backport on the 6.6 tree. The
> > respective commit hash is given in the backport line. Is this additional information you would like
> > to have in the commit message?
> >
> > > >
> > > > >
> > > > > (fuzzy picked from commit f47c834a9131ae64bee3c462f4e610c67b0a000f)
> > > > > Applied with LLM-adjusted hunks for 1 functions from us.amazon.nova
> > > > > - Changed the function call from `exportfs_encode_fid` to `exportfs_encode_inode_fh` to match
> > > > > the destination code.
> > >
> > > Wait, that was just fuzz matching, the real body didn't even change.
> > >
> > > > > - Removed the warning message as per the patch.
> > >
> > > I do not understand this change, what exactly was this?
> >
> > I need to rewrite (here: drop) this manually. The LLM was also describing the content of the
> > original patch, not only the diff it created.
> >
> > >
> > > > Please put this in the proper place, and in the proper format, if you
> > > > want to add "notes" to the backport.
> >
> > IIUC, the changes applied to the patch so that it applies should come above my SOB, no? What's the
> > format requirement (except the 80-100 char limit)?
> >
> > I am aware of the discussions about AI generated code. I wanted to explicitly mention the AI use, if
> > it was used as backporting helper. Do you suggest to still move this into the notes section of the
> > commit and sent patch, instead of having this in the commit itself?
> >
> > > >
> > > > But really, it took a LLM to determine an abi change?  That feels like
> > > > total overkill as you then had to actually manually check it as well.
> > > > But hey, it's your cpu cycles to burn, not mine...
> >
> > I prefer reviewing the code instead of writing/massaging all of it, and on success have the change
> > tested/validated automatically before I reviewing.
> >
> > >
> > >
> > > Again, total overkill, 1 minute doing a simple git merge resolution
> > > would have done the same thing, right?
> >
> 
> 1 minute is a lot of time when multiplied by the number of "almost
> cleanly applied"
> backports ;)

In that 1 minute you will "know" you got it right.  Otherwise you will
have to spend more than 1 minute reviewing the result of an automated
too.

Again, your waste of cpu cycles and review cycles, not mine :)

have fun!

greg k-h