[PATCH 1/4] fuse: replace remaining make_bad_inode() with fuse_make_bad()

[PATCH 1/4] fuse: replace remaining make_bad_inode() with fuse_make_bad()

[Miklos Szeredi]

fuse_do_statx() was added with the wrong helper.

Fixes: d3045530bdd2 ("fuse: implement statx")
Cc: <stable@vger.kernel.org> # v6.6
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 25c7c97f774b..ce6a38c56d54 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1214,7 +1214,7 @@ static int fuse_do_statx(struct inode *inode, struct file *file,
 	if (((sx->mask & STATX_SIZE) && !fuse_valid_size(sx->size)) ||
 	    ((sx->mask & STATX_TYPE) && (!fuse_valid_type(sx->mode) ||
 					 inode_wrong_type(inode, sx->mode)))) {
-		make_bad_inode(inode);
+		fuse_make_bad(inode);
 		return -EIO;
 	}
 
-- 
2.43.2

[PATCH 2/4] fuse: fix root lookup with nonzero generation

[Miklos Szeredi]

The root inode has a fixed nodeid and generation (1, 0).

Prior to the commit 15db16837a35 ("fuse: fix illegal access to inode with
reused nodeid") generation number on lookup was ignored.  After this commit
lookup with the wrong generation number resulted in the inode being
unhashed.  This is correct for non-root inodes, but replacing the root
inode is wrong and results in weird behavior.

Fix by reverting to the old behavior if ignoring the generation for the
root inode, but issuing a warning in dmesg.

Reported-by: Antonio SJ Musumeci <trapexit@spawn.link>
Closes: https://lore.kernel.org/all/CAOQ4uxhek5ytdN8Yz2tNEOg5ea4NkBb4nk0FGPjPk_9nz-VG3g@mail.gmail.com/
Fixes: 15db16837a35 ("fuse: fix illegal access to inode with reused nodeid")
Cc: <stable@vger.kernel.org> # v5.14
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dir.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index ce6a38c56d54..befb7dfe387a 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -391,6 +391,10 @@ int fuse_lookup_name(struct super_block *sb, u64 nodeid, const struct qstr *name
 	err = -EIO;
 	if (fuse_invalid_attr(&outarg->attr))
 		goto out_put_forget;
+	if (outarg->nodeid == FUSE_ROOT_ID && outarg->generation != 0) {
+		pr_warn_once("root generation should be zero\n");
+		outarg->generation = 0;
+	}
 
 	*inode = fuse_iget(sb, outarg->nodeid, outarg->generation,
 			   &outarg->attr, ATTR_TIMEOUT(outarg),
-- 
2.43.2

[PATCH 3/4] fuse: don't unhash root

[Miklos Szeredi]

The root inode is assumed to be always hashed.  Do not unhash the root
inode even if it is marked BAD.

Fixes: 5d069dbe8aaf ("fuse: fix bad inode")
Cc: <stable@vger.kernel.org> # v5.11
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/fuse_i.h | 1 -
 fs/fuse/inode.c  | 7 +++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 7bd3552b1e80..4ef6087f0e5c 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -994,7 +994,6 @@ static inline bool fuse_stale_inode(const struct inode *inode, int generation,
 
 static inline void fuse_make_bad(struct inode *inode)
 {
-	remove_inode_hash(inode);
 	set_bit(FUSE_I_BAD, &get_fuse_inode(inode)->state);
 }
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index c26a84439934..aa0614e8791c 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -475,8 +475,11 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
 	} else if (fuse_stale_inode(inode, generation, attr)) {
 		/* nodeid was reused, any I/O on the old inode should fail */
 		fuse_make_bad(inode);
-		iput(inode);
-		goto retry;
+		if (inode != d_inode(sb->s_root)) {
+			remove_inode_hash(inode);
+			iput(inode);
+			goto retry;
+		}
 	}
 	fi = get_fuse_inode(inode);
 	spin_lock(&fi->lock);
-- 
2.43.2

[PATCH 4/4] fuse: use FUSE_ROOT_ID in fuse_get_root_inode()

[Miklos Szeredi]

...when calling fuse_iget().

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index aa0614e8791c..0a59062deb30 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -979,7 +979,7 @@ static struct inode *fuse_get_root_inode(struct super_block *sb, unsigned mode)
 	attr.mode = mode;
 	attr.ino = FUSE_ROOT_ID;
 	attr.nlink = 1;
-	return fuse_iget(sb, 1, 0, &attr, 0, 0);
+	return fuse_iget(sb, FUSE_ROOT_ID, 0, &attr, 0, 0);
 }
 
 struct fuse_inode_handle {
-- 
2.43.2

Re: [PATCH 3/4] fuse: don't unhash root

[Bernd Schubert]

On 2/28/24 17:02, Miklos Szeredi wrote:
> The root inode is assumed to be always hashed.  Do not unhash the root
> inode even if it is marked BAD.
> 
> Fixes: 5d069dbe8aaf ("fuse: fix bad inode")
> Cc: <stable@vger.kernel.org> # v5.11
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/fuse/fuse_i.h | 1 -
>  fs/fuse/inode.c  | 7 +++++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 7bd3552b1e80..4ef6087f0e5c 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -994,7 +994,6 @@ static inline bool fuse_stale_inode(const struct inode *inode, int generation,
>  
>  static inline void fuse_make_bad(struct inode *inode)
>  {
> -	remove_inode_hash(inode);
>  	set_bit(FUSE_I_BAD, &get_fuse_inode(inode)->state);
>  }

Hmm, what about callers like fuse_direntplus_link? It now never removes
the inode hash for these? Depend on lookup/revalidate?


Thanks,
Bernd

>  
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index c26a84439934..aa0614e8791c 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -475,8 +475,11 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
>  	} else if (fuse_stale_inode(inode, generation, attr)) {
>  		/* nodeid was reused, any I/O on the old inode should fail */
>  		fuse_make_bad(inode);
> -		iput(inode);
> -		goto retry;
> +		if (inode != d_inode(sb->s_root)) {
> +			remove_inode_hash(inode);
> +			iput(inode);
> +			goto retry;
> +		}
>  	}
>  	fi = get_fuse_inode(inode);
>  	spin_lock(&fi->lock);

Re: [PATCH 3/4] fuse: don't unhash root

[Miklos Szeredi]

On Wed, 28 Feb 2024 at 17:34, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 2/28/24 17:02, Miklos Szeredi wrote:
> > The root inode is assumed to be always hashed.  Do not unhash the root
> > inode even if it is marked BAD.
> >
> > Fixes: 5d069dbe8aaf ("fuse: fix bad inode")
> > Cc: <stable@vger.kernel.org> # v5.11
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >  fs/fuse/fuse_i.h | 1 -
> >  fs/fuse/inode.c  | 7 +++++--
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 7bd3552b1e80..4ef6087f0e5c 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -994,7 +994,6 @@ static inline bool fuse_stale_inode(const struct inode *inode, int generation,
> >
> >  static inline void fuse_make_bad(struct inode *inode)
> >  {
> > -     remove_inode_hash(inode);
> >       set_bit(FUSE_I_BAD, &get_fuse_inode(inode)->state);
> >  }
>
> Hmm, what about callers like fuse_direntplus_link? It now never removes
> the inode hash for these? Depend on lookup/revalidate?

Good questions.

In that case the dentry will be unhashed, and after retrying it will
go through fuse_iget(), which will unhash the inode.

So AFAICS the only place the inode needs to be unhashed is in
fuse_iget(), which is the real fix in 775c5033a0d1 ("fuse: fix live
lock in fuse_iget()").

Thanks,
Miklos