[PATCH] nfsd: wrong index used in inner loop

[PATCH] nfsd: wrong index used in inner loop

[roel]

Index i was already used in the outer loop

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 1275b86..615f0a9 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
 
 	READ_BUF(16);
@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);
 			break;
 		case RPC_AUTH_GSS:

Re: [PATCH] nfsd: wrong index used in inner loop

[J. Bruce Fields]

On Tue, Mar 08, 2011 at 10:32:26PM +0100, roel wrote:
> Index i was already used in the outer loop
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
>  fs/nfsd/nfs4xdr.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> Not 100% sure this one is needed but it looks suspicious.

Looks bad to me, thanks.

nfsd4_decode_create_session should probably really be broken up a little
bit; if it wasn't so long this would have been more obvious.

I'll see if I can slip this into 2.6.38 with a couple other last-minute
patches....  Otherwise, it'll be in 2.6.39.

--b.

> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 1275b86..615f0a9 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>  
>  	u32 dummy;
>  	char *machine_name;
> -	int i;
> +	int i, j;
>  	int nr_secflavs;
>  
>  	READ_BUF(16);
> @@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>  			READ_BUF(4);
>  			READ32(dummy);
>  			READ_BUF(dummy * 4);
> -			for (i = 0; i < dummy; ++i)
> +			for (j = 0; j < dummy; ++j)
>  				READ32(dummy);
>  			break;
>  		case RPC_AUTH_GSS:

Re: [PATCH] nfsd: wrong index used in inner loop

[Andrew Morton]

On Tue, 08 Mar 2011 22:32:26 +0100
roel <roel.kluin@gmail.com> wrote:

> Index i was already used in the outer loop
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
>  fs/nfsd/nfs4xdr.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> Not 100% sure this one is needed but it looks suspicious.
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 1275b86..615f0a9 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>  
>  	u32 dummy;
>  	char *machine_name;
> -	int i;
> +	int i, j;
>  	int nr_secflavs;
>  
>  	READ_BUF(16);
> @@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>  			READ_BUF(4);
>  			READ32(dummy);
>  			READ_BUF(dummy * 4);
> -			for (i = 0; i < dummy; ++i)
> +			for (j = 0; j < dummy; ++j)
>  				READ32(dummy);
>  			break;
>  		case RPC_AUTH_GSS:

ooh, big bug.

I wonder why it was not previously detected at runtime.  Perhaps
nr_secflavs is always 1.

afacit this bug will allow a well-crafted packet to cause an
infinite-until-it-oopses loop in the kernel.

Re: [PATCH] nfsd: wrong index used in inner loop

[J. Bruce Fields]

On Wed, Mar 09, 2011 at 03:42:30PM -0800, Andrew Morton wrote:
> On Tue, 08 Mar 2011 22:32:26 +0100
> roel <roel.kluin@gmail.com> wrote:
> 
> > Index i was already used in the outer loop
> > 
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> >  fs/nfsd/nfs4xdr.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > Not 100% sure this one is needed but it looks suspicious.
> > 
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 1275b86..615f0a9 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
> >  
> >  	u32 dummy;
> >  	char *machine_name;
> > -	int i;
> > +	int i, j;
> >  	int nr_secflavs;
> >  
> >  	READ_BUF(16);
> > @@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
> >  			READ_BUF(4);
> >  			READ32(dummy);
> >  			READ_BUF(dummy * 4);
> > -			for (i = 0; i < dummy; ++i)
> > +			for (j = 0; j < dummy; ++j)
> >  				READ32(dummy);
> >  			break;
> >  		case RPC_AUTH_GSS:
> 
> ooh, big bug.
> 
> I wonder why it was not previously detected at runtime.  Perhaps
> nr_secflavs is always 1.

Yeah, no client uses this calback security information yet.

Mi Jinlong, do you think this is something we could have caught with
another pynfs test?

--b.

Re: [PATCH] nfsd: wrong index used in inner loop

[Mi Jinlong]

