On Wed, Jul 16, 2014 at 02:57:24PM -0400, J. Bruce Fields wrote: > On Sat, Jul 12, 2014 at 08:31:15PM +0800, Kinglong Mee wrote: > > I think it is caused by kfree an uninitialized address. > > Can you test with the patch listed in following url, > > I have send some days before ? > > > > "[PATCH 1/4] NFSD: Fix memory leak in encoding denied lock" > > http://www.spinics.net/lists/linux-nfs/msg44719.html > > I have this queued for 3.17, but if it causes a crash then it should go > to 3.16 now. > > However, I'm confused: the only explicit initialization of lk_denied is > in the case vfs_lock_file() returns -EAGAIN. Our usual tests (cthon, > pynfs) do lots of succesful locks, so should have hit this before. > > OK, I see: this memory zeroed by a memset in svc_process_common(): > > memset(rqstp->rq_argp, 0, procp->pc_argsize); > > *But* in the case of the NFSv4 compound operation, we only have enough > space in rq_argp for 8 operations, anything more is allocated in > fs/nfsd/nfs4xdr.c:nfsd4_decode_compound: > > if (argp->opcnt > ARRAY_SIZE(argp->iops)) { > argp->ops = kmalloc(argp->opcnt * sizeof(*argp->ops), GFP_KERNEL); > ... > > So, perhaps we got a compound with more than 8 operations, with the LOCK > operation in the 9th or later position? > > But the Linux NFS client doesn't do that, so I don't understand how > Toralf hit this. > > Am I missing anything here? > > Toralf, is that crash reproduceable? If so, does replacing the above > kmalloc by a kcalloc also fix it? Sorry, that should be kzalloc. We should probably fix that regardless. But I still don't understand how you hit this case.... --b. commit 5d6031ca742f Author: J. Bruce Fields <bfields@xxxxxxxxxx> Date: Thu Jul 17 16:20:39 2014 -0400 nfsd4: zero op arguments beyond the 8th compound op The first 8 ops of the compound are zeroed since they're a part of the argument that's zeroed by the memset(rqstp->rq_argp, 0, procp->pc_argsize); in svc_process_common(). But we handle larger compounds by allocating the memory on the fly in nfsd4_decode_compound(). Other than code recently fixed by 01529e3f8179 "NFSD: Fix memory leak in encoding denied lock", I don't know of any examples of code depending on this initialization. But it definitely seems possible, and I'd rather be safe. Compounds this long are unusual so I'm much more worried about failure in this poorly tested cases than about an insignificant performance hit. Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 01023a595163..628b430e743e 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -1635,7 +1635,7 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp) goto xdr_error; if (argp->opcnt > ARRAY_SIZE(argp->iops)) { - argp->ops = kmalloc(argp->opcnt * sizeof(*argp->ops), GFP_KERNEL); + argp->ops = kzalloc(argp->opcnt * sizeof(*argp->ops), GFP_KERNEL); if (!argp->ops) { argp->ops = argp->iops; dprintk("nfsd: couldn't allocate room for COMPOUND\n"); -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
