* [PATCH] nfsd: wrong index used in inner loop @ 2011-03-08 21:32 roel 2011-03-09 0:49 ` J. Bruce Fields 2011-03-09 23:42 ` Andrew Morton 0 siblings, 2 replies; 12+ messages in thread From: roel @ 2011-03-08 21:32 UTC (permalink / raw) To: J. Bruce Fields, Neil Brown, linux-nfs, Andrew Morton, LKML 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: ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-08 21:32 [PATCH] nfsd: wrong index used in inner loop roel @ 2011-03-09 0:49 ` J. Bruce Fields 2011-03-11 4:13 ` Mi Jinlong 2011-03-09 23:42 ` Andrew Morton 1 sibling, 1 reply; 12+ messages in thread From: J. Bruce Fields @ 2011-03-09 0:49 UTC (permalink / raw) To: roel; +Cc: Neil Brown, linux-nfs, Andrew Morton, LKML 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: ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-09 0:49 ` J. Bruce Fields @ 2011-03-11 4:13 ` Mi Jinlong 2011-03-14 22:22 ` J. Bruce Fields 0 siblings, 1 reply; 12+ messages in thread From: Mi Jinlong @ 2011-03-11 4:13 UTC (permalink / raw) To: J. Bruce Fields; +Cc: roel, Neil Brown, linux-nfs, Andrew Morton, LKML 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-11 4:13 ` Mi Jinlong @ 2011-03-14 22:22 ` J. Bruce Fields 2011-03-14 23:52 ` Trond Myklebust 2011-03-15 2:31 ` Mi Jinlong 0 siblings, 2 replies; 12+ messages in thread From: J. Bruce Fields @ 2011-03-14 22:22 UTC (permalink / raw) To: Mi Jinlong; +Cc: roel, Neil Brown, linux-nfs, Andrew Morton, LKML 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-14 22:22 ` J. Bruce Fields @ 2011-03-14 23:52 ` Trond Myklebust 2011-03-16 22:55 ` J. Bruce Fields 2011-03-15 2:31 ` Mi Jinlong 1 sibling, 1 reply; 12+ messages in thread From: Trond Myklebust @ 2011-03-14 23:52 UTC (permalink / raw) To: J. Bruce Fields Cc: Mi Jinlong, roel, Neil Brown, linux-nfs, Andrew Morton, LKML 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-14 23:52 ` Trond Myklebust @ 2011-03-16 22:55 ` J. Bruce Fields 0 siblings, 0 replies; 12+ messages in thread From: J. Bruce Fields @ 2011-03-16 22:55 UTC (permalink / raw) To: Trond Myklebust Cc: Mi Jinlong, roel, Neil Brown, linux-nfs, Andrew Morton, LKML 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-14 22:22 ` J. Bruce Fields 2011-03-14 23:52 ` Trond Myklebust @ 2011-03-15 2:31 ` Mi Jinlong 2011-03-17 17:52 ` J. Bruce Fields 1 sibling, 1 reply; 12+ messages in thread From: Mi Jinlong @ 2011-03-15 2:31 UTC (permalink / raw) To: J. Bruce Fields; +Cc: roel, Neil Brown, linux-nfs, Andrew Morton, LKML 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-15 2:31 ` Mi Jinlong @ 2011-03-17 17:52 ` J. Bruce Fields 0 siblings, 0 replies; 12+ messages in thread From: J. Bruce Fields @ 2011-03-17 17:52 UTC (permalink / raw) To: Mi Jinlong; +Cc: roel, Neil Brown, linux-nfs, Andrew Morton, LKML 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"); ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-08 21:32 [PATCH] nfsd: wrong index used in inner loop roel 2011-03-09 0:49 ` J. Bruce Fields @ 2011-03-09 23:42 ` Andrew Morton 2011-03-10 18:08 ` J. Bruce Fields 1 sibling, 1 reply; 12+ messages in thread From: Andrew Morton @ 2011-03-09 23:42 UTC (permalink / raw) To: roel; +Cc: J. Bruce Fields, Neil Brown, linux-nfs, LKML 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-09 23:42 ` Andrew Morton @ 2011-03-10 18:08 ` J. Bruce Fields 2011-03-11 3:52 ` Mi Jinlong 0 siblings, 1 reply; 12+ messages in thread From: J. Bruce Fields @ 2011-03-10 18:08 UTC (permalink / raw) To: Andrew Morton; +Cc: roel, Neil Brown, linux-nfs, LKML, Mi Jinlong 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-10 18:08 ` J. Bruce Fields @ 2011-03-11 3:52 ` Mi Jinlong 2011-03-14 19:35 ` J. Bruce Fields 0 siblings, 1 reply; 12+ messages in thread From: Mi Jinlong @ 2011-03-11 3:52 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Andrew Morton, roel, Neil Brown, linux-nfs, LKML 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] nfsd: wrong index used in inner loop 2011-03-11 3:52 ` Mi Jinlong @ 2011-03-14 19:35 ` J. Bruce Fields 0 siblings, 0 replies; 12+ messages in thread From: J. Bruce Fields @ 2011-03-14 19:35 UTC (permalink / raw) To: Mi Jinlong; +Cc: Andrew Morton, roel, Neil Brown, linux-nfs, LKML 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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-03-17 17:52 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-08 21:32 [PATCH] nfsd: wrong index used in inner loop roel 2011-03-09 0:49 ` J. Bruce Fields 2011-03-11 4:13 ` Mi Jinlong 2011-03-14 22:22 ` J. Bruce Fields 2011-03-14 23:52 ` Trond Myklebust 2011-03-16 22:55 ` J. Bruce Fields 2011-03-15 2:31 ` Mi Jinlong 2011-03-17 17:52 ` J. Bruce Fields 2011-03-09 23:42 ` Andrew Morton 2011-03-10 18:08 ` J. Bruce Fields 2011-03-11 3:52 ` Mi Jinlong 2011-03-14 19:35 ` J. Bruce Fields
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).