[OpenAFS-devel] [PATCH] fix symlink crashes on RHEL4 kernels

Christopher Allen Wing wingc@engin.umich.edu
Thu, 22 Mar 2007 15:17:56 -0400 (EDT)


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
wingc@engin.umich.edu


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], [