[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