Received-SPF: pass (google.com: domain of linux-kernel+bounces-43416-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 29 Jan 2024 19:35:15 +0000
From: Will Deacon <will@kernel.org>
To: Petr =?utf-8?B?VGVzYcWZw61r?= <petr@tesarici.cz>
Cc: linux-kernel@vger.kernel.org, kernel-team@android.com,
	iommu@lists.linux.dev, Christoph Hellwig <hch@lst.de>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Robin Murphy <robin.murphy@arm.com>,
	Petr Tesarik <petr.tesarik1@huawei-partners.com>,
	Dexuan Cui <decui@microsoft.com>
Subject: Re: [PATCH 2/2] swiotlb: Enforce page alignment in swiotlb_alloc()
Message-ID: <20240129193515.GD12631@willie-the-truck>
References: <20240126151956.10014-1-will@kernel.org>
 <20240126151956.10014-3-will@kernel.org>
 <20240126172355.22a03d13@meshulam.tesarici.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240126172355.22a03d13@meshulam.tesarici.cz>
User-Agent: Mutt/1.10.1 (2018-07-13)

On Fri, Jan 26, 2024 at 05:23:55PM +0100, Petr Tesařík wrote:
> On Fri, 26 Jan 2024 15:19:56 +0000
> Will Deacon <will@kernel.org> wrote:
> 
> > When allocating pages from a restricted DMA pool in swiotlb_alloc(),
> > the buffer address is blindly converted to a 'struct page *' that is
> > returned to the caller. In the unlikely event of an allocation bug,
> > page-unaligned addresses are not detected and slots can silently be
> > double-allocated.
> > 
> > Add a simple check of the buffer alignment in swiotlb_alloc() to make
> > debugging a little easier if something has gone wonky.
> > 
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> > Cc: Dexuan Cui <decui@microsoft.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  kernel/dma/swiotlb.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index 25febb9e670c..92433ea9f2d2 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -1647,6 +1647,12 @@ struct page *swiotlb_alloc(struct device *dev, size_t size)
> >  		return NULL;
> >  
> >  	tlb_addr = slot_addr(pool->start, index);
> > +	if (!PAGE_ALIGNED(tlb_addr)) {
> > +		dev_WARN_ONCE(dev, 1, "Cannot return 'struct page *' for non page-aligned swiotlb addr 0x%pa.\n",
> > +			      &tlb_addr);
> > +		swiotlb_release_slots(dev, tlb_addr);
> > +		return NULL;
> > +	}
> 
> Is there a reason not to use BUG_ON()? If yes, I would at least go for:
> 
> +	if (unlikely(!PAGE_ALIGNED(tlb_addr))) {
> 
> Other than that, yes, such cheap sanity checking looks like a good idea.

BUG_ON() is generally frowned upon unless there's really no way to proceed.
Since we can fail the allocation here, I think that's the best bet (and hope
that whoever wanted the buffer isn't all that important).

I'll add the unlikely() in v2, although it sounds like Christoph wants
this moving anyway.

Will