On Tue, Apr 20, 2010 at 3:39 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> On Tue, Apr 20, 2010 at 03:24:59PM -0400, William A. (Andy) Adamson wrote:
>> On Tue, Apr 20, 2010 at 12:51 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>> > On Tue, Apr 20, 2010 at 12:16:52PM +1000, Neil Brown wrote:
>> >>
>> >> 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.
>> >
>> >> 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.
>>
>> 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.
>
> This is the server-side write-decoding, so you could see:
>
>
> rpc header | putfh | write ... data ... | getattr
> ^
> |
> page boundary here
ulp - you're right.
-->Andy
>
> --b.
>
>>
>> -->Andy
>>
>> >
>> > --b.
>> >
>> >> [[
>> >> 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
>> >>
>> >>
>> >>
>> >>
>> >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> >> index e170317..34ccf81 100644
>> >> --- a/fs/nfsd/nfs4xdr.c
>> >> +++ b/fs/nfsd/nfs4xdr.c
>> >> @@ -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;
>> >> }
>> >> }
>> >>
>> >>
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> > the body of a message to majordomo@xxxxxxxxxxxxxxx
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
