rnfs: rq_respages pointer is bad

rnfs: rq_respages pointer is bad

[David J. Wilder]

Tom

I have been chasing an rnfs related Oops in svc_process().  I have found
the source of the Oops but I am not sure of my fix.  I am seeing the
problem on ppc64, kernel 2.6.32, I have not tried other arch yet.

The source of the problem is in rdma_read_complete(), I am finding that
rqstp->rq_respages is set to point past the end of the rqstp->rq_pages
page list.  This results in a NULL reference in svc_process() when
passing rq_respages[0] to page_address().

In rdma_read_complete() we are using rqstp->rq_arg.pages as the base of
the page list then indexing by page_no, however rq_arg.pages is not
pointing to the start of the list so rq_respages ends up pointing to:

rqstp->rq_pages[(head->count+1) + head->hdr_count]

In my case, it ends up pointing one past the end of the list by one.

Here is the change I made.

static int rdma_read_complete(struct svc_rqst *rqstp,
                              struct svc_rdma_op_ctxt *head)
{
        int page_no;
        int ret;

        BUG_ON(!head);

        /* Copy RPC pages */
        for (page_no = 0; page_no < head->count; page_no++) {
                put_page(rqstp->rq_pages[page_no]);
                rqstp->rq_pages[page_no] = head->pages[page_no];
        }
        /* Point rq_arg.pages past header */
        rqstp->rq_arg.pages = &rqstp->rq_pages[head->hdr_count];
        rqstp->rq_arg.page_len = head->arg.page_len;
        rqstp->rq_arg.page_base = head->arg.page_base;

