I looked into this more closely and it's not as bad as I originally thought. The situation is:

linux-2.4 - 2.6.8
        Kernel provides page_readlink(), page_follow_link() API; is safe
        for OpenAFS to use.

linux-2.6.9
        Kernel provides page_readlink(), page_follow_link() API, and also
        generic_readlink() and page_follow_link_light() API.  The former
        is safe for OpenAFS to use, the latter (page_follow_link_light())
        will crash.

linux-2.6.10 - 2.6.12
        Kernel only provides page_follow_link_light() API,
        page_follow_link() is removed.  It is unsafe for OpenAFS to use
        symlink text caching on these kernels.

linux-2.6.13+
        Kernel developers realize their mistake and fix
        page_follow_link_light() API to no longer crash.  It is safe for
        OpenAFS to use symlink text caching on 2.6.13 and newer kernels.



Only linux-2.6.10 - 2.6.12 are broken. Unfortunately, the RHEL4 kernel contains the changes from 2.6.10 and thus OpenAFS will crash on all RHEL4 kernels.

Attached is my proposed patch to fix the problem. It disables symlink text caching altogether if a usable API is not found in the kernel. Autoconf tests are used to check the API, not the kernel release number. The result will be that people running RHEL4 kernels will get slightly less efficient path name lookups when symlinks are involved, but that's better than crashes.

I tested this on a 2.4 kernel (RHEL3), as well as a 2.6 kernel with the broken API (RHEL4), and a newer 2.6 kernel which does not have the broken API (RHEL5). The patch should make no changes whatsoever to the source code unless you are building on a broken 2.6 kernel.


The patch is against openafs-stable-1_4_x but it should also apply fine to HEAD, as there do not seem to be any conflicting changes. (the only changes I noticed in src/afs/LINUX are some code rearrangement and the addition of the NFS exporter)


You can also download this patch from:
        
http://www-personal.engin.umich.edu/~wingc/openafs/patches/openafs-stable-1_4_x-20070320-symlink26.patch


-Chris Wing
[EMAIL PROTECTED]


diff -uNr openafs.orig/acinclude.m4 openafs/acinclude.m4
--- openafs.orig/acinclude.m4   2007-02-22 16:48:58.000000000 -0500
+++ openafs/acinclude.m4        2007-03-22 11:24:15.000000000 -0400
@@ -606,6 +606,7 @@
                 LINUX_IOP_I_CREATE_TAKES_NAMEIDATA
                 LINUX_IOP_I_LOOKUP_TAKES_NAMEIDATA
                 LINUX_IOP_I_PERMISSION_TAKES_NAMEIDATA
+                LINUX_IOP_I_PUT_LINK_TAKES_COOKIE
                 LINUX_DOP_D_REVALIDATE_TAKES_NAMEIDATA
                 LINUX_AOP_WRITEBACK_CONTROL
                 LINUX_FS_STRUCT_FOP_HAS_FLOCK
@@ -848,6 +849,11 @@
                 if test "x$ac_cv_linux_exports_tasklist_lock" = "xyes" ; then
                  AC_DEFINE(EXPORTED_TASKLIST_LOCK, 1, [define if tasklist_lock 
exported])
                 fi
+                if test "x$ac_cv_linux_kernel_page_follow_link" = "xyes" -o 
"x$ac_cv_linux_func_i_put_link_takes_cookie" = "xyes"; then
+                 AC_DEFINE(USABLE_KERNEL_PAGE_SYMLINK_CACHE, 1, [define if 
your kernel has a usable symlink cache API])
+                else
+                 AC_MSG_WARN([your kernel does not have a usable symlink cache 
API])
+                fi
                 :
                fi
 esac
diff -uNr openafs.orig/src/afs/LINUX/osi_vnodeops.c 
openafs/src/afs/LINUX/osi_vnodeops.c
--- openafs.orig/src/afs/LINUX/osi_vnodeops.c   2007-02-20 13:06:24.000000000 
-0500
+++ openafs/src/afs/LINUX/osi_vnodeops.c        2007-03-22 11:39:13.000000000 
-0400
@@ -1279,7 +1279,7 @@
        return -code;
 }

-#if !defined(AFS_LINUX24_ENV)
+#if !defined(USABLE_KERNEL_PAGE_SYMLINK_CACHE)
 /* afs_linux_readlink
  * Fill target (which is in user space) with contents of symlink.
  */
@@ -1299,6 +1299,39 @@
 /* afs_linux_follow_link
  * a file system dependent link following routine.
  */
