From 2b32b130f534068251ce9fd1b621de6e480d56d7 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Fri, 5 May 2023 16:47:45 -0500 Subject: [PATCH] LINUX: Set .getattr for symlinks On Linux, we can define a .getattr callback for files and directories, which is called when a caller requests metadata for the file, such as during lstat(). For regular files and directories, we set this to afs_linux_getattr(), which updates the metadata for the file in question if it's out of date (CStatd not set). For symlinks, however, we don't set .getattr at all. This would seem to allow symlink metadata to become stale if another client changes it, but the metadata often happens to stay up to date via other means. For example, we can see the following happen: - Another client changes, for example, the owner of a symlink we have cached. - The fileserver sends us a callback break, and so we clear CStatd for the vcache for the symlink. - lstat() is called for the symlink, which causes afs_linux_dentry_revalidate() to be called for the cached dentry. Since CStatd is not set for the vcache, we verify the entry by calling afs_lookup(), and then update the symlink's metadata via afs_getattr() and vattr2inode(). However, if CStatd _is_ set for the symlink when lstat() is called, afs_linux_dentry_revalidate() will not update anything, and will just return success. So, if we manage to set CStatd for the symlink without updating it's Linux VFS metadata, the Linux metadata won't be updated, and we'll report the old metadata to the caller of lstat(). We can set CStatd without updating the Linux VFS info in a few different ways. A few pioctls such as PRemoveMount or PFlushMount can do this if they encounter an error, but the only code paths that call these pioctls in OpenAFS (via the 'fs' utility) also lstat() the relevant path, so this doesn't happen in practice. A more common way that this can occur is via afs_DoBulkStat(). If userspace triggers a bulkstat that includes the symlink in question, the symlink would gain the CStatd flag without any interaction with the Linux VFS. For example, say a symlink was chown'd from 'adeason' to 'root'. On another client with the symlink cached, running 'ls -l' on the symlink itself would show the updated owner, because afs_linux_dentry_revalidate() updates the metadata: $ ls -l dir.slink lrwxr-xr-x. 1 root root 3 May 5 14:48 dir.slink -> dir But if we 'ls -l' the entire directory, which contains other entries, we will bulkstat many of the entries, possibly including the symlink. And so we may see the old metadata: $ ls -l total 9 [...] lrwxr-xr-x. 1 adeason root 3 May 5 14:48 dir.slink -> dir Triggering this behavior requires a bulkstat to be triggered before we access the symlink itself, and so triggering this behavior depends on the order of the entries in the directory as well as whether the other items in the dir are cached. As such, triggering this behavior during normal operation tends to be inconsistent and confusing. The only lstat() info for symlinks that can change like this is the owner, group, and modtime; mode bits cannot change, and neither can the length/size (or the contents in general). So, stale metadata tends to not be very noticeable. To fix all of this, set .getattr to afs_linux_getattr() for symlinks, just like we do for regular files and directories. This ensures that we will update the Linux VFS metadata for the symlink when it is requested, so we won't return stale metadata to callers. This behavior appears to have existed for symlinks on Linux for quite a while, possibly since our Linux 2.6 support was added. The behavoir may have been introduced around commit b860b359d58 (initial-linux24-support-20001105) or commit 00543744955 (linux22-fix-20040405). Before those commits, we defined a .revalidate callback for symlinks, which was called on older Linux versions before fetching file metadata, and so probably ensured that an lstat() on a symlink returned properly updated info. Change-Id: I90d751e3175e24e9f9409cea6c079b097c78b51b Reviewed-on: https://gerrit.openafs.org/15423 Reviewed-by: Cheyenne Wills Tested-by: BuildBot Reviewed-by: Benjamin Kaduk --- src/afs/LINUX/osi_vnodeops.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/afs/LINUX/osi_vnodeops.c b/src/afs/LINUX/osi_vnodeops.c index 27c0ea630b..aa71dc8d11 100644 --- a/src/afs/LINUX/osi_vnodeops.c +++ b/src/afs/LINUX/osi_vnodeops.c @@ -3742,6 +3742,7 @@ static struct inode_operations afs_symlink_iops = { .put_link = afs_linux_put_link, #endif /* USABLE_KERNEL_PAGE_SYMLINK_CACHE */ .setattr = afs_notify_change, + .getattr = afs_linux_getattr, }; void