[PATCH] fuse: don't truncate cached, mutated symlink

[PATCH] fuse: don't truncate cached, mutated symlink

[Miklos Szeredi]

Fuse allows the value of a symlink to change and this property is exploited
by some filesystems (e.g. CVMFS).

It has been observed, that sometimes after changing the symlink contents,
the value is truncated to the old size.

This is caused by fuse_getattr() racing with fuse_reverse_inval_inode().
fuse_reverse_inval_inode() updates the fuse_inode's attr_version, which
results in fuse_change_attributes() exiting before updating the cached
attributes

This is okay, as the cached attributes remain invalid and the next call to
fuse_change_attributes() will likely update the inode with the correct
values.

The reason this causes problems is that cached symlinks will be
returned through page_get_link(), which truncates the symlink to
inode->i_size.  This is correct for filesystems that don't mutate
symlinks, but in this case it causes bad behavior.

The solution is to just remove this truncation.  This can cause a
regression in a filesystem that relies on supplying a symlink larger than
the file size, but this is unlikely.  If that happens we'd need to make
this behavior conditional.

Reported-by: Laura Promberger <laura.promberger@cern.ch>
Tested-by: Sam Lewis <samclewis@google.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dir.c      |  2 +-
 fs/namei.c         | 24 +++++++++++++++++++-----
 include/linux/fs.h |  2 ++
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 589e88822368..83c56ce6ad20 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1645,7 +1645,7 @@ static const char *fuse_get_link(struct dentry *dentry, struct inode *inode,
 		goto out_err;
 
 	if (fc->cache_symlinks)
-		return page_get_link(dentry, inode, callback);
+		return page_get_link_raw(dentry, inode, callback);
 
 	err = -ECHILD;
 	if (!dentry)
diff --git a/fs/namei.c b/fs/namei.c
index 3ab9440c5b93..ecb7b95c2ca3 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -5356,10 +5356,9 @@ const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done)
 EXPORT_SYMBOL(vfs_get_link);
 
 /* get the link contents into pagecache */
-const char *page_get_link(struct dentry *dentry, struct inode *inode,
-			  struct delayed_call *callback)
+static char *__page_get_link(struct dentry *dentry, struct inode *inode,
+			     struct delayed_call *callback)
 {
-	char *kaddr;
 	struct page *page;
 	struct address_space *mapping = inode->i_mapping;
 
@@ -5378,8 +5377,23 @@ const char *page_get_link(struct dentry *dentry, struct inode *inode,
 	}
 	set_delayed_call(callback, page_put_link, page);
 	BUG_ON(mapping_gfp_mask(mapping) & __GFP_HIGHMEM);
-	kaddr = page_address(page);
-	nd_terminate_link(kaddr, inode->i_size, PAGE_SIZE - 1);
+	return page_address(page);
+}
+
+const char *page_get_link_raw(struct dentry *dentry, struct inode *inode,
+			      struct delayed_call *callback)
+{
+	return __page_get_link(dentry, inode, callback);
+}
+EXPORT_SYMBOL_GPL(page_get_link_raw);
+
+const char *page_get_link(struct dentry *dentry, struct inode *inode,
+					struct delayed_call *callback)
+{
+	char *kaddr = __page_get_link(dentry, inode, callback);
+
+	if (!IS_ERR(kaddr))
+		nd_terminate_link(kaddr, inode->i_size, PAGE_SIZE - 1);
 	return kaddr;
 }
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2c3b2f8a621f..9346adf28f7b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3452,6 +3452,8 @@ extern const struct file_operations generic_ro_fops;
 
 extern int readlink_copy(char __user *, int, const char *, int);
 extern int page_readlink(struct dentry *, char __user *, int);
+extern const char *page_get_link_raw(struct dentry *, struct inode *,
+				     struct delayed_call *);
 extern const char *page_get_link(struct dentry *, struct inode *,
 				 struct delayed_call *);
 extern void page_put_link(void *);
-- 
2.48.1

Re: [PATCH] fuse: don't truncate cached, mutated symlink

[Bernd Schubert]

