Re: [PATCH] scatterlist: Validate page before calling PageSlab()

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Oct 01, 2019 at 10:09:48AM -0700, Alan Mikhak wrote:
> On Tue, Oct 1, 2019 at 5:16 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Mon, Sep 30, 2019 at 04:22:35PM -0700, Alan Mikhak wrote:
> > > From: Alan Mikhak <alan.mikhak@sifive.com>
> > >
> > > Modify sg_miter_stop() to validate the page pointer
> > > before calling PageSlab(). This check prevents a crash
> > > that will occur if PageSlab() gets called with a page
> > > pointer that is not backed by page struct.
> > >
> > > A virtual address obtained from ioremap() for a physical
> > > address in PCI address space can be assigned to a
> > > scatterlist segment using the public scatterlist API
> > > as in the following example:
> > >
> > > my_sg_set_page(struct scatterlist *sg,
> > >                const void __iomem *ioaddr,
> > >                size_t iosize)
> > > {
> > >       sg_set_page(sg,
> > >               virt_to_page(ioaddr),
> > >               (unsigned int)iosize,
> > >               offset_in_page(ioaddr));
> > >       sg_init_marker(sg, 1);
> > > }
> > >
> > > If the virtual address obtained from ioremap() is not
> > > backed by a page struct, virt_to_page() returns an
> > > invalid page pointer. However, sg_copy_buffer() can
> > > correctly recover the original virtual address. Such
> > > addresses can successfully be assigned to scatterlist
> > > segments to transfer data across the PCI bus with
> > > sg_copy_buffer() if it were not for the crash in
> > > PageSlab() when called by sg_miter_stop().
> >
> > I thought we already agreed in general that putting things that don't
> > have struct page into the scatter list was not allowed?
> >
> > Jason
> 
> Thanks Jason for your comment.
> 
> Cost of adding page structs to a large PCI I/O address range can be
> quite substantial. Allowing PCI I/O pages without page structs may be
> desirable. Perhaps it is worth considering this cost.

This is generally agreed, but nobody has figured out a solution.

> Scatterlist has no problem doing its memcpy() from pages without a
> page struct that were obtained from ioremap(). It is only at

Calling memcpy on pointers from ioremap is very much not allowed. Code
has to use the iomem safe memcpy.

Jason