linux-kernel - Re: [PATCH v1 09/14] vfio/type1: Refactor pfn

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: <20210310080149.GC662265@infradead.org>
Date:   Wed, 10 Mar 2021 08:01:49 +0000
From:   Christoph Hellwig <hch@...radead.org>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     cohuck@...hat.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, jgg@...dia.com, peterx@...hat.com
Subject: Re: [PATCH v1 09/14] vfio/type1: Refactor pfn_list clearing

> +/* Return 1 if iommu->lock dropped and notified, 0 if done */

A bool would be more useful for that pattern.

> +static int unmap_dma_pfn_list(struct vfio_iommu *iommu, struct vfio_dma *dma,
> +			      struct vfio_dma **dma_last, int *retries)
> +{
> +	if (!RB_EMPTY_ROOT(&dma->pfn_list)) {

Just return early when it is empty to remove a level of indentation for
the whole function?

> +		struct vfio_iommu_type1_dma_unmap nb_unmap;
> +
> +		if (*dma_last == dma) {
> +			BUG_ON(++(*retries) > 10);
> +		} else {
> +			*dma_last = dma;
> +			*retries = 0;
> +		}
> +
> +		nb_unmap.iova = dma->iova;
> +		nb_unmap.size = dma->size;
> +
> +		/*
> +		 * Notify anyone (mdev vendor drivers) to invalidate and
> +		 * unmap iovas within the range we're about to unmap.
> +		 * Vendor drivers MUST unpin pages in response to an
> +		 * invalidation.
> +		 */
> +		mutex_unlock(&iommu->lock);
> +		blocking_notifier_call_chain(&iommu->notifier,
> +					     VFIO_IOMMU_NOTIFY_DMA_UNMAP,
> +					     &nb_unmap);
> +		return 1;

Conditional locking is a horrible pattern.  I'd rather only factor the
code until before the unlock into the helper, and then leave the
unlock and notify to the caller to avoid that anti-pattern.

Also vendor driver isn't really Linux terminology for a subdriver,
so I'd suggest to switch to something else while you're at it.