From d7211350eec18b30e9ccf30f5e95fb58162c637d Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 15 Jun 2017 15:29:17 -0500 Subject: [PATCH] afs_linux_lookup: Avoid d_add on afs_lookup error Currently, afs_linux_lookup looks roughly like this pseudocode: { code = afs_lookup(&vcp); if (!code) { ip = AFSTOV(vcp); error = process_ip(ip); if (error) { goto done; } } process_dp(dp); newdp = d_splice_alias(ip, dp); done: cleanup(); } Note that if there is an error while processing the looked-up inode (ip), we jump over d_splice_alias. But if we encounter an error from afs_lookup itself, we do not jump over d_splice_alias. This means that if afs_lookup encounters any error, we initialize the given dentry (dp) as a negative entry, effectively telling the Linux kernel that the requested name does not exist. This is correct for ENOENT errors, of course, but is incorrect for any other error. For non-ENOENT errors we later return an error from the function, but this does not invalidate the generated dentry. The result is that when afs_lookup encounters an error, that error will be propagated to userspace, but subsequent lookups for the same name will yield an ENOENT error (until the dentry is invalidated). This can easily cause a file to seem to mysteriously disappear, if a transient error like network problems caused the afs_lookup call to fail. To fix this, treat ENOENT as a non-error, like the comments already suggest. In our case, ENOENT is not really an error; it just means we populate the given dentry differently. So if we get ENOENT from afs_lookup, set our vcache to NULL and clear the error, and continue. This also has the side effect of not treating ENOENT errors from afs_CreateAttr identically to ENOENT errors from afs_lookup. That shouldn't happen, but there have been abuses of the ENOENT error code in the past, so it is probably better to be cautious. Many thanks to Gaja Sophie Peters for assistance in tracking down and testing fixes for this issue, including providing access to test systems experiencing the buggy behavior. FIXES 133654 Change-Id: Ia9aab289d5c041557ab6b00f1d41de2edfc97a89 Reviewed-on: https://gerrit.openafs.org/12637 Reviewed-by: Benjamin Kaduk Tested-by: BuildBot Reviewed-by: Joe Gorse Reviewed-by: Michael Meffie Tested-by: Michael Meffie --- src/afs/LINUX/osi_vnodeops.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/afs/LINUX/osi_vnodeops.c b/src/afs/LINUX/osi_vnodeops.c index 97b5486..357751c 100644 --- a/src/afs/LINUX/osi_vnodeops.c +++ b/src/afs/LINUX/osi_vnodeops.c @@ -1525,9 +1525,21 @@ afs_linux_lookup(struct inode *dip, struct dentry *dp) int code; AFS_GLOCK(); + code = afs_lookup(VTOAFS(dip), (char *)comp, &vcp, credp); + if (code == ENOENT) { + /* It's ok for the file to not be found. That's noted by the caller by + * seeing that the dp->d_inode field is NULL (set by d_splice_alias or + * d_add, below). */ + code = 0; + osi_Assert(vcp == NULL); + } + if (code) { + AFS_GUNLOCK(); + goto done; + } - if (!code) { + if (vcp) { struct vattr *vattr = NULL; struct vcache *parent_vc = VTOAFS(dip); @@ -1603,9 +1615,7 @@ afs_linux_lookup(struct inode *dip, struct dentry *dp) return NULL; } - /* It's ok for the file to not be found (ENOENT). That's noted by the - * caller by seeing that the dp->d_inode field is NULL. */ - if (code && code != ENOENT) { + if (code) { if (ip) iput(ip); return ERR_PTR(afs_convert_code(code)); -- 1.9.4