On 2/20/25 11:02, Miklos Szeredi wrote:
> Fuse allows the value of a symlink to change and this property is exploited
> by some filesystems (e.g. CVMFS).
> 
> It has been observed, that sometimes after changing the symlink contents,
> the value is truncated to the old size.
> 
> This is caused by fuse_getattr() racing with fuse_reverse_inval_inode().
> fuse_reverse_inval_inode() updates the fuse_inode's attr_version, which
> results in fuse_change_attributes() exiting before updating the cached
> attributes
> 
> This is okay, as the cached attributes remain invalid and the next call to
> fuse_change_attributes() will likely update the inode with the correct
> values.
> 
> The reason this causes problems is that cached symlinks will be
> returned through page_get_link(), which truncates the symlink to
> inode->i_size.  This is correct for filesystems that don't mutate
> symlinks, but in this case it causes bad behavior.
> 
> The solution is to just remove this truncation.  This can cause a
> regression in a filesystem that relies on supplying a symlink larger than
> the file size, but this is unlikely.  If that happens we'd need to make
> this behavior conditional.
> 
> Reported-by: Laura Promberger <laura.promberger@cern.ch>
> Tested-by: Sam Lewis <samclewis@google.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/fuse/dir.c      |  2 +-
>  fs/namei.c         | 24 +++++++++++++++++++-----
>  include/linux/fs.h |  2 ++
>  3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 589e88822368..83c56ce6ad20 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1645,7 +1645,7 @@ static const char *fuse_get_link(struct dentry *dentry, struct inode *inode,
>  		goto out_err;
>  
>  	if (fc->cache_symlinks)
> -		return page_get_link(dentry, inode, callback);
> +		return page_get_link_raw(dentry, inode, callback);
>  
>  	err = -ECHILD;
>  	if (!dentry)
> diff --git a/fs/namei.c b/fs/namei.c
> index 3ab9440c5b93..ecb7b95c2ca3 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -5356,10 +5356,9 @@ const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done)
>  EXPORT_SYMBOL(vfs_get_link);
>  
>  /* get the link contents into pagecache */
> -const char *page_get_link(struct dentry *dentry, struct inode *inode,
> -			  struct delayed_call *callback)
> +static char *__page_get_link(struct dentry *dentry, struct inode *inode,
> +			     struct delayed_call *callback)
>  {
> -	char *kaddr;
>  	struct page *page;
>  	struct address_space *mapping = inode->i_mapping;
>  
> @@ -5378,8 +5377,23 @@ const char *page_get_link(struct dentry *dentry, struct inode *inode,
>  	}
>  	set_delayed_call(callback, page_put_link, page);
>  	BUG_ON(mapping_gfp_mask(mapping) & __GFP_HIGHMEM);
> -	kaddr = page_address(page);
> -	nd_terminate_link(kaddr, inode->i_size, PAGE_SIZE - 1);
> +	return page_address(page);
> +}
> +
> +const char *page_get_link_raw(struct dentry *dentry, struct inode *inode,
> +			      struct delayed_call *callback)
> +{
> +	return __page_get_link(dentry, inode, callback);
> +}
> +EXPORT_SYMBOL_GPL(page_get_link_raw);
> +
> +const char *page_get_link(struct dentry *dentry, struct inode *inode,
> +					struct delayed_call *callback)
> +{
> +	char *kaddr = __page_get_link(dentry, inode, callback);
> +
> +	if (!IS_ERR(kaddr))
> +		nd_terminate_link(kaddr, inode->i_size, PAGE_SIZE - 1);
>  	return kaddr;
>  }
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2c3b2f8a621f..9346adf28f7b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3452,6 +3452,8 @@ extern const struct file_operations generic_ro_fops;
>  
>  extern int readlink_copy(char __user *, int, const char *, int);
>  extern int page_readlink(struct dentry *, char __user *, int);
> +extern const char *page_get_link_raw(struct dentry *, struct inode *,
> +				     struct delayed_call *);
>  extern const char *page_get_link(struct dentry *, struct inode *,
>  				 struct delayed_call *);
>  extern void page_put_link(void *);

I had looked at it last year already, but always wanted to write a test
for it (and never found the time).

Reviewed-by: Bernd Schubert <bschubert@ddn.com>

Re: [PATCH] fuse: don't truncate cached, mutated symlink

[Christian Brauner]

On Thu, 20 Feb 2025 11:02:58 +0100, Miklos Szeredi wrote:
> Fuse allows the value of a symlink to change and this property is exploited
> by some filesystems (e.g. CVMFS).
> 
> It has been observed, that sometimes after changing the symlink contents,
> the value is truncated to the old size.
> 
> This is caused by fuse_getattr() racing with fuse_reverse_inval_inode().
> fuse_reverse_inval_inode() updates the fuse_inode's attr_version, which
> results in fuse_change_attributes() exiting before updating the cached
> attributes
> 
> [...]

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] fuse: don't truncate cached, mutated symlink
      https://git.kernel.org/vfs/vfs/c/b4c173dfbb6c

Re: [PATCH] fuse: don't truncate cached, mutated symlink

[Luis Henriques]

On Thu, Feb 20 2025, Miklos Szeredi wrote:

