Re: [PATCH 8/9] vdap: solidrun: Replace deprecated PCI functions

Christophe JAILLET
2024-08-20T10:50:24Z

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, ...);
)