+#if defined(AFS_LINUX24_ENV)
+static int afs_linux_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+    int code;
+    char *name;
+
+    AFS_GLOCK();
+    name = osi_Alloc(PATH_MAX + 1);
+    if (!name) {
+       AFS_GUNLOCK();
+       return -EIO;
+    }
+
+    code = afs_linux_ireadlink(dentry->d_inode, name, PATH_MAX, AFS_UIOSYS);
+    AFS_GUNLOCK();
+
+    if (code < 0) {
+       goto out;
+    }
+
+    name[code] = '\0';
+    code = vfs_follow_link(nd, name);
+
+out:
+    AFS_GLOCK();
+    osi_Free(name, PATH_MAX + 1);
+    AFS_GUNLOCK();
+
+    return code;
+}
+
+#else /* !defined(AFS_LINUX24_ENV) */
+
 static struct dentry *
 afs_linux_follow_link(struct dentry *dp, struct dentry *basep,
                      unsigned int follow)
@@ -1332,7 +1365,8 @@
     AFS_GUNLOCK();
     return res;
 }
-#endif
+#endif /* AFS_LINUX24_ENV */
+#endif /* USABLE_KERNEL_PAGE_SYMLINK_CACHE */

 /* afs_linux_readpage
  * all reads come through here. A strategy-like read call.
@@ -1697,7 +1731,7 @@
 /* We really need a separate symlink set of ops, since do_follow_link()
  * determines if it _is_ a link by checking if the follow_link op is set.
  */
-#if defined(AFS_LINUX24_ENV)
+#if defined(USABLE_KERNEL_PAGE_SYMLINK_CACHE)
 static int
 afs_symlink_filler(struct file *file, struct page *page)
 {
@@ -1732,10 +1766,10 @@
 static struct address_space_operations afs_symlink_aops = {
   .readpage =  afs_symlink_filler
 };
-#endif
+#endif /* USABLE_KERNEL_PAGE_SYMLINK_CACHE */

 static struct inode_operations afs_symlink_iops = {
-#if defined(AFS_LINUX24_ENV)
+#if defined(USABLE_KERNEL_PAGE_SYMLINK_CACHE)
   .readlink =          page_readlink,
 #if defined(HAVE_KERNEL_PAGE_FOLLOW_LINK)
   .follow_link =       page_follow_link,
@@ -1743,13 +1777,17 @@
   .follow_link =       page_follow_link_light,
   .put_link =           page_put_link,
 #endif
-  .setattr =           afs_notify_change,
-#else
+#else /* !defined(USABLE_KERNEL_PAGE_SYMLINK_CACHE) */
   .readlink =          afs_linux_readlink,
   .follow_link =       afs_linux_follow_link,
+#if !defined(AFS_LINUX24_ENV)
   .permission =                afs_linux_permission,
   .revalidate =                afs_linux_revalidate,
 #endif
+#endif /* USABLE_KERNEL_PAGE_SYMLINK_CACHE */
+#if defined(AFS_LINUX24_ENV)
+  .setattr =           afs_notify_change,
+#endif
 };

 void
@@ -1775,7 +1813,7 @@

     } else if (S_ISLNK(ip->i_mode)) {
        ip->i_op = &afs_symlink_iops;
-#if defined(AFS_LINUX24_ENV)
+#if defined(USABLE_KERNEL_PAGE_SYMLINK_CACHE)
        ip->i_data.a_ops = &afs_symlink_aops;
        ip->i_mapping = &ip->i_data;
 #endif
diff -uNr openafs.orig/src/cf/linux-test4.m4 openafs/src/cf/linux-test4.m4
--- openafs.orig/src/cf/linux-test4.m4  2007-02-26 12:53:33.000000000 -0500
+++ openafs/src/cf/linux-test4.m4       2007-03-22 10:48:52.000000000 -0400
@@ -644,6 +644,22 @@
   AC_MSG_RESULT($ac_cv_linux_func_i_permission_takes_nameidata)])


+AC_DEFUN([LINUX_IOP_I_PUT_LINK_TAKES_COOKIE], [
+  AC_MSG_CHECKING([whether inode_operations.put_link takes an opaque cookie])
+  AC_CACHE_VAL([ac_cv_linux_func_i_put_link_takes_cookie], [
+    AC_TRY_KBUILD(
+[#include <linux/fs.h>
+#include <linux/namei.h>],
+[struct inode _inode;
+struct dentry _dentry;
+struct nameidata _nameidata;
+void *cookie;
+(void)_inode.i_op->put_link(&_dentry, &_nameidata, cookie);],
+      ac_cv_linux_func_i_put_link_takes_cookie=yes,
+      ac_cv_linux_func_i_put_link_takes_cookie=no)])
+  AC_MSG_RESULT($ac_cv_linux_func_i_put_link_takes_cookie)])
+
+
 AC_DEFUN([LINUX_DOP_D_REVALIDATE_TAKES_NAMEIDATA], [
   AC_MSG_CHECKING([whether dentry_operations.d_revalidate takes a nameidata])
   AC_CACHE_VAL([ac_cv_linux_func_d_revalidate_takes_nameidata], [
_______________________________________________
OpenAFS-devel mailing list
[email protected]
https://lists.openafs.org/mailman/listinfo/openafs-devel

Reply via email to