[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d35a962d-dc95-4469-867e-95b704cca474@wanadoo.fr>
Date: Tue, 20 Aug 2024 12:50:17 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Philipp Stanner <pstanner@...hat.com>
Cc: alexandre.torgue@...s.st.com, alvaro.karsz@...id-run.com,
andy@...nel.org, axboe@...nel.dk, bhelgaas@...gle.com, brgl@...ev.pl,
broonie@...nel.org, christophe.jaillet@...adoo.fr, corbet@....net,
davem@...emloft.net, edumazet@...gle.com, eperezma@...hat.com,
hao.wu@...el.com, jasowang@...hat.com, joabreu@...opsys.com,
kuba@...nel.org, linus.walleij@...aro.org,
linux-arm-kernel@...ts.infradead.org, linux-block@...r.kernel.org,
linux-doc@...r.kernel.org, linux-fpga@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
mcoquelin.stm32@...il.com, mdf@...nel.org, mst@...hat.com,
netdev@...r.kernel.org, pabeni@...hat.com, richardcochran@...il.com,
trix@...hat.com, virtualization@...ts.linux.dev, xuanzhuo@...ux.alibaba.com,
yilun.xu@...el.com
Subject: Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions
Le 20/08/2024 à 10:09, Philipp Stanner a écrit :
>>> @@ -556,33 +556,24 @@ static const struct vdpa_config_ops
>>> snet_config_ops = {
>>> static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet
>>> *psnet)
>>> {
>>> char name[50];
>>> - int ret, i, mask = 0;
>>> + int i;
>>> +
>>> + snprintf(name, sizeof(name), "psnet[%s]-bars",
>>> pci_name(pdev));
>>> +
>>> /* We don't know which BAR will be used to communicate..
>>> * We will map every bar with len > 0.
>>> *
>>> * Later, we will discover the BAR and unmap all other
>>> BARs.
>>> */
>>> for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>>> - if (pci_resource_len(pdev, i))
>>> - mask |= (1 << i);
>>> - }
>>> -
>>> - /* No BAR can be used.. */
>>> - if (!mask) {
>>> - SNET_ERR(pdev, "Failed to find a PCI BAR\n");
>>> - return -ENODEV;
>>> - }
>>> -
>>> - snprintf(name, sizeof(name), "psnet[%s]-bars",
>>> pci_name(pdev));
>>> - ret = pcim_iomap_regions(pdev, mask, name);
>>> - if (ret) {
>>> - SNET_ERR(pdev, "Failed to request and map PCI
>>> BARs\n");
>>> - return ret;
>>> - }
>>> + if (pci_resource_len(pdev, i)) {
>>> + psnet->bars[i] = pcim_iomap_region(pdev,
>>> i, name);
>>
>> Hi,
>>
>> Unrelated to the patch, but is is safe to have 'name' be on the
>> stack?
>>
>> pcim_iomap_region()
>> --> __pcim_request_region()
>> --> __pcim_request_region_range()
>> --> request_region() or __request_mem_region()
>> --> __request_region()
>> --> __request_region_locked()
>> --> res->name = name;
>>
>> So an address on the stack ends in the 'name' field of a "struct
>> resource".
>
> Oh oh...
>
>>
>> According to a few grep, it looks really unusual.
>>
>> I don't know if it is used, but it looks strange to me.
>
>
> I have seen it used in the kernel ringbuffer log when you try to
> request something that's already owned. I think it's here, right in
> __request_region_locked():
>
> /*
> * mm/hmm.c reserves physical addresses which then
> * become unavailable to other users. Conflicts are
> * not expected. Warn to aid debugging if encountered.
> */
> if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) {
> pr_warn("Unaddressable device %s %pR conflicts with %pR",
> conflict->name, conflict, res);
> }
>
>
> Assuming I interpret the code correctly:
> The conflicting resource is found when a new caller (e.g. another
> driver) tries to get the same region. So conflict->name on the original
> requester's stack is by now gone and you do get UB.
>
> Very unlikely UB, since only rarely drivers race for the same resource,
> but still UB.
>
> But there's also a few other places. Grep for "conflict->name".
>
>>
>>
>> If it is an issue, it was apparently already there before this patch.
>
> I think this has to be fixed.
>
> Question would just be whether one wants to fix it locally in this
> driver, or prevent it from happening globally by making the common
> infrastructure copy the string.
>
>
> P.
>
Not a perfect script, but the below coccinelle script only find this
place, so I would +1 only fixing things here only.
Agree?
CJ
@@
identifier name;
expression x;
constant N;
@@
char name[N];
...
(
* pcim_iomap_region(..., name, ...);
|
* pcim_iomap_regions(..., name, ...);
|
* request_region(..., name, ...);
|
* x = pcim_iomap_region(..., name, ...);
|
* x = pcim_iomap_regions(..., name, ...);
|
* x = request_region(..., name, ...);
)
Powered by blists - more mailing lists