linux-kernel - Re: [PATCH v4 0/3] vfio: handle DMA map/unmap up to the addressable limit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aPjwZ1Fh9hmFJyok@devgpu012.nha5.facebook.com>
Date: Wed, 22 Oct 2025 07:55:35 -0700
From: Alex Mastro <amastro@...com>
To: David Matlack <dmatlack@...gle.com>
CC: Alex Williamson <alex@...zbot.org>,
        Alejandro Jimenez
	<alejandro.j.jimenez@...cle.com>,
        Jason Gunthorpe <jgg@...pe.ca>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 0/3] vfio: handle DMA map/unmap up to the addressable
 limit

Thanks David -- this is good feedback. Will roll these suggestions into v5.

On Tue, Oct 21, 2025 at 05:38:31PM -0700, David Matlack wrote:
> On Tue, Oct 21, 2025 at 12:13 PM Alex Mastro <amastro@...com> wrote:
> > I updated the *_unmap function signatures to return the count of bytes unmapped,
> > since that is part of the test pass criteria. Also added unmap_all flavors,
> > since those exercise different code paths than range-based unmap.
> 
> When you send, can you introduce these in a separate commit and update
> the existing test function in vfio_dma_mapping_test.c to assert on it?

SGTM

> > +#undef FIXTURE_VARIANT_ADD_IOMMU_MODE
> 
> I think this can/should go just after the
> FIXTURE_VARIANT_ADD_ALL_IOMMU_MODES(); statement. The same below.

Ack.

> > +       unmapped = vfio_pci_dma_unmap_all(self->device);
> > +       ASSERT_EQ(unmapped, size);
> 
> The unmap_all test should probably be in a separate TEST_F. You can
> put the struct vfio_dma_region in the FIXTURE and initialize it in the
> FIXTURE_SETUP() to reduce code duplication.
> > +}

Make sense.

> Would it be useful to add negative map/unmap tests as well? If so we'd
> need a way to plumb the return value of the ioctl up to the caller so
> you can assert that it failed, which will conflict with returning the
> amount of unmapped bytes.

Testing negative cases would be useful. Not sure about the mechanics yet.

> 
> Maybe we should make unmapped an output parameter like so?
> 
> int __vfio_pci_dma_map(struct vfio_pci_device *device,
>         struct vfio_dma_region *region);
> 
> void vfio_pci_dma_map(struct vfio_pci_device *device,
>         struct vfio_dma_region *region);
> 
> int __vfio_pci_dma_unmap(struct vfio_pci_device *device,
>         struct vfio_dma_region *region, u64 *unmapped);
> 
> void vfio_pci_dma_unmap(struct vfio_pci_device *device,
>         struct vfio_dma_region *region, u64 *unmapped);
> 
> int __vfio_pci_dma_unmap_all(struct vfio_pci_device *device, u64 *unmapped);
> void vfio_pci_dma_unmap_all(struct vfio_pci_device *device, u64 *unmapped);
> 
> unmapped can be optional and callers that don't care can pass in NULL.
> It'll be a little gross though to see NULL on all the unmap calls
> though... Maybe unmapped can be restricted to __vfio_pci_dma_unmap().
> So something like this:
> 
> int __vfio_pci_dma_unmap(struct vfio_pci_device *device,
>         struct vfio_dma_region *region, u64 *unmapped);
> 
> void vfio_pci_dma_unmap(struct vfio_pci_device *device,
>         struct vfio_dma_region *region);

I'll put some thought into this and propose something in v5.