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

[Alan Mikhak]

On Tue, Oct 1, 2019 at 10:12 AM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
> 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@xxxxxxxx> wrote:
> > >
> > > On Mon, Sep 30, 2019 at 04:22:35PM -0700, Alan Mikhak wrote:
> > > > From: Alan Mikhak <alan.mikhak@xxxxxxxxxx>
> > > >
> > > > 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.
>
Is it in the realm of possible to add support for such PCI I/O pages
to scatterlist? Perhaps some solution is possible by adding a new
function, say sg_set_iomem_page(), and a new SG_MITER_IOMEM flag that
tells scatterlist to use iomem safe memcpy functions when the page is
not backed by page struct because it was obtained from ioremap(). This
flag can also be used at sg_miter_stop() to not call PageSlab() or
flush_kernel_dcache_page().

If supporting PCI I/O pages is not possible, would it be possible to
check for invalid page pointers in sg_set_page() and communicate the
requirement for a page struct backing in its description?

Alan
> Jason