On Thu, Apr 23, 2009 at 04:27:05PM -0400, bfields wrote:
> On Thu, Apr 23, 2009 at 03:40:23PM +0900, hooanon05@xxxxxxxxxxx wrote:
> >
> > "J. Bruce Fields":
> > > > Isn't it better to test it BEFORE fh_compose()?
> > :::
> > > Yes, I think you're right.
> >
> > Then here you are.
>
> The nfsv4 readdir callback needs a similar fix.
>
> Also, it looks to me like this results in us encoding an entry for this
> deleted file in the readdir reply, but with an empty filehandle. From a
> quick glance at the rfc it's not clear to me whether this is really
> legal. I suspect it may cause odd behavior on clients. At the least it
> would seem cleaner to check for this condition early enough that we can
> just skip the entry entirely.
Err, no, I was confused, the v3 spec does clearly state that the
filehandle field here is just an optional optimization.
But now that I look fh_compose() seems perfectly capable of dealing with
negative dentries, so I don't think your patch is necessary after all.
In the v4 case it looks like we actually have to return an error (if
only in the special rdattr_error attribute defined for this case), and
it seems to me that a client might legimately be unhappy with a server
returning a readdir entry with the rdattr_error set to -ENOENT. So I
think we need something like the following for v4.--b.
commit 97a452f918403493309357146af3d8d8360be970
Author: J. Bruce Fields <bfields@xxxxxxxxxxxxxx>
Date: Tue May 5 19:04:29 2009 -0400
nfsd4: check for negative dentry before use in nfsv4 readdir
After 2f9092e1020246168b1309b35e085ecd7ff9ff72 "Fix i_mutex vs. readdir
handling in nfsd" (and 14f7dd63 "Copy XFS readdir hack into nfsd code"),
an entry may be removed between the first mutex_unlock and the second
mutex_lock. In this case, lookup_one_len() will return a negative
dentry. Check for this case to avoid a NULL dereference.
Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxx
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index b820c31..fe91427 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2204,14 +2204,12 @@ static inline int attributes_need_mount(u32 *bmval)
static __be32
nfsd4_encode_dirent_fattr(struct nfsd4_readdir *cd,
- const char *name, int namlen, __be32 *p, int *buflen)
+ struct dentry *dentry, __be32 *p, int *buflen)
{
struct svc_export *exp = cd->rd_fhp->fh_export;
- struct dentry *dentry;
__be32 nfserr;
int ignore_crossmnt = 0;
- dentry = lookup_one_len(name, cd->rd_fhp->fh_dentry, namlen);
if (IS_ERR(dentry))
return nfserrno(PTR_ERR(dentry));
@@ -2274,6 +2272,7 @@ nfsd4_encode_dirent(void *ccdv, const char *name, int namlen,
{
struct readdir_cd *ccd = ccdv;
struct nfsd4_readdir *cd = container_of(ccd, struct nfsd4_readdir, common);
+ struct dentry *dentry;
int buflen;
__be32 *p = cd->buffer;
__be32 nfserr = nfserr_toosmall;
@@ -2283,20 +2282,32 @@ nfsd4_encode_dirent(void *ccdv, const char *name, int namlen,
cd->common.err = nfs_ok;
return 0;
}
+ dentry = lookup_one_len(name, cd->rd_fhp->fh_dentry, namlen);
+ /* Whoops, it vanished after we dropped the i_sem: */
+ if (!IS_ERR(dentry) && !dentry->d_inode)
+ return 0;
+ /*
+ * (Note that nfsd4_encode_dirent_fattr() checks for other
+ * lookup errors below, allowing us to return them in the
+ * RDATTR_ERROR attribute instead of failing the whole readdir.
+ */
if (cd->offset)
xdr_encode_hyper(cd->offset, (u64) offset);
buflen = cd->buflen - 4 - XDR_QUADLEN(namlen);
- if (buflen < 0)
+ if (buflen < 0) {
+ dput(dentry);
goto fail;
+ }
*p++ = xdr_one; /* mark entry present */
cd->offset = p; /* remember pointer */
p = xdr_encode_hyper(p, NFS_OFFSET_MAX); /* offset of next entry */
p = xdr_encode_array(p, name, namlen); /* name length & name */
- nfserr = nfsd4_encode_dirent_fattr(cd, name, namlen, p, &buflen);
+ /* This consumes a reference to dentry: */
+ nfserr = nfsd4_encode_dirent_fattr(cd, dentry, p, &buflen);
switch (nfserr) {
case nfs_ok:
p += buflen;
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