> Fuse allows the value of a symlink to change and this property is exploited
> by some filesystems (e.g. CVMFS).
>
> It has been observed, that sometimes after changing the symlink contents,
> the value is truncated to the old size.
>
> This is caused by fuse_getattr() racing with fuse_reverse_inval_inode().
> fuse_reverse_inval_inode() updates the fuse_inode's attr_version, which
> results in fuse_change_attributes() exiting before updating the cached
> attributes
>
> This is okay, as the cached attributes remain invalid and the next call to
> fuse_change_attributes() will likely update the inode with the correct
> values.
>
> The reason this causes problems is that cached symlinks will be
> returned through page_get_link(), which truncates the symlink to
> inode->i_size.  This is correct for filesystems that don't mutate
> symlinks, but in this case it causes bad behavior.
>
> The solution is to just remove this truncation.  This can cause a
> regression in a filesystem that relies on supplying a symlink larger than
> the file size, but this is unlikely.  If that happens we'd need to make
> this behavior conditional.
>
> Reported-by: Laura Promberger <laura.promberger@cern.ch>
> Tested-by: Sam Lewis <samclewis@google.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

OK, I finally managed to reproduce the bug (thanks for the hints, Sam!)
and I can also confirm this patch fixes it.

So, feel free to add my

Reviewed-by: Luis Henriques <luis@igalia.com>
Tested-by: Luis Henriques <luis@igalia.com>

Cheers,
-- 
Luís

> ---
>  fs/fuse/dir.c      |  2 +-
>  fs/namei.c         | 24 +++++++++++++++++++-----
>  include/linux/fs.h |  2 ++
>  3 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 589e88822368..83c56ce6ad20 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1645,7 +1645,7 @@ static const char *fuse_get_link(struct dentry *dentry, struct inode *inode,
>  		goto out_err;
>  
>  	if (fc->cache_symlinks)
> -		return page_get_link(dentry, inode, callback);
> +		return page_get_link_raw(dentry, inode, callback);
>  
>  	err = -ECHILD;
>  	if (!dentry)
> diff --git a/fs/namei.c b/fs/namei.c
> index 3ab9440c5b93..ecb7b95c2ca3 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -5356,10 +5356,9 @@ const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done)
>  EXPORT_SYMBOL(vfs_get_link);
>  
>  /* get the link contents into pagecache */
> -const char *page_get_link(struct dentry *dentry, struct inode *inode,
> -			  struct delayed_call *callback)
> +static char *__page_get_link(struct dentry *dentry, struct inode *inode,
> +			     struct delayed_call *callback)
>  {
> -	char *kaddr;
>  	struct page *page;
>  	struct address_space *mapping = inode->i_mapping;
>  
> @@ -5378,8 +5377,23 @@ const char *page_get_link(struct dentry *dentry, struct inode *inode,
>  	}
>  	set_delayed_call(callback, page_put_link, page);
>  	BUG_ON(mapping_gfp_mask(mapping) & __GFP_HIGHMEM);
> -	kaddr = page_address(page);
> -	nd_terminate_link(kaddr, inode->i_size, PAGE_SIZE - 1);
> +	return page_address(page);
> +}
> +
> +const char *page_get_link_raw(struct dentry *dentry, struct inode *inode,
> +			      struct delayed_call *callback)
> +{
> +	return __page_get_link(dentry, inode, callback);
> +}
> +EXPORT_SYMBOL_GPL(page_get_link_raw);
> +
> +const char *page_get_link(struct dentry *dentry, struct inode *inode,
> +					struct delayed_call *callback)
> +{
> +	char *kaddr = __page_get_link(dentry, inode, callback);
> +
> +	if (!IS_ERR(kaddr))
> +		nd_terminate_link(kaddr, inode->i_size, PAGE_SIZE - 1);
>  	return kaddr;
>  }
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2c3b2f8a621f..9346adf28f7b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3452,6 +3452,8 @@ extern const struct file_operations generic_ro_fops;
>  
>  extern int readlink_copy(char __user *, int, const char *, int);
>  extern int page_readlink(struct dentry *, char __user *, int);
> +extern const char *page_get_link_raw(struct dentry *, struct inode *,
> +				     struct delayed_call *);
>  extern const char *page_get_link(struct dentry *, struct inode *,
>  				 struct delayed_call *);
>  extern void page_put_link(void *);
> -- 
>
> 2.48.1
>
>

Re: [PATCH] fuse: don't truncate cached, mutated symlink

[Al Viro]

On Thu, Feb 20, 2025 at 11:02:58AM +0100, Miklos Szeredi wrote:

> The solution is to just remove this truncation.  This can cause a
> regression in a filesystem that relies on supplying a symlink larger than
> the file size, but this is unlikely.  If that happens we'd need to make
> this behavior conditional.

