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 <kaduk@mit.edu>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Joe Gorse <jhgorse@gmail.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: Michael Meffie <mmeffie@sinenomine.net>

diff --git a/src/afs/LINUX/osi_vnodeops.c b/src/afs/LINUX/osi_vnodeops.c
index 97b5486..357751c 100644 (file)
--- 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));