Re: [Qemu-devel] [RFC][PATCH 06/45] msix: Prevent bogus mask updates on MMIO accesses

[Michael S. Tsirkin]

On Mon, Oct 17, 2011 at 01:23:46PM +0200, Jan Kiszka wrote:
> On 2011-10-17 13:10, Michael S. Tsirkin wrote:
> > On Mon, Oct 17, 2011 at 11:27:40AM +0200, Jan Kiszka wrote:
> >> Only accesses to the MSI-X table must trigger a call to
> >> msix_handle_mask_update or a notifier invocation.
> >>
> >> Signed-off-by: Jan Kiszka <address@hidden>
> > 
> > Why would msix_mmio_write be called on an access
> > outside the table?
> 
> Because it handles both the table and the PBA.

Hmm. Interesting. Is there a bug in how we handle PBA
updates then? If yes I'd like a separate patch for that
to apply to the stable tree.

BTW, this code will go away if PBA can get stored separately?


> > 
> >> ---
> >>  hw/msix.c |   16 ++++++++++------
> >>  1 files changed, 10 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/msix.c b/hw/msix.c
> >> index 2c4de21..33cb716 100644
> >> --- a/hw/msix.c
> >> +++ b/hw/msix.c
> >> @@ -264,18 +264,22 @@ static void msix_mmio_write(void *opaque, 
> >> target_phys_addr_t addr,
> >>  {
> >>      PCIDevice *dev = opaque;
> >>      unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> >> -    int vector = offset / PCI_MSIX_ENTRY_SIZE;
> >> +    unsigned int vector = offset / PCI_MSIX_ENTRY_SIZE;
> > 
> > Why the int/unsigned change? this has no chance to overflow, and using
> > unsigned causes signed/unsigned comparison below,
> > and unsigned/signed conversion on calls such as msix_is_masked.
> 
> Vectors should be unsigned int, this is just one step in that direction

Not sure why, but if you want to rework this we
would be better off doing the conversion in one go. Making half the code
use unsigned and half signed is way worse.

> as we are at it.

Should be a separate patch.

> Even if the overflow is practically impossible, this
> remains cleaner.

I have to say this change if done throughout would introduce
a lot of code churn. The potential of introducing bugs
seems higher than the potential to find/fix them.

> > 
> >>      int was_masked = msix_is_masked(dev, vector);
> >>      pci_set_long(dev->msix_table_page + offset, val);
> >>      if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> >>          kvm_msix_update(dev, vector, was_masked, msix_is_masked(dev, 
> >> vector));
> >>      }
> > 
> > I would say if we need to check the address, check it first thing
> > and return if the address is out of a sensible range.
> 
> Will do that later when generalized MSI-X support.

But then do we need this patch at all?

> > For example, are you worried about kvm_msix_update calls with
> > a sensible mask?
> 
> No, that kvm code will die anyway.

Yes but we care about stable too, if there's a bug there
we need to fix it.

> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux