On 9/25/24 16:25, Miklos Szeredi wrote: > On Wed, 25 Sept 2024 at 16:07, Bernd Schubert > <bernd.schubert@xxxxxxxxxxx> wrote: >> >> Hi Miklos, >> >> On 9/25/24 14:20, Miklos Szeredi wrote: >>> On Thu, 15 Aug 2024 at 16:45, Laura Promberger <laura.promberger@xxxxxxx> wrote: >>> >>>> - But for corrupted symlinks `fuse_change_attributes()` exits before `fuse_change_attributes_common()` is called and as such the length stays the old one. >>> >>> The reason is that the attr_version check fails. The trace logs show >>> a zero attr_version value, which suggests that the check can not fail. >>> But we know that fuse_dentry_revalidate() supplies a non-zero >>> attr_version to fuse_change_attributes() and if there's a racing >>> fuse_reverse_inval_inode() which updates the fuse_inode's >>> attr_version, then it would result in fuse_change_attributes() exiting >>> before updating the cached attributes, which is what you observe. >> >> >> I'm a bit confused by this, especially due to "fuse_reverse_inval_inode()", >> isn't this about FUSE_NOTIFY_INVAL_ENTRY and the additional flag >> FUSE_EXPIRE_ONLY? I.e. the used code path is fuse_reverse_inval_entry()? >> And that path doesn't change the attr_version? Which I'm also confused >> about. > > The trace does have several fuse_reverse_inval_inode() calls, which > made me conclude that this was the cause. Yeah, you are right, I checked cvmfs and it uses both. > >>> This is probably 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 for cvmfs it causes problems. >>> >>> My proposed solution would be 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. >> >> I wonder if we can just repeat operations if we detect changes in the >> middle. Hard started to work on a patch, but got distracted and I >> first would like to create a passthrough reproducer. > > I think in this case it's much cleaner to just ignore the file size. > Old, non-cached readlink code never did anything with i_size, why > should the cached one care about it? Yeah, I see your point. (Probably just my too long out-of-tree habit to avoid vfs changes whenever possible). Thanks, Bernd
