On Tue, Mar 14, 2023 at 04:57:08PM +0000, Al Viro wrote: > On Tue, Mar 14, 2023 at 10:55:39AM +0100, Christian Brauner wrote: > > On Mon, Mar 13, 2023 at 11:32:08AM +0100, Greg KH wrote: > > > On Mon, Mar 13, 2023 at 03:54:55PM +0530, Kirtikumar Anandrao Ramchandani wrote: > > > > Seems like again it got rejected. I am sending it in the body if it works: > > > > > > > > >From 839cae91705e044b49397590f2d85a5dd289f0c5 Mon Sep 17 00:00:00 2001 > > > > From: KirtiRamchandani <kirtar15502@xxxxxxxxx> > > > > Date: Mon, 13 Mar 2023 15:05:08 +0530 > > > > Subject: [PATCH] Fix bug in affs_rename() function. The `affs_rename()` > > > > function in the AFFS filesystem has a bug that can cause the `retval` > > > > variable to be overwritten before it is used. Specifically, the function > > > > assigns `retval` a value in two separate code blocks, but then only checks > > > > its value in one of those blocks. This commit fixes the bug by ensuring > > > > that > > > > `retval` is properly checked in both code blocks. > > > > > > > > Signed-off-by: KirtiRamchandani <kirtar15502@xxxxxxxxx> > > > > --- > > > > namei.c | 4++++-- > > > > 1 file changed, 4 insertions(+), 2 deletion(-) > > > > > > > > diff --git a/fs/affs/namei.c b/fs/affs/namei.c > > > > index d1084e5..a54c700 100644 > > > > --- a/fs/affs/namei.c > > > > +++ b/fs/affs/namei.c > > > > @@ -488,7 +488,8 @@ affs_xrename(struct inode *old_dir, struct dentry > > > > *old_dentry, > > > > affs_lock_dir(new_dir); > > > > retval = affs_insert_hash(new_dir, bh_old); > > > > affs_unlock_dir(new_dir); > > > > - > > > > + if (retval) > > > > + goto done; > > > > > > The patch is corrupted and can not be applied. > > > > Yeah, that patch is pretty borked. This should probably be sm like: > > > > >From f3a7758bb53cc776820656c6ac66b13fb8ed9022 Mon Sep 17 00:00:00 2001 > > From: KirtiRamchandani <kirtar15502@xxxxxxxxx> > > Date: Tue, 14 Mar 2023 10:49:38 +0100 > > Subject: [PATCH] affs: handle errors in affs_xrename() > > > > Fix a bug in the affs_xrename() function. The affs_xrename() function in > > the AFFS filesystem has a bug that can cause the retval variable to be > > overwritten before it is used. Specifically, the function assigns retval > > a value in two separate code blocks, but then only checks its value in > > one of those blocks. This commit fixes the bug by ensuring that retval > > is properly checked in both code blocks. > > "Properly checked" as in...? > > > Signed-off-by: KirtiRamchandani <kirtar15502@xxxxxxxxx> > > --- > > fs/affs/namei.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/fs/affs/namei.c b/fs/affs/namei.c > > index d12ccfd2a83d..98525d69391d 100644 > > --- a/fs/affs/namei.c > > +++ b/fs/affs/namei.c > > @@ -488,6 +488,8 @@ affs_xrename(struct inode *old_dir, struct dentry *old_dentry, > > affs_lock_dir(new_dir); > > retval = affs_insert_hash(new_dir, bh_old); > > affs_unlock_dir(new_dir); > > + if (retval) > > + goto done; > > OK, so you've got an IO error and insertion has failed. Both entries had already > been removed from their directories. Sure, we must report an error, but why is > leaking *both* entries the right thing to do? > > > /* Insert new into the old directory with the old name. */ > > affs_copy_name(AFFS_TAIL(sb, bh_new)->name, old_dentry); > > @@ -495,6 +497,8 @@ affs_xrename(struct inode *old_dir, struct dentry *old_dentry, > > affs_lock_dir(old_dir); > > retval = affs_insert_hash(old_dir, bh_new); > > affs_unlock_dir(old_dir); > > + if (retval) > > + goto done; > > done: > > Really? How could that possibly make any sense? I mean, look for the target of > that goto... > > The bug here (AFFS awful layout aside) is that error from the first insert_hash > is always lost. And it needs to be reported. But this is no way to fix that. Note that I formatted that thing into something we can look at on the list from the borked attachment; not acked it...
