Index i was already used in the outer loop
Signed-off-by: Roel Kluin <[email protected]>
---
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:
On Tue, 08 Mar 2011 22:32:26 +0100, roel <[email protected]> wrote:
> Index i was already used in the outer loop
>
Is this as serious as it looks ?
Is it worth tho flag your own loved distro to hurry on a kernel
patch ?
TIA
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> 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:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
J.A. Magallon <jamagallon()ono!com> \ Software is like sex:
\ It's better when it's free
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 <[email protected]>
> ---
> 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:
On Tue, 08 Mar 2011 22:32:26 +0100
roel <[email protected]> wrote:
> Index i was already used in the outer loop
>
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> 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.
On Wed, Mar 09, 2011 at 03:42:30PM -0800, Andrew Morton wrote:
> On Tue, 08 Mar 2011 22:32:26 +0100
> roel <[email protected]> wrote:
>
> > Index i was already used in the outer loop
> >
> > Signed-off-by: Roel Kluin <[email protected]>
> > ---
> > 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.
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 <[email protected]> wrote:
>>
>>> Index i was already used in the outer loop
>>>
>>> Signed-off-by: Roel Kluin <[email protected]>
>>> ---
>>> 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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
>> ---
>> 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 <[email protected]>
---
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
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 <[email protected]> wrote:
> >>
> >>> Index i was already used in the outer loop
> >>>
> >>> Signed-off-by: Roel Kluin <[email protected]>
> >>> ---
> >>> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
>
>
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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
> ---
> 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
>
>
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 <[email protected]>
> > >> ---
> > >> 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
[email protected]
http://www.netapp.com
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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
>> ---
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
----
thanks
Mi Jinlong
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.
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 <[email protected]>
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 <[email protected]>
[[email protected]: Trond points out READ_BUF alone is sufficient.]
Cc: [email protected]
Signed-off-by: J. Bruce Fields <[email protected]>
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");