[OpenAFS-devel] [in progress PATCH] fix openafs symlink crashes on pre-2.6.13 kernels

Christopher Allen Wing wingc@engin.umich.edu
Wed, 21 Mar 2007 18:05:50 -0400 (EDT)


Here is a first cut at a patch.  It survives the test which otherwise 
reliably crashes my RHEL4 kernel (fs flushvol while traversing symlinks).

The patch combines several changes:

     - autoconf test to check if your kernel has working page_*link() API
     - adds alternate symlink cache code which is compiled if you have
       pre-2.6.13 kernel
     - disables symlink caching on 2.4 (because I assume it's also broken)
     - remove references to page_follow_link(), since this function is now
       known to be unusable
     - use generic_readlink() on 2.6 since that seems to be the way to go
     - removes a bit of unnecessary code for 2.6.13+

The patch is against openafs-stable-1_4_x.  I have only tested it so far 
on a RHEL4 kernel.


This patch borrows a bit of code from the NFS client in the pre-2.6.13 
kernel, so there may be licensing issues to resolve if we want to try to 
use that approach to enable symlink caching.


I'll come up with an simpler patch tomorrow that just disables the use of 
the symlink cache entirely unless the kernel has the non-broken API. 
This will punish people who run old kernels by slowing down path lookups 
involving symlinks, but that's better than crashing.  If that seems 
reasonable then you can decide if it's worth the effort to try to get 
non-crashing symlink caching working on pre-2.6.13 kernels.


-Chris
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-20 22:28: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
@@ -833,6 +834,9 @@
  		 if test "x$ac_cv_linux_func_i_permission_takes_nameidata" = "xyes" ; then
  		  AC_DEFINE(IOP_PERMISSION_TAKES_NAMEIDATA, 1, [define if your iops.permission takes a nameidata argument])
  		 fi
+		 if test "x$ac_cv_linux_func_i_put_link_takes_cookie" = "xyes" ; then
+		  AC_DEFINE(IOP_PUT_LINK_TAKES_COOKIE, 1, [define if your iops.put_link takes an opaque cookie])
+		 fi
  		 if test "x$ac_cv_linux_func_d_revalidate_takes_nameidata" = "xyes" ; then
  		  AC_DEFINE(DOP_REVALIDATE_TAKES_NAMEIDATA, 1, [define if your dops.d_revalidate takes a nameidata argument])
  		 fi
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-21 17:31:47.000000000 -0400
@@ -47,6 +47,9 @@

  #if defined(AFS_LINUX26_ENV)
  #define UnlockPage(pp) unlock_page(pp)
+#if !defined(IOP_PUT_LINK_TAKES_COOKIE)
+#include "safe_page_symlink.h"
+#endif
  #endif

  extern struct vcache *afs_globalVp;
@@ -1279,7 +1282,7 @@
  	return -code;
  }

-#if !defined(AFS_LINUX24_ENV)
+#if !defined(AFS_LINUX26_ENV)
  /* afs_linux_readlink
   * Fill target (which is in user space) with contents of symlink.
   */
@@ -1299,6 +1302,40 @@
  /* 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;
+    }
+
+    AFS_GLOCK();
+    code = afs_linux_ireadlink(dp->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,6 +1369,7 @@
      AFS_GUNLOCK();
      return res;
  }
+#endif /* AFS_LINUX24_ENV */
  #endif

  /* afs_linux_readpage
@@ -1697,17 +1735,24 @@
  /* 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)
-static int
-afs_symlink_filler(struct file *file, struct page *page)
+#if defined(AFS_LINUX26_ENV)
+
+#if !defined(IOP_PUT_LINK_TAKES_COOKIE)
+#define LINK_TXT_OFFSET	offsetof(struct safe_page_symlink, body)
+static int afs_symlink_filler(struct inode *ip, struct page *page)
+{
+#else
+#define LINK_TXT_OFFSET	0
+static int afs_symlink_readpage(struct file *file, struct page *page)
  {
      struct inode *ip = (struct inode *)page->mapping->host;
-    char *p = (char *)kmap(page);
+#endif
+    char *p = (char *)kmap(page) + LINK_TXT_OFFSET;
      int code;

      lock_kernel();
      AFS_GLOCK();
-    code = afs_linux_ireadlink(ip, p, PAGE_SIZE, AFS_UIOSYS);
+    code = afs_linux_ireadlink(ip, p, PAGE_SIZE - LINK_TXT_OFFSET, AFS_UIOSYS);
      AFS_GUNLOCK();

      if (code < 0)
@@ -1729,27 +1774,42 @@
      return code;
  }

+/* work around broken symlink cache in pre-2.6.11 */
+#if !defined(IOP_PUT_LINK_TAKES_COOKIE)
+static int afs_safe_page_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+    nd_set_link(nd, afs_safe_page_get_link(dentry->d_inode, afs_symlink_filler));
+    return 0;
+}
+#else	/* defined(IOP_PUT_LINK_TAKES_COOKIE) */
  static struct address_space_operations afs_symlink_aops = {
-  .readpage =	afs_symlink_filler
+  .readpage =	afs_symlink_readpage
  };
+#endif	/* IOP_PUT_LINK_TAKES_COOKIE */
  #endif

  static struct inode_operations afs_symlink_iops = {
-#if defined(AFS_LINUX24_ENV)
-  .readlink = 		page_readlink,
-#if defined(HAVE_KERNEL_PAGE_FOLLOW_LINK)
-  .follow_link =	page_follow_link,
+#if defined(AFS_LINUX26_ENV)
+  .readlink = 		generic_readlink,
+/* prior to 2.6.13, page_*link() is unsafe to use */
+#if !defined(IOP_PUT_LINK_TAKES_COOKIE)
+  .follow_link =	afs_safe_page_follow_link,
+  .put_link =		afs_safe_page_put_link,
  #else
    .follow_link =	page_follow_link_light,
    .put_link =           page_put_link,
  #endif
-  .setattr =		afs_notify_change,
-#else
+#else /* !defined(AFS_LINUX26_ENV) */
    .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 /* AFS_LINUX26_ENV */
+#if defined(AFS_LINUX24_ENV)
+  .setattr =		afs_notify_change,
+#endif
  };

  void
