Kernel update 3.5.7 -> 3.6.3 breaks NFS4

Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

Hello,

after updating the kernel of my NFS4-Server (because of 3.5.x EOL) I ended
up with NFS4 stopped working.

Kernel config is more or less the same as with 3.5.7, as I used "make
oldconfig". The only new stuff in the NFS area seems to be swap over NFS
which is disabled.

So starting from a working setup with 3.5.7 on the server (client uses
kernel 3.6.3) NFS4 stops working after upgrading the server to 3.6.3.

What happens is that mounting seems to hang infinitely when trying to mount
the server using the following command:
mount -v -t nfs4 -o sec=krb5 <server>:/<dir> /mnt/

However everything works fine again when the server has been rebooted into kernel
3.5.7.

Here is the relevant part of .config:
CONFIG_NETWORK_FILESYSTEMS=y
CONFIG_NFS_FS=m
CONFIG_NFS_V2=m
CONFIG_NFS_V3=m
CONFIG_NFS_V3_ACL=y
CONFIG_NFS_V4=m
# CONFIG_NFS_SWAP is not set
# CONFIG_NFS_V4_1 is not set
# CONFIG_NFS_USE_LEGACY_DNS is not set
CONFIG_NFS_USE_KERNEL_DNS=y
CONFIG_NFS_DEBUG=y
CONFIG_NFSD=y
CONFIG_NFSD_V2_ACL=y
CONFIG_NFSD_V3=y
CONFIG_NFSD_V3_ACL=y
CONFIG_NFSD_V4=y
# CONFIG_NFSD_FAULT_INJECTION is not set
CONFIG_LOCKD=y
CONFIG_LOCKD_V4=y
CONFIG_NFS_ACL_SUPPORT=y
CONFIG_NFS_COMMON=y
CONFIG_SUNRPC=y
CONFIG_SUNRPC_GSS=y
CONFIG_RPCSEC_GSS_KRB5=y
CONFIG_SUNRPC_DEBUG=y


Sven

P.S.: The system is using nfs-utils 1.2.6 on debian wheezy if this should be
relevant.

-- 
"Those who do not understand Unix are condemned to reinvent it, poorly"
(Henry Spencer)

/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[VDR User]

I use debian testing here and NFSv4. My clients are running kernel
3.6.3, server is running 3.6.2.  No problems here.

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

On Fri, Oct 26, 2012 at 03:58:34PM +0000, Sven Geggus wrote:
> Hello,
> 
> after updating the kernel of my NFS4-Server (because of 3.5.x EOL) I ended
> up with NFS4 stopped working.
> 
> Kernel config is more or less the same as with 3.5.7, as I used "make
> oldconfig". The only new stuff in the NFS area seems to be swap over NFS
> which is disabled.
> 
> So starting from a working setup with 3.5.7 on the server (client uses
> kernel 3.6.3) NFS4 stops working after upgrading the server to 3.6.3.
> 
> What happens is that mounting seems to hang infinitely when trying to mount
> the server using the following command:
> mount -v -t nfs4 -o sec=krb5 <server>:/<dir> /mnt/

Could I see a network trace?

	tcpdump -s0 -wtmp.pcap 'host <myclient> && host <myserver>'

then try the mount, then kill tcpdump and send me a copy of tmp.pcap.

--b.

> 
> However everything works fine again when the server has been rebooted into kernel
> 3.5.7.
> 
> Here is the relevant part of .config:
> CONFIG_NETWORK_FILESYSTEMS=y
> CONFIG_NFS_FS=m
> CONFIG_NFS_V2=m
> CONFIG_NFS_V3=m
> CONFIG_NFS_V3_ACL=y
> CONFIG_NFS_V4=m
> # CONFIG_NFS_SWAP is not set
> # CONFIG_NFS_V4_1 is not set
> # CONFIG_NFS_USE_LEGACY_DNS is not set
> CONFIG_NFS_USE_KERNEL_DNS=y
> CONFIG_NFS_DEBUG=y
> CONFIG_NFSD=y
> CONFIG_NFSD_V2_ACL=y
> CONFIG_NFSD_V3=y
> CONFIG_NFSD_V3_ACL=y
> CONFIG_NFSD_V4=y
> # CONFIG_NFSD_FAULT_INJECTION is not set
> CONFIG_LOCKD=y
> CONFIG_LOCKD_V4=y
> CONFIG_NFS_ACL_SUPPORT=y
> CONFIG_NFS_COMMON=y
> CONFIG_SUNRPC=y
> CONFIG_SUNRPC_GSS=y
> CONFIG_RPCSEC_GSS_KRB5=y
> CONFIG_SUNRPC_DEBUG=y
> 
> 
> Sven
> 
> P.S.: The system is using nfs-utils 1.2.6 on debian wheezy if this should be
> relevant.
> 
> -- 
> "Those who do not understand Unix are condemned to reinvent it, poorly"
> (Henry Spencer)
> 
> /me is giggls@ircnet, http://sven.gegg.us/ on the Web
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

Re-adding linux-nfs to cc:

On Mon, Oct 29, 2012 at 10:40:38AM +0100, Sven Geggus wrote:
> J. Bruce Fields schrieb am Freitag, den 26. Oktober um 19:15 Uhr:
> 
> > Could I see a network trace?
> 
> Shure. I can also do one with the working kernel.
> 
> > 	tcpdump -s0 -wtmp.pcap 'host <myclient> && host <myserver>'
> > 
> > then try the mount, then kill tcpdump and send me a copy of tmp.pcap.
> 
> pcap file attached.

Thanks.  So running "wireshark nfs4.pcap", I see:

	frame 13, 15: create a gss context with handle 0x01000000
	frame 17, 18: client sends a DESTROY for context 0x01000000 and
		a TCP FIN.  The RPC is malformed in that it has no verifier
		field.  The server doesn't respond.
	frame 19: client sends a PUTROOTFH using context 0x01000000.
	frame 25-31: a minute has passed, the client gives up, closes
		the connection and retries the PUTROOTFH, again with the
		same context.

That DESTROY is sent over the same connection as the context creation,
so must have been done by gssd.  So gssd has a bug.  Well, two: first,
the DESTROY is malformed, second, it shouldn't be sending it anyway.

I don't understand why the server is dropping requests instead of
returning errors.  I actually would have expected it to return BADVERF
to the DESTROY request and then accept the PUTROOTFH normally, which
might have allowed the mount to succeed despite the bizarre rpc.gssd
behavior.

I'd be curious to understand what changed on the server to make a
difference.  I can't think of anything.  Looking at a network trace from
a successful mount with 3.5.7 might be useful.

--b.

> 
> This is what I called:
> mount -t nfs4 -v -o sec=krb5 centauri:/storage /mnt
> 
> Client ist venus (10.1.7.30), server is centauri (10.1.7.67) kerberos realm
> (AD) is PC.IITB.FHG.DE
> 
> What I should probably also metion is that the machine is a redundant system
> using drbd. The IP-address 10.1.7.67 can be migrated to the the slave
> machine in case of a hardware failure. So if something seems to be missing I
> can also create a capture file including the other IP-address of the
> server system.
> 
> Regards
> 
> Sven
> 
> -- 
> "Those who do not understand Unix are condemned to reinvent it, poorly"
> (Henry Spencer)
> 
> /me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

J. Bruce Fields schrieb am Montag, den 29. Oktober um 16:02 Uhr:

> Re-adding linux-nfs to cc

OK

> I don't understand why the server is dropping requests instead of
> returning errors.  I actually would have expected it to return BADVERF
> to the DESTROY request and then accept the PUTROOTFH normally, which
> might have allowed the mount to succeed despite the bizarre rpc.gssd
> behavior.

Hm rpc.gssd is the one from debian nfs-utils 1.2.6 looking at their custom
patches inside the debian package there is nothing which could cause this.

> I'd be curious to understand what changed on the server to make a
> difference. 

Nothing but the kernel. I'm currently dual booting either 3.5.7 or 3.6.3
vanilla kernels on the same system.

> Looking at a network trace from a successful mount with 3.5.7 might be
> useful.

attached.

Sven

-- 
"C Is Quirky, Flawed, And An Enormous Success."
(Dennis M. Ritchie)

/me is giggls@ircnet, http://sven.gegg.us/ on the Web

[-- Attachment #2: nfs4-ok.pcap --]
[-- Type: application/cap, Size: 11440 bytes --]

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

On Mon, Oct 29, 2012 at 05:33:23PM +0100, Sven Geggus wrote:
> J. Bruce Fields schrieb am Montag, den 29. Oktober um 16:02 Uhr:
> 
> > Re-adding linux-nfs to cc
> 
> OK
> 
> > I don't understand why the server is dropping requests instead of
> > returning errors.  I actually would have expected it to return BADVERF
> > to the DESTROY request and then accept the PUTROOTFH normally, which
> > might have allowed the mount to succeed despite the bizarre rpc.gssd
> > behavior.
> 
> Hm rpc.gssd is the one from debian nfs-utils 1.2.6 looking at their custom
> patches inside the debian package there is nothing which could cause this.
> 
> > I'd be curious to understand what changed on the server to make a
> > difference. 
> 
> Nothing but the kernel. I'm currently dual booting either 3.5.7 or 3.6.3
> vanilla kernels on the same system.
> 
> > Looking at a network trace from a successful mount with 3.5.7 might be
> > useful.
> 
> attached.

Thanks!

The sequence of events is pretty much what I described for the "bad"
trace, except that PUTROOTFH gets a succesful reply.

One other odd difference: in the "bad" case, the timing is a little
different: the socket gssd created doesn't get shut down in the same
way, and the PUTROOTFH comes more quickly on the heels of the FIN.
Which shouldn't make a difference.

Hm.

--b.

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

VDR User <user.vdr@gmail.com> wrote:

> I use debian testing here and NFSv4. My clients are running kernel
> 3.6.3, server is running 3.6.2.  No problems here.

Are you using sec=krb5?

Sven

-- 
"Der wichtigste Aspekt, den Sie vor der Entscheidung für ein Open
Source-Betriebssystem bedenken sollten, ist, dass Sie kein
Windows-Betriebssystem erhalten." (von http://www.dell.de/ubuntu)
/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

Sven Geggus <lists@fuchsschwanzdomain.de> wrote:

>> I'd be curious to understand what changed on the server to make a
>> difference. 
> 
> Nothing but the kernel. I'm currently dual booting either 3.5.7 or 3.6.3
> vanilla kernels on the same system.

I did some further investigation on this using a freshly installed
system on virtualbox with either kernel.

Same odd behaviour. Everything work using Kernel 3.5.7. Server does not work using
Kernel 3.6.3.

There are even different filesystems on the vm (ext4) and on the real server
(xfs).

Looks like I need to cherry-pick nfs4 server changes between 3.5.7 and 3.6.3 now :(

Regards

Sven

-- 
"Those who do not understand Unix are condemned to reinvent it, poorly"
(Henry Spencer)

/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[VDR User]

On Wed, Oct 31, 2012 at 5:52 AM, Sven Geggus
<lists@fuchsschwanzdomain.de> wrote:
>>> I'd be curious to understand what changed on the server to make a
>>> difference.
>>
>> Nothing but the kernel. I'm currently dual booting either 3.5.7 or 3.6.3
>> vanilla kernels on the same system.
>
> I did some further investigation on this using a freshly installed
> system on virtualbox with either kernel.
>
> Same odd behaviour. Everything work using Kernel 3.5.7. Server does not work using
> Kernel 3.6.3.
>
> There are even different filesystems on the vm (ext4) and on the real server
> (xfs).
>
> Looks like I need to cherry-pick nfs4 server changes between 3.5.7 and 3.6.3 now :(

In case you missed my previous message, "I use debian testing here and
NFSv4. My clients are running kernel
3.6.3, server is running 3.6.2.  No problems here". It would seem you
only need to cherry-pick server changes between 3.6.2 and 3.6.3...?

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

VDR User <user.vdr@gmail.com> wrote:

> In case you missed my previous message, "I use debian testing here and
> NFSv4. My clients are running kernel 3.6.3, server is running 3.6.2.  No
> problems here".

No, I did not miss your post. I even asked if you are using sec=krb5. I did
not try if it works without using kerberos, because I am not interested in
"No File Security" NFS3-like mode.

> It would seem you only need to cherry-pick server changes between 3.6.2
> and 3.6.3...?

Unfortunately not. It already did not work using 3.6.2

Regards

Sven

-- 
"Der wichtigste Aspekt, den Sie vor der Entscheidung für ein Open
Source-Betriebssystem bedenken sollten, ist, dass Sie kein
Windows-Betriebssystem erhalten." (von http://www.dell.de/ubuntu)
/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[VDR User]

On Wed, Oct 31, 2012 at 8:33 AM, Sven Geggus
<lists@fuchsschwanzdomain.de> wrote:
>> In case you missed my previous message, "I use debian testing here and
>> NFSv4. My clients are running kernel 3.6.3, server is running 3.6.2.  No
>> problems here".
>
> No, I did not miss your post. I even asked if you are using sec=krb5. I did
> not try if it works without using kerberos, because I am not interested in
> "No File Security" NFS3-like mode.

Ahh sorry. I misread your post and thought it was directed towards
someone else. No, I'm not using sec=krb5.

>> It would seem you only need to cherry-pick server changes between 3.6.2
>> and 3.6.3...?
>
> Unfortunately not. It already did not work using 3.6.2

You could always write a simple script to download & compile each
kernel inbetween that range, then just boot into each and find where
the breakage occured that way. Sometimes its easier than sifting
through a sea of changes. :)

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

VDR User <user.vdr@gmail.com> wrote:

> Ahh sorry. I misread your post and thought it was directed towards
> someone else. No, I'm not using sec=krb5.

OK so this is not a comparable use case.

Sven

-- 
"Ich fürchte mich nicht vor der Rückkehr der Faschisten in der Maske der
Faschisten, sondern vor der Rückkehr der Faschisten in der Maske der
Demokraten" (Theodor W. Adorno)
/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

J. Bruce Fields schrieb am Montag, den 29. Oktober um 16:02 Uhr:

> I'd be curious to understand what changed on the server to make a
> difference.  I can't think of anything.

OK, I now figured out which commit did cause the problem:

commit 08843b79fb35d33859e0f8f11a7318341076e4d1
Merge: cc8362b 2c142ba
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue Jul 31 14:42:28 2012 -0700

    Merge branch 'nfsd-next' of git://linux-nfs.org/~bfields/linux
    
    Pull nfsd changes from J. Bruce Fields:
     "This has been an unusually quiet cycle--mostly bugfixes and cleanup.
      The one large piece is Stanislav's work to containerize the server's
      grace period--but that in itself is just one more step in a
      not-yet-complete project to allow fully containerized nfs service.
    
      There are a number of outstanding delegation, container, v4 state, and
      gss patches that aren't quite ready yet; 3.7 may be wilder."
    
    * 'nfsd-next' of git://linux-nfs.org/~bfields/linux: (35 commits)
      NFSd: make boot_time variable per network namespace
      NFSd: make grace end flag per network namespace
      Lockd: move grace period management from lockd() to per-net functions
      LockD: pass actual network namespace to grace period management
      functions
      LockD: manage grace list per network namespace
      SUNRPC: service request network namespace helper introduced
      NFSd: make nfsd4_manager allocated per network namespace context.
      LockD: make lockd manager allocated per network namespace
      LockD: manage grace period per network namespace
      Lockd: add more debug to host shutdown functions
      Lockd: host complaining function introduced
      LockD: manage used host count per networks namespace
      LockD: manage garbage collection timeout per networks namespace
      LockD: make garbage collector network namespace aware.
      LockD: mark host per network namespace on garbage collect
      nfsd4: fix missing fault_inject.h include
      locks: move lease-specific code out of locks_delete_lock
      locks: prevent side-effects of locks_release_private before file_lock
      is initialized
      NFSd: set nfsd_serv to NULL after service destruction
      NFSd: introduce nfsd_destroy() helper
      ...

Thus "git diff 08843b79..cc8362b1" on a linux-stable tree from
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git should
give us the relevant changes.


Regards

Sven

-- 
Das allgemeine Persönlichkeitsrecht (Art. 2 Abs.1 i.V.m. Art.1 Abs. 1GG)
umfasst das Grundrecht auf Gewährleistung der Vertraulichkeit und Integrität
informationstechnischer Systeme. (BVerfG, 1BvR 370/07)
/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

Sven Geggus <lists@fuchsschwanzdomain.de> wrote:

> OK, I now figured out which commit did cause the problem:
> 
> Thus "git diff 08843b79..cc8362b1" on a linux-stable tree from
> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git should
> give us the relevant changes.

After a private conversation with J. Bruce Fields I figured out that
I have not been quite there yet. So here comes a FTR post what
exactly caused my problem.

It is the following change:

$ git diff d9c2ede63c74048dfddbb129c59ac01176b0ab71 bbf43dc888833ac0539e437dbaeb28bfd4fbab9f
diff --git a/include/linux/sunrpc/cache.h
b/include/linux/sunrpc/cache.h
index 6def1f6..af42596 100644
--- a/include/linux/sunrpc/cache.h
+++ b/include/linux/sunrpc/cache.h
@@ -217,8 +217,6 @@ extern int qword_get(char **bpp, char *dest, int
bufsize);
 static inline int get_int(char **bpp, int *anint)
 {
        char buf[50];
-       char *ep;
-       int rv;
        int len = qword_get(bpp, buf, sizeof(buf));
 
        if (len < 0)
@@ -226,11 +224,9 @@ static inline int get_int(char **bpp, int
*anint)
        if (len == 0)
                return -ENOENT;
 
-       rv = simple_strtol(buf, &ep, 0);
-       if (*ep)
+       if (kstrtoint(buf, 0, anint))
                return -EINVAL;
 
-       *anint = rv;
        return 0;
 }

Reverting this change on recent kernels makes them work for me again.

Sven


-- 
Unix is simple and coherent, but it takes a genius – or at any rate a
programmer – to understand and appreciate the simplicity
(Dennis M. Ritchie)
/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

On Fri, Nov 09, 2012 at 06:45:32PM +0000, Sven Geggus wrote:
> Sven Geggus <lists@fuchsschwanzdomain.de> wrote:
> 
> > OK, I now figured out which commit did cause the problem:
> > 
> > Thus "git diff 08843b79..cc8362b1" on a linux-stable tree from
> > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git should
> > give us the relevant changes.
> 
> After a private conversation with J. Bruce Fields I figured out that
> I have not been quite there yet. So here comes a FTR post what
> exactly caused my problem.

Thanks for tracking this down--not what I would have guessed!

Given that the trace showed a problem starting around context creation
time, I'm most suspicious of the callers in rsc_parse, which are mostly
parsing uid's.

Is it possible that your system has very large uid's?  (Large enough
that they'd look like negative numbers when cast to ints?)

Output from

	strace -p $(pidof rpc.mountd) -s4096 -e trace=open,close,read,write

(while reproducing the bug) might help confirm that.

--b.


> 
> It is the following change:
> 
> $ git diff d9c2ede63c74048dfddbb129c59ac01176b0ab71 bbf43dc888833ac0539e437dbaeb28bfd4fbab9f
> diff --git a/include/linux/sunrpc/cache.h
> b/include/linux/sunrpc/cache.h
> index 6def1f6..af42596 100644
> --- a/include/linux/sunrpc/cache.h
> +++ b/include/linux/sunrpc/cache.h
> @@ -217,8 +217,6 @@ extern int qword_get(char **bpp, char *dest, int
> bufsize);
>  static inline int get_int(char **bpp, int *anint)
>  {
>         char buf[50];
> -       char *ep;
> -       int rv;
>         int len = qword_get(bpp, buf, sizeof(buf));
>  
>         if (len < 0)
> @@ -226,11 +224,9 @@ static inline int get_int(char **bpp, int
> *anint)
>         if (len == 0)
>                 return -ENOENT;
>  
> -       rv = simple_strtol(buf, &ep, 0);
> -       if (*ep)
> +       if (kstrtoint(buf, 0, anint))
>                 return -EINVAL;
>  
> -       *anint = rv;
>         return 0;
>  }
> 
> Reverting this change on recent kernels makes them work for me again.
> 
> Sven
> 
> 
> -- 
> Unix is simple and coherent, but it takes a genius – or at any rate a
> programmer – to understand and appreciate the simplicity
> (Dennis M. Ritchie)
> /me is giggls@ircnet, http://sven.gegg.us/ on the Web
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

On Fri, Nov 09, 2012 at 03:07:30PM -0500, bfields wrote:
> On Fri, Nov 09, 2012 at 06:45:32PM +0000, Sven Geggus wrote:
> > Sven Geggus <lists@fuchsschwanzdomain.de> wrote:
> > 
> > > OK, I now figured out which commit did cause the problem:
> > > 
> > > Thus "git diff 08843b79..cc8362b1" on a linux-stable tree from
> > > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git should
> > > give us the relevant changes.
> > 
> > After a private conversation with J. Bruce Fields I figured out that
> > I have not been quite there yet. So here comes a FTR post what
> > exactly caused my problem.
> 
> Thanks for tracking this down--not what I would have guessed!
> 
> Given that the trace showed a problem starting around context creation
> time, I'm most suspicious of the callers in rsc_parse, which are mostly
> parsing uid's.
> 
> Is it possible that your system has very large uid's?  (Large enough
> that they'd look like negative numbers when cast to ints?)
> 
> Output from
> 
> 	strace -p $(pidof rpc.mountd) -s4096 -e trace=open,close,read,write
> 
> (while reproducing the bug) might help confirm that.

And this might help.

--b.

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index ec76f3a..31b4e95 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -430,7 +430,7 @@ static int rsc_parse(struct cache_detail *cd,
 		goto out;
 
 	/* uid, or NEGATIVE */
-	rv = get_int(&mesg, &rsci.cred.cr_uid);
+	rv = get_uint(&mesg, &rsci.cred.cr_uid);
 	if (rv == -EINVAL)
 		goto out;
 	if (rv == -ENOENT)
@@ -439,7 +439,7 @@ static int rsc_parse(struct cache_detail *cd,
 		int N, i;
 
 		/* gid */
-		if (get_int(&mesg, &rsci.cred.cr_gid))
+		if (get_uint(&mesg, &rsci.cred.cr_gid))
 			goto out;
 
 		/* number of additional gid's */
@@ -455,7 +455,7 @@ static int rsc_parse(struct cache_detail *cd,
 		for (i=0; i<N; i++) {
 			gid_t gid;
 			kgid_t kgid;
-			if (get_int(&mesg, &gid))
+			if (get_uint(&mesg, &gid))
 				goto out;
 			kgid = make_kgid(&init_user_ns, gid);
 			if (!gid_valid(kgid))

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

In article <20121109200730.GI6171@fieldses.org> you wrote:

> Is it possible that your system has very large uid's?  (Large enough
> that they'd look like negative numbers when cast to ints?)

Shurely not. however, mounting is done as root which might get mapped to
nobody which usually is 65534.

> Output from
> 
>        strace -p $(pidof rpc.mountd) -s4096 -e trace=open,close,read,write
> 
> (while reproducing the bug) might help confirm that.

While doing the hanging or while doing the proper mount?

Sven

-- 
Microsoft ist offenbar die einzige Firma, die in der Lage ist, ein mit
Office nicht kompatibles Bürosoftwarepaket einzuführen.
                            (Florian Weimer in de.alt.sysadmin.recovery)
/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Eldad Zack]

On Fri, 9 Nov 2012, J. Bruce Fields wrote:
> On Fri, Nov 09, 2012 at 06:45:32PM +0000, Sven Geggus wrote:
> > Sven Geggus <lists@fuchsschwanzdomain.de> wrote:
> > 
> > > OK, I now figured out which commit did cause the problem:
> > > 
> > > Thus "git diff 08843b79..cc8362b1" on a linux-stable tree from
> > > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git should
> > > give us the relevant changes.
> > 
> > After a private conversation with J. Bruce Fields I figured out that
> > I have not been quite there yet. So here comes a FTR post what
> > exactly caused my problem.
> 
> Thanks for tracking this down--not what I would have guessed!
> 
> Given that the trace showed a problem starting around context creation
> time, I'm most suspicious of the callers in rsc_parse, which are mostly
> parsing uid's.

Uh oh. Sorry about that.
This might help pinpoint the problem - have simple_strto* WARN
(or WARN_ONCE maybe?) just before masking out the overflow.

I'm running with this now and so far I get no warnings.

Cheers,
Eldad

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c56de78..8a748c6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -49,6 +49,8 @@ unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int bas
 	cp = _parse_integer_fixup_radix(cp, &base);
 	rv = _parse_integer(cp, base, &result);
 	/* FIXME */
+	WARN((rv & KSTRTOX_OVERFLOW) == KSTRTOX_OVERFLOW,
+		"simple_strtox overflow while parsing '%s' (base %d)\n", cp, base);
 	cp += (rv & ~KSTRTOX_OVERFLOW);
 
 	if (endp)

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

On Fri, Nov 09, 2012 at 11:45:41PM +0100, Sven Geggus wrote:
> In article <20121109200730.GI6171@fieldses.org> you wrote:
> 
> > Is it possible that your system has very large uid's?  (Large enough
> > that they'd look like negative numbers when cast to ints?)
> 
> Shurely not. however, mounting is done as root which might get mapped to
> nobody which usually is 65534.
> 
> > Output from
> > 
> >        strace -p $(pidof rpc.mountd) -s4096 -e trace=open,close,read,write
> > 
> > (while reproducing the bug) might help confirm that.
> 
> While doing the hanging or while doing the proper mount?

Restart the server, start strace, then try the mount, let it hang a few
seconds just to make sure you got anything interesting, then kill strace
and send the output.

I guess the results in the succesful (good kernel) case might be
interesting too, but probably the bad case is enough.

--b.

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

J. Bruce Fields schrieb am Samstag, den 10. November um 00:24 Uhr:

OK, back at work and here is what I get:

> Restart the server, start strace, then try the mount, let it hang a few
> seconds just to make sure you got anything interesting, then kill strace
> and send the output.

OK, back at work and here is what I get...

read(3, "nfsd 10.1.7.30\n", 2048)       = 15
close(15)                               = 0
open("/var/lib/nfs/etab", O_RDONLY)     = 15
close(15)                               = 0
close(15)                               = 0
write(3, "nfsd 10.1.7.30 1352710828 * \n", 29) = 29
read(4, "4294967295\n", 2048)           = 11
close(16)                               = 0
close(15)                               = 0
read(15,
"\2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\0\0\0\0\0\0\0\0\0\0\0\0",
36) = 36
close(15)                               = 0
write(4, "4294967295 1352710828 0 \n", 25) = -1 EINVAL (Invalid argument)

4294967295 is UINT_MAX and this place is where it behaves differently on a good
kernel where the write call will succeed:

write(4, "4294967295 1352710828 0 \n", 25) = 25

Sven

P.S.: Your patched svcauth_gss.c will give me an "access denied by server"
while mounting instead of the infinite delay:
 ~/ # mount -t nfs4 -o sec=krb5 testsrv:/storage /mnt/
mount.nfs4: access denied by server while mounting testsrv:/storage

-- 
Those who would give up Essential Liberty to purchase a little Temporary
Safety, deserve neither Liberty nor Safety (Benjamin Franklin)

/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

On Mon, Nov 12, 2012 at 10:17:17AM +0100, Sven Geggus wrote:
> J. Bruce Fields schrieb am Samstag, den 10. November um 00:24 Uhr:
> 
> OK, back at work and here is what I get:
> 
> > Restart the server, start strace, then try the mount, let it hang a few
> > seconds just to make sure you got anything interesting, then kill strace
> > and send the output.
> 
> OK, back at work and here is what I get...
> 
> read(3, "nfsd 10.1.7.30\n", 2048)       = 15
> close(15)                               = 0
> open("/var/lib/nfs/etab", O_RDONLY)     = 15
> close(15)                               = 0
> close(15)                               = 0
> write(3, "nfsd 10.1.7.30 1352710828 * \n", 29) = 29
> read(4, "4294967295\n", 2048)           = 11
> close(16)                               = 0
> close(15)                               = 0
> read(15,
> "\2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\0\0\0\0\0\0\0\0\0\0\0\0",
> 36) = 36
> close(15)                               = 0
> write(4, "4294967295 1352710828 0 \n", 25) = -1 EINVAL (Invalid argument)

I suspect that error's coming from
net/sunrpc/svcauth_unix.c:unix_gid_parse().

> 4294967295 is UINT_MAX and this place is where it behaves differently on a good
> kernel where the write call will succeed:
> 
> write(4, "4294967295 1352710828 0 \n", 25) = 25
> 
> Sven
> 
> P.S.: Your patched svcauth_gss.c will give me an "access denied by server"
> while mounting instead of the infinite delay:
>  ~/ # mount -t nfs4 -o sec=krb5 testsrv:/storage /mnt/
> mount.nfs4: access denied by server while mounting testsrv:/storage

So, looks like the same get_int problem exists in several other places.
Could you try the following instead of the previous patch?  I think I
got them all this time....

--b.

commit 664f26313a738f539a32c4eadd5351905e301bf2
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Fri Nov 9 15:16:02 2012 -0500

    svcrpc: fix parsing of uids and gids in gss contexts
    
    bbf43dc888833ac0539e437dbaeb28bfd4fbab9f "sunrpc/cache.h: replace
    simple_strtoul" introduced new range-checking which could cause get_int
    to fail if given an unsigned integer too large to represent as an int.
    
    Symptoms were hangs on krb5 mounts after upgrading an NFS server.
    
    Cc: Eldad Zack <eldad@fogrefinery.com>
    Reported-by: Sven Geggus <lists@fuchsschwanzdomain.de>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index a3946cf..8481961 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -491,7 +491,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 	int err;
 	struct auth_domain *dom = NULL;
 	struct svc_export exp = {}, *expp;
-	int an_int;
+	unsigned int an_int;
 
 	if (mesg[mlen-1] != '\n')
 		return -EINVAL;
@@ -531,7 +531,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 		goto out3;
 
 	/* flags */
-	err = get_int(&mesg, &an_int);
+	err = get_uint(&mesg, &an_int);
 	if (err == -ENOENT) {
 		err = 0;
 		set_bit(CACHE_NEGATIVE, &exp.h.flags);
@@ -541,19 +541,19 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 		exp.ex_flags= an_int;
 	
 		/* anon uid */
-		err = get_int(&mesg, &an_int);
+		err = get_uint(&mesg, &an_int);
 		if (err)
 			goto out3;
 		exp.ex_anon_uid= an_int;
 
 		/* anon gid */
-		err = get_int(&mesg, &an_int);
+		err = get_uint(&mesg, &an_int);
 		if (err)
 			goto out3;
 		exp.ex_anon_gid= an_int;
 
 		/* fsid */
-		err = get_int(&mesg, &an_int);
+		err = get_uint(&mesg, &an_int);
 		if (err)
 			goto out3;
 		exp.ex_fsid = an_int;
diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
index a1f10c0..e2c9317 100644
--- a/fs/nfsd/nfs4idmap.c
+++ b/fs/nfsd/nfs4idmap.c
@@ -415,7 +415,7 @@ nametoid_parse(struct cache_detail *cd, char *buf, int buflen)
 		goto out;
 
 	/* ID */
-	error = get_int(&buf, &ent.id);
+	error = get_uint(&buf, &ent.id);
 	if (error == -EINVAL)
 		goto out;
 	if (error == -ENOENT)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 73e9573..243d180 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -444,7 +444,7 @@ static int rsc_parse(struct cache_detail *cd,
 		goto out;
 
 	/* uid, or NEGATIVE */
-	rv = get_int(&mesg, &rsci.cred.cr_uid);
+	rv = get_uint(&mesg, &rsci.cred.cr_uid);
 	if (rv == -EINVAL)
 		goto out;
 	if (rv == -ENOENT)
@@ -453,7 +453,7 @@ static int rsc_parse(struct cache_detail *cd,
 		int N, i;
 
 		/* gid */
-		if (get_int(&mesg, &rsci.cred.cr_gid))
+		if (get_uint(&mesg, &rsci.cred.cr_gid))
 			goto out;
 
 		/* number of additional gid's */
@@ -469,7 +469,7 @@ static int rsc_parse(struct cache_detail *cd,
 		for (i=0; i<N; i++) {
 			gid_t gid;
 			kgid_t kgid;
-			if (get_int(&mesg, &gid))
+			if (get_uint(&mesg, &gid))
 				goto out;
 			kgid = make_kgid(&init_user_ns, gid);
 			if (!gid_valid(kgid))
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 4d01292..5d7020a 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -493,7 +493,7 @@ static int unix_gid_parse(struct cache_detail *cd,
 		return -EINVAL;
 	mesg[mlen-1] = 0;
 
-	rv = get_int(&mesg, &uid);
+	rv = get_uint(&mesg, &uid);
 	if (rv)
 		return -EINVAL;
 	ug.uid = uid;
@@ -513,7 +513,7 @@ static int unix_gid_parse(struct cache_detail *cd,
 	for (i = 0 ; i < gids ; i++) {
 		int gid;
 		kgid_t kgid;
-		rv = get_int(&mesg, &gid);
+		rv = get_uint(&mesg, &gid);
 		err = -EINVAL;
 		if (rv)
 			goto out;

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

On Tue, Nov 13, 2012 at 05:40:05PM -0500, J. Bruce Fields wrote:
> On Mon, Nov 12, 2012 at 10:17:17AM +0100, Sven Geggus wrote:
> > J. Bruce Fields schrieb am Samstag, den 10. November um 00:24 Uhr:
> > 
> > OK, back at work and here is what I get:
> > 
> > > Restart the server, start strace, then try the mount, let it hang a few
> > > seconds just to make sure you got anything interesting, then kill strace
> > > and send the output.
> > 
> > OK, back at work and here is what I get...
> > 
> > read(3, "nfsd 10.1.7.30\n", 2048)       = 15
> > close(15)                               = 0
> > open("/var/lib/nfs/etab", O_RDONLY)     = 15
> > close(15)                               = 0
> > close(15)                               = 0
> > write(3, "nfsd 10.1.7.30 1352710828 * \n", 29) = 29
> > read(4, "4294967295\n", 2048)           = 11
> > close(16)                               = 0
> > close(15)                               = 0
> > read(15,
> > "\2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\0\0\0\0\0\0\0\0\0\0\0\0",
> > 36) = 36
> > close(15)                               = 0
> > write(4, "4294967295 1352710828 0 \n", 25) = -1 EINVAL (Invalid argument)
> 
> I suspect that error's coming from
> net/sunrpc/svcauth_unix.c:unix_gid_parse().
> 
> > 4294967295 is UINT_MAX and this place is where it behaves differently on a good
> > kernel where the write call will succeed:
> > 
> > write(4, "4294967295 1352710828 0 \n", 25) = 25
> > 
> > Sven
> > 
> > P.S.: Your patched svcauth_gss.c will give me an "access denied by server"
> > while mounting instead of the infinite delay:
> >  ~/ # mount -t nfs4 -o sec=krb5 testsrv:/storage /mnt/
> > mount.nfs4: access denied by server while mounting testsrv:/storage
> 
> So, looks like the same get_int problem exists in several other places.
> Could you try the following instead of the previous patch?  I think I
> got them all this time....

Oh, cripes, but this isn't good enough--svcgssd actually passes down -1
id's.  Ugh--I'll take a closer look tomorrow.

--b.

> 
> --b.
> 
> commit 664f26313a738f539a32c4eadd5351905e301bf2
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Fri Nov 9 15:16:02 2012 -0500
> 
>     svcrpc: fix parsing of uids and gids in gss contexts
>     
>     bbf43dc888833ac0539e437dbaeb28bfd4fbab9f "sunrpc/cache.h: replace
>     simple_strtoul" introduced new range-checking which could cause get_int
>     to fail if given an unsigned integer too large to represent as an int.
>     
>     Symptoms were hangs on krb5 mounts after upgrading an NFS server.
>     
>     Cc: Eldad Zack <eldad@fogrefinery.com>
>     Reported-by: Sven Geggus <lists@fuchsschwanzdomain.de>
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index a3946cf..8481961 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -491,7 +491,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
>  	int err;
>  	struct auth_domain *dom = NULL;
>  	struct svc_export exp = {}, *expp;
> -	int an_int;
> +	unsigned int an_int;
>  
>  	if (mesg[mlen-1] != '\n')
>  		return -EINVAL;
> @@ -531,7 +531,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
>  		goto out3;
>  
>  	/* flags */
> -	err = get_int(&mesg, &an_int);
> +	err = get_uint(&mesg, &an_int);
>  	if (err == -ENOENT) {
>  		err = 0;
>  		set_bit(CACHE_NEGATIVE, &exp.h.flags);
> @@ -541,19 +541,19 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
>  		exp.ex_flags= an_int;
>  	
>  		/* anon uid */
> -		err = get_int(&mesg, &an_int);
> +		err = get_uint(&mesg, &an_int);
>  		if (err)
>  			goto out3;
>  		exp.ex_anon_uid= an_int;
>  
>  		/* anon gid */
> -		err = get_int(&mesg, &an_int);
> +		err = get_uint(&mesg, &an_int);
>  		if (err)
>  			goto out3;
>  		exp.ex_anon_gid= an_int;
>  
>  		/* fsid */
> -		err = get_int(&mesg, &an_int);
> +		err = get_uint(&mesg, &an_int);
>  		if (err)
>  			goto out3;
>  		exp.ex_fsid = an_int;
> diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
> index a1f10c0..e2c9317 100644
> --- a/fs/nfsd/nfs4idmap.c
> +++ b/fs/nfsd/nfs4idmap.c
> @@ -415,7 +415,7 @@ nametoid_parse(struct cache_detail *cd, char *buf, int buflen)
>  		goto out;
>  
>  	/* ID */
> -	error = get_int(&buf, &ent.id);
> +	error = get_uint(&buf, &ent.id);
>  	if (error == -EINVAL)
>  		goto out;
>  	if (error == -ENOENT)
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 73e9573..243d180 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -444,7 +444,7 @@ static int rsc_parse(struct cache_detail *cd,
>  		goto out;
>  
>  	/* uid, or NEGATIVE */
> -	rv = get_int(&mesg, &rsci.cred.cr_uid);
> +	rv = get_uint(&mesg, &rsci.cred.cr_uid);
>  	if (rv == -EINVAL)
>  		goto out;
>  	if (rv == -ENOENT)
> @@ -453,7 +453,7 @@ static int rsc_parse(struct cache_detail *cd,
>  		int N, i;
>  
>  		/* gid */
> -		if (get_int(&mesg, &rsci.cred.cr_gid))
> +		if (get_uint(&mesg, &rsci.cred.cr_gid))
>  			goto out;
>  
>  		/* number of additional gid's */
> @@ -469,7 +469,7 @@ static int rsc_parse(struct cache_detail *cd,
>  		for (i=0; i<N; i++) {
>  			gid_t gid;
>  			kgid_t kgid;
> -			if (get_int(&mesg, &gid))
> +			if (get_uint(&mesg, &gid))
>  				goto out;
>  			kgid = make_kgid(&init_user_ns, gid);
>  			if (!gid_valid(kgid))
> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index 4d01292..5d7020a 100644
> --- a/net/sunrpc/svcauth_unix.c
> +++ b/net/sunrpc/svcauth_unix.c
> @@ -493,7 +493,7 @@ static int unix_gid_parse(struct cache_detail *cd,
>  		return -EINVAL;
>  	mesg[mlen-1] = 0;
>  
> -	rv = get_int(&mesg, &uid);
> +	rv = get_uint(&mesg, &uid);
>  	if (rv)
>  		return -EINVAL;
>  	ug.uid = uid;
> @@ -513,7 +513,7 @@ static int unix_gid_parse(struct cache_detail *cd,
>  	for (i = 0 ; i < gids ; i++) {
>  		int gid;
>  		kgid_t kgid;
> -		rv = get_int(&mesg, &gid);
> +		rv = get_uint(&mesg, &gid);
>  		err = -EINVAL;
>  		if (rv)
>  			goto out;

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

On Tue, Nov 13, 2012 at 07:58:15PM -0500, J. Bruce Fields wrote:
> On Tue, Nov 13, 2012 at 05:40:05PM -0500, J. Bruce Fields wrote:
> > On Mon, Nov 12, 2012 at 10:17:17AM +0100, Sven Geggus wrote:
> > > J. Bruce Fields schrieb am Samstag, den 10. November um 00:24 Uhr:
> > > 
> > > OK, back at work and here is what I get:
> > > 
> > > > Restart the server, start strace, then try the mount, let it hang a few
> > > > seconds just to make sure you got anything interesting, then kill strace
> > > > and send the output.
> > > 
> > > OK, back at work and here is what I get...
> > > 
> > > read(3, "nfsd 10.1.7.30\n", 2048)       = 15
> > > close(15)                               = 0
> > > open("/var/lib/nfs/etab", O_RDONLY)     = 15
> > > close(15)                               = 0
> > > close(15)                               = 0
> > > write(3, "nfsd 10.1.7.30 1352710828 * \n", 29) = 29
> > > read(4, "4294967295\n", 2048)           = 11
> > > close(16)                               = 0
> > > close(15)                               = 0
> > > read(15,
> > > "\2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\0\0\0\0\0\0\0\0\0\0\0\0",
> > > 36) = 36
> > > close(15)                               = 0
> > > write(4, "4294967295 1352710828 0 \n", 25) = -1 EINVAL (Invalid argument)
> > 
> > I suspect that error's coming from
> > net/sunrpc/svcauth_unix.c:unix_gid_parse().
> > 
> > > 4294967295 is UINT_MAX and this place is where it behaves differently on a good
> > > kernel where the write call will succeed:
> > > 
> > > write(4, "4294967295 1352710828 0 \n", 25) = 25
> > > 
> > > Sven
> > > 
> > > P.S.: Your patched svcauth_gss.c will give me an "access denied by server"
> > > while mounting instead of the infinite delay:
> > >  ~/ # mount -t nfs4 -o sec=krb5 testsrv:/storage /mnt/
> > > mount.nfs4: access denied by server while mounting testsrv:/storage
> > 
> > So, looks like the same get_int problem exists in several other places.
> > Could you try the following instead of the previous patch?  I think I
> > got them all this time....
> 
> Oh, cripes, but this isn't good enough--svcgssd actually passes down -1
> id's.  Ugh--I'll take a closer look tomorrow.

Yeah, for backwards compatibility reasons we probably don't want to
reject either -1 or 4294967295.

So I'm inclined to revert unless Eldad has a better idea.

--b.

commit 664f26313a738f539a32c4eadd5351905e301bf2
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Fri Nov 9 15:16:02 2012 -0500

    svcrpc: fix parsing of uids and gids in gss contexts
    
    bbf43dc888833ac0539e437dbaeb28bfd4fbab9f "sunrpc/cache.h: replace
    simple_strtoul" introduced new range-checking which could cause get_int
    to fail if given an unsigned integer too large to represent as an int.
    
    Symptoms were hangs on krb5 mounts after upgrading an NFS server.
    
    Cc: Eldad Zack <eldad@fogrefinery.com>
    Reported-by: Sven Geggus <lists@fuchsschwanzdomain.de>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index a3946cf..8481961 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -491,7 +491,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 	int err;
 	struct auth_domain *dom = NULL;
 	struct svc_export exp = {}, *expp;
-	int an_int;
+	unsigned int an_int;
 
 	if (mesg[mlen-1] != '\n')
 		return -EINVAL;
@@ -531,7 +531,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 		goto out3;
 
 	/* flags */
-	err = get_int(&mesg, &an_int);
+	err = get_uint(&mesg, &an_int);
 	if (err == -ENOENT) {
 		err = 0;
 		set_bit(CACHE_NEGATIVE, &exp.h.flags);
@@ -541,19 +541,19 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 		exp.ex_flags= an_int;
 	
 		/* anon uid */
-		err = get_int(&mesg, &an_int);
+		err = get_uint(&mesg, &an_int);
 		if (err)
 			goto out3;
 		exp.ex_anon_uid= an_int;
 
 		/* anon gid */
-		err = get_int(&mesg, &an_int);
+		err = get_uint(&mesg, &an_int);
 		if (err)
 			goto out3;
 		exp.ex_anon_gid= an_int;
 
 		/* fsid */
-		err = get_int(&mesg, &an_int);
+		err = get_uint(&mesg, &an_int);
 		if (err)
 			goto out3;
 		exp.ex_fsid = an_int;
diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
index a1f10c0..e2c9317 100644
--- a/fs/nfsd/nfs4idmap.c
+++ b/fs/nfsd/nfs4idmap.c
@@ -415,7 +415,7 @@ nametoid_parse(struct cache_detail *cd, char *buf, int buflen)
 		goto out;
 
 	/* ID */
-	error = get_int(&buf, &ent.id);
+	error = get_uint(&buf, &ent.id);
 	if (error == -EINVAL)
 		goto out;
 	if (error == -ENOENT)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 73e9573..243d180 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -444,7 +444,7 @@ static int rsc_parse(struct cache_detail *cd,
 		goto out;
 
 	/* uid, or NEGATIVE */
-	rv = get_int(&mesg, &rsci.cred.cr_uid);
+	rv = get_uint(&mesg, &rsci.cred.cr_uid);
 	if (rv == -EINVAL)
 		goto out;
 	if (rv == -ENOENT)
@@ -453,7 +453,7 @@ static int rsc_parse(struct cache_detail *cd,
 		int N, i;
 
 		/* gid */
-		if (get_int(&mesg, &rsci.cred.cr_gid))
+		if (get_uint(&mesg, &rsci.cred.cr_gid))
 			goto out;
 
 		/* number of additional gid's */
@@ -469,7 +469,7 @@ static int rsc_parse(struct cache_detail *cd,
 		for (i=0; i<N; i++) {
 			gid_t gid;
 			kgid_t kgid;
-			if (get_int(&mesg, &gid))
+			if (get_uint(&mesg, &gid))
 				goto out;
 			kgid = make_kgid(&init_user_ns, gid);
 			if (!gid_valid(kgid))
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 4d01292..5d7020a 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -493,7 +493,7 @@ static int unix_gid_parse(struct cache_detail *cd,
 		return -EINVAL;
 	mesg[mlen-1] = 0;
 
-	rv = get_int(&mesg, &uid);
+	rv = get_uint(&mesg, &uid);
 	if (rv)
 		return -EINVAL;
 	ug.uid = uid;
@@ -513,7 +513,7 @@ static int unix_gid_parse(struct cache_detail *cd,
 	for (i = 0 ; i < gids ; i++) {
 		int gid;
 		kgid_t kgid;
-		rv = get_int(&mesg, &gid);
+		rv = get_uint(&mesg, &gid);
 		err = -EINVAL;
 		if (rv)
 			goto out;

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

On Wed, Nov 14, 2012 at 11:07:13AM -0500, J. Bruce Fields wrote:
> On Tue, Nov 13, 2012 at 07:58:15PM -0500, J. Bruce Fields wrote:
> > On Tue, Nov 13, 2012 at 05:40:05PM -0500, J. Bruce Fields wrote:
> > > On Mon, Nov 12, 2012 at 10:17:17AM +0100, Sven Geggus wrote:
> > > > J. Bruce Fields schrieb am Samstag, den 10. November um 00:24 Uhr:
> > > > 
> > > > OK, back at work and here is what I get:
> > > > 
> > > > > Restart the server, start strace, then try the mount, let it hang a few
> > > > > seconds just to make sure you got anything interesting, then kill strace
> > > > > and send the output.
> > > > 
> > > > OK, back at work and here is what I get...
> > > > 
> > > > read(3, "nfsd 10.1.7.30\n", 2048)       = 15
> > > > close(15)                               = 0
> > > > open("/var/lib/nfs/etab", O_RDONLY)     = 15
> > > > close(15)                               = 0
> > > > close(15)                               = 0
> > > > write(3, "nfsd 10.1.7.30 1352710828 * \n", 29) = 29
> > > > read(4, "4294967295\n", 2048)           = 11
> > > > close(16)                               = 0
> > > > close(15)                               = 0
> > > > read(15,
> > > > "\2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\377\377\377\377\377\377\377\377\0\0\0\0\0\0\0\0\0\0\0\0",
> > > > 36) = 36
> > > > close(15)                               = 0
> > > > write(4, "4294967295 1352710828 0 \n", 25) = -1 EINVAL (Invalid argument)
> > > 
> > > I suspect that error's coming from
> > > net/sunrpc/svcauth_unix.c:unix_gid_parse().
> > > 
> > > > 4294967295 is UINT_MAX and this place is where it behaves differently on a good
> > > > kernel where the write call will succeed:
> > > > 
> > > > write(4, "4294967295 1352710828 0 \n", 25) = 25
> > > > 
> > > > Sven
> > > > 
> > > > P.S.: Your patched svcauth_gss.c will give me an "access denied by server"
> > > > while mounting instead of the infinite delay:
> > > >  ~/ # mount -t nfs4 -o sec=krb5 testsrv:/storage /mnt/
> > > > mount.nfs4: access denied by server while mounting testsrv:/storage
> > > 
> > > So, looks like the same get_int problem exists in several other places.
> > > Could you try the following instead of the previous patch?  I think I
> > > got them all this time....
> > 
> > Oh, cripes, but this isn't good enough--svcgssd actually passes down -1
> > id's.  Ugh--I'll take a closer look tomorrow.
> 
> Yeah, for backwards compatibility reasons we probably don't want to
> reject either -1 or 4294967295.
> 
> So I'm inclined to revert unless Eldad has a better idea.
> 
> --b.

Oops, sending the right thing this time.--b.

commit 8688bcb10bd006111b1b46c23a27081ea359e140
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Wed Nov 14 10:48:05 2012 -0500

    svcrpc: Revert "sunrpc/cache.h: replace simple_strtoul"
    
    Commit bbf43dc888833ac0539e437dbaeb28bfd4fbab9f "sunrpc/cache.h: replace
    simple_strtoul" introduced new range-checking which could cause get_int
    to fail on unsigned integers to large to be represented as an int.
    
    We could parse them as unsigned instead--but it turns out svcgssd is
    actually passing down "-1" in some cases.  Which is perhaps stupid, but
    there's nothing we can do about it now.
    
    So just revert back to the previous "sloppy" behavior that accepts
    either representation.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
index f792794..5dc9ee4 100644
--- a/include/linux/sunrpc/cache.h
+++ b/include/linux/sunrpc/cache.h
@@ -217,6 +217,8 @@ extern int qword_get(char **bpp, char *dest, int bufsize);
 static inline int get_int(char **bpp, int *anint)
 {
 	char buf[50];
+	char *ep;
+	int rv;
 	int len = qword_get(bpp, buf, sizeof(buf));
 
 	if (len < 0)
@@ -224,9 +226,11 @@ static inline int get_int(char **bpp, int *anint)
 	if (len == 0)
 		return -ENOENT;
 
-	if (kstrtoint(buf, 0, anint))
+	rv = simple_strtol(buf, &ep, 0);
+	if (*ep)
 		return -EINVAL;
 
+	*anint = rv;
 	return 0;
 }
 

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Eldad Zack]

On Wed, 14 Nov 2012, J. Bruce Fields wrote:
> On Tue, Nov 13, 2012 at 07:58:15PM -0500, J. Bruce Fields wrote:
> > On Tue, Nov 13, 2012 at 05:40:05PM -0500, J. Bruce Fields wrote:
> > > On Mon, Nov 12, 2012 at 10:17:17AM +0100, Sven Geggus wrote:
> > > > J. Bruce Fields schrieb am Samstag, den 10. November um 00:24 Uhr:
> > > > 
> > > > 4294967295 is UINT_MAX and this place is where it behaves differently on a good
> > > > kernel where the write call will succeed:
> > > > 
> > > > write(4, "4294967295 1352710828 0 \n", 25) = 25
> > > > 
> > > > Sven
> > > > 
> > > > P.S.: Your patched svcauth_gss.c will give me an "access denied by server"
> > > > while mounting instead of the infinite delay:
> > > >  ~/ # mount -t nfs4 -o sec=krb5 testsrv:/storage /mnt/
> > > > mount.nfs4: access denied by server while mounting testsrv:/storage
> > > 
> > > So, looks like the same get_int problem exists in several other places.
> > > Could you try the following instead of the previous patch?  I think I
> > > got them all this time....
> > 
> > Oh, cripes, but this isn't good enough--svcgssd actually passes down -1
> > id's.  Ugh--I'll take a closer look tomorrow.
> 
> Yeah, for backwards compatibility reasons we probably don't want to
> reject either -1 or 4294967295.
> 
> So I'm inclined to revert unless Eldad has a better idea.

I support that - please revert. I don't know my way around the code 
enough to suggest a good solution at this point.

Cheers,
Eldad

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

J. Bruce Fields schrieb am Mittwoch, den 14. November um 17:08 Uhr:

> commit 8688bcb10bd006111b1b46c23a27081ea359e140
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Wed Nov 14 10:48:05 2012 -0500
> 
>     svcrpc: Revert "sunrpc/cache.h: replace simple_strtoul"
>     
>     Commit bbf43dc888833ac0539e437dbaeb28bfd4fbab9f "sunrpc/cache.h: replace
>     simple_strtoul" introduced new range-checking which could cause get_int
>     to fail on unsigned integers to large to be represented as an int.
>     
>     We could parse them as unsigned instead--but it turns out svcgssd is
>     actually passing down "-1" in some cases.  Which is perhaps stupid, but
>     there's nothing we can do about it now.
>     
>     So just revert back to the previous "sloppy" behavior that accepts
>     either representation.
>     
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
> index f792794..5dc9ee4 100644
> --- a/include/linux/sunrpc/cache.h
> +++ b/include/linux/sunrpc/cache.h
> @@ -217,6 +217,8 @@ extern int qword_get(char **bpp, char *dest, int bufsize);
>  static inline int get_int(char **bpp, int *anint)
>  {
>  	char buf[50];
> +	char *ep;
> +	int rv;
>  	int len = qword_get(bpp, buf, sizeof(buf));
>  
>  	if (len < 0)
> @@ -224,9 +226,11 @@ static inline int get_int(char **bpp, int *anint)
>  	if (len == 0)
>  		return -ENOENT;
>  
> -	if (kstrtoint(buf, 0, anint))
> +	rv = simple_strtol(buf, &ep, 0);
> +	if (*ep)
>  		return -EINVAL;
>  
> +	*anint = rv;
>  	return 0;
>  }

OK, so this can be marked as resolved (for now) on my todo list, right? Will
this go into 3.6.7 and of course 3.7?

Sven

-- 
"Those who do not understand Unix are condemned to reinvent it, poorly"
(Henry Spencer)

/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

On Thu, Nov 15, 2012 at 05:58:24PM +0100, Sven Geggus wrote:
> OK, so this can be marked as resolved (for now) on my todo list, right? Will
> this go into 3.6.7 and of course 3.7?

Yep, I'll pass this along by next week.

--b.

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[Sven Geggus]

J. Bruce Fields <bfields@fieldses.org> wrote:

>> OK, so this can be marked as resolved (for now) on my todo list, right? Will
>> this go into 3.6.7 and of course 3.7?
> 
> Yep, I'll pass this along by next week.

As 3.7 has been relased now:

It looks like the unreverted Version is still present in Linux 3.7.0 as well
as 3.6.10

While it is easy enough to use
"git revert bbf43dc888833ac0539e437dbaeb28bfd4fbab9f" I would rather be able
to use vanilla kernels again.

Regards

Sven

-- 
Das Internet wird vor allem von Leuten genutzt, die sich Pornografie
ansehen, während sie Bier trinken, es ist daher für Wahlen nicht
geeignet (Jaroslaw Kaczynski)
/me is giggls@ircnet, http://sven.gegg.us/ on the Web

Re: Kernel update 3.5.7 -> 3.6.3 breaks NFS4

[J. Bruce Fields]

Please don't drop me off the cc: line.

On Wed, Dec 12, 2012 at 11:15:27AM +0000, Sven Geggus wrote:
> J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> >> OK, so this can be marked as resolved (for now) on my todo list, right? Will
> >> this go into 3.6.7 and of course 3.7?
> > 
> > Yep, I'll pass this along by next week.
> 
> As 3.7 has been relased now:
> 
> It looks like the unreverted Version is still present in Linux 3.7.0 as well
> as 3.6.10
> 
> While it is easy enough to use
> "git revert bbf43dc888833ac0539e437dbaeb28bfd4fbab9f" I would rather be able
> to use vanilla kernels again.

Right, apologies, I ended up deciding to hold that for the merge window.
It's marked for stable so should make it into 3.7.x as well.

--b.