Re: [dpdk-dev] [PATCH v2] vfio: Fix overflow while assigning vfio BAR region offset and size

[Alejandro Lucero <alejandro.lucero@netronome.com>]

I submitted a patch for fixing this issue on the 25th of June. I did not
notice someone had reported this before. The last patch from Rahul does not
solve the problem. For those cases where the MSI-X table is in one of the
BARs to map, the memreg array is still in use.

My fix was using unsigned long instead of uint32_t for the memreg array as
this is used as  a parameter for mmap system call which expects such a type
for the offset (and size). This worked for me but I did not realize this
has to be compiled for 32 bit systems as well. In that case unsigned long
will work for the mmap but not for the VFIO kernel API which expects
uint64_t for the offset and size inside the struct vfio_region_info.

The point is, the offset param from the vfio_region_info has the index BAR
to map. For this VFIO kernel code uses VFIO_PCI_INDEX_TO_OFFSET:

 #define VFIO_PCI_OFFSET_SHIFT
<http://lxr.free-electrons.com/ident?v=3.13;i=VFIO_PCI_OFFSET_SHIFT>
40
 #define VFIO_PCI_INDEX_TO_OFFSET
<http://lxr.free-electrons.com/ident?v=3.13;i=VFIO_PCI_INDEX_TO_OFFSET>(index
<http://lxr.free-electrons.com/ident?v=3.13;i=index>) ((u64
<http://lxr.free-electrons.com/ident?v=3.13;i=u64>)(index
<http://lxr.free-electrons.com/ident?v=3.13;i=index>) <<
VFIO_PCI_OFFSET_SHIFT
<http://lxr.free-electrons.com/ident?v=3.13;i=VFIO_PCI_OFFSET_SHIFT>)

This index will be used by the VFIO mmap implementation when the DPDK code
tries to map the BARs. That code does the opposite for getting the index:

    index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);

In this case PAGE_SHIFT needs to be used because the mmap system call
modifies the offset previously.

In a 32-bit system mmap system call and VFIO mmap implementation will get
an unsigned long offset, as it does the struct vma_area_struct for
vm_pgoff. VFIO will not be able to map the right BAR except for BAR 0.

So, basically, VFIO kernel code does not work for 32 bit systems.

I think we should define memreg as unsigned long and to report this problem
to the VFIO kernel maintainer.




On Tue, Jun 30, 2015 at 10:12 PM, Thomas Monjalon <thomas.monjalon@6wind.com
> wrote:

> Hi Anatoly,
> Please could you review this fix to allow Chelsio using VFIO?
> Thanks
>
> 2015-06-23 20:30, Rahul Lakkireddy:
> > When using vfio, the probe fails over Chelsio T5 adapters after
> > commit-id 90a1633b2 (eal/linux: allow to map BARs with MSI-X tables).
> >
> > While debugging further, found that the BAR region offset and size read
> from
> > vfio are u64, but are assigned to uint32_t variables.  This results in
> the u64
> > value getting truncated to 0 and passing wrong offset and size to mmap
> for
> > subsequent BAR regions (i.e. trying to overwrite previously allocated
> BAR 0
> > region).
> >
> > The fix is to use these region offset and size directly rather than
> assigning
> > to uint32_t variables.
> >
> > Fixes: 90a1633b2347 ("eal/linux: allow to map BARs with MSI-X tables")
> > Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> > Signed-off-by: Kumar Sanghvi <kumaras@chelsio.com>
>
>