@@ -1775,9 +1835,10 @@

      } else if (S_ISLNK(ip->i_mode)) {
  	ip->i_op = &afs_symlink_iops;
-#if defined(AFS_LINUX24_ENV)
+#if defined(AFS_LINUX26_ENV)
+#if defined(IOP_PUT_LINK_TAKES_COOKIE)
  	ip->i_data.a_ops = &afs_symlink_aops;
-	ip->i_mapping = &ip->i_data;
+#endif
  #endif
      }

diff -uNr openafs.orig/src/afs/LINUX/safe_page_symlink.c openafs/src/afs/LINUX/safe_page_symlink.c
--- openafs.orig/src/afs/LINUX/safe_page_symlink.c	1969-12-31 19:00:00.000000000 -0500
+++ openafs/src/afs/LINUX/safe_page_symlink.c	2007-03-21 16:59:32.000000000 -0400
@@ -0,0 +1,55 @@
+/* work around broken symlink cache in pre-2.6.13, based on linux kernel
+   NFS client code; see:
+
+	http://www.uwsg.indiana.edu/hypermail/linux/kernel/0508.2/0923.html
+	http://marc.info/?l=linux-kernel&m=112447444702991
+ */
+
+#if !defined(IOP_PUT_LINK_TAKES_COOKIE)
+
+#include <afsconfig.h>
+#include "afs/param.h"
+#include "afs/sysincludes.h"
+#include "afsincludes.h"
+
+#include "safe_page_symlink.h"
+
+char *afs_safe_page_get_link(struct inode *inode, int (*filler)(struct inode *, struct page *))
+{
+	struct safe_page_symlink *p;
+	struct page *page;
+
+	page = read_cache_page(&inode->i_data, 0, (filler_t *)filler, inode);
+	if (IS_ERR(page)) {
+		goto read_failed;
+	}
+	if (!PageUptodate(page)) {
+		goto getlink_read_error;
+	}
+	p = kmap(page);
+	p->page = page;
+	return p->body;
+
+getlink_read_error:
+	page_cache_release(page);
+	page = ERR_PTR(-EIO);
+read_failed:
+	return (char *)page;	/* error */
+}
+
+void afs_safe_page_put_link(struct dentry *dentry, struct nameidata *nd)
+{
+	char *s = nd_get_link(nd);
+	if (!IS_ERR(s)) {
+		struct safe_page_symlink *p;
+		struct page *page;
+
+		p = container_of(s, struct safe_page_symlink, body[0]);
+		page = p->page;
+
+		kunmap(page);
+		page_cache_release(page);
+	}
+}
+
+#endif
diff -uNr openafs.orig/src/afs/LINUX/safe_page_symlink.h openafs/src/afs/LINUX/safe_page_symlink.h
--- openafs.orig/src/afs/LINUX/safe_page_symlink.h	1969-12-31 19:00:00.000000000 -0500
+++ openafs/src/afs/LINUX/safe_page_symlink.h	2007-03-21 16:59:25.000000000 -0400
@@ -0,0 +1,13 @@
+/* work around broken symlink cache in pre-2.6.13 kernel, see:
+
+	http://www.uwsg.indiana.edu/hypermail/linux/kernel/0508.2/0923.html
+	http://marc.info/?l=linux-kernel&m=112447444702991
+ */
+
+struct safe_page_symlink {
+	struct page *page;
+	char body[0];
+};
+
+char *afs_safe_page_get_link(struct inode *inode, int (*filler)(struct inode *, struct page *));
+void afs_safe_page_put_link(struct dentry *dentry, struct nameidata *nd);
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-20 22:20:06.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], [
diff -uNr openafs.orig/src/libafs/Makefile.common.in openafs/src/libafs/Makefile.common.in
--- openafs.orig/src/libafs/Makefile.common.in	2006-06-20 17:40:46.000000000 -0400
+++ openafs/src/libafs/Makefile.common.in	2007-03-21 17:07:19.000000000 -0400
@@ -394,6 +394,8 @@
  	$(CRULE_NOOPT)
  osi_sysctl.o: $(TOP_SRCDIR)/afs/$(MKAFS_OSTYPE)/osi_sysctl.c
  	$(CRULE_NOOPT)
+safe_page_symlink.o: $(TOP_SRCDIR)/afs/$(MKAFS_OSTYPE)/safe_page_symlink.c
+	$(CRULE_NOOPT)
  osi_flush.o: $(TOP_SRCDIR)/afs/$(MKAFS_OSTYPE)/osi_flush.s
  	$(CRULE_OPT)
  osi_alloc.o: $(TOP_SRCDIR)/afs/$(MKAFS_OSTYPE)/osi_alloc.c
diff -uNr openafs.orig/src/libafs/MakefileProto.LINUX.in openafs/src/libafs/MakefileProto.LINUX.in
--- openafs.orig/src/libafs/MakefileProto.LINUX.in	2005-12-01 10:19:38.000000000 -0500
+++ openafs/src/libafs/MakefileProto.LINUX.in	2007-03-21 17:06:08.000000000 -0400
@@ -26,6 +26,8 @@
  	osi_vm.o \
  <ppc64_linux26>
  	osi_flush.o \
+<linux26 linux_26>
+	safe_page_symlink.o \
  <all>
  	osi_vnodeops.o