[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH]pci-assign: Fix memory out of bound when MSI
From: |
Gonglei (Arei) |
Subject: |
Re: [Qemu-devel] [RFC PATCH]pci-assign: Fix memory out of bound when MSI-X table not fit in a single page |
Date: |
Wed, 2 Apr 2014 08:50:38 +0000 |
Hi,
> > >
> > Actually I move the judge in function assigned_dev_register_msix_mmio.
> > Because assigned_dev_register_msix_mmio do not address the return value,
> > if dev->msix_table is null, this will result a segfault. Right?
>
> I see the confusion, there is a bug there but I think it should be fixed
> by setting msix_table to NULL in the MAP_FAILED case. The intent of
> assigned_dev_msix_reset() is to put the msix table in the state it would
> be in at reset. Without a memset() that doesn't happen. I believe the
> reason we test msix_table in assigned_dev_msix_reset() is so that the
> function could be called from anywhere, such as reset_assigned_device()
> even though it's currently not called from there. So, if the memset()
> gets removed, then the whole function should be dissolved. I'd prefer
> to keep it and store or recalculate the size for memset.
>
Yep, agreed. Thank you so much. I want to post a formal patch, can I?
> > > > memset(dev->msix_table, 0, MSIX_PAGE_SIZE);
> > >
> > > This memset may no longer cover the entire table
> > >
> > Yeah, this memset is useless. Do it in assigned_dev_register_msix_mmio.
>
> Not useless, see above.
>
> > > >
> > > > for (i = 0, entry = dev->msix_table; i < dev->msix_max; i++,
> > > > entry++)
> {
> > > > @@ -1604,13 +1600,31 @@ static void
> > > assigned_dev_msix_reset(AssignedDevice *dev)
> > > >
> > > > static int assigned_dev_register_msix_mmio(AssignedDevice *dev)
> > > > {
> > > > - dev->msix_table = mmap(NULL, MSIX_PAGE_SIZE,
> > > PROT_READ|PROT_WRITE,
> > > > + int nr_pages;
> > > > + int size;
> > > > + int entry_per_page = MSIX_PAGE_SIZE / sizeof(struct
> MSIXTableEntry);
> > > > +
> > > > + if (dev->msix_max > entry_per_page) {
> > > > + nr_pages = dev->msix_max / entry_per_page;
> > > > + if (dev->msix_max % entry_per_page) {
> > > > + nr_pages += 1;
> > > > + }
> > > > + } else {
> > > > + nr_pages = 1;
> > > > + }
> > > > +
> > > > + size = MSIX_PAGE_SIZE * nr_pages;
> > >
> > > Agree with the ROUND_UP comments:
> > >
> > > size = ROUND_UP(dev->msix_max * sizeof(struct MSIXTableEntry),
> > > MSIX_PAGE_SIZE);
> > >
> > Nice. I don't know the macro before. Thank you so much!
> > > > + dev->msix_table = mmap(NULL, size, PROT_READ|PROT_WRITE,
> > > > MAP_ANONYMOUS|MAP_PRIVATE, 0,
> 0);
> > > > if (dev->msix_table == MAP_FAILED) {
> > > > error_report("fail allocate msix_table! %s", strerror(errno));
> > > > return -EFAULT;
> > > > }
> > > > + if (!dev->msix_table) {
> > > > + return -EFAULT;
> > > > + }
> > >
> > > This is a bogus test for mmap(2)
> > >
> > > >
> > > > + memset(dev->msix_table, 0, size);
> > >
> > > This is unnecessary when assigned_dev_msix_reset() is fixed to memset
> > > the whole mmap.
> > >
> > > > assigned_dev_msix_reset(dev);
> > > >
> > > > memory_region_init_io(&dev->mmio, OBJECT(dev),
> > > &assigned_dev_msix_mmio_ops,
> > >
> > > This memory_region_init_io also requires a size parameter that isn't
> > > updated for the new size.
> > >
> > Nice catch.
> > > As noted by other comments, the munmap() size isn't addressed.
> > >
> > Get it.
> > > IMHO, the correct way to fix this would be to update pci-assign to use
> > > the msix infrastructure, but legacy KVM assignment is being phased out
> > > and replaced by VFIO. Is there something that ties you to pci-assign
> > > instead of using vfio-pci? Thanks,
> > >
> > I will have a try. Alex, What's your opinion about my former letter about
> > the
> MSI-X maximum entry.
>
> From other thread:
>
> > BTW, do you think the KVM should upsize the max support MSI-X entry to
> 2048.
> > Because the MSI-X supports a maximum table size of 2048 entries, which is
> descript in
> > PCI specification 3.0 version 6.8.3.2: "MSI-X Configuration".
> >
> > The history patch about downsize the MSI-X entry size to 256:
> >
> http://thread.gmane.org/gmane.comp.emulators.kvm.devel/38852/focus=388
> 49
>
> No, I think we should deprecate KVM device assignment and use VFIO.
> Thanks,
>
OK. I will send the result of using VFIO later.
Best regards,
-Gonglei