2.6.38 nfsd bugfixes

2.6.38 nfsd bugfixes

[J. Bruce Fields]

The following bugfixes for 2.6.38 are available from:

	git://linux-nfs.org/~bfields/linux.git for-2.6.38

(Note several of the "nfsd4:" patches are just one fix for a
merge-window regression, with some preperatory cleanup rather finely
split out.)

--b.

Benny Halevy (1):
      NFSD: use nfserr for status after decode_cb_op_status

J. Bruce Fields (9):
      nfsd: don't leak dentry count on mnt_want_write failure
      nfsd4: split up nfsd_break_deleg_cb
      nfsd4: add helper function for lease setup
      nfsd4: fix leak on allocation error
      nfsd4: split lease setting into separate function
      nfsd4: remove unused deleg dprintk's.
      nfsd4: modify fi_delegations under recall_lock
      nfsd4: acquire only one lease per file
      nfsd: break lease on unlink due to rename

Konstantin Khorenko (1):
      NFSD: memory corruption due to writing beyond the stat array

 fs/nfsd/nfs4callback.c |    6 +-
 fs/nfsd/nfs4state.c    |  186 +++++++++++++++++++++++++++---------------------
 fs/nfsd/state.h        |    5 +-
 fs/nfsd/vfs.c          |   21 ++++--
 4 files changed, 125 insertions(+), 93 deletions(-)

Re: 2.6.38 nfsd bugfixes

[J. Bruce Fields]

Doh--I missed one patch in my previous pull request; see

  git://linux-nfs.org/~bfields/linux.git for-2.6.38

Please apply.

--b.

commit 47c85291d3dd1a51501555000b90f8e281a0458e
Author: NeilBrown <neilb@suse.de>
Date:   Wed Feb 16 13:08:35 2011 +1100

    nfsd: correctly handle return value from nfsd_map_name_to_*
    
    These functions return an nfs status, not a host_err.  So don't
    try to convert  before returning.
    
    This is a regression introduced by
    3c726023402a2f3b28f49b9d90ebf9e71151157d; I fixed up two of the callers,
    but missed these two.
    
    Cc: stable@kernel.org
    Reported-by: Herbert Poetzl <herbert@13thfloor.at>
    Signed-off-by: NeilBrown <neilb@suse.de>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 956629b..1275b86 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -317,8 +317,8 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 		READ_BUF(dummy32);
 		len += (XDR_QUADLEN(dummy32) << 2);
 		READMEM(buf, dummy32);
-		if ((host_err = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid)))
-			goto out_nfserr;
+		if ((status = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid)))
+			return status;
 		iattr->ia_valid |= ATTR_UID;
 	}
 	if (bmval[1] & FATTR4_WORD1_OWNER_GROUP) {
@@ -328,8 +328,8 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 		READ_BUF(dummy32);
 		len += (XDR_QUADLEN(dummy32) << 2);
 		READMEM(buf, dummy32);
-		if ((host_err = nfsd_map_name_to_gid(argp->rqstp, buf, dummy32, &iattr->ia_gid)))
-			goto out_nfserr;
+		if ((status = nfsd_map_name_to_gid(argp->rqstp, buf, dummy32, &iattr->ia_gid)))
+			return status;
 		iattr->ia_valid |= ATTR_GID;
 	}
 	if (bmval[1] & FATTR4_WORD1_TIME_ACCESS_SET) {

Re: 2.6.38 nfsd bugfixes

[Linus Torvalds]

On Wed, Feb 16, 2011 at 8:25 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> -               if ((host_err = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid)))
> -                       goto out_nfserr;
> +               if ((status = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid)))
> +                       return status;

Btw, can we please just agree to not doing those idiotic double parenthesis?

There is a really trivial solution to the gcc warning - write your
code like a sane person, instead of some ex-LISP hacker that has
withdrawal symptoms. IOW, the above should be written as

  status = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid);
  if (status)
    return status;

which is a hell of a lot more readable, no?

There is never any real excuse to put an assignment inside a regular
if-statement.

Inside a while/for loop? Sure. There are real syntactic reasons for
doing things like

   while ((c = getchar()) != EOF) {
   }

that actually make the code better and denser and avoid extra control
flow crap or duplicate code.

Inside a macro expansion? Again, there may be good reasons to try to
make it a single statement.

But a simple if-statement? There just isn't any reason for it, since
the obvious thing is to just write it as two separate statements: the
assignment, and the if-statement. So why do it and make the code
uglier and harder to parse?

                                   Linus

Re: 2.6.38 nfsd bugfixes

[J. Bruce Fields]

On Wed, Feb 16, 2011 at 08:32:06PM -0800, Linus Torvalds wrote:
> On Wed, Feb 16, 2011 at 8:25 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > -               if ((host_err = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid)))
> > -                       goto out_nfserr;
> > +               if ((status = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid)))
> > +                       return status;
> 
> Btw, can we please just agree to not doing those idiotic double parenthesis?

Fine by me; I don't write new code that way.

I already committed it like that, so would rather just do any cleanup as
another patch for the next merge window; but let me know what you want.

--b.

> 
> There is a really trivial solution to the gcc warning - write your
> code like a sane person, instead of some ex-LISP hacker that has
> withdrawal symptoms. IOW, the above should be written as
> 
>   status = nfsd_map_name_to_uid(argp->rqstp, buf, dummy32, &iattr->ia_uid);
>   if (status)
>     return status;
> 
> which is a hell of a lot more readable, no?
> 
> There is never any real excuse to put an assignment inside a regular
> if-statement.
> 
> Inside a while/for loop? Sure. There are real syntactic reasons for
> doing things like
> 
>    while ((c = getchar()) != EOF) {
>    }
> 
> that actually make the code better and denser and avoid extra control
> flow crap or duplicate code.
> 
> Inside a macro expansion? Again, there may be good reasons to try to
> make it a single statement.
> 
> But a simple if-statement? There just isn't any reason for it, since
> the obvious thing is to just write it as two separate statements: the
> assignment, and the if-statement. So why do it and make the code
> uglier and harder to parse?
> 
>                                    Linus

Re: 2.6.38 nfsd bugfixes

[Linus Torvalds]

On Wed, Feb 16, 2011 at 8:51 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>
> I already committed it like that, so would rather just do any cleanup as
> another patch for the next merge window;

That's fine. I just want to point these out as I see them, and ask
that when people fix bugs, they also aim to make the code readable at
the same time.

                                    Linus

Re: 2.6.38 nfsd bugfixes

[J. Bruce Fields]

On Wed, Feb 16, 2011 at 09:54:49PM -0800, Linus Torvalds wrote:
> On Wed, Feb 16, 2011 at 8:51 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > I already committed it like that, so would rather just do any cleanup as
> > another patch for the next merge window;
> 
> That's fine. I just want to point these out as I see them, and ask
> that when people fix bugs, they also aim to make the code readable at
> the same time.

OK, makes sense.

--b.