Note, BTW, that page *contents* must not change at all, so truncation is
only safe if we have ->i_size guaranteed to be stable.

Core pathwalk really counts upon the string remaining immutable, and that
does include the string returned by ->get_link().

Re: [PATCH] fuse: don't truncate cached, mutated symlink

[Miklos Szeredi]

On Sun, 23 Feb 2025 at 01:28, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Feb 20, 2025 at 11:02:58AM +0100, Miklos Szeredi wrote:
>
> > The solution is to just remove this truncation.  This can cause a
> > regression in a filesystem that relies on supplying a symlink larger than
> > the file size, but this is unlikely.  If that happens we'd need to make
> > this behavior conditional.
>
> Note, BTW, that page *contents* must not change at all, so truncation is
> only safe if we have ->i_size guaranteed to be stable.
>
> Core pathwalk really counts upon the string remaining immutable, and that
> does include the string returned by ->get_link().

Page contents will not change after initial readlink, but page could
get invalidated and a fresh page filled with new value for the same
object.

It's weird behavior all right, but seems to be useful in a couple of
cases where invalidating the dentry wouldn't work (I don't remember
the details, cvmfs folks can help with that).

Thanks,
Miklos

Re: [PATCH] fuse: don't truncate cached, mutated symlink

[Al Viro]

On Sun, Feb 23, 2025 at 08:12:21PM +0100, Miklos Szeredi wrote:
> On Sun, 23 Feb 2025 at 01:28, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Thu, Feb 20, 2025 at 11:02:58AM +0100, Miklos Szeredi wrote:
> >
> > > The solution is to just remove this truncation.  This can cause a
> > > regression in a filesystem that relies on supplying a symlink larger than
> > > the file size, but this is unlikely.  If that happens we'd need to make
> > > this behavior conditional.
> >
> > Note, BTW, that page *contents* must not change at all, so truncation is
> > only safe if we have ->i_size guaranteed to be stable.
> >
> > Core pathwalk really counts upon the string remaining immutable, and that
> > does include the string returned by ->get_link().
> 
> Page contents will not change after initial readlink, but page could
> get invalidated and a fresh page filled with new value for the same
> object.

*nod*

My point is that truncation of something that might be traversed by another
pathwalk is a hard bug - we only get away with doing that in page_get_link()
because it's idempotent.  If ->i_size can change, we are not allowed to
do that.

Re: [PATCH] fuse: don't truncate cached, mutated symlink

[Laura Promberger]

Hello Miklos,

Thank you so much for the patch!

I finally managed to build RHEL9 with your patch and I confirm that truncation of the symlinks is not happening anymore.

However, I will need to do some further testing as I see that sometimes the updating of symlinks is not propagated correctly.
This should not be a kernel problem.
I believe this is more a CVMFS problem, its revision-based update  process, that there is no way to pause user interaction with the fuse mount, and the asynchronous removal from inodes/dentries.

Thanks again
Laura


________________________________________
From: Al Viro <viro@ftp.linux.org.uk> on behalf of Al Viro <viro@zeniv.linux.org.uk>
Sent: Sunday, February 23, 2025 20:41
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: Miklos Szeredi <mszeredi@redhat.com>; linux-fsdevel@vger.kernel.org <linux-fsdevel@vger.kernel.org>; Christian Brauner <brauner@kernel.org>; Bernd Schubert <bernd.schubert@fastmail.fm>; Laura Promberger <laura.promberger@cern.ch>; Sam Lewis <samclewis@google.com>
Subject: Re: [PATCH] fuse: don't truncate cached, mutated symlink
 
On Sun, Feb 23, 2025 at 08:12:21PM +0100, Miklos Szeredi wrote:
> On Sun, 23 Feb 2025 at 01:28, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Thu, Feb 20, 2025 at 11:02:58AM +0100, Miklos Szeredi wrote:
> >
> > > The solution is to just remove this truncation.  This can cause a
> > > regression in a filesystem that relies on supplying a symlink larger than
> > > the file size, but this is unlikely.  If that happens we'd need to make
> > > this behavior conditional.
> >
> > Note, BTW, that page *contents* must not change at all, so truncation is
> > only safe if we have ->i_size guaranteed to be stable.
> >
> > Core pathwalk really counts upon the string remaining immutable, and that
> > does include the string returned by ->get_link().
>
> Page contents will not change after initial readlink, but page could
> get invalidated and a fresh page filled with new value for the same
> object.

*nod*

My point is that truncation of something that might be traversed by another
pathwalk is a hard bug - we only get away with doing that in page_get_link()
because it's idempotent.  If ->i_size can change, we are not allowed to
do that.