J. Bruce Fields:
> On Wed, Mar 09, 2011 at 03:42:30PM -0800, Andrew Morton wrote:
>> On Tue, 08 Mar 2011 22:32:26 +0100
>> roel <roel.kluin@gmail.com> wrote:
>>
>>> Index i was already used in the outer loop
>>>
>>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>>> ---
>>>  fs/nfsd/nfs4xdr.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> Not 100% sure this one is needed but it looks suspicious.
>>>
>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>> index 1275b86..615f0a9 100644
>>> --- a/fs/nfsd/nfs4xdr.c
>>> +++ b/fs/nfsd/nfs4xdr.c
>>> @@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>>>  
>>>  	u32 dummy;
>>>  	char *machine_name;
>>> -	int i;
>>> +	int i, j;
>>>  	int nr_secflavs;
>>>  
>>>  	READ_BUF(16);
>>> @@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>>>  			READ_BUF(4);
>>>  			READ32(dummy);
>>>  			READ_BUF(dummy * 4);
>>> -			for (i = 0; i < dummy; ++i)
>>> +			for (j = 0; j < dummy; ++j)
>>>  				READ32(dummy);
>>>  			break;
>>>  		case RPC_AUTH_GSS:
>> ooh, big bug.
>>
>> I wonder why it was not previously detected at runtime.  Perhaps
>> nr_secflavs is always 1.
> 
> Yeah, no client uses this calback security information yet.
> 
> Mi Jinlong, do you think this is something we could have caught with
> another pynfs test?

  Yes, we must test it.

  After testing, the following test case is OK.

--
thanks,
Mi Jinlong


>From 1afac3444b37bac66970f19c409660a304a53fb4 Mon Sep 17 00:00:00 2001
From: Mi Jinlong <mijinlong@cn.fujitsu.com>
Date: Sun, 11 Mar 2011 09:05:22 +0800
Subject: [PATCH] CLNT: test a decode problem which use wrong index

Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
---
 nfs4.1/server41tests/st_create_session.py |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/nfs4.1/server41tests/st_create_session.py b/nfs4.1/server41tests/st_create_session.py
index ff55d10..e3a8421 100644
--- a/nfs4.1/server41tests/st_create_session.py
+++ b/nfs4.1/server41tests/st_create_session.py
@@ -252,6 +252,22 @@ def testCbSecParms(t, env):
     c1 = env.c1.new_client(env.testname(t))
     sess1 = c1.create_session(sec=sec)
 
+def testCbSecParmsDec(t, env):
+    """A decode problem was found at NFS server that 
+       wrong index used in inner loop, 
+       http://marc.info/?l=linux-kernel&m=129961996327640&w=2
+
+    FLAGS: create_session all
+    CODE: CSESS16a
+    """
+    sec = [callback_sec_parms4(AUTH_NONE),
+           callback_sec_parms4(RPCSEC_GSS, cbsp_gss_handles=gss_cb_handles4(RPC_GSS_SVC_PRIVACY, "Handle from server", "Client handle")),
+           callback_sec_parms4(AUTH_SYS, cbsp_sys_cred=authsys_parms(5, "Random machine name", 7, 11, [])),
+           ]
+                               
+    c1 = env.c1.new_client(env.testname(t))
+    sess1 = c1.create_session(sec=sec)
+
 def testRdmaArray0(t, env):
     """Test 0 length rdma arrays
 
-- 
1.7.4.1

Re: [PATCH] nfsd: wrong index used in inner loop

[Mi Jinlong]

J. Bruce Fields:
> On Tue, Mar 08, 2011 at 10:32:26PM +0100, roel wrote:
>> Index i was already used in the outer loop
>>
>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>> ---
>>  fs/nfsd/nfs4xdr.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> Not 100% sure this one is needed but it looks suspicious.
> 
> Looks bad to me, thanks.
> 
> nfsd4_decode_create_session should probably really be broken up a little
> bit; if it wasn't so long this would have been more obvious.
> 
> I'll see if I can slip this into 2.6.38 with a couple other last-minute
> patches....  Otherwise, it'll be in 2.6.39.
> 
> --b.
> 
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 1275b86..615f0a9 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>>  
>>  	u32 dummy;
>>  	char *machine_name;
>> -	int i;
>> +	int i, j;
>>  	int nr_secflavs;
>>  
>>  	READ_BUF(16);
>> @@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>>  			READ_BUF(4);
>>  			READ32(dummy);
>>  			READ_BUF(dummy * 4);
>> -			for (i = 0; i < dummy; ++i)
>> +			for (j = 0; j < dummy; ++j)
>>  				READ32(dummy);

  We must not use dummy for index here.
  After the first index, READ32(dummy) will change dummy!!!!

  The following patch fix this problem.

--
thanks,
Mi Jinlong
============================================================

We must not use dummy for index.
After the first index, READ32(dummy) will change dummy!!!!

Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 615f0a9..8dd70d0 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1140,7 +1140,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 {
 	DECODE_HEAD;
 
-	u32 dummy;
+	u32 dummy, tmp;
 	char *machine_name;
 	int i, j;
 	int nr_secflavs;
@@ -1216,7 +1216,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ32(dummy);
 			READ_BUF(dummy * 4);
 			for (j = 0; j < dummy; ++j)
-				READ32(dummy);
+				READ32(tmp);
 			break;
 		case RPC_AUTH_GSS:
 			dprintk("RPC_AUTH_GSS callback secflavor "
-- 
1.7.4.1

Re: [PATCH] nfsd: wrong index used in inner loop

[J. Bruce Fields]

On Fri, Mar 11, 2011 at 11:52:14AM +0800, Mi Jinlong wrote:
> 
> 
> J. Bruce Fields:
> > On Wed, Mar 09, 2011 at 03:42:30PM -0800, Andrew Morton wrote:
> >> On Tue, 08 Mar 2011 22:32:26 +0100
> >> roel <roel.kluin@gmail.com> wrote:
> >>
> >>> Index i was already used in the outer loop
> >>>
> >>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> >>> ---
> >>>  fs/nfsd/nfs4xdr.c |    4 ++--
> >>>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> Not 100% sure this one is needed but it looks suspicious.
> >>>
> >>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >>> index 1275b86..615f0a9 100644
> >>> --- a/fs/nfsd/nfs4xdr.c
> >>> +++ b/fs/nfsd/nfs4xdr.c
> >>> @@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
> >>>  
> >>>  	u32 dummy;
> >>>  	char *machine_name;
> >>> -	int i;
> >>> +	int i, j;
> >>>  	int nr_secflavs;
> >>>  
> >>>  	READ_BUF(16);
> >>> @@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
> >>>  			READ_BUF(4);
> >>>  			READ32(dummy);
> >>>  			READ_BUF(dummy * 4);
> >>> -			for (i = 0; i < dummy; ++i)
> >>> +			for (j = 0; j < dummy; ++j)
> >>>  				READ32(dummy);
> >>>  			break;
> >>>  		case RPC_AUTH_GSS:
> >> ooh, big bug.
> >>
> >> I wonder why it was not previously detected at runtime.  Perhaps
> >> nr_secflavs is always 1.
> > 
> > Yeah, no client uses this calback security information yet.
> > 
> > Mi Jinlong, do you think this is something we could have caught with
> > another pynfs test?
> 
>   Yes, we must test it.
> 
>   After testing, the following test case is OK.

I've pushed out a tree with your additional tests to

	git://linux-nfs.org/~bfields/pynfs41.git

Let me know what I'm missing.

Thanks again.--b.

> 
> --
> thanks,
> Mi Jinlong
> 
> 
> >From 1afac3444b37bac66970f19c409660a304a53fb4 Mon Sep 17 00:00:00 2001
> From: Mi Jinlong <mijinlong@cn.fujitsu.com>
> Date: Sun, 11 Mar 2011 09:05:22 +0800
> Subject: [PATCH] CLNT: test a decode problem which use wrong index
> 
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
>  nfs4.1/server41tests/st_create_session.py |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/nfs4.1/server41tests/st_create_session.py b/nfs4.1/server41tests/st_create_session.py
> index ff55d10..e3a8421 100644
> --- a/nfs4.1/server41tests/st_create_session.py
> +++ b/nfs4.1/server41tests/st_create_session.py
> @@ -252,6 +252,22 @@ def testCbSecParms(t, env):
>      c1 = env.c1.new_client(env.testname(t))
>      sess1 = c1.create_session(sec=sec)
>  
> +def testCbSecParmsDec(t, env):
> +    """A decode problem was found at NFS server that 
> +       wrong index used in inner loop, 
> +       http://marc.info/?l=linux-kernel&m=129961996327640&w=2
> +
> +    FLAGS: create_session all
> +    CODE: CSESS16a
> +    """
> +    sec = [callback_sec_parms4(AUTH_NONE),
> +           callback_sec_parms4(RPCSEC_GSS, cbsp_gss_handles=gss_cb_handles4(RPC_GSS_SVC_PRIVACY, "Handle from server", "Client handle")),
> +           callback_sec_parms4(AUTH_SYS, cbsp_sys_cred=authsys_parms(5, "Random machine name", 7, 11, [])),
> +           ]
> +                               
> +    c1 = env.c1.new_client(env.testname(t))
> +    sess1 = c1.create_session(sec=sec)
> +
>  def testRdmaArray0(t, env):
>      """Test 0 length rdma arrays
>  
> -- 
> 1.7.4.1
> 
> 

Re: [PATCH] nfsd: wrong index used in inner loop

[J. Bruce Fields]

On Fri, Mar 11, 2011 at 12:13:55PM +0800, Mi Jinlong wrote:
> 
> 
> J. Bruce Fields:
> > On Tue, Mar 08, 2011 at 10:32:26PM +0100, roel wrote:
> >> Index i was already used in the outer loop
> >>
> >> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> >> ---
> >>  fs/nfsd/nfs4xdr.c |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> Not 100% sure this one is needed but it looks suspicious.
> > 
> > Looks bad to me, thanks.
> > 
> > nfsd4_decode_create_session should probably really be broken up a little
> > bit; if it wasn't so long this would have been more obvious.
> > 
> > I'll see if I can slip this into 2.6.38 with a couple other last-minute
> > patches....  Otherwise, it'll be in 2.6.39.
> > 
> > --b.
> > 
> >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >> index 1275b86..615f0a9 100644
> >> --- a/fs/nfsd/nfs4xdr.c
> >> +++ b/fs/nfsd/nfs4xdr.c
> >> @@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
> >>  
> >>  	u32 dummy;
> >>  	char *machine_name;
> >> -	int i;
> >> +	int i, j;
> >>  	int nr_secflavs;
> >>  
> >>  	READ_BUF(16);
> >> @@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
> >>  			READ_BUF(4);
> >>  			READ32(dummy);
> >>  			READ_BUF(dummy * 4);
> >> -			for (i = 0; i < dummy; ++i)
> >> +			for (j = 0; j < dummy; ++j)
> >>  				READ32(dummy);
> 
>   We must not use dummy for index here.
>   After the first index, READ32(dummy) will change dummy!!!!

Actually, wait, this is kind of silly.  I don't see why we couldn't just
skip the loop and do

	p += dummy;

Also, your new test is still failing with a BAD_XDR error.  Well, maybe
the test should fail--we don't really implement this yet anyway--but it
should at least be getting past the xdr decoding.  So something else is
still wrong.

--b.

> 
>   The following patch fix this problem.
> 
> --
> thanks,
> Mi Jinlong
> ============================================================
> 
> We must not use dummy for index.
> After the first index, READ32(dummy) will change dummy!!!!
> 
> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
> ---
>  fs/nfsd/nfs4xdr.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 615f0a9..8dd70d0 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1140,7 +1140,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>  {
>  	DECODE_HEAD;
>  
> -	u32 dummy;
> +	u32 dummy, tmp;
>  	char *machine_name;
>  	int i, j;
>  	int nr_secflavs;
> @@ -1216,7 +1216,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>  			READ32(dummy);
>  			READ_BUF(dummy * 4);
>  			for (j = 0; j < dummy; ++j)
> -				READ32(dummy);
> +				READ32(tmp);
>  			break;
>  		case RPC_AUTH_GSS:
>  			dprintk("RPC_AUTH_GSS callback secflavor "
> -- 
> 1.7.4.1
> 
> 

Re: [PATCH] nfsd: wrong index used in inner loop

[Trond Myklebust]

On Mon, 2011-03-14 at 18:22 -0400, J. Bruce Fields wrote:
> On Fri, Mar 11, 2011 at 12:13:55PM +0800, Mi Jinlong wrote:
> > 
> > 
> > J. Bruce Fields:
> > > On Tue, Mar 08, 2011 at 10:32:26PM +0100, roel wrote:
> > >> Index i was already used in the outer loop
> > >>
> > >> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > >> ---
> > >>  fs/nfsd/nfs4xdr.c |    4 ++--
> > >>  1 files changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> Not 100% sure this one is needed but it looks suspicious.
> > > 
> > > Looks bad to me, thanks.
> > > 
> > > nfsd4_decode_create_session should probably really be broken up a little
> > > bit; if it wasn't so long this would have been more obvious.
> > > 
> > > I'll see if I can slip this into 2.6.38 with a couple other last-minute
> > > patches....  Otherwise, it'll be in 2.6.39.
> > > 
> > > --b.
> > > 
> > >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > >> index 1275b86..615f0a9 100644
> > >> --- a/fs/nfsd/nfs4xdr.c
> > >> +++ b/fs/nfsd/nfs4xdr.c
> > >> @@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
> > >>  
> > >>  	u32 dummy;
> > >>  	char *machine_name;
> > >> -	int i;
> > >> +	int i, j;
> > >>  	int nr_secflavs;
> > >>  
> > >>  	READ_BUF(16);
> > >> @@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
> > >>  			READ_BUF(4);
> > >>  			READ32(dummy);
> > >>  			READ_BUF(dummy * 4);
> > >> -			for (i = 0; i < dummy; ++i)
> > >> +			for (j = 0; j < dummy; ++j)
> > >>  				READ32(dummy);
> > 
> >   We must not use dummy for index here.
> >   After the first index, READ32(dummy) will change dummy!!!!
> 
> Actually, wait, this is kind of silly.  I don't see why we couldn't just
> skip the loop and do
> 
> 	p += dummy;

This is exactly why I _hate_ the READ*() macros and their ilk, and am
really happy we got rid of them in the client.

READ_BUF() _sets_ p to whatever the value of argp->p is, and then
updates argp->p. It is just very very very hard to see that due to the
lack of transparency.

IOW: You don't need the "p += dummy" either. That happens automatically
when you next invoke READ_BUF().

Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] nfsd: wrong index used in inner loop

[Mi Jinlong]

J. Bruce Fields:
> On Fri, Mar 11, 2011 at 12:13:55PM +0800, Mi Jinlong wrote:
>>
>> J. Bruce Fields:
>>> On Tue, Mar 08, 2011 at 10:32:26PM +0100, roel wrote:
>>>> Index i was already used in the outer loop
>>>>
>>>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>>>> ---
>>>>  fs/nfsd/nfs4xdr.c |    4 ++--
>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> Not 100% sure this one is needed but it looks suspicious.
>>> Looks bad to me, thanks.
>>>
>>> nfsd4_decode_create_session should probably really be broken up a little
>>> bit; if it wasn't so long this would have been more obvious.
>>>
>>> I'll see if I can slip this into 2.6.38 with a couple other last-minute
>>> patches....  Otherwise, it'll be in 2.6.39.
>>>
>>> --b.
>>>
>>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>>> index 1275b86..615f0a9 100644
>>>> --- a/fs/nfsd/nfs4xdr.c
>>>> +++ b/fs/nfsd/nfs4xdr.c
>>>> @@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>>>>  
>>>>  	u32 dummy;
>>>>  	char *machine_name;
>>>> -	int i;
>>>> +	int i, j;
>>>>  	int nr_secflavs;
>>>>  
>>>>  	READ_BUF(16);
>>>> @@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>>>>  			READ_BUF(4);
>>>>  			READ32(dummy);
>>>>  			READ_BUF(dummy * 4);
>>>> -			for (i = 0; i < dummy; ++i)
>>>> +			for (j = 0; j < dummy; ++j)
>>>>  				READ32(dummy);
>>   We must not use dummy for index here.
>>   After the first index, READ32(dummy) will change dummy!!!!
> 
> Actually, wait, this is kind of silly.  I don't see why we couldn't just
> skip the loop and do
> 
> 	p += dummy;
> 
> Also, your new test is still failing with a BAD_XDR error.  Well, maybe
> the test should fail--we don't really implement this yet anyway--but it
> should at least be getting past the xdr decoding.  So something else is
> still wrong.

  How did you modify it??

  When testing it, I modify as 
 
    -                       for (j = 0; j < dummy; ++j)
    -                               READ32(dummy);
    +                       p += dummy;

  or
    
    -                       for (j = 0; j < dummy; ++j)
    -                               READ32(dummy);

  Test case CSESS16 and CSESS16a are PASS, 
  I can't get BAD_XDR error as you said.

--
thanks,
Mi Jinlong

> 
> --b.
> 
>>   The following patch fix this problem.
>>
>> --
>> thanks,
>> Mi Jinlong
>> ============================================================
>>
>> We must not use dummy for index.
>> After the first index, READ32(dummy) will change dummy!!!!
>>
>> Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
>> ---
>>  fs/nfsd/nfs4xdr.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 615f0a9..8dd70d0 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -1140,7 +1140,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>>  {
>>  	DECODE_HEAD;
>>  
>> -	u32 dummy;
>> +	u32 dummy, tmp;
>>  	char *machine_name;
>>  	int i, j;
>>  	int nr_secflavs;
>> @@ -1216,7 +1216,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
>>  			READ32(dummy);
>>  			READ_BUF(dummy * 4);
>>  			for (j = 0; j < dummy; ++j)
>> -				READ32(dummy);
>> +				READ32(tmp);
>>  			break;
>>  		case RPC_AUTH_GSS:
>>  			dprintk("RPC_AUTH_GSS callback secflavor "
>> -- 
>> 1.7.4.1
>>
>>
> --
> 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
> 

-- 
----
thanks
Mi Jinlong

Re: [PATCH] nfsd: wrong index used in inner loop

[J. Bruce Fields]

On Mon, Mar 14, 2011 at 07:52:11PM -0400, Trond Myklebust wrote:
> On Mon, 2011-03-14 at 18:22 -0400, J. Bruce Fields wrote:
> > On Fri, Mar 11, 2011 at 12:13:55PM +0800, Mi Jinlong wrote:
> > > 
> > > 
> > > J. Bruce Fields:
> > > > On Tue, Mar 08, 2011 at 10:32:26PM +0100, roel wrote:
> > > >> @@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
> > > >>  			READ_BUF(4);
> > > >>  			READ32(dummy);
> > > >>  			READ_BUF(dummy * 4);
> > > >> -			for (i = 0; i < dummy; ++i)
> > > >> +			for (j = 0; j < dummy; ++j)
> > > >>  				READ32(dummy);
> > > 
> > >   We must not use dummy for index here.
> > >   After the first index, READ32(dummy) will change dummy!!!!
> > 
> > Actually, wait, this is kind of silly.  I don't see why we couldn't just
> > skip the loop and do
> > 
> > 	p += dummy;
> 
> This is exactly why I _hate_ the READ*() macros and their ilk, and am
> really happy we got rid of them in the client.

Agreed, I'm all for getting rid of them completely.

> READ_BUF() _sets_ p to whatever the value of argp->p is, and then
> updates argp->p. It is just very very very hard to see that due to the
> lack of transparency.
>
> IOW: You don't need the "p += dummy" either. That happens automatically
> when you next invoke READ_BUF().

Yes, you're right, we could remove that silly loop entirely.

--b.

Re: [PATCH] nfsd: wrong index used in inner loop

[J. Bruce Fields]

On Tue, Mar 15, 2011 at 10:31:45AM +0800, Mi Jinlong wrote:
> 
> 
> J. Bruce Fields:
> > Actually, wait, this is kind of silly.  I don't see why we couldn't just
> > skip the loop and do
> > 
> > 	p += dummy;
> > 
> > Also, your new test is still failing with a BAD_XDR error.  Well, maybe
> > the test should fail--we don't really implement this yet anyway--but it
> > should at least be getting past the xdr decoding.  So something else is
> > still wrong.
> 
>   How did you modify it??
> 
>   When testing it, I modify as 
>  
>     -                       for (j = 0; j < dummy; ++j)
>     -                               READ32(dummy);
>     +                       p += dummy;
> 
>   or
>     
>     -                       for (j = 0; j < dummy; ++j)
>     -                               READ32(dummy);
> 
>   Test case CSESS16 and CSESS16a are PASS, 
>   I can't get BAD_XDR error as you said.

Yes, I thought I had the former, but perhaps I had the wrong kernel
running on my test server.  I've confirmed those tests pass after the
following patch.

--b.

commit 5a02ab7c3c4580f94d13c683721039855b67cda6
Author: Mi Jinlong <mijinlong@cn.fujitsu.com>
Date:   Fri Mar 11 12:13:55 2011 +0800

    nfsd: wrong index used in inner loop
    
    We must not use dummy for index.
    After the first index, READ32(dummy) will change dummy!!!!
    
    Signed-off-by: Mi Jinlong <mijinlong@cn.fujitsu.com>
    [bfields@redhat.com: Trond points out READ_BUF alone is sufficient.]
    Cc: stable@kernel.org
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 615f0a9..c6766af 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i, j;
+	int i;
 	int nr_secflavs;
 
 	READ_BUF(16);
@@ -1215,8 +1215,6 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (j = 0; j < dummy; ++j)
-				READ32(dummy);
 			break;
 		case RPC_AUTH_GSS:
 			dprintk("RPC_AUTH_GSS callback secflavor "
@@ -1232,7 +1230,6 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy);
-			p += XDR_QUADLEN(dummy);
 			break;
 		default:
 			dprintk("Illegal callback secflavor\n");