linux-kernel - Re: [PATCH 1/2] PCI: endpoint: strictly apply bar fixed size to allocate space

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: <1jwmc5tgbe.fsf@starbuckisacylon.baylibre.com>
Date: Mon, 31 Mar 2025 16:39:33 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: Niklas Cassel <cassel@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,  Krzysztof
 Wilczyński <kw@...ux.com>,  Kishon Vijay Abraham I
 <kishon@...nel.org>,
  Bjorn Helgaas <bhelgaas@...gle.com>,  Lorenzo Pieralisi
 <lpieralisi@...nel.org>,  Jon Mason <jdmason@...zu.us>,  Dave Jiang
 <dave.jiang@...el.com>,  Allen Hubbe <allenbh@...il.com>,  Marek Vasut
 <marek.vasut+renesas@...il.com>,  Yoshihiro Shimoda
 <yoshihiro.shimoda.uh@...esas.com>,  Yuya Hamamachi
 <yuya.hamamachi.sx@...esas.com>,  linux-pci@...r.kernel.org,
  linux-kernel@...r.kernel.org,  ntb@...ts.linux.dev,  dlemoal@...nel.org
Subject: Re: [PATCH 1/2] PCI: endpoint: strictly apply bar fixed size to
 allocate space

On Mon 31 Mar 2025 at 10:14, Niklas Cassel <cassel@...nel.org> wrote:

> Hello Jerome,
>
> On Fri, Mar 28, 2025 at 03:53:42PM +0100, Jerome Brunet wrote:
>> When trying to allocate space for an endpoint function on a BAR with a
>> fixed size, that size should be used regardless of the alignment.
>
> Why?
>
>
>> 
>> Some controller may have specified an alignment, but do have a BAR with a
>> fixed size smaller that alignment. In such case, pci_epf_alloc_space()
>> tries to allocate a space that matches the alignment and it won't work.
>
> Could you please elaborate "won't work".
>

As I explained in the cover letter, I'm trying to enable vNTB on the
renesas platform. It started off with different Oopses, apparently
accessing unmapped area, so I started digging in the code for anything
that looked fishy. There was several problems leading to this but it
ended with errors in pci_epc_set_bar() as you are pointing out bellow.

>
>> 
>> When the BAR size is fixed, pci_epf_alloc_space() should not deviate
>> from this fixed size.
>
> I think that this commit is wrong.
>
> In your specific SoC:
>  	.msix_capable = false,
>  	.bar[BAR_1] = { .type = BAR_RESERVED, },
>  	.bar[BAR_3] = { .type = BAR_RESERVED, },
> 	.bar[BAR_4] = { .type = BAR_FIXED, .fixed_size = 256 },
>  	.bar[BAR_5] = { .type = BAR_RESERVED, },
>  	.align = SZ_1M,
>
> fixed_size is 256B, inbound iATU alignment is 1 MB, which means that the
> smallest area that the iATU can map is 1 MB.
>
> I do think that it makes sense to have backing memory for the whole area
> that the iATU will have mapped.
>
> The reason why the the ALIGN() is done, is so that the size sent in to
> dma_alloc_coherent() will return addresses that are aligned to the inbound
> iATU alignment requirement.
>

Makes sense and thanks a lot for the detailed explanation. Much appreciated.

>
> I guess the problem is that your driver has a fixed size BAR that is smaller
> than the inbound iATU alignment requirement, something that has never been a
> problem before, because no SoC has previously defined such a fixed size BAR.
>

There is always a first I guess ;)

> I doubt the problem is allocating such a BAR, so where is it you actually
> encounter a problem? My guess is in .set_bar().

pci_epc_set_bar() indeed. It seems the underlying dwc-ep driver does not
care too much what it is given for a fixed bar:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-ep.c#n409

>
> Perhaps the solution is to add another struct member to struct pci_epf_bar,
> size (meaning actual BAR size, which will be written to the BAR mask register)
> and backing_mem_size.
>
> Or.. we modify pci_epf_alloc_space() to allocate an aligned size, but the
> size that we store in (struct pci_epf_bar).size is the unaligned size.

I tried this and it works. As pointed above, as long as pci_epc_set_bar() is
happy, it will work for me since the dwc-ep driver does not really care for
the size given with fixed BARs.

However, when doing so, it gets a bit trick to properly call
dma_free_coherent() as we don't have the size actually allocated
anymore. It is possible to compute it again but it is rather ugly.

It would probably be best to add a parameter indeed, to track the size
allocated with dma_alloc_coherent(). What about .aligned_size ? Keeping
.size to track the actual bar size requires less modification I think.

>
>
> Kind regards,
> Niklas

-- 
Jerome