kern/112658: [PATCH] Smbfs and caching problems (resolves
bin/111004)
Ganael LAPLANCHE
ganael.laplanche at martymac.com
Mon May 14 15:20:02 UTC 2007
>Number: 112658
>Category: kern
>Synopsis: [PATCH] Smbfs and caching problems (resolves bin/111004)
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Mon May 14 15:20:02 GMT 2007
>Closed-Date:
>Last-Modified:
>Originator: Ganael LAPLANCHE
>Release: 6.2-STABLE
>Organization:
http://contribs.martymac.com
>Environment:
FreeBSD home.martymac.com 6.2-STABLE FreeBSD 6.2-STABLE #0: Mon Apr 30 08:41:10 UTC 2007 martymac at home.martymac.com:/usr/src/sys/i386/compile/MYKERNEL i386
>Description:
This PR includes a patch for PR bin/111004.
I've encountered caching problems with smbfs mounts when the server is case-insensitive (e.g. with 'case sensitive = no' in smb.conf on the server - no problem with 'case sensitive = yes'). Sometimes, a wrong entry is created in the cache and does not match the real filename's case on the server. This happens because the server answers : 'yes, the file exists' while it has a different case. Sometimes then, this cache entry is found and used (because of cache's case-sensitiveness), while it should not.
----
Here is a test you can perform (on a smbfs mount point) :
$ touch foo && rm -f foo && touch FOO && rm -f FOO
This is to purge cache entries explicitly.
$ touch foo && ls -i foo
2864008422 foo
$ ls -i FOO
2864008422 FOO
This is OK since we should see the inode of foo.
$ rm -f foo
$ touch FOO && ls -i FOO
2864008422 FOO
Nope, we should get another inode (2322004806) since inodes depend on the filename and the parent directory. Here, a wrong cache entry has been used.
$ rm -f FOO
$ touch FOO && ls -i FOO
2322004806 FOO
This is OK again (because the cache entry has been explicitely removed before).
----
The attached patch is inspired from Opendarwin's code and uses the filename provided by the server when the file is found (instead of using the requested filename that may differ). This allows a better cache synchronization and solves this inode/caching problem.
Moreover, I've added a fix for PR bin/111004 which now allows to modify the case of a file (to 'rename' it).
This patch applies cleanly (and works ;-)) on 6.2-STABLE. It also applies to -CURRENT, but I have not tested it on this version.
Best regards,
--
Ganael LAPLANCHE.
ganael.laplanche at martymac.com
http://contribs.martymac.com
>How-To-Repeat:
Try the test above.
>Fix:
Patch attached with submission follows:
diff -aur smbfs.orig/smbfs_node.c smbfs/smbfs_node.c
--- smbfs.orig/smbfs_node.c Thu May 3 18:20:57 2007
+++ smbfs/smbfs_node.c Mon May 14 14:00:59 2007
@@ -108,7 +108,7 @@
return 0;
}
-static char *
+char *
smbfs_name_alloc(const u_char *name, int nmlen)
{
u_char *cp;
@@ -130,7 +130,7 @@
return cp;
}
-static void
+void
smbfs_name_free(u_char *name)
{
#ifdef SMBFS_NAME_DEBUG
diff -aur smbfs.orig/smbfs_node.h smbfs/smbfs_node.h
--- smbfs.orig/smbfs_node.h Thu May 3 18:20:57 2007
+++ smbfs/smbfs_node.h Mon May 14 14:00:59 2007
@@ -87,6 +87,9 @@
struct smbfattr *fap, struct vnode **vpp);
u_int32_t smbfs_hash(const u_char *name, int nmlen);
+char *smbfs_name_alloc(const u_char *name, int nmlen);
+void smbfs_name_free(u_char *name);
+
int smbfs_getpages(struct vop_getpages_args *);
int smbfs_putpages(struct vop_putpages_args *);
int smbfs_readvnode(struct vnode *vp, struct uio *uiop, struct ucred *cred);
diff -aur smbfs.orig/smbfs_smb.c smbfs/smbfs_smb.c
--- smbfs.orig/smbfs_smb.c Thu May 3 18:20:57 2007
+++ smbfs/smbfs_smb.c Mon May 14 14:00:59 2007
@@ -1470,37 +1470,50 @@
}
int
-smbfs_smb_lookup(struct smbnode *dnp, const char *name, int nmlen,
+smbfs_smb_lookup(struct smbnode *dnp, char **namep, int *nmlenp,
struct smbfattr *fap, struct smb_cred *scred)
{
struct smbfs_fctx *ctx;
int error;
- if (dnp == NULL || (dnp->n_ino == 2 && name == NULL)) {
+ if (dnp == NULL ||
+ (dnp->n_ino == 2 && (namep == NULL || *namep == NULL))) {
bzero(fap, sizeof(*fap));
fap->fa_attr = SMB_FA_DIR;
fap->fa_ino = 2;
return 0;
}
- if (nmlen == 1 && name[0] == '.') {
- error = smbfs_smb_lookup(dnp, NULL, 0, fap, scred);
+ if (nmlenp && *nmlenp == 1 && namep && (*namep)[0] == '.') {
+ error = smbfs_smb_lookup(dnp, NULL, NULL, fap, scred);
return error;
- } else if (nmlen == 2 && name[0] == '.' && name[1] == '.') {
- error = smbfs_smb_lookup(VTOSMB(dnp->n_parent), NULL, 0, fap,
- scred);
+ } else if (nmlenp && *nmlenp == 2 && namep && (*namep)[0] == '.' &&
+ (*namep)[1] == '.') {
+ error = smbfs_smb_lookup(VTOSMB(dnp->n_parent), NULL, NULL,
+ fap, scred);
printf("%s: knows NOTHING about '..'\n", __func__);
return error;
}
- error = smbfs_findopen(dnp, name, nmlen,
- SMB_FA_SYSTEM | SMB_FA_HIDDEN | SMB_FA_DIR, scred, &ctx);
+ error = smbfs_findopen(dnp, namep ? *namep : NULL, nmlenp ? *nmlenp : 0,
+ SMB_FA_SYSTEM | SMB_FA_HIDDEN | SMB_FA_DIR, scred, &ctx);
if (error)
return error;
ctx->f_flags |= SMBFS_RDD_FINDSINGLE;
error = smbfs_findnext(ctx, 1, scred);
if (error == 0) {
*fap = ctx->f_attr;
- if (name == NULL)
+ if (namep == NULL || *namep == NULL)
fap->fa_ino = dnp->n_ino;
+ if (namep && *namep && nmlenp && *nmlenp) {
+ /* Return the *real* name and length of the file
+ * found on the server if necessary. If a new allocation
+ * is done here, memory will be freed later */
+ if((ctx->f_nmlen != *nmlenp) ||
+ (bcmp(ctx->f_name, *namep, *nmlenp) != 0)) {
+ SMBVDEBUG("lookuped filename and server's filename differ\n");
+ *namep = smbfs_name_alloc((u_char *)(ctx->f_name), ctx->f_nmlen);
+ *nmlenp = ctx->f_nmlen;
+ }
+ }
}
smbfs_findclose(ctx, scred);
return error;
diff -aur smbfs.orig/smbfs_subr.h smbfs/smbfs_subr.h
--- smbfs.orig/smbfs_subr.h Thu May 3 18:20:57 2007
+++ smbfs/smbfs_subr.h Mon May 14 14:00:59 2007
@@ -171,7 +171,7 @@
int smbfs_findclose(struct smbfs_fctx *ctx, struct smb_cred *scred);
int smbfs_fullpath(struct mbchain *mbp, struct smb_vc *vcp,
struct smbnode *dnp, const char *name, int nmlen);
-int smbfs_smb_lookup(struct smbnode *dnp, const char *name, int nmlen,
+int smbfs_smb_lookup(struct smbnode *dnp, char **namep, int *nmlenp,
struct smbfattr *fap, struct smb_cred *scred);
int smbfs_fname_tolocal(struct smb_vc *vcp, char *name, int *nmlen, int caseopt);
diff -aur smbfs.orig/smbfs_vfsops.c smbfs/smbfs_vfsops.c
--- smbfs.orig/smbfs_vfsops.c Thu May 3 18:20:57 2007
+++ smbfs/smbfs_vfsops.c Mon May 14 14:00:59 2007
@@ -333,7 +333,7 @@
return vget(*vpp, LK_EXCLUSIVE | LK_RETRY, td);
}
smb_makescred(&scred, td, cred);
- error = smbfs_smb_lookup(NULL, NULL, 0, &fattr, &scred);
+ error = smbfs_smb_lookup(NULL, NULL, NULL, &fattr, &scred);
if (error)
return error;
error = smbfs_nget(mp, NULL, "TheRooT", 7, &fattr, &vp);
diff -aur smbfs.orig/smbfs_vnops.c smbfs/smbfs_vnops.c
--- smbfs.orig/smbfs_vnops.c Thu May 3 18:20:57 2007
+++ smbfs/smbfs_vnops.c Mon May 14 14:06:01 2007
@@ -264,14 +264,15 @@
u_quad_t oldsize;
int error;
- SMBVDEBUG("%lx: '%s' %d\n", (long)vp, np->n_name, (vp->v_vflag & VV_ROOT) != 0);
+ SMBVDEBUG("%lx: '%s' %d\n", (long)vp, np->n_name,
+ (vp->v_vflag & VV_ROOT) != 0);
error = smbfs_attr_cachelookup(vp, va);
if (!error)
return 0;
SMBVDEBUG("not in the cache\n");
smb_makescred(&scred, ap->a_td, ap->a_cred);
oldsize = np->n_size;
- error = smbfs_smb_lookup(np, NULL, 0, &fattr, &scred);
+ error = smbfs_smb_lookup(np, NULL, NULL, &fattr, &scred);
if (error) {
SMBVDEBUG("error %d\n", error);
return error;
@@ -488,15 +489,21 @@
error = smbfs_smb_create(dnp, name, nmlen, &scred);
if (error)
return error;
- error = smbfs_smb_lookup(dnp, name, nmlen, &fattr, &scred);
+ /* smbfs_smb_lookup can allocate a new "name" so use "out" from
+ * here on */
+ error = smbfs_smb_lookup(dnp, &name, &nmlen, &fattr, &scred);
if (error)
- return error;
+ goto out;
error = smbfs_nget(VTOVFS(dvp), dvp, name, nmlen, &fattr, &vp);
if (error)
- return error;
+ goto out;
*vpp = vp;
if (cnp->cn_flags & MAKEENTRY)
cache_enter(dvp, vp, cnp);
+
+out:
+ if (name != cnp->cn_nameptr)
+ smbfs_name_free((u_char *)name);
return error;
}
@@ -689,14 +696,21 @@
error = smbfs_smb_mkdir(dnp, name, len, &scred);
if (error)
return error;
- error = smbfs_smb_lookup(dnp, name, len, &fattr, &scred);
+ /* smbfs_smb_lookup can allocate a new "name" so use "out" from
+ * here on */
+ error = smbfs_smb_lookup(dnp, &name, &len, &fattr, &scred);
if (error)
- return error;
+ goto out;
error = smbfs_nget(VTOVFS(dvp), dvp, name, len, &fattr, &vp);
if (error)
- return error;
+ goto out;
*ap->a_vpp = vp;
- return 0;
+ error = 0;
+
+out:
+ if (name != cnp->cn_nameptr)
+ smbfs_name_free((u_char *)name);
+ return error;
}
/*
@@ -1100,7 +1114,7 @@
cp = name + nmlen;
c = *cp;
*cp = 0;
- SMBVDEBUG("%d '%s' in '%s' id=d\n", nameiop, name,
+ SMBVDEBUG("nameiop = %d for '%s' in '%s'\n", nameiop, name,
VTOSMB(dvp)->n_name);
*cp = c;
}
@@ -1120,7 +1134,8 @@
return ENOENT;
error = cache_lookup(dvp, vpp, cnp);
- SMBVDEBUG("cache_lookup returned %d\n", error);
+ SMBVDEBUG("cache_lookup for '%s' returned %d\n", cnp->cn_nameptr,
+ error);
if (error > 0)
return error;
if (error) { /* name was found */
@@ -1144,10 +1159,10 @@
killit = 1;
else if (error == 0
/* && vattr.va_ctime.tv_sec == VTOSMB(vp)->n_ctime*/) {
- if (nameiop != LOOKUP && islastcn)
- cnp->cn_flags |= SAVENAME;
- SMBVDEBUG("use cached vnode\n");
- return (0);
+ if (nameiop != LOOKUP && islastcn)
+ cnp->cn_flags |= SAVENAME;
+ SMBVDEBUG("use cached vnode\n");
+ return (0);
}
cache_purge(vp);
/*
@@ -1165,77 +1180,99 @@
*vpp = NULLVP;
}
/*
- * entry is not in the cache or has been expired
+ * entry is not in the cache, has been expired
+ * or entry in the cache did not match input
+ * filename's case
*/
error = 0;
*vpp = NULLVP;
smb_makescred(&scred, td, cnp->cn_cred);
fap = &fattr;
if (flags & ISDOTDOT) {
- error = smbfs_smb_lookup(VTOSMB(dnp->n_parent), NULL, 0, fap,
- &scred);
- SMBVDEBUG("result of dotdot lookup: %d\n", error);
+ error = smbfs_smb_lookup(VTOSMB(dnp->n_parent), NULL, NULL, fap,
+ &scred);
+ SMBVDEBUG("result of dotdot smbfs_smb_lookup: %d\n", error);
} else {
- fap = &fattr;
- error = smbfs_smb_lookup(dnp, name, nmlen, fap, &scred);
+ /* smbfs_smb_lookup can allocate a new "name" so use "out" from
+ * here on */
/* if (cnp->cn_namelen == 1 && cnp->cn_nameptr[0] == '.')*/
+ error = smbfs_smb_lookup(dnp, &name, &nmlen, fap, &scred);
SMBVDEBUG("result of smbfs_smb_lookup: %d\n", error);
}
if (error && error != ENOENT)
- return error;
- if (error) { /* entry not found */
+ goto out;
+ if (error) {
+ /* entry not found */
+ SMBVDEBUG("entry not found on server\n");
+
/*
* Handle RENAME or CREATE case...
*/
if ((nameiop == CREATE || nameiop == RENAME) && islastcn) {
error = VOP_ACCESS(dvp, VWRITE, cnp->cn_cred, td);
if (error)
- return error;
+ goto out;
cnp->cn_flags |= SAVENAME;
- return (EJUSTRETURN);
+ error = EJUSTRETURN;
+ goto out;
}
- return ENOENT;
- }/* else {
- SMBVDEBUG("Found entry %s with id=%d\n", fap->entryName, fap->dirEntNum);
- }*/
+ error = ENOENT;
+ goto out;
+ }
+
+ /* entry found */
+ SMBVDEBUG("entry found on server: '%s'\n", name);
+
/*
* handle DELETE case ...
*/
if (nameiop == DELETE && islastcn) { /* delete last component */
error = VOP_ACCESS(dvp, VWRITE, cnp->cn_cred, td);
if (error)
- return error;
+ goto out;
if (isdot) {
VREF(dvp);
*vpp = dvp;
- return 0;
+ error = 0;
+ goto out;
}
error = smbfs_nget(mp, dvp, name, nmlen, fap, &vp);
if (error)
- return error;
+ goto out;
*vpp = vp;
cnp->cn_flags |= SAVENAME;
- return 0;
+ error = 0;
+ goto out;
}
if (nameiop == RENAME && islastcn) {
+ if (name != cnp->cn_nameptr) {
+ /* Target has been found on the server. Just return here
+ * to avoir falling to the source vnode, which would lead
+ * to NOT call the rename syscall */
+ error = EJUSTRETURN;
+ goto out;
+ }
error = VOP_ACCESS(dvp, VWRITE, cnp->cn_cred, td);
if (error)
- return error;
- if (isdot)
- return EISDIR;
+ goto out;
+ if (isdot) {
+ error = EISDIR;
+ goto out;
+ }
error = smbfs_nget(mp, dvp, name, nmlen, fap, &vp);
if (error)
- return error;
+ goto out;
*vpp = vp;
cnp->cn_flags |= SAVENAME;
- return 0;
+ error = 0;
+ goto out;
}
if (flags & ISDOTDOT) {
VOP_UNLOCK(dvp, 0, td);
error = smbfs_nget(mp, dvp, name, nmlen, NULL, &vp);
vn_lock(dvp, LK_EXCLUSIVE | LK_RETRY, td);
if (error)
- return error;
+ goto out;
*vpp = vp;
} else if (isdot) {
vref(dvp);
@@ -1243,7 +1280,7 @@
} else {
error = smbfs_nget(mp, dvp, name, nmlen, fap, &vp);
if (error)
- return error;
+ goto out;
*vpp = vp;
SMBVDEBUG("lookup: getnewvp!\n");
}
@@ -1251,5 +1288,10 @@
/* VTOSMB(*vpp)->n_ctime = VTOSMB(*vpp)->n_vattr.va_ctime.tv_sec;*/
cache_enter(dvp, *vpp, cnp);
}
- return 0;
+ error = 0;
+
+out:
+ if (name != cnp->cn_nameptr)
+ smbfs_name_free((u_char *)name);
+ return error;
}
>Release-Note:
>Audit-Trail:
>Unformatted:
More information about the freebsd-bugs
mailing list