If, after registering a logical PIO range, the driver probe later fails,
the logical PIO range memory will be released automatically.
This causes an issue, in that the logical PIO range is not unregistered
(that is not supported) and the released range memory may be later
referenced
Allocate the logical PIO range with kzalloc() to avoid this potential
issue.
Fixes: adf38bb0b5956 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings")
Signed-off-by: John Garry <[email protected]>
---
Change to v1:
- add comment, as advised by Joe Perches
diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
index 19d7b6ff2f17..5f0130a693fe 100644
--- a/drivers/bus/hisi_lpc.c
+++ b/drivers/bus/hisi_lpc.c
@@ -599,7 +599,8 @@ static int hisi_lpc_probe(struct platform_device *pdev)
if (IS_ERR(lpcdev->membase))
return PTR_ERR(lpcdev->membase);
- range = devm_kzalloc(dev, sizeof(*range), GFP_KERNEL);
+ /* Logical PIO may reference 'range' memory even if the probe fails */
+ range = kzalloc(sizeof(*range), GFP_KERNEL);
if (!range)
return -ENOMEM;
@@ -610,6 +611,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
ret = logic_pio_register_range(range);
if (ret) {
dev_err(dev, "register IO range failed (%d)!\n", ret);
+ kfree(range);
return ret;
}
lpcdev->io_host = range;
--
2.17.1
[+cc Zhichang, logic_pio author]
On Fri, Jun 14, 2019 at 5:26 AM John Garry <[email protected]> wrote:
>
> If, after registering a logical PIO range, the driver probe later fails,
> the logical PIO range memory will be released automatically.
>
> This causes an issue, in that the logical PIO range is not unregistered
> (that is not supported) and the released range memory may be later
> referenced
>
> Allocate the logical PIO range with kzalloc() to avoid this potential
> issue.
>
> Fixes: adf38bb0b5956 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings")
> Signed-off-by: John Garry <[email protected]>
> ---
>
> Change to v1:
> - add comment, as advised by Joe Perches
>
> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> index 19d7b6ff2f17..5f0130a693fe 100644
> --- a/drivers/bus/hisi_lpc.c
> +++ b/drivers/bus/hisi_lpc.c
> @@ -599,7 +599,8 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> if (IS_ERR(lpcdev->membase))
> return PTR_ERR(lpcdev->membase);
>
> - range = devm_kzalloc(dev, sizeof(*range), GFP_KERNEL);
> + /* Logical PIO may reference 'range' memory even if the probe fails */
> + range = kzalloc(sizeof(*range), GFP_KERNEL);
This doesn't feel like the correct way to fix this. If the probe
fails, everything done by the probe should be undone, including the
allocation and registration of "range". I don't know what the best
mechanism is, but I'm skeptical about this one.
> if (!range)
> return -ENOMEM;
>
> @@ -610,6 +611,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> ret = logic_pio_register_range(range);
> if (ret) {
> dev_err(dev, "register IO range failed (%d)!\n", ret);
> + kfree(range);
> return ret;
> }
> lpcdev->io_host = range;
> --
> 2.17.1
>
[try another address for Zhichang; first one bounced]
On Fri, Jun 14, 2019 at 8:20 AM Bjorn Helgaas <[email protected]> wrote:
>
> [+cc Zhichang, logic_pio author]
>
> On Fri, Jun 14, 2019 at 5:26 AM John Garry <[email protected]> wrote:
> >
> > If, after registering a logical PIO range, the driver probe later fails,
> > the logical PIO range memory will be released automatically.
> >
> > This causes an issue, in that the logical PIO range is not unregistered
> > (that is not supported) and the released range memory may be later
> > referenced
> >
> > Allocate the logical PIO range with kzalloc() to avoid this potential
> > issue.
> >
> > Fixes: adf38bb0b5956 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings")
> > Signed-off-by: John Garry <[email protected]>
> > ---
> >
> > Change to v1:
> > - add comment, as advised by Joe Perches
> >
> > diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> > index 19d7b6ff2f17..5f0130a693fe 100644
> > --- a/drivers/bus/hisi_lpc.c
> > +++ b/drivers/bus/hisi_lpc.c
> > @@ -599,7 +599,8 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> > if (IS_ERR(lpcdev->membase))
> > return PTR_ERR(lpcdev->membase);
> >
> > - range = devm_kzalloc(dev, sizeof(*range), GFP_KERNEL);
> > + /* Logical PIO may reference 'range' memory even if the probe fails */
> > + range = kzalloc(sizeof(*range), GFP_KERNEL);
>
> This doesn't feel like the correct way to fix this. If the probe
> fails, everything done by the probe should be undone, including the
> allocation and registration of "range". I don't know what the best
> mechanism is, but I'm skeptical about this one.
>
> > if (!range)
> > return -ENOMEM;
> >
> > @@ -610,6 +611,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> > ret = logic_pio_register_range(range);
> > if (ret) {
> > dev_err(dev, "register IO range failed (%d)!\n", ret);
> > + kfree(range);
> > return ret;
> > }
> > lpcdev->io_host = range;
> > --
> > 2.17.1
> >
On Fri, Jun 14, 2019 at 8:47 AM John Garry <[email protected]> wrote:
> On 14/06/2019 14:23, Bjorn Helgaas wrote:
> > On Fri, Jun 14, 2019 at 8:20 AM Bjorn Helgaas <[email protected]> wrote:
> >> On Fri, Jun 14, 2019 at 5:26 AM John Garry <[email protected]> wrote:
> >>>
> >>> If, after registering a logical PIO range, the driver probe later fails,
> >>> the logical PIO range memory will be released automatically.
> >>>
> >>> This causes an issue, in that the logical PIO range is not unregistered
> >>> (that is not supported) and the released range memory may be later
> >>> referenced
> >>>
> >>> Allocate the logical PIO range with kzalloc() to avoid this potential
> >>> issue.
> >>>
> >>> Fixes: adf38bb0b5956 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings")
> >>> Signed-off-by: John Garry <[email protected]>
> >>> ---
> >>>
> >>> Change to v1:
> >>> - add comment, as advised by Joe Perches
> >>>
> >>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> >>> index 19d7b6ff2f17..5f0130a693fe 100644
> >>> --- a/drivers/bus/hisi_lpc.c
> >>> +++ b/drivers/bus/hisi_lpc.c
> >>> @@ -599,7 +599,8 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> >>> if (IS_ERR(lpcdev->membase))
> >>> return PTR_ERR(lpcdev->membase);
> >>>
> >>> - range = devm_kzalloc(dev, sizeof(*range), GFP_KERNEL);
> >>> + /* Logical PIO may reference 'range' memory even if the probe fails */
> >>> + range = kzalloc(sizeof(*range), GFP_KERNEL);
> >>
> >> This doesn't feel like the correct way to fix this. If the probe
> >> fails, everything done by the probe should be undone, including the
> >> allocation and registration of "range". I don't know what the best
> >> mechanism is, but I'm skeptical about this one.
>
> I understand your concern.
>
> For the logical PIO framework, it was written to match what was done
> originally for PCI IO port management in pci_register_io_range(), cf
> https://elixir.bootlin.com/linux/v4.4.180/source/drivers/of/address.c#L691
>
> That is, no method to unregister ranges. As such, it leaks IO port
> ranges. I can come up with a few guesses why the original PCI IO port
> management author did not add an unregistration method.
I think that was written before the era of support for hot-pluggable
host bridges and loadable drivers for them.
> Anyway, we can work on adding support to unregister regions, at least at
> probe time. It may become more tricky to do this once the host children
> have probed and are accessing the IO port regions.
I think we *do* need support for unregistering regions because we do
claim to support hot-pluggable host bridges, and the I/O port regions
below them should go away when the host bridge does.
Could you just move the logic_pio_register_range() call farther down
in hisi_lpc_probe()? IIUC, once logic_pio_register_range() returns,
an inb() with the right port number will try to access that port, so
we should be prepared for that, i.e., maybe this in the wrong order to
begin with?
> But, for now, I would like to get this simple fix added to mainline and
> backported.
OK, I don't really like code that looks "obviously wrong" but has
extenuating circumstances that need explanation because it sows
confusion and can get copied elsewhere. But I'm just kibbitzing here
since I don't maintain this, so it's up to you.
> FYI, there should be no possible further memory leak for continuously
> probing the driver.
That's good.
You might consider a subject line that includes key words like "avoid
use-after-free" or some similar higher-level description that tells
the reader *why* this change is important.
Bjorn
> >>> if (!range)
> >>> return -ENOMEM;
> >>>
> >>> @@ -610,6 +611,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> >>> ret = logic_pio_register_range(range);
> >>> if (ret) {
> >>> dev_err(dev, "register IO range failed (%d)!\n", ret);
> >>> + kfree(range);
> >>> return ret;
> >>> }
> >>> lpcdev->io_host = range;
> >>> --
> >>> 2.17.1
> >>>
> >
> > .
> >
>
>
On Mon, Jun 17, 2019 at 3:47 AM John Garry <[email protected]> wrote:
>
> >>
> >> For the logical PIO framework, it was written to match what was done
> >> originally for PCI IO port management in pci_register_io_range(), cf
> >> https://elixir.bootlin.com/linux/v4.4.180/source/drivers/of/address.c#L691
> >>
> >> That is, no method to unregister ranges. As such, it leaks IO port
> >> ranges. I can come up with a few guesses why the original PCI IO port
> >> management author did not add an unregistration method.
> >
>
> Hi Bjorn,
>
> > I think that was written before the era of support for hot-pluggable
> > host bridges and loadable drivers for them.
>
> I see that the original support was added in 41f8bba7f555. I don't know
> how this coincides with hot-pluggable host bridges and their loadable
> drivers support.
>
> >
> >> Anyway, we can work on adding support to unregister regions, at least at
> >> probe time. It may become more tricky to do this once the host children
> >> have probed and are accessing the IO port regions.
> >
> > I think we *do* need support for unregistering regions because we do
> > claim to support hot-pluggable host bridges, and the I/O port regions
> > below them should go away when the host bridge does.
>
> It's now on my todo list.
>
> I'll need advice on how to test this for hot-pluggable host bridges.
>
> >
> > Could you just move the logic_pio_register_range() call farther down
> > in hisi_lpc_probe()? IIUC, once logic_pio_register_range() returns,
> > an inb() with the right port number will try to access that port, so
> > we should be prepared for that, i.e., maybe this in the wrong order to
> > begin with?
>
> No, unfortunately we can't. The reason is that we need the logical PIO
> base for that range before we enumerate the children of that host. We
> need that base address for "translating" the child bus addresses to
> logical PIO addresses.
Ah, yeah, that makes sense. I think. We do assume that we know all
the MMIO and I/O port translations before enumerating devices. It's
*conceivable* that could be changed someday since we don't actually
need the translations until a driver claims the device, and it would
gain some flexibility if we didn't have to program the host bridge
windows until we know how much space is required. But I don't see
that happening anytime soon.
Bjorn
>>
- zhichang personal mail
>> It's now on my todo list.
>>
>> I'll need advice on how to test this for hot-pluggable host bridges.
>>
>>>
>>> Could you just move the logic_pio_register_range() call farther down
>>> in hisi_lpc_probe()? IIUC, once logic_pio_register_range() returns,
>>> an inb() with the right port number will try to access that port, so
>>> we should be prepared for that, i.e., maybe this in the wrong order to
>>> begin with?
>>
>> No, unfortunately we can't. The reason is that we need the logical PIO
>> base for that range before we enumerate the children of that host. We
>> need that base address for "translating" the child bus addresses to
>> logical PIO addresses.
>
Hi Bjorn,
> Ah, yeah, that makes sense. I think. We do assume that we know all
> the MMIO and I/O port translations before enumerating devices. It's
> *conceivable* that could be changed someday since we don't actually
> need the translations until a driver claims the device,
We actually need them before a driver claims the device.
The reason is that when we create that child platform device we set the
device's IORESOURCE_IO resources according to the translated logic PIO
addresses, and not the host bus address. This is what makes the host
transparent to the child device driver.
and it would
> gain some flexibility if we didn't have to program the host bridge
> windows until we know how much space is required. But I don't see
> that happening anytime soon.
>
> Bjorn
>
> .
>
BTW, as you may have noticed, in v3 I said I would drop this patch and
fix it all properly.
My problem is that I need to ensure that the new logical PIO unregister
function works ok for hot-pluggable host bridges. I need to get some way
to test this. Advice?
Thanks,
John
[+cc Rafael, Mika, Jiang, linux-pci for ACPI host bridge hotplug test question]
On Tue, Jun 18, 2019 at 3:44 AM John Garry <[email protected]> wrote:
> >>> Could you just move the logic_pio_register_range() call farther down
> >>> in hisi_lpc_probe()? IIUC, once logic_pio_register_range() returns,
> >>> an inb() with the right port number will try to access that port, so
> >>> we should be prepared for that, i.e., maybe this in the wrong order to
> >>> begin with?
> >>
> >> No, unfortunately we can't. The reason is that we need the logical PIO
> >> base for that range before we enumerate the children of that host. We
> >> need that base address for "translating" the child bus addresses to
> >> logical PIO addresses.
> > Ah, yeah, that makes sense. I think. We do assume that we know all
> > the MMIO and I/O port translations before enumerating devices. It's
> > *conceivable* that could be changed someday since we don't actually
> > need the translations until a driver claims the device,
>
> We actually need them before a driver claims the device.
>
> The reason is that when we create that child platform device we set the
> device's IORESOURCE_IO resources according to the translated logic PIO
> addresses, and not the host bus address. This is what makes the host
> transparent to the child device driver.
I think you need it to set pdev->resource[], which is currently done
long before the driver claims the device (though one could imagine
delaying it even as far as pci_enable_device()-time). I don't think
the translation is actually *used* until the driver claims the device
because only the driver knows how to do any inb/outb to the device.
But of course, that's all speculative and doesn't change what you need
to do now. The current code assumes we know the translations during
enumeration, so you need to do the logic_pio registration before
enumerating.
> > and it would
> > gain some flexibility if we didn't have to program the host bridge
> > windows until we know how much space is required. But I don't see
> > that happening anytime soon.
> My problem is that I need to ensure that the new logical PIO unregister
> function works ok for hot-pluggable host bridges. I need to get some way
> to test this. Advice?
Good question. The ACPI host bridge driver (drivers/acpi/pci_root.c)
should support hotplug, but I'm not sure if there's a manual way to
trigger it via sysfs or something similar. If there is, and you have
a machine with more than one host bridge, you might be able to remove
one that leads to non-essential devices.
Bjorn
On 18/06/2019 18:50, Bjorn Helgaas wrote:
> [+cc Rafael, Mika, Jiang, linux-pci for ACPI host bridge hotplug test question]
>
> On Tue, Jun 18, 2019 at 3:44 AM John Garry <[email protected]> wrote:
>
>>>>> Could you just move the logic_pio_register_range() call farther down
>>>>> in hisi_lpc_probe()? IIUC, once logic_pio_register_range() returns,
>>>>> an inb() with the right port number will try to access that port, so
>>>>> we should be prepared for that, i.e., maybe this in the wrong order to
>>>>> begin with?
>>>>
>>>> No, unfortunately we can't. The reason is that we need the logical PIO
>>>> base for that range before we enumerate the children of that host. We
>>>> need that base address for "translating" the child bus addresses to
>>>> logical PIO addresses.
>
>>> Ah, yeah, that makes sense. I think. We do assume that we know all
>>> the MMIO and I/O port translations before enumerating devices. It's
>>> *conceivable* that could be changed someday since we don't actually
>>> need the translations until a driver claims the device,
>>
>> We actually need them before a driver claims the device.
>>
>> The reason is that when we create that child platform device we set the
>> device's IORESOURCE_IO resources according to the translated logic PIO
>> addresses, and not the host bus address. This is what makes the host
>> transparent to the child device driver.
>
> I think you need it to set pdev->resource[], which is currently done
> long before the driver claims the device (though one could imagine
> delaying it even as far as pci_enable_device()-time). I don't think
> the translation is actually *used* until the driver claims the device
> because only the driver knows how to do any inb/outb to the device.
>
> But of course, that's all speculative and doesn't change what you need
> to do now. The current code assumes we know the translations during
> enumeration, so you need to do the logic_pio registration before
> enumerating.
>
>>> and it would
>>> gain some flexibility if we didn't have to program the host bridge
>>> windows until we know how much space is required. But I don't see
>>> that happening anytime soon.
>
>> My problem is that I need to ensure that the new logical PIO unregister
>> function works ok for hot-pluggable host bridges. I need to get some way
>> to test this. Advice?
>
Hi Bjorn,
> Good question. The ACPI host bridge driver (drivers/acpi/pci_root.c)
> should support hotplug, but I'm not sure if there's a manual way to
> trigger it via sysfs or something similar. If there is, and you have
> a machine with more than one host bridge, you might be able to remove
> one that leads to non-essential devices.
For one of our earlier boards I don't think that it had any essential
devices on the host bridge. But I need to find out about possibility of
removal. Hmmm.
>
> Bjorn
>
Further to the topic of supporting hotplug and unregistering IO port
regions, we don't even release IO port regions in the error path of PCI
host enumeration. We have pci_register_io_range(), but no unregister
equivalent.
Looking at the history here, pci_register_io_range() was originally in
OF code. And in the OF code, calling pci_register_io_range() is a
side-effect of parsing the device tree. As such, I can see why there was
no unregister function.
It would be worth noting this discussion, where the same was mentioned:
https://lore.kernel.org/linux-pci/20180403140410.GE27789@ulmo/
The tegra PCI host probe can defer, but, since there is no tidy-up of
pci_register_io_range() when deferring, we need to ensure that the port
IO management code can handle re-attempts to register the same range.
It looks like this can be cleaned up also.
Thanks,
John
> .
>
On 18/06/2019 18:50, Bjorn Helgaas wrote:
> [+cc Rafael, Mika, Jiang, linux-pci for ACPI host bridge hotplug test question]
>
Resend with bouncing addresses removed/fixed
> On Tue, Jun 18, 2019 at 3:44 AM John Garry <[email protected]> wrote:
>
>>>>> Could you just move the logic_pio_register_range() call farther down
>>>>> in hisi_lpc_probe()? IIUC, once logic_pio_register_range() returns,
>>>>> an inb() with the right port number will try to access that port, so
>>>>> we should be prepared for that, i.e., maybe this in the wrong order to
>>>>> begin with?
>>>>
>>>> No, unfortunately we can't. The reason is that we need the logical PIO
>>>> base for that range before we enumerate the children of that host. We
>>>> need that base address for "translating" the child bus addresses to
>>>> logical PIO addresses.
>
>>> Ah, yeah, that makes sense. I think. We do assume that we know all
>>> the MMIO and I/O port translations before enumerating devices. It's
>>> *conceivable* that could be changed someday since we don't actually
>>> need the translations until a driver claims the device,
>>
>> We actually need them before a driver claims the device.
>>
>> The reason is that when we create that child platform device we set the
>> device's IORESOURCE_IO resources according to the translated logic PIO
>> addresses, and not the host bus address. This is what makes the host
>> transparent to the child device driver.
>
> I think you need it to set pdev->resource[], which is currently done
> long before the driver claims the device (though one could imagine
> delaying it even as far as pci_enable_device()-time). I don't think
> the translation is actually *used* until the driver claims the device
> because only the driver knows how to do any inb/outb to the device.
>
> But of course, that's all speculative and doesn't change what you need
> to do now. The current code assumes we know the translations during
> enumeration, so you need to do the logic_pio registration before
> enumerating.
>
>>> and it would
>>> gain some flexibility if we didn't have to program the host bridge
>>> windows until we know how much space is required. But I don't see
>>> that happening anytime soon.
>
>> My problem is that I need to ensure that the new logical PIO unregister
>> function works ok for hot-pluggable host bridges. I need to get some way
>> to test this. Advice?
>
Hi Bjorn,
> Good question. The ACPI host bridge driver (drivers/acpi/pci_root.c)
> should support hotplug, but I'm not sure if there's a manual way to
> trigger it via sysfs or something similar. If there is, and you have
> a machine with more than one host bridge, you might be able to remove
> one that leads to non-essential devices.
>
For one of our earlier boards I don't think that it had any essential
devices on the host bridge. But I need to find out about possibility of
removal. Hmmm.
> Bjorn
>
Further to the topic of supporting hotplug and unregistering IO port
regions, we don't even release IO port regions in the error path of PCI
host enumeration. We have pci_register_io_range(), but no unregister
equivalent.
Looking at the history here, pci_register_io_range() was originally in
OF code. And in the OF code, calling pci_register_io_range() is a
side-effect of parsing the device tree. As such, I can see why there was
no unregister function.
It would be worth noting this discussion, where the same was mentioned:
https://lore.kernel.org/linux-pci/20180403140410.GE27789@ulmo/
The tegra PCI host probe can defer, but, since there is no tidy-up of
pci_register_io_range() when deferring, we need to ensure that the port
IO management code can handle re-attempts to register the same range.
It looks like this can be cleaned up also.
Thanks,
John
> .
>