Skip to contentSkip to conversation

bug in read_buf

PATCH In review Neil Brown 3 participants 8 messages Apr 20, 2010 – Apr 22, 2010 1 message not in the archive

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Neil Brown

author
#0 lore raw

PATCH 1/1 bug in read_buf

Surely this can never have worked... which implies that the code has never been used?

When read_buf is called to move over to the next page in the pagelist of an NFSv4 request, it sets argp->end to essentially a random number, certainly not an address within the page which argp->p now points to. So subsequent calls to READ_BUF will think there is much more than a page of spare space (the cast to u32 ensures an unsigned comparison) so we can expect to fall off the end of the second page.

I guess we never ever receive requests with any operation starting beyond the first page!

[[
I found this while looking at why fsstress over NFS over RDMA caused a bad memory dereference in READ32, suggesting that 'p' had a bad value. However it was ffff8801299188f0, which is not an "I've fallen off the end of the page" sort of value. So I think it must be a different bug :-( It is as if the page is being unmapped underneath us...
]]
NeilBrown

fs/nfsd/nfs4xdr.c +4 -4
index e170317..34ccf81 100644
@@ -161,10 +161,10 @@ static __be32 *read_buf(struct nfsd4_compoundargs *argp, u32 nbytes)
 	argp->p = page_address(argp->pagelist[0]);
 	argp->pagelist++;
 	if (argp->pagelen < PAGE_SIZE) {
-		argp->end = p + (argp->pagelen>>2);
+		argp->end = argp->p + (argp->pagelen>>2);
 		argp->pagelen = 0;
 	} else {
-		argp->end = p + (PAGE_SIZE>>2);
+		argp->end = argp->p + (PAGE_SIZE>>2);
 		argp->pagelen -= PAGE_SIZE;
 	}
 	memcpy(((char*)p)+avail, argp->p, (nbytes - avail));

@@ -1426,10 +1426,10 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
 			argp->p = page_address(argp->pagelist[0]);
 			argp->pagelist++;
 			if (argp->pagelen < PAGE_SIZE) {
-				argp->end = p + (argp->pagelen>>2);
+				argp->end = argp->p + (argp->pagelen>>2);
 				argp->pagelen = 0;
 			} else {
-				argp->end = p + (PAGE_SIZE>>2);
+				argp->end = argp->p + (PAGE_SIZE>>2);
 				argp->pagelen -= PAGE_SIZE;
 			}
 		}
#1 lore raw
Quoted 11 lines · Neil Brown

Surely this can never have worked... which implies that the code has never been used?

When read_buf is called to move over to the next page in the pagelist of an NFSv4 request, it sets argp->end to essentially a random number, certainly not an address within the page which argp->p now points to. So subsequent calls to READ_BUF will think there is much more than a page of spare space (the cast to u32 ensures an unsigned comparison) so we can expect to fall off the end of the second page.

Yipes, thanks.

Quoted 2 lines

I guess we never ever receive requests with any operation starting beyond the first page!

putfh-write-getattr, for example, is common enough. The write decoding should leave arg->end set correctly. But there are two read_buf()'s in decode_getattr(), and I can't see why we don't hit this bug on a write that leaves that final getattr exactly straddling a page boundary.

--b.

Quoted 45 lines

[[
I found this while looking at why fsstress over NFS over RDMA caused a bad memory dereference in READ32, suggesting that 'p' had a bad value. However it was ffff8801299188f0, which is not an "I've fallen off the end of the page" sort of value. So I think it must be a different bug :-( It is as if the page is being unmapped underneath us...
]]
NeilBrown

fs/nfsd/nfs4xdr.c +4 -4
index e170317..34ccf81 100644
@@ -161,10 +161,10 @@ static __be32 *read_buf(struct nfsd4_compoundargs *argp, u32 nbytes)
 	argp->p = page_address(argp->pagelist[0]);
 	argp->pagelist++;
 	if (argp->pagelen < PAGE_SIZE) {
-		argp->end = p + (argp->pagelen>>2);
+		argp->end = argp->p + (argp->pagelen>>2);
 		argp->pagelen = 0;
 	} else {
-		argp->end = p + (PAGE_SIZE>>2);
+		argp->end = argp->p + (PAGE_SIZE>>2);
 		argp->pagelen -= PAGE_SIZE;
 	}
 	memcpy(((char*)p)+avail, argp->p, (nbytes - avail));

@@ -1426,10 +1426,10 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp)
 			argp->p = page_address(argp->pagelist[0]);
 			argp->pagelist++;
 			if (argp->pagelen < PAGE_SIZE) {
-				argp->end = p + (argp->pagelen>>2);
+				argp->end = argp->p + (argp->pagelen>>2);
 				argp->pagelen = 0;
 			} else {
-				argp->end = p + (PAGE_SIZE>>2);
+				argp->end = argp->p + (PAGE_SIZE>>2);
 				argp->pagelen -= PAGE_SIZE;
 			}
 		}
#2 lore raw

On Tue, Apr 20, 2010 at 12:51 PM, J. Bruce Fields <[email protected]=

Quoted 7 lines

wrote:

Quoted 5 lines · Neil Brown

Surely this can never have worked... which implies that the code has never been used?

When read_buf is called to move over to the next page in the pagelis=

t

Quoted 3 lines
Quoted 3 lines

of an NFSv4 request, it sets argp->end to essentially a random number, certainly not an address within the page which argp->p now points to. =A0So subsequent calls to READ_BUF will think there is mu=

ch

Quoted 10 lines
Quoted 3 lines

more than a page of spare space (the cast to u32 ensures an unsigned comparison) so we can expect to fall off the end of the second page.

Yipes, thanks.

Quoted 2 lines

I guess we never ever receive requests with any operation starting beyond the first page!

putfh-write-getattr, for example, is common enough. =A0The write deco=

ding

Quoted 1 line

should leave arg->end set correctly. =A0But there are two read_buf()'=

s in

Quoted 1 line

decode_getattr(), and I can't see why we don't hit this bug on a writ=

e

Quoted 1 line

that leaves that final getattr exactly straddling a page boundary.

The write data is dumped into the rq_vec which has non-contiguous pages. So the xdr_buf head only holds the putfh result, the short write response header (v4 stateid, offset, how, length, etc), and then the getattr. so there is plenty of space.

-->Andy

Quoted 7 lines

--b.

Quoted 4 lines

[[
I found this while looking at why fsstress over NFS over RDMA caused a bad memory dereference in READ32, suggesting that 'p' had a bad value. =A0However it was ffff8801299188f0, which is not an "I've fal=

len

Quoted 2 lines
Quoted 2 lines

off the end of the page" sort of value. =A0So I think it must be a different bug :-( =A0It is as if the page is being unmapped undernea=

th

Quoted 7 lines
Quoted 7 lines

us...
]]
NeilBrown

Quoted 5 lines

fs/nfsd/nfs4xdr.c static __be32 *read_buf(struct nfsd4_compounda=

Quoted 5 lines
fs/nfsd/nfs4xdr.c +0 -0
index e170317..34ccf81 100644
@@ -161,10 +161,10 @@ static __be32 *read_buf(struct nfsd4_compounda=

rgs *argp, u32 nbytes)

Quoted 5 lines

fs/nfsd/nfs4xdr.c static __be32 *read_buf(struct nfsd4_compounda= · line 164

Quoted 5 lines
fs/nfsd/nfs4xdr.c +1 -1
 =A0 =A0 =A0 argp->p =3D page_address(argp->pagelist[0]);
 =A0 =A0 =A0 argp->pagelist++;
 =A0 =A0 =A0 if (argp->pagelen < PAGE_SIZE) {
- =A0 =A0 =A0 =A0 =A0 =A0 argp->end =3D p + (argp->pagelen>>2);
+ =A0 =A0 =A0 =A0 =A0 =A0 argp->end =3D argp->p + (argp->pagelen>>2)=

;

Quoted 8 lines

fs/nfsd/nfs4xdr.c nfsd4_decode_compound(struct nfsd4_compounda=

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->pagelen =3D 0;
 =A0 =A0 =A0 } else {
- =A0 =A0 =A0 =A0 =A0 =A0 argp->end =3D p + (PAGE_SIZE>>2);
+ =A0 =A0 =A0 =A0 =A0 =A0 argp->end =3D argp->p + (PAGE_SIZE>>2);
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->pagelen -=3D PAGE_SIZE;
 =A0 =A0 =A0 }
 =A0 =A0 =A0 memcpy(((char*)p)+avail, argp->p, (nbytes - avail));

@@ -1426,10 +1426,10 @@ nfsd4_decode_compound(struct nfsd4_compounda=

rgs *argp)

Quoted 1 line
Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->p =3D page_address=

(argp->pagelist[0]);

Quoted 2 lines
Quoted 2 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->pagelist++; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (argp->pagelen < PAGE=

_SIZE) {

Quoted 1 line
Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->end =

=3D p + (argp->pagelen>>2);

Quoted 1 line
Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->end =

=3D argp->p + (argp->pagelen>>2);

Quoted 1 line
Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->pa=

gelen =3D 0;

Quoted 2 lines
Quoted 2 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->end =

=3D p + (PAGE_SIZE>>2);

Quoted 1 line
Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->end =

=3D argp->p + (PAGE_SIZE>>2);

Quoted 1 line
Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->pa=

gelen -=3D PAGE_SIZE;

Quoted 6 lines
Quoted 4 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
=A0 =A0 =A0 =A0 =A0 =A0 =A0 }

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" =

in

Quoted 3 lines

the body of a message to [email protected]
More majordomo info at =A0http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): g2k89c397151004201224wb35ae389g961523bbef23f452-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#4 lore raw

On Tue, Apr 20, 2010 at 03:24:59PM -0400, William A. (Andy) Adamson wro= te:

Quoted 1 line

On Tue, Apr 20, 2010 at 12:51 PM, J. Bruce Fields <[email protected]=

Quoted 3 lines · rg> wrote:
Quoted 3 lines · Neil Brown
Quoted 2 lines

Surely this can never have worked... which implies that the code h=

as

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

never been used?

When read_buf is called to move over to the next page in the pagel=

ist

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

of an NFSv4 request, it sets argp->end to essentially a random number, certainly not an address within the page which argp->p now points to. =C2=A0So subsequent calls to READ_BUF will think there =

is much

Quoted 1 line
Quoted 1 line
Quoted 1 line

more than a page of spare space (the cast to u32 ensures an unsign=

ed

Quoted 9 lines
Quoted 9 lines
Quoted 2 lines

comparison) so we can expect to fall off the end of the second page.

Yipes, thanks.

Quoted 2 lines

I guess we never ever receive requests with any operation starting beyond the first page!

putfh-write-getattr, for example, is common enough. =C2=A0The write=

decoding

Quoted 1 line
Quoted 1 line

should leave arg->end set correctly. =C2=A0But there are two read_b=

uf()'s in

Quoted 1 line
Quoted 1 line

decode_getattr(), and I can't see why we don't hit this bug on a wr=

ite

Quoted 5 lines
Quoted 1 line

that leaves that final getattr exactly straddling a page boundary.

=20
The write data is dumped into the rq_vec which has non-contiguous pages. So the xdr_buf head only holds the putfh result, the short write response header (v4 stateid, offset, how, length, etc), and the=

n

Quoted 1 line

the getattr. so there is plenty of space.

This is the server-side write-decoding, so you could see:

	rpc header | putfh | write ... data ... | getattr
						     ^
						     |
						page boundary here

--b.

Quoted 8 lines

=20
-->Andy
=20

Quoted 5 lines

--b.

Quoted 2 lines

[[
I found this while looking at why fsstress over NFS over RDMA caus=

ed

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

a bad memory dereference in READ32, suggesting that 'p' had a bad value. =C2=A0However it was ffff8801299188f0, which is not an "I'v=

e fallen

Quoted 1 line
Quoted 1 line
Quoted 1 line

off the end of the page" sort of value. =C2=A0So I think it must b=

e a

Quoted 1 line
Quoted 1 line
Quoted 1 line

different bug :-( =C2=A0It is as if the page is being unmapped und=

erneath

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

us...
]]
NeilBrown

Quoted 5 lines
Quoted 5 lines

fs/nfsd/nfs4xdr.c static __be32 *read_buf(struct nfsd4_compoun=

Quoted 5 lines
fs/nfsd/nfs4xdr.c +0 -0
index e170317..34ccf81 100644
@@ -161,10 +161,10 @@ static __be32 *read_buf(struct nfsd4_compoun=

dargs *argp, u32 nbytes)

Quoted 4 lines
Quoted 4 lines

fs/nfsd/nfs4xdr.c static __be32 *read_buf(struct nfsd4_compoun= · line 164

Quoted 4 lines
fs/nfsd/nfs4xdr.c +0 -1
 =C2=A0 =C2=A0 =C2=A0 argp->p =3D page_address(argp->pagelist[0]);
 =C2=A0 =C2=A0 =C2=A0 argp->pagelist++;
 =C2=A0 =C2=A0 =C2=A0 if (argp->pagelen < PAGE_SIZE) {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D p + (arg=

p->pagelen>>2);

Quoted 1 line
Quoted 1 line

fs/nfsd/nfs4xdr.c static __be32 *read_buf(struct nfsd4_compoun= · line 164

Quoted 1 line
fs/nfsd/nfs4xdr.c +1 -0
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D argp->p =

+ (argp->pagelen>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->pagelen =3D=

0;

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

=C2=A0 =C2=A0 =C2=A0 } else {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D p + (PAG=

E_SIZE>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D argp->p =

+ (PAGE_SIZE>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->pagelen -=3D=

PAGE_SIZE;

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

=C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 memcpy(((char*)p)+avail, argp->p, (nbytes - a=

vail));

Quoted 1 line
Quoted 1 line

fs/nfsd/nfs4xdr.c nfsd4_decode_compound(struct nfsd4_compoun=

Quoted 1 line
fs/nfsd/nfs4xdr.c +0 -0
@@ -1426,10 +1426,10 @@ nfsd4_decode_compound(struct nfsd4_compoun=

dargs *argp)

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 argp->p =3D page_address(argp->pagelist[0]);

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 argp->pagelist++;

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 if (argp->pagelen < PAGE_SIZE) {

Quoted 1 line
Quoted 1 line
Quoted 1 line

- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D p + (argp->pagelen>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D argp->p + (argp->pagelen>=

Quoted 2 lines

2);

Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->pagelen =3D 0;

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 } else {

Quoted 1 line
Quoted 1 line
Quoted 1 line

- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D p + (PAGE_SIZE>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D argp->p + (PAGE_SIZE>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->pagelen -=3D PAGE_SIZE;

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 }

Quoted 5 lines
Quoted 5 lines
Quoted 3 lines

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs=

" in

Quoted 2 lines
Quoted 2 lines

the body of a message to [email protected]
More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.=

html

Quoted 1 line
Quoted 1 line
#5 lore raw
Quoted 1 line · J. Bruce Fields

On Tue, Apr 20, 2010 at 03:24:59PM -0400, William A. (Andy) Adamson w=

rote:

Quoted 1 line
Quoted 1 line

On Tue, Apr 20, 2010 at 12:51 PM, J. Bruce Fields <bfields@fieldses=

Quoted 3 lines · =2Eorg> wrote:
Quoted 3 lines
Quoted 3 lines · Neil Brown
Quoted 2 lines

Surely this can never have worked... which implies that the code=

has

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

never been used?

When read_buf is called to move over to the next page in the pag=

elist

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

of an NFSv4 request, it sets argp->end to essentially a random number, certainly not an address within the page which argp->p n=

ow

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

points to. =C2=A0So subsequent calls to READ_BUF will think ther=

e is much

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

more than a page of spare space (the cast to u32 ensures an unsi=

gned

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

comparison) so we can expect to fall off the end of the second page.

Yipes, thanks.

Quoted 1 line

I guess we never ever receive requests with any operation starti=

ng

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line

beyond the first page!

putfh-write-getattr, for example, is common enough. =C2=A0The wri=

te decoding

Quoted 1 line
Quoted 1 line
Quoted 1 line

should leave arg->end set correctly. =C2=A0But there are two read=

_buf()'s in

Quoted 1 line
Quoted 1 line
Quoted 1 line

decode_getattr(), and I can't see why we don't hit this bug on a =

write

Quoted 1 line
Quoted 1 line
Quoted 1 line

that leaves that final getattr exactly straddling a page boundary=

=2E

Quoted 4 lines
Quoted 4 lines

=20
The write data is dumped into the rq_vec which has non-contiguous pages. So the xdr_buf head only holds the putfh result, the short write response header (v4 stateid, offset, how, length, etc), and t=

hen

Quoted 9 lines
Quoted 1 line

the getattr. so there is plenty of space.

=20
This is the server-side write-decoding, so you could see:
=20
=20

	rpc header | putfh | write ... data ... | getattr
						     ^
						     |
						page boundary here

Hm, I guess even when argp->end is wrong, argp->p is always set to something sane; so on the next READ_BUF(), when you hit the

	nbytes <=3D (u32)((char *)argp->end - (char *)argp->p

case, you do

	p =3D argp->p;
	argp->p +=3D XDR_QUADLEN(nbytes);

and p is something reasonable. "end" stays wrong, but that won't be a problem until you run past the end of the *next* page, which it would take a very unusual compound to do.

--b.

Quoted 11 lines

=20
--b.
=20

Quoted 8 lines

=20
-->Andy
=20

Quoted 5 lines

--b.

Quoted 2 lines

[[
I found this while looking at why fsstress over NFS over RDMA ca=

used

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

a bad memory dereference in READ32, suggesting that 'p' had a ba=

d

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

value. =C2=A0However it was ffff8801299188f0, which is not an "I=

've fallen

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

off the end of the page" sort of value. =C2=A0So I think it must=

be a

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

different bug :-( =C2=A0It is as if the page is being unmapped u=

nderneath

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

us...
]]
NeilBrown

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

fs/nfsd/nfs4xdr.c static __be32 *read_buf(struct nfsd4_compo=

Quoted 5 lines
fs/nfsd/nfs4xdr.c +0 -0
index e170317..34ccf81 100644
@@ -161,10 +161,10 @@ static __be32 *read_buf(struct nfsd4_compo=

undargs *argp, u32 nbytes)

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 argp->p =3D page_address(argp->pagelist[0])=

;

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

=C2=A0 =C2=A0 =C2=A0 argp->pagelist++;
=C2=A0 =C2=A0 =C2=A0 if (argp->pagelen < PAGE_SIZE) {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D p + (a=

rgp->pagelen>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D argp->=

p + (argp->pagelen>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->pagelen =3D=

0;

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

=C2=A0 =C2=A0 =C2=A0 } else {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D p + (P=

AGE_SIZE>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D argp->=

p + (PAGE_SIZE>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->pagelen -=

=3D PAGE_SIZE;

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

=C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 memcpy(((char*)p)+avail, argp->p, (nbytes -=

avail));

Quoted 1 line
Quoted 1 line
Quoted 1 line

fs/nfsd/nfs4xdr.c nfsd4_decode_compound(struct nfsd4_compo=

Quoted 1 line
fs/nfsd/nfs4xdr.c +0 -0
@@ -1426,10 +1426,10 @@ nfsd4_decode_compound(struct nfsd4_compo=

undargs *argp)

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 argp->p =3D page_address(argp->pagelist[0]);

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 argp->pagelist++;

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 if (argp->pagelen < PAGE_SIZE) {

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D p + (argp->pagelen>>2=
);

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D argp->p + (argp->page=
len>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->pagelen =3D 0;

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 } else {

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D p + (PAGE_SIZE>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->end =3D argp->p + (PAGE_SIZE>=

Quoted 2 lines

2);

Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 argp->pagelen -=3D PAGE_SIZE;

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=

=A0 =C2=A0 }

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }

--
To unsubscribe from this list: send the line "unsubscribe linux-n=

fs" in

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

the body of a message to [email protected]
More majordomo info at =C2=A0http://vger.kernel.org/majordomo-inf=

o.html

Quoted 1 line
Quoted 1 line
Quoted 1 line
#6 lore raw
Quoted 2 lines · J. Bruce Fields
Quoted 1 line · J. Bruce Fields

On Tue, Apr 20, 2010 at 03:24:59PM -0400, William A. (Andy) Adamson=

Quoted 1 line · wrote:
Quoted 1 line
Quoted 1 line

On Tue, Apr 20, 2010 at 12:51 PM, J. Bruce Fields <bfields@fields=

Quoted 3 lines · es.org> wrote:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines · Neil Brown
Quoted 2 lines

Surely this can never have worked... which implies that the co=

de has

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

never been used?

When read_buf is called to move over to the next page in the p=

agelist

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

of an NFSv4 request, it sets argp->end to essentially a random number, certainly not an address within the page which argp->p=

now

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

points to. =C2=A0So subsequent calls to READ_BUF will think th=

ere is much

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

more than a page of spare space (the cast to u32 ensures an un=

signed

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

comparison) so we can expect to fall off the end of the second page.

Yipes, thanks.

Quoted 1 line

I guess we never ever receive requests with any operation star=

ting

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line

beyond the first page!

putfh-write-getattr, for example, is common enough. =C2=A0The w=

rite decoding

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

should leave arg->end set correctly. =C2=A0But there are two re=

ad_buf()'s in

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

decode_getattr(), and I can't see why we don't hit this bug on =

a write

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

that leaves that final getattr exactly straddling a page bounda=

ry.

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

=20
The write data is dumped into the rq_vec which has non-contiguous pages. So the xdr_buf head only holds the putfh result, the short write response header (v4 stateid, offset, how, length, etc), and=

then

Quoted 13 lines
Quoted 9 lines
Quoted 1 line

the getattr. so there is plenty of space.

=20
This is the server-side write-decoding, so you could see:
=20
=20

	rpc header | putfh | write ... data ... | getattr
						     ^
						     |
						page boundary here

=20
Hm, I guess even when argp->end is wrong, argp->p is always set to something sane; so on the next READ_BUF(), when you hit the =20

Quoted 8 lines
	nbytes <=3D (u32)((char *)argp->end - (char *)argp->p
=20
case, you do
=20
	p =3D argp->p;
	argp->p +=3D XDR_QUADLEN(nbytes);

=20
and p is something reasonable. "end" stays wrong, but that won't be =

a

Quoted 2 lines

problem until you run past the end of the *next* page, which it would take a very unusual compound to do.

(Nevertheless: applied, for 2.6.34 and stable.)

--b.

author
#7 lore raw
Quoted 5 lines · On Wed, 21 Apr 2010 18:36:05 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 5 lines

Hm, I guess even when argp->end is wrong, argp->p is always set to something sane; so on the next READ_BUF(), when you hit the

	nbytes <= (u32)((char *)argp->end - (char *)argp->p
Quoted 8 lines
Quoted 8 lines

case, you do

	p = argp->p;
	argp->p += XDR_QUADLEN(nbytes);

and p is something reasonable. "end" stays wrong, but that won't be a problem until you run past the end of the *next* page, which it would take a very unusual compound to do.

Yes, it would not be an easy bug to trigger ... it takes away some of the thrill of finding a bug when you discover that it only affects a corner case that never ever happens :-(

Quoted 2 lines

(Nevertheless: applied, for 2.6.34 and stable.)

Thanks.

NeilBrown

#8 lore raw
In reply to J. Bruce Fields
Quoted 1 line · On Tue, Apr 20, 2010 at 3:39 PM, J. Bruce Fields <[email protected]>= wrote:

On Tue, Apr 20, 2010 at 03:24:59PM -0400, William A. (Andy) Adamson w=

rote:

Quoted 1 line
Quoted 1 line

On Tue, Apr 20, 2010 at 12:51 PM, J. Bruce Fields <bfields@fieldses.=

Quoted 3 lines · org> wrote:
Quoted 3 lines
Quoted 3 lines · Neil Brown
Quoted 2 lines

Surely this can never have worked... which implies that the code =

has

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

never been used?

When read_buf is called to move over to the next page in the page=

list

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

of an NFSv4 request, it sets argp->end to essentially a random number, certainly not an address within the page which argp->p no=

w

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

points to. =A0So subsequent calls to READ_BUF will think there is=

much

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

more than a page of spare space (the cast to u32 ensures an unsig=

ned

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines

comparison) so we can expect to fall off the end of the second page.

Yipes, thanks.

Quoted 1 line

I guess we never ever receive requests with any operation startin=

g

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line

beyond the first page!

putfh-write-getattr, for example, is common enough. =A0The write d=

ecoding

Quoted 1 line
Quoted 1 line
Quoted 1 line

should leave arg->end set correctly. =A0But there are two read_buf=

()'s in

Quoted 1 line
Quoted 1 line
Quoted 1 line

decode_getattr(), and I can't see why we don't hit this bug on a w=

rite

Quoted 5 lines
Quoted 5 lines
Quoted 1 line

that leaves that final getattr exactly straddling a page boundary.

The write data is dumped into the rq_vec which has non-contiguous pages. So the xdr_buf head only holds the putfh result, the short write response header (v4 stateid, offset, how, length, etc), and th=

en

Quoted 7 lines
Quoted 1 line

the getattr. so there is plenty of space.

This is the server-side write-decoding, so you could see:

=A0 =A0 =A0 =A0rpc header | putfh | write ... data ... | getattr =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ^

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 |

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 =A0 =A0 =A0 =A0 =A0page boundary here

ulp - you're right.

-->Andy

Quoted 11 lines

--b.

Quoted 8 lines

-->Andy

Quoted 5 lines

--b.

Quoted 2 lines

[[
I found this while looking at why fsstress over NFS over RDMA cau=

sed

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

a bad memory dereference in READ32, suggesting that 'p' had a bad value. =A0However it was ffff8801299188f0, which is not an "I've =

fallen

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

off the end of the page" sort of value. =A0So I think it must be =

a

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

different bug :-( =A0It is as if the page is being unmapped under=

neath

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

us...
]]
NeilBrown

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

fs/nfsd/nfs4xdr.c static __be32 *read_buf(struct nfsd4_compou=

Quoted 5 lines
fs/nfsd/nfs4xdr.c +0 -0
index e170317..34ccf81 100644
@@ -161,10 +161,10 @@ static __be32 *read_buf(struct nfsd4_compou=

ndargs *argp, u32 nbytes)

Quoted 6 lines
Quoted 5 lines
Quoted 5 lines

fs/nfsd/nfs4xdr.c static __be32 *read_buf(struct nfsd4_compou= · line 164

Quoted 5 lines
fs/nfsd/nfs4xdr.c +1 -1
 =A0 =A0 =A0 argp->p =3D page_address(argp->pagelist[0]);
 =A0 =A0 =A0 argp->pagelist++;
 =A0 =A0 =A0 if (argp->pagelen < PAGE_SIZE) {
- =A0 =A0 =A0 =A0 =A0 =A0 argp->end =3D p + (argp->pagelen>>2);
+ =A0 =A0 =A0 =A0 =A0 =A0 argp->end =3D argp->p + (argp->pagelen>=

2);

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

fs/nfsd/nfs4xdr.c nfsd4_decode_compound(struct nfsd4_compou=

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->pagelen =3D 0;
 =A0 =A0 =A0 } else {
- =A0 =A0 =A0 =A0 =A0 =A0 argp->end =3D p + (PAGE_SIZE>>2);
+ =A0 =A0 =A0 =A0 =A0 =A0 argp->end =3D argp->p + (PAGE_SIZE>>2);
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->pagelen -=3D PAGE_SIZE;
 =A0 =A0 =A0 }
 =A0 =A0 =A0 memcpy(((char*)p)+avail, argp->p, (nbytes - avail));

@@ -1426,10 +1426,10 @@ nfsd4_decode_compound(struct nfsd4_compou=

ndargs *argp)

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->p =3D page_addr=

ess(argp->pagelist[0]);

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->pagelist++; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (argp->pagelen < P=

AGE_SIZE) {

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->e=

nd =3D p + (argp->pagelen>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->e=

nd =3D argp->p + (argp->pagelen>>2);

Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp-=

pagelen =3D 0;

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->e=

nd =3D p + (PAGE_SIZE>>2);

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp->e=

nd =3D argp->p + (PAGE_SIZE>>2);

Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 argp-=

pagelen -=3D PAGE_SIZE;

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
=A0 =A0 =A0 =A0 =A0 =A0 =A0 }

--
To unsubscribe from this list: send the line "unsubscribe linux-nf=

s" in

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

the body of a message to [email protected]
More majordomo info at =A0http://vger.kernel.org/majordomo-info.ht=

ml

Quoted 2 lines
Quoted 1 line
Quoted 1 line