        /* rq_respages starts after the last arg page */
-       rqstp->rq_respages = &rqstp->rq_arg.pages[page_no];
+	rqstp->rq_respages = &rqstp->rq_pages[page_no];
.
.
.

The change works for me, but I am not sure it is safe to assume the
rqstp->rq_pages[head->count] will always point to the last arg page.

Dave.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rnfs: rq_respages pointer is bad

[Tom Tucker]

Hi David:

That looks like a bug to me and it looks like what you propose is the 
correct fix. My only reservation is that if you are correct then how did 
this work at all without data corruption for large writes on x86_64?

I'm on the road right now, so I can't dig too deep until Wednesday, but 
at this point your analysis looks correct to me.

Tom


David J. Wilder wrote:
> Tom
>
> I have been chasing an rnfs related Oops in svc_process().  I have found
> the source of the Oops but I am not sure of my fix.  I am seeing the
> problem on ppc64, kernel 2.6.32, I have not tried other arch yet.
>
> The source of the problem is in rdma_read_complete(), I am finding that
> rqstp->rq_respages is set to point past the end of the rqstp->rq_pages
> page list.  This results in a NULL reference in svc_process() when
> passing rq_respages[0] to page_address().
>
> In rdma_read_complete() we are using rqstp->rq_arg.pages as the base of
> the page list then indexing by page_no, however rq_arg.pages is not
> pointing to the start of the list so rq_respages ends up pointing to:
>
> rqstp->rq_pages[(head->count+1) + head->hdr_count]
>
> In my case, it ends up pointing one past the end of the list by one.
>
> Here is the change I made.
>
> static int rdma_read_complete(struct svc_rqst *rqstp,
>                               struct svc_rdma_op_ctxt *head)
> {
>         int page_no;
>         int ret;
>
>         BUG_ON(!head);
>
>         /* Copy RPC pages */
>         for (page_no = 0; page_no < head->count; page_no++) {
>                 put_page(rqstp->rq_pages[page_no]);
>                 rqstp->rq_pages[page_no] = head->pages[page_no];
>         }
>         /* Point rq_arg.pages past header */
>         rqstp->rq_arg.pages = &rqstp->rq_pages[head->hdr_count];
>         rqstp->rq_arg.page_len = head->arg.page_len;
>         rqstp->rq_arg.page_base = head->arg.page_base;
>
>         /* rq_respages starts after the last arg page */
> -       rqstp->rq_respages = &rqstp->rq_arg.pages[page_no];
> +	rqstp->rq_respages = &rqstp->rq_pages[page_no];
> .
> .
> .
>
> The change works for me, but I am not sure it is safe to assume the
> rqstp->rq_pages[head->count] will always point to the last arg page.
>
> Dave.
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rnfs: rq_respages pointer is bad

[David J. Wilder]

On Mon, 2010-03-01 at 21:35 -0600, Tom Tucker wrote:
> Hi David:
> 
> That looks like a bug to me and it looks like what you propose is the 
> correct fix. My only reservation is that if you are correct then how did 
> this work at all without data corruption for large writes on x86_64?

The size of the page array is determined by page size (and some other
parameters).  I am using 64k pages, that may be the key.  I don't know
what page size was used on X86-64 but we may have been lucky and not
fallen off the end of the array thus avoided the problem.

> 
> I'm on the road right now, so I can't dig too deep until Wednesday, but 
> at this point your analysis looks correct to me.
> 
> Tom
> 
> 
> David J. Wilder wrote:
> > Tom
> >
> > I have been chasing an rnfs related Oops in svc_process().  I have found
> > the source of the Oops but I am not sure of my fix.  I am seeing the
> > problem on ppc64, kernel 2.6.32, I have not tried other arch yet.
> >
> > The source of the problem is in rdma_read_complete(), I am finding that
> > rqstp->rq_respages is set to point past the end of the rqstp->rq_pages
> > page list.  This results in a NULL reference in svc_process() when
> > passing rq_respages[0] to page_address().
> >
> > In rdma_read_complete() we are using rqstp->rq_arg.pages as the base of
> > the page list then indexing by page_no, however rq_arg.pages is not
> > pointing to the start of the list so rq_respages ends up pointing to:
> >
> > rqstp->rq_pages[(head->count+1) + head->hdr_count]
> >
> > In my case, it ends up pointing one past the end of the list by one.
> >
> > Here is the change I made.
> >
> > static int rdma_read_complete(struct svc_rqst *rqstp,
> >                               struct svc_rdma_op_ctxt *head)
> > {
> >         int page_no;
> >         int ret;
> >
> >         BUG_ON(!head);
> >
> >         /* Copy RPC pages */
> >         for (page_no = 0; page_no < head->count; page_no++) {
> >                 put_page(rqstp->rq_pages[page_no]);
> >                 rqstp->rq_pages[page_no] = head->pages[page_no];
> >         }
> >         /* Point rq_arg.pages past header */
> >         rqstp->rq_arg.pages = &rqstp->rq_pages[head->hdr_count];
> >         rqstp->rq_arg.page_len = head->arg.page_len;
> >         rqstp->rq_arg.page_base = head->arg.page_base;
> >
> >         /* rq_respages starts after the last arg page */
> > -       rqstp->rq_respages = &rqstp->rq_arg.pages[page_no];
> > +	rqstp->rq_respages = &rqstp->rq_pages[page_no];
> > .
> > .
> > .
> >
> > The change works for me, but I am not sure it is safe to assume the
> > rqstp->rq_pages[head->count] will always point to the last arg page.
> >
> > Dave.
> >   
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rnfs: rq_respages pointer is bad

[Tom Tucker]

David J. Wilder wrote:
> Tom
>
> I have been chasing an rnfs related Oops in svc_process().  I have found
> the source of the Oops but I am not sure of my fix.  I am seeing the
> problem on ppc64, kernel 2.6.32, I have not tried other arch yet.
>
> The source of the problem is in rdma_read_complete(), I am finding that
> rqstp->rq_respages is set to point past the end of the rqstp->rq_pages
> page list.  This results in a NULL reference in svc_process() when
> passing rq_respages[0] to page_address().
>
> In rdma_read_complete() we are using rqstp->rq_arg.pages as the base of
> the page list then indexing by page_no, however rq_arg.pages is not
> pointing to the start of the list so rq_respages ends up pointing to:
>
> rqstp->rq_pages[(head->count+1) + head->hdr_count]
>
> In my case, it ends up pointing one past the end of the list by one.
>
> Here is the change I made.
>
> static int rdma_read_complete(struct svc_rqst *rqstp,
>                               struct svc_rdma_op_ctxt *head)
> {
>         int page_no;
>         int ret;
>
>         BUG_ON(!head);
>
>         /* Copy RPC pages */
>         for (page_no = 0; page_no < head->count; page_no++) {
>                 put_page(rqstp->rq_pages[page_no]);
>                 rqstp->rq_pages[page_no] = head->pages[page_no];
>         }
>         /* Point rq_arg.pages past header */
>         rqstp->rq_arg.pages = &rqstp->rq_pages[head->hdr_count];
>         rqstp->rq_arg.page_len = head->arg.page_len;
>         rqstp->rq_arg.page_base = head->arg.page_base;
>
>         /* rq_respages starts after the last arg page */
> -       rqstp->rq_respages = &rqstp->rq_arg.pages[page_no];
> +	rqstp->rq_respages = &rqstp->rq_pages[page_no];
>   

This might be clearer as:

        rqstp->rq_respages = &rqstp->rq_pages[head->count];

> .
> .
> .
>
> The change works for me, but I am not sure it is safe to assume the
> rqstp->rq_pages[head->count] will always point to the last arg page.
>
> Dave.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rnfs: rq_respages pointer is bad

[Roland Dreier]

Someone please make sure that a final patch with a full description gets
sent to the NFS guys for merging.  Tom, are you going to handle this?
-- 
Roland Dreier  <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rnfs: rq_respages pointer is bad

[Tom Tucker]

Roland Dreier wrote:
> Someone please make sure that a final patch with a full description gets
> sent to the NFS guys for merging.  Tom, are you going to handle this?
>   
Yes, and I have several more in queue.

Tom

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rnfs: rq_respages pointer is bad

[Roland Dreier]

 > The source of the problem is in rdma_read_complete(), I am finding that
 > rqstp->rq_respages is set to point past the end of the rqstp->rq_pages
 > page list.  This results in a NULL reference in svc_process() when
 > passing rq_respages[0] to page_address().

did this ever end up getting fixed upstream?
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: rnfs: rq_respages pointer is bad

[Tom Tucker]

On 5/5/10 5:58 PM, Roland Dreier wrote:
>   >  The source of the problem is in rdma_read_complete(), I am finding that
>   >  rqstp->rq_respages is set to point past the end of the rqstp->rq_pages
>   >  page list.  This results in a NULL reference in svc_process() when
>   >  passing rq_respages[0] to page_address().
>
> did this ever end up getting fixed upstream?
>    
I believe it did, but I'll check to be sure. It may be in Bruce's queue too.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html