Re: Issues with a rather unusual configured NFS server

[J. Bruce Fields]

On Tue, Aug 13, 2013 at 05:53:14PM -0400, bfields wrote:
> On Mon, Aug 12, 2013 at 04:36:40PM +0200, Jan Kara wrote:
> > On Sun 11-08-13 11:48:49, Toralf FÃrster wrote:
> > > so that the server either crashes (if it is a user mode linux image) or at least its reboot functionality got broken
> > > - if the NFS server is hammered with scary NFS calls using a fuzzy tool running at a remote NFS client under a non-privileged user id.
> > > 
> > > It can re reproduced, if
> > > 	- the NFS share is an EXT3 or EXT4 directory
> > > 	- and it is created at file located at tempfs and mounted via loop device
> > > 	- and the NFS server is forced to umount the NFS share
> > > 	- and the server forced to restart the NSF service afterwards
> > > 	- and trinity is used
> > > 
> > > I could find a scenario for an automated bisect. 2 times it brought this commit 
> > > commit 68a3396178e6688ad7367202cdf0af8ed03c8727
> > > Author: J. Bruce Fields <bfields@xxxxxxxxxx>
> > > Date:   Thu Mar 21 11:21:50 2013 -0400
> > > 
> > >     nfsd4: shut down more of delegation earlier
> 
> Thanks for the report.  I think I see the problem--after this commit
> nfs4_set_delegation() failures result in nfs4_put_delegation being
> called, but nfs4_put_delegation doesn't free the nfs4_file that has
> already been set by alloc_init_deleg().
> 
> Let me think about how to fix that....

Sorry for the slow response--can you check whether this fixes the
problem?

--b.

commit 624a0ee0375940ce4aa36330b0b5a70af6d2b6f5
Author: J. Bruce Fields <bfields@xxxxxxxxxx>
Date:   Thu Aug 15 16:55:26 2013 -0400

    nfsd4: fix leak of inode reference on delegation failure
    
    This fixes a regression from 68a3396178e6688ad7367202cdf0af8ed03c8727
    "nfsd4: shut down more of delegation earlier".
    
    After that commit, nfs4_set_delegation() failures result in
    nfs4_put_delegation being called, but nfs4_put_delegation doesn't free
    the nfs4_file that has already been set by alloc_init_deleg().
    
    This can result in an oops on later unmounting the exported filesystem.
    
    Note also delaying the fi_had_conflict check we're able to return a
    better error (hence give 4.1 clients a better idea why the delegation
    failed; though note CONFLICT isn't an exact match here, as that's
    supposed to indicate a current conflict, but all we know here is that
    there was one recently).
    
    Reported-by: Toralf FÃrster <toralf.foerster@xxxxxx>
    Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index eb9cf81..0874998 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -368,11 +368,8 @@ static struct nfs4_delegation *
 alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct svc_fh *current_fh)
 {
 	struct nfs4_delegation *dp;
-	struct nfs4_file *fp = stp->st_file;
 
 	dprintk("NFSD alloc_init_deleg\n");
-	if (fp->fi_had_conflict)
-		return NULL;
 	if (num_delegations > max_delegations)
 		return NULL;
 	dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
@@ -389,8 +386,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
 	INIT_LIST_HEAD(&dp->dl_perfile);
 	INIT_LIST_HEAD(&dp->dl_perclnt);
 	INIT_LIST_HEAD(&dp->dl_recall_lru);
-	get_nfs4_file(fp);
-	dp->dl_file = fp;
+	dp->dl_file = NULL;
 	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
 	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
 	dp->dl_time = 0;
@@ -3044,22 +3040,35 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 	return 0;
 }
 
-static int nfs4_set_delegation(struct nfs4_delegation *dp)
+static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
 {
-	struct nfs4_file *fp = dp->dl_file;
+	int status;
 
-	if (!fp->fi_lease)
-		return nfs4_setlease(dp);
+	if (fp->fi_had_conflict)
+		return -EAGAIN;
+	get_nfs4_file(fp);
+	dp->dl_file = fp;
+	if (!fp->fi_lease) {
+		status = nfs4_setlease(dp);
+		if (status)
+			goto out_free;
+		return 0;
+	}
 	spin_lock(&recall_lock);
 	if (fp->fi_had_conflict) {
 		spin_unlock(&recall_lock);
-		return -EAGAIN;
+		status = -EAGAIN;
+		goto out_free;
 	}
 	atomic_inc(&fp->fi_delegees);
 	list_add(&dp->dl_perfile, &fp->fi_delegations);
 	spin_unlock(&recall_lock);
 	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
 	return 0;
+out_free:
+	put_nfs4_file(fp);
+	dp->dl_file = fp;
+	return status;
 }
 
 static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
@@ -3134,7 +3143,7 @@ nfs4_open_delegation(struct net *net, struct svc_fh *fh,
 	dp = alloc_init_deleg(oo->oo_owner.so_client, stp, fh);
 	if (dp == NULL)
 		goto out_no_deleg;
-	status = nfs4_set_delegation(dp);
+	status = nfs4_set_delegation(dp, stp->st_file);
 	if (status)
 		goto out_free;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/