* [PATCH v2 0/3] Fix system crash for accessing unmapped IO port regions @ 2019-03-20 18:14 John Garry 2019-03-20 18:14 ` [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource John Garry ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: John Garry @ 2019-03-20 18:14 UTC (permalink / raw) To: bhelgaas, rafael, arnd, lorenzo.pieralisi, bp Cc: linux, linux-kernel, linux-pci, wangkefeng.wang, linuxarm, agraf, andy.shevchenko, John Garry It was reported some time ago that systems will crash if a driver attempts to access IO port addresses when the PCI IO port region has not been mapped [1]. More recently, a similar crash is where the system PCI host probe fails, and the IPMI driver crashes the system while attempting to do some IO port accesses [2]. This patchset attempts to keep the kernel alive in such situations by 2 complementary methods: 1. Rejecting IO port resource requests until PCI IO port regions have been mapped (in a pci_remap_iospace() call). 2. Rejecting logic PIO access to PCI IO regions until, again, PCI IO port regions have been mapped About 1: Currently the PCI IO port region is initialized to the full range, {0, IO_SPACE_LIMIT}. As such, any IO port region requests would not fail because of PCI IO port regions not being mapped. Patch 1/3 looks to remedy this issue by ensuring IO port requests are made to direct children of ioport_resource (PCI host IO port regions), similar to Arnd's solution, mentioned in [1]: "I see that ioport_resource gets initialized to the {0, IO_SPACE_LIMIT} range. If we could change it so that pci_remap_iospace() hooks up to ioport_resource and extends it whenever something gets mapped there up to IO_SPACE_LIMIT, we can change the default range to {0,0}, which would fail for any request_region call before the first pci_remap_iospace." I didn't use this solution, as logical PIO space is sparse in {0, IO_SPACE_LIMIT}, so we cannot simply grow the region. I marked the patch as RFC, as the solution is not ideal, i.e. calling __release_region() if the region is not suitable. In addition, regressions may be seen, so I would like input first. About 2: Some drivers - like f71805f hwmon driver - do not call request_{muxed_}region() prior to accessing IO port regions, as they should do. So patch 2/3 adds a safeguard against this, in that unwarranted PIO IO accesses will be discarded in the low-level accessors. About the issue of f71805f driver not requesting the IO port region - many drivers do this, and need to be fixed up separately. 1. https://www.spinics.net/lists/linux-pci/msg49821.html 2. https://www.spinics.net/lists/arm-kernel/msg694702.html Differences to v1 patchset: https://lkml.org/lkml/2019/3/14/630 - Drop f71805f fix - it can be done in a separate patchset - Change implementation in resource.c patch to check if parent of region is ioport_resource - Add patch to fix some logic_pio.c prints John Garry (3): resource: Request IO port regions from children of ioport_resource lib: logic_pio: Reject access to unregistered CPU MMIO regions lib: logic_pio: Make some prints explicitly hex include/linux/ioport.h | 12 +++++-- kernel/resource.c | 28 ++++++++++++++++ lib/logic_pio.c | 74 ++++++++++++++++++++++++------------------ 3 files changed, 79 insertions(+), 35 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource 2019-03-20 18:14 [PATCH v2 0/3] Fix system crash for accessing unmapped IO port regions John Garry @ 2019-03-20 18:14 ` John Garry 2019-03-25 23:32 ` Bjorn Helgaas 2019-03-20 18:14 ` [PATCH v2 2/3] lib: logic_pio: Reject access to unregistered CPU MMIO regions John Garry 2019-03-20 18:14 ` [PATCH v2 3/3] lib: logic_pio: Make some prints explicitly hex John Garry 2 siblings, 1 reply; 17+ messages in thread From: John Garry @ 2019-03-20 18:14 UTC (permalink / raw) To: bhelgaas, rafael, arnd, lorenzo.pieralisi, bp Cc: linux, linux-kernel, linux-pci, wangkefeng.wang, linuxarm, agraf, andy.shevchenko, John Garry Currently when we request an IO port region, the request is made directly to the top resource, ioport_resource. There is an issue here, in that drivers may successfully request an IO port region even if the IO port region has not even been mapped in (in pci_remap_iospace()). This may lead to crashes when the system has no PCI host, or, has a host but it has failed enumeration, while drivers still attempt to access PCI IO ports, as below: root@(none)$root@(none)$ insmod f71882fg.ko [ 152.215377] Unable to handle kernel paging request at virtual address ffff7dfffee0002e [ 152.231299] Mem abort info: [ 152.236898] ESR = 0x96000046 [ 152.243019] Exception class = DABT (current EL), IL = 32 bits [ 152.254905] SET = 0, FnV = 0 [ 152.261024] EA = 0, S1PTW = 0 [ 152.267320] Data abort info: [ 152.273091] ISV = 0, ISS = 0x00000046 [ 152.280784] CM = 0, WnR = 1 [ 152.286730] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) [ 152.300537] [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000 [ 152.318016] Internal error: Oops: 96000046 [#1] PREEMPT SMP [ 152.329199] Modules linked in: f71882fg(+) [ 152.337415] CPU: 8 PID: 2732 Comm: insmod Not tainted 5.1.0-rc1-00002-gab1a0e9200b8-dirty #102 [ 152.354712] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 [ 152.373058] pstate: 80000005 (Nzcv daif -PAN -UAO) [ 152.382675] pc : logic_outb+0x54/0xb8 [ 152.390017] lr : f71882fg_find+0x64/0x390 [f71882fg] [ 152.399977] sp : ffff000013393aa0 [ 152.406618] x29: ffff000013393aa0 x28: ffff000008b98b10 [ 152.417278] x27: ffff000013393df0 x26: 0000000000000100 [ 152.427938] x25: ffff801f8c872d30 x24: ffff000011420000 [ 152.438598] x23: ffff801fb49d2940 x22: ffff000011291000 [ 152.449257] x21: 000000000000002e x20: 0000000000000087 [ 152.459917] x19: ffff000013393b44 x18: ffffffffffffffff [ 152.470577] x17: 0000000000000000 x16: 0000000000000000 [ 152.481236] x15: ffff00001127d6c8 x14: ffff801f8cfd691c [ 152.491896] x13: 0000000000000000 x12: 0000000000000000 [ 152.502555] x11: 0000000000000003 x10: 0000801feace2000 [ 152.513215] x9 : 0000000000000000 x8 : ffff841fa654f280 [ 152.523874] x7 : 0000000000000000 x6 : 0000000000ffc0e3 [ 152.534534] x5 : ffff000011291360 x4 : ffff801fb4949f00 [ 152.545194] x3 : 0000000000ffbffe x2 : 76e767a63713d500 [ 152.555853] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000 [ 152.566514] Process insmod (pid: 2732, stack limit = 0x(____ptrval____)) [ 152.579968] Call trace: [ 152.584863] logic_outb+0x54/0xb8 [ 152.591506] f71882fg_find+0x64/0x390 [f71882fg] [ 152.600768] f71882fg_init+0x38/0xc70 [f71882fg] [ 152.610031] do_one_initcall+0x5c/0x198 [ 152.617723] do_init_module+0x54/0x1b0 [ 152.625237] load_module+0x1dc4/0x2158 [ 152.632752] __se_sys_init_module+0x14c/0x1e8 [ 152.641490] __arm64_sys_init_module+0x18/0x20 [ 152.650404] el0_svc_common+0x5c/0x100 [ 152.657919] el0_svc_handler+0x2c/0x80 [ 152.665433] el0_svc+0x8/0xc [ 152.671202] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034) [ 152.683434] ---[ end trace fd4f35b610829a48 ]--- Segmentation fault root@(none)$ Note that the f71882fg driver correctly calls request_muxed_region(). This issue was originally reported in [1]. This patch changes the functionality of request{muxed_}_region() to request a region from a direct child descendent of the top ioport_resource. In this, if the IO port region has not been mapped for a particular IO region, the PCI IO resource would also not have been inserted, and so a suitable child region will not exist. As such, request_{muxed_}region() calls will fail. A side note: there are many drivers in the kernel which fail to even call request_{muxed_}region() prior to IO port accesses, and they also need to be fixed (to call request_{muxed_}region(), as appropriate) separately. [1] https://www.spinics.net/lists/linux-pci/msg49821.html Signed-off-by: John Garry <john.garry@huawei.com> --- include/linux/ioport.h | 12 +++++++++--- kernel/resource.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/include/linux/ioport.h b/include/linux/ioport.h index da0ebaec25f0..d7b7e1e08291 100644 --- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -217,19 +217,25 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2) /* Convenience shorthand with allocation */ -#define request_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), 0) -#define request_muxed_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED) +#define request_region(start,n,name) __request_region_from_children(&ioport_resource, (start), (n), (name), 0) +#define request_muxed_region(start,n,name) __request_region_from_children(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED) #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl) #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0) #define request_mem_region_exclusive(start,n,name) \ __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE) #define rename_region(region, newname) do { (region)->name = (newname); } while (0) -extern struct resource * __request_region(struct resource *, +extern struct resource *__request_region(struct resource *, resource_size_t start, resource_size_t n, const char *name, int flags); +extern struct resource *__request_region_from_children(struct resource *, + resource_size_t start, + resource_size_t n, + const char *name, int flags); + + /* Compatibility cruft */ #define release_region(start,n) __release_region(&ioport_resource, (start), (n)) #define release_mem_region(start,n) __release_region(&iomem_resource, (start), (n)) diff --git a/kernel/resource.c b/kernel/resource.c index 92190f62ebc5..87ed200eda8b 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -1097,6 +1097,34 @@ resource_size_t resource_alignment(struct resource *res) static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait); +/** + * __request_region_from_children - create a new busy region from a child + * @parent: parent resource descriptor + * @start: resource start address + * @n: resource region size + * @name: reserving caller's ID string + * @flags: IO resource flags + */ +struct resource *__request_region_from_children(struct resource *parent, + resource_size_t start, + resource_size_t n, + const char *name, int flags) +{ + struct resource *res = __request_region(parent, start, n, name, flags); + + if (res && res->parent == parent) { + /* + * This is a direct descendent of the parent, which is + * what we didn't want. + */ + __release_region(parent, start, n); + res = NULL; + } + + return res; +} +EXPORT_SYMBOL(__request_region_from_children); + /** * __request_region - create a new busy resource region * @parent: parent resource descriptor -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource 2019-03-20 18:14 ` [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource John Garry @ 2019-03-25 23:32 ` Bjorn Helgaas 2019-03-26 16:33 ` John Garry 0 siblings, 1 reply; 17+ messages in thread From: Bjorn Helgaas @ 2019-03-25 23:32 UTC (permalink / raw) To: John Garry Cc: rafael, arnd, lorenzo.pieralisi, bp, linux, linux-kernel, linux-pci, wangkefeng.wang, linuxarm, agraf, andy.shevchenko Hi John, On Thu, Mar 21, 2019 at 02:14:08AM +0800, John Garry wrote: > Currently when we request an IO port region, the request is made directly > to the top resource, ioport_resource. Let's be explicit here, e.g., Currently request_region() requests an IO port region directly from the top resource, ioport_resource. > There is an issue here, in that drivers may successfully request an IO > port region even if the IO port region has not even been mapped in > (in pci_remap_iospace()). > > This may lead to crashes when the system has no PCI host, or, has a host > but it has failed enumeration, while drivers still attempt to access PCI > IO ports, as below: I don't understand the strategy here. f71882fg is not a driver for a PCI device, so it should work even if there is no PCI host in the system. On x86, I think inb/inw/inl from a port where nothing responds probably just returns ~0, and outb/outw/outl just get dropped. Shouldn't arm64 do the same, without crashing? > root@(none)$root@(none)$ insmod f71882fg.ko > [ 152.215377] Unable to handle kernel paging request at virtual address ffff7dfffee0002e > [ 152.231299] Mem abort info: > [ 152.236898] ESR = 0x96000046 > [ 152.243019] Exception class = DABT (current EL), IL = 32 bits > [ 152.254905] SET = 0, FnV = 0 > [ 152.261024] EA = 0, S1PTW = 0 > [ 152.267320] Data abort info: > [ 152.273091] ISV = 0, ISS = 0x00000046 > [ 152.280784] CM = 0, WnR = 1 > [ 152.286730] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) > [ 152.300537] [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000 > [ 152.318016] Internal error: Oops: 96000046 [#1] PREEMPT SMP > [ 152.329199] Modules linked in: f71882fg(+) > [ 152.337415] CPU: 8 PID: 2732 Comm: insmod Not tainted 5.1.0-rc1-00002-gab1a0e9200b8-dirty #102 > [ 152.354712] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 > [ 152.373058] pstate: 80000005 (Nzcv daif -PAN -UAO) > [ 152.382675] pc : logic_outb+0x54/0xb8 > [ 152.390017] lr : f71882fg_find+0x64/0x390 [f71882fg] > [ 152.399977] sp : ffff000013393aa0 > [ 152.406618] x29: ffff000013393aa0 x28: ffff000008b98b10 > [ 152.417278] x27: ffff000013393df0 x26: 0000000000000100 > [ 152.427938] x25: ffff801f8c872d30 x24: ffff000011420000 > [ 152.438598] x23: ffff801fb49d2940 x22: ffff000011291000 > [ 152.449257] x21: 000000000000002e x20: 0000000000000087 > [ 152.459917] x19: ffff000013393b44 x18: ffffffffffffffff > [ 152.470577] x17: 0000000000000000 x16: 0000000000000000 > [ 152.481236] x15: ffff00001127d6c8 x14: ffff801f8cfd691c > [ 152.491896] x13: 0000000000000000 x12: 0000000000000000 > [ 152.502555] x11: 0000000000000003 x10: 0000801feace2000 > [ 152.513215] x9 : 0000000000000000 x8 : ffff841fa654f280 > [ 152.523874] x7 : 0000000000000000 x6 : 0000000000ffc0e3 > [ 152.534534] x5 : ffff000011291360 x4 : ffff801fb4949f00 > [ 152.545194] x3 : 0000000000ffbffe x2 : 76e767a63713d500 > [ 152.555853] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000 > [ 152.566514] Process insmod (pid: 2732, stack limit = 0x(____ptrval____)) > [ 152.579968] Call trace: > [ 152.584863] logic_outb+0x54/0xb8 > [ 152.591506] f71882fg_find+0x64/0x390 [f71882fg] > [ 152.600768] f71882fg_init+0x38/0xc70 [f71882fg] > [ 152.610031] do_one_initcall+0x5c/0x198 > [ 152.617723] do_init_module+0x54/0x1b0 > [ 152.625237] load_module+0x1dc4/0x2158 > [ 152.632752] __se_sys_init_module+0x14c/0x1e8 > [ 152.641490] __arm64_sys_init_module+0x18/0x20 > [ 152.650404] el0_svc_common+0x5c/0x100 > [ 152.657919] el0_svc_handler+0x2c/0x80 > [ 152.665433] el0_svc+0x8/0xc > [ 152.671202] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034) > [ 152.683434] ---[ end trace fd4f35b610829a48 ]--- > Segmentation fault > root@(none)$ Please remove the timestamps (because they don't contribute useful information) and indent the example a couple spaces (which is conventional for quoted material). > Note that the f71882fg driver correctly calls request_muxed_region(). > > This issue was originally reported in [1]. > > This patch changes the functionality of request{muxed_}_region() to > request a region from a direct child descendent of the top > ioport_resource. > > In this, if the IO port region has not been mapped for a particular IO > region, the PCI IO resource would also not have been inserted, and so a > suitable child region will not exist. As such, > request_{muxed_}region() calls will fail. > > A side note: there are many drivers in the kernel which fail to even call > request_{muxed_}region() prior to IO port accesses, and they also need to > be fixed (to call request_{muxed_}region(), as appropriate) separately. > > [1] https://www.spinics.net/lists/linux-pci/msg49821.html Please use a https://lore.kernel.org/ URL instead of spinics.net. > Signed-off-by: John Garry <john.garry@huawei.com> > --- > include/linux/ioport.h | 12 +++++++++--- > kernel/resource.c | 28 ++++++++++++++++++++++++++++ > 2 files changed, 37 insertions(+), 3 deletions(-) > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > index da0ebaec25f0..d7b7e1e08291 100644 > --- a/include/linux/ioport.h > +++ b/include/linux/ioport.h > @@ -217,19 +217,25 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2) > > > /* Convenience shorthand with allocation */ > -#define request_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), 0) > -#define request_muxed_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED) > +#define request_region(start,n,name) __request_region_from_children(&ioport_resource, (start), (n), (name), 0) > +#define request_muxed_region(start,n,name) __request_region_from_children(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED) > #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl) > #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0) > #define request_mem_region_exclusive(start,n,name) \ > __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE) > #define rename_region(region, newname) do { (region)->name = (newname); } while (0) > > -extern struct resource * __request_region(struct resource *, > +extern struct resource *__request_region(struct resource *, > resource_size_t start, > resource_size_t n, > const char *name, int flags); > > +extern struct resource *__request_region_from_children(struct resource *, > + resource_size_t start, > + resource_size_t n, > + const char *name, int flags); > + > + > /* Compatibility cruft */ > #define release_region(start,n) __release_region(&ioport_resource, (start), (n)) > #define release_mem_region(start,n) __release_region(&iomem_resource, (start), (n)) > diff --git a/kernel/resource.c b/kernel/resource.c > index 92190f62ebc5..87ed200eda8b 100644 > --- a/kernel/resource.c > +++ b/kernel/resource.c > @@ -1097,6 +1097,34 @@ resource_size_t resource_alignment(struct resource *res) > > static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait); > > +/** > + * __request_region_from_children - create a new busy region from a child > + * @parent: parent resource descriptor > + * @start: resource start address > + * @n: resource region size > + * @name: reserving caller's ID string > + * @flags: IO resource flags > + */ > +struct resource *__request_region_from_children(struct resource *parent, > + resource_size_t start, > + resource_size_t n, > + const char *name, int flags) > +{ > + struct resource *res = __request_region(parent, start, n, name, flags); > + > + if (res && res->parent == parent) { > + /* > + * This is a direct descendent of the parent, which is > + * what we didn't want. > + */ > + __release_region(parent, start, n); > + res = NULL; > + } > + > + return res; > +} > +EXPORT_SYMBOL(__request_region_from_children); > + > /** > * __request_region - create a new busy resource region > * @parent: parent resource descriptor > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource 2019-03-25 23:32 ` Bjorn Helgaas @ 2019-03-26 16:33 ` John Garry 2019-03-26 22:48 ` Bjorn Helgaas 0 siblings, 1 reply; 17+ messages in thread From: John Garry @ 2019-03-26 16:33 UTC (permalink / raw) To: Bjorn Helgaas Cc: rafael, arnd, lorenzo.pieralisi, bp, linux, linux-kernel, linux-pci, wangkefeng.wang, linuxarm, andy.shevchenko On 25/03/2019 23:32, Bjorn Helgaas wrote: > Hi John, > Hi Bjorn, Thanks for reviewing this. > On Thu, Mar 21, 2019 at 02:14:08AM +0800, John Garry wrote: >> Currently when we request an IO port region, the request is made directly >> to the top resource, ioport_resource. > > Let's be explicit here, e.g., > > Currently request_region() requests an IO port region directly from the > top resource, ioport_resource. ok > >> There is an issue here, in that drivers may successfully request an IO >> port region even if the IO port region has not even been mapped in >> (in pci_remap_iospace()). >> >> This may lead to crashes when the system has no PCI host, or, has a host >> but it has failed enumeration, while drivers still attempt to access PCI >> IO ports, as below: > > I don't understand the strategy here. f71882fg is not a driver for a > PCI device, so it should work even if there is no PCI host in the > system. From my checking, the f71882fg hwmon is accessed via the super-io interface on the PCH on x86. The super-io interface is at fixed addresses, those being 0x2e and 0x4e. Please see the following: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hwmon/f71805f.c?h=v5.1-rc2#n1621 and https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/8-series-chipset-pch-datasheet.pdf (Table 9.2). On x86 systems, these PCH IO ports will be mapped on a PCI bus, like: $more /proc/ioports 0000-0cf7 : PCI Bus 0000:00 0000-001f : dma1 0020-0021 : pic1 0040-0043 : timer0 0050-0053 : timer1 0060-0060 : keyboard 0064-0064 : keyboard 0070-0077 : rtc0 0080-008f : dma page reg 00a0-00a1 : pic2 00c0-00df : dma2 00f0-00ff : fpu So, the idea in the patch is that if PCI Bus 0000:00 does not exist because of no PCI host, then we should fail a request to an IO port region. > > On x86, I think inb/inw/inl from a port where nothing responds > probably just returns ~0, and outb/outw/outl just get dropped. > Shouldn't arm64 do the same, without crashing? That would be ideal and we're doing something similar in patch 2/3. So on ARM64 we have to IO remap the PCI IO resource. If this mapping is not done (due to no PCI host), then any inb/inw/inl calls will crash the system. So in patch 2/3, I am also making the change to the logical PIO inb/inw/inl accessors to discard accesses when no PCI MMIO regions are registered in logical PIO space. This is really a second line of defense (this patch being the first). > >> root@(none)$root@(none)$ insmod f71882fg.ko >> [ 152.215377] Unable to handle kernel paging request at virtual address ffff7dfffee0002e >> [ 152.231299] Mem abort info: >> [ 152.236898] ESR = 0x96000046 >> [ 152.243019] Exception class = DABT (current EL), IL = 32 bits >> [ 152.254905] SET = 0, FnV = 0 >> [ 152.261024] EA = 0, S1PTW = 0 >> [ 152.267320] Data abort info: >> [ 152.273091] ISV = 0, ISS = 0x00000046 >> [ 152.280784] CM = 0, WnR = 1 >> [ 152.286730] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) >> [ 152.300537] [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000 >> [ 152.318016] Internal error: Oops: 96000046 [#1] PREEMPT SMP >> [ 152.329199] Modules linked in: f71882fg(+) >> [ 152.337415] CPU: 8 PID: 2732 Comm: insmod Not tainted 5.1.0-rc1-00002-gab1a0e9200b8-dirty #102 >> [ 152.354712] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 >> [ 152.373058] pstate: 80000005 (Nzcv daif -PAN -UAO) >> [ 152.382675] pc : logic_outb+0x54/0xb8 >> [ 152.390017] lr : f71882fg_find+0x64/0x390 [f71882fg] >> [ 152.399977] sp : ffff000013393aa0 >> [ 152.406618] x29: ffff000013393aa0 x28: ffff000008b98b10 >> [ 152.417278] x27: ffff000013393df0 x26: 0000000000000100 >> [ 152.427938] x25: ffff801f8c872d30 x24: ffff000011420000 >> [ 152.438598] x23: ffff801fb49d2940 x22: ffff000011291000 >> [ 152.449257] x21: 000000000000002e x20: 0000000000000087 >> [ 152.459917] x19: ffff000013393b44 x18: ffffffffffffffff >> [ 152.470577] x17: 0000000000000000 x16: 0000000000000000 >> [ 152.481236] x15: ffff00001127d6c8 x14: ffff801f8cfd691c >> [ 152.491896] x13: 0000000000000000 x12: 0000000000000000 >> [ 152.502555] x11: 0000000000000003 x10: 0000801feace2000 >> [ 152.513215] x9 : 0000000000000000 x8 : ffff841fa654f280 >> [ 152.523874] x7 : 0000000000000000 x6 : 0000000000ffc0e3 >> [ 152.534534] x5 : ffff000011291360 x4 : ffff801fb4949f00 >> [ 152.545194] x3 : 0000000000ffbffe x2 : 76e767a63713d500 >> [ 152.555853] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000 >> [ 152.566514] Process insmod (pid: 2732, stack limit = 0x(____ptrval____)) >> [ 152.579968] Call trace: >> [ 152.584863] logic_outb+0x54/0xb8 >> [ 152.591506] f71882fg_find+0x64/0x390 [f71882fg] >> [ 152.600768] f71882fg_init+0x38/0xc70 [f71882fg] >> [ 152.610031] do_one_initcall+0x5c/0x198 >> [ 152.617723] do_init_module+0x54/0x1b0 >> [ 152.625237] load_module+0x1dc4/0x2158 >> [ 152.632752] __se_sys_init_module+0x14c/0x1e8 >> [ 152.641490] __arm64_sys_init_module+0x18/0x20 >> [ 152.650404] el0_svc_common+0x5c/0x100 >> [ 152.657919] el0_svc_handler+0x2c/0x80 >> [ 152.665433] el0_svc+0x8/0xc >> [ 152.671202] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034) >> [ 152.683434] ---[ end trace fd4f35b610829a48 ]--- >> Segmentation fault >> root@(none)$ > > Please remove the timestamps (because they don't contribute useful > information) and indent the example a couple spaces (which is > conventional for quoted material). ok > >> Note that the f71882fg driver correctly calls request_muxed_region(). >> >> This issue was originally reported in [1]. >> >> This patch changes the functionality of request{muxed_}_region() to >> request a region from a direct child descendent of the top >> ioport_resource. >> >> In this, if the IO port region has not been mapped for a particular IO >> region, the PCI IO resource would also not have been inserted, and so a >> suitable child region will not exist. As such, >> request_{muxed_}region() calls will fail. >> >> A side note: there are many drivers in the kernel which fail to even call >> request_{muxed_}region() prior to IO port accesses, and they also need to >> be fixed (to call request_{muxed_}region(), as appropriate) separately. >> >> [1] https://www.spinics.net/lists/linux-pci/msg49821.html > > Please use a https://lore.kernel.org/ URL instead of spinics.net. ok, I hope that I can find this old thread. > >> Signed-off-by: John Garry <john.garry@huawei.com> >> --- Thanks! >> include/linux/ioport.h | 12 +++++++++--- >> kernel/resource.c | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 37 insertions(+), 3 deletions(-) >> Leaving remaing text as a reference. >> diff --git a/include/linux/ioport.h b/include/linux/ioport.h >> index da0ebaec25f0..d7b7e1e08291 100644 >> --- a/include/linux/ioport.h >> +++ b/include/linux/ioport.h >> @@ -217,19 +217,25 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2) >> >> >> /* Convenience shorthand with allocation */ >> -#define request_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), 0) >> -#define request_muxed_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED) >> +#define request_region(start,n,name) __request_region_from_children(&ioport_resource, (start), (n), (name), 0) >> +#define request_muxed_region(start,n,name) __request_region_from_children(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED) >> #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl) >> #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0) >> #define request_mem_region_exclusive(start,n,name) \ >> __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE) >> #define rename_region(region, newname) do { (region)->name = (newname); } while (0) >> >> -extern struct resource * __request_region(struct resource *, >> +extern struct resource *__request_region(struct resource *, >> resource_size_t start, >> resource_size_t n, >> const char *name, int flags); >> >> +extern struct resource *__request_region_from_children(struct resource *, >> + resource_size_t start, >> + resource_size_t n, >> + const char *name, int flags); >> + >> + >> /* Compatibility cruft */ >> #define release_region(start,n) __release_region(&ioport_resource, (start), (n)) >> #define release_mem_region(start,n) __release_region(&iomem_resource, (start), (n)) >> diff --git a/kernel/resource.c b/kernel/resource.c >> index 92190f62ebc5..87ed200eda8b 100644 >> --- a/kernel/resource.c >> +++ b/kernel/resource.c >> @@ -1097,6 +1097,34 @@ resource_size_t resource_alignment(struct resource *res) >> >> static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait); >> >> +/** >> + * __request_region_from_children - create a new busy region from a child >> + * @parent: parent resource descriptor >> + * @start: resource start address >> + * @n: resource region size >> + * @name: reserving caller's ID string >> + * @flags: IO resource flags >> + */ >> +struct resource *__request_region_from_children(struct resource *parent, >> + resource_size_t start, >> + resource_size_t n, >> + const char *name, int flags) >> +{ >> + struct resource *res = __request_region(parent, start, n, name, flags); >> + >> + if (res && res->parent == parent) { >> + /* >> + * This is a direct descendent of the parent, which is >> + * what we didn't want. >> + */ >> + __release_region(parent, start, n); >> + res = NULL; >> + } >> + >> + return res; >> +} >> +EXPORT_SYMBOL(__request_region_from_children); >> + >> /** >> * __request_region - create a new busy resource region >> * @parent: parent resource descriptor >> -- >> 2.17.1 >> > > . > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource 2019-03-26 16:33 ` John Garry @ 2019-03-26 22:48 ` Bjorn Helgaas 2019-03-27 11:24 ` John Garry 2019-03-28 17:46 ` Lorenzo Pieralisi 0 siblings, 2 replies; 17+ messages in thread From: Bjorn Helgaas @ 2019-03-26 22:48 UTC (permalink / raw) To: John Garry Cc: rafael, arnd, lorenzo.pieralisi, bp, linux, linux-kernel, linux-pci, wangkefeng.wang, linuxarm, andy.shevchenko, Catalin Marinas, Will Deacon, linux-arm-kernel [+cc Catalin, Will, linux-arm-kernel] On Tue, Mar 26, 2019 at 04:33:55PM +0000, John Garry wrote: > On 25/03/2019 23:32, Bjorn Helgaas wrote: > > On Thu, Mar 21, 2019 at 02:14:08AM +0800, John Garry wrote: > > > Currently when we request an IO port region, the request is made directly > > > to the top resource, ioport_resource. > > > > Let's be explicit here, e.g., > > > > Currently request_region() requests an IO port region directly from the > > top resource, ioport_resource. > > ok > > > > There is an issue here, in that drivers may successfully request an IO > > > port region even if the IO port region has not even been mapped in > > > (in pci_remap_iospace()). > > > > > > This may lead to crashes when the system has no PCI host, or, has a host > > > but it has failed enumeration, while drivers still attempt to access PCI > > > IO ports, as below: > > > > I don't understand the strategy here. f71882fg is not a driver for a > > PCI device, so it should work even if there is no PCI host in the > > system. > > From my checking, the f71882fg hwmon is accessed via the super-io interface > on the PCH on x86. The super-io interface is at fixed addresses, those being > 0x2e and 0x4e. > > Please see the following: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hwmon/f71805f.c?h=v5.1-rc2#n1621 > > and > > https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/8-series-chipset-pch-datasheet.pdf > (Table 9.2). > > On x86 systems, these PCH IO ports will be mapped on a PCI bus, like: > > $more /proc/ioports > 0000-0cf7 : PCI Bus 0000:00 > 0000-001f : dma1 > 0020-0021 : pic1 > 0040-0043 : timer0 > 0050-0053 : timer1 > 0060-0060 : keyboard > 0064-0064 : keyboard > 0070-0077 : rtc0 > 0080-008f : dma page reg > 00a0-00a1 : pic2 > 00c0-00df : dma2 > 00f0-00ff : fpu > > So, the idea in the patch is that if PCI Bus 0000:00 does not exist because > of no PCI host, then we should fail a request to an IO port region. I'm not convinced about this last sentence. It's true that on most modern systems, including that Intel PCH, the Super I/O controller is attached via an LPC bridge on a PCI bus. But I don't think it's an actual requirement that PCI be involved. There certainly once were systems, e.g., PC/104, that had ISA devices but no PCI. Maybe Super I/O attached via ISA is obsolete enough that we don't care any more, but I really don't know. > > On x86, I think inb/inw/inl from a port where nothing responds > > probably just returns ~0, and outb/outw/outl just get dropped. > > Shouldn't arm64 do the same, without crashing? > > That would be ideal and we're doing something similar in patch 2/3. > > So on ARM64 we have to IO remap the PCI IO resource. If this mapping is not > done (due to no PCI host), then any inb/inw/inl calls will crash the system. My take is that ARM64 is responsible for implementing inb/inw/inl in such a way that they don't crash. I don't think it's practical to update all the old ISA drivers or even the core code to work around that. > So in patch 2/3, I am also making the change to the logical PIO inb/inw/inl > accessors to discard accesses when no PCI MMIO regions are registered in > logical PIO space. > > This is really a second line of defense (this patch being the first). > > > > root@(none)$root@(none)$ insmod f71882fg.ko > > > [ 152.215377] Unable to handle kernel paging request at virtual address ffff7dfffee0002e > > > [ 152.231299] Mem abort info: > > > [ 152.236898] ESR = 0x96000046 > > > [ 152.243019] Exception class = DABT (current EL), IL = 32 bits > > > [ 152.254905] SET = 0, FnV = 0 > > > [ 152.261024] EA = 0, S1PTW = 0 > > > [ 152.267320] Data abort info: > > > [ 152.273091] ISV = 0, ISS = 0x00000046 > > > [ 152.280784] CM = 0, WnR = 1 > > > [ 152.286730] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) > > > [ 152.300537] [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000 > > > [ 152.318016] Internal error: Oops: 96000046 [#1] PREEMPT SMP > > > [ 152.329199] Modules linked in: f71882fg(+) > > > [ 152.337415] CPU: 8 PID: 2732 Comm: insmod Not tainted 5.1.0-rc1-00002-gab1a0e9200b8-dirty #102 > > > [ 152.354712] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 > > > [ 152.373058] pstate: 80000005 (Nzcv daif -PAN -UAO) > > > [ 152.382675] pc : logic_outb+0x54/0xb8 > > > [ 152.390017] lr : f71882fg_find+0x64/0x390 [f71882fg] > > > [ 152.399977] sp : ffff000013393aa0 > > > [ 152.406618] x29: ffff000013393aa0 x28: ffff000008b98b10 > > > [ 152.417278] x27: ffff000013393df0 x26: 0000000000000100 > > > [ 152.427938] x25: ffff801f8c872d30 x24: ffff000011420000 > > > [ 152.438598] x23: ffff801fb49d2940 x22: ffff000011291000 > > > [ 152.449257] x21: 000000000000002e x20: 0000000000000087 > > > [ 152.459917] x19: ffff000013393b44 x18: ffffffffffffffff > > > [ 152.470577] x17: 0000000000000000 x16: 0000000000000000 > > > [ 152.481236] x15: ffff00001127d6c8 x14: ffff801f8cfd691c > > > [ 152.491896] x13: 0000000000000000 x12: 0000000000000000 > > > [ 152.502555] x11: 0000000000000003 x10: 0000801feace2000 > > > [ 152.513215] x9 : 0000000000000000 x8 : ffff841fa654f280 > > > [ 152.523874] x7 : 0000000000000000 x6 : 0000000000ffc0e3 > > > [ 152.534534] x5 : ffff000011291360 x4 : ffff801fb4949f00 > > > [ 152.545194] x3 : 0000000000ffbffe x2 : 76e767a63713d500 > > > [ 152.555853] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000 > > > [ 152.566514] Process insmod (pid: 2732, stack limit = 0x(____ptrval____)) > > > [ 152.579968] Call trace: > > > [ 152.584863] logic_outb+0x54/0xb8 > > > [ 152.591506] f71882fg_find+0x64/0x390 [f71882fg] > > > [ 152.600768] f71882fg_init+0x38/0xc70 [f71882fg] > > > [ 152.610031] do_one_initcall+0x5c/0x198 > > > [ 152.617723] do_init_module+0x54/0x1b0 > > > [ 152.625237] load_module+0x1dc4/0x2158 > > > [ 152.632752] __se_sys_init_module+0x14c/0x1e8 > > > [ 152.641490] __arm64_sys_init_module+0x18/0x20 > > > [ 152.650404] el0_svc_common+0x5c/0x100 > > > [ 152.657919] el0_svc_handler+0x2c/0x80 > > > [ 152.665433] el0_svc+0x8/0xc > > > [ 152.671202] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034) > > > [ 152.683434] ---[ end trace fd4f35b610829a48 ]--- > > > Segmentation fault > > > root@(none)$ > > > > > Note that the f71882fg driver correctly calls request_muxed_region(). > > > > > > This issue was originally reported in [1]. > > > > > > This patch changes the functionality of request{muxed_}_region() to > > > request a region from a direct child descendent of the top > > > ioport_resource. > > > > > > In this, if the IO port region has not been mapped for a particular IO > > > region, the PCI IO resource would also not have been inserted, and so a > > > suitable child region will not exist. As such, > > > request_{muxed_}region() calls will fail. > > > > > > A side note: there are many drivers in the kernel which fail to even call > > > request_{muxed_}region() prior to IO port accesses, and they also need to > > > be fixed (to call request_{muxed_}region(), as appropriate) separately. > > > > > > [1] https://www.spinics.net/lists/linux-pci/msg49821.html > > > > Please use a https://lore.kernel.org/ URL instead of spinics.net. > > ok, I hope that I can find this old thread. The beauty of lore.kernel.org is that the URL contains the Message-ID, so it's easy build the URL and it would contain useful information even if lore.kernel.org disappeared: https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com Bjorn > > > Signed-off-by: John Garry <john.garry@huawei.com> > > > --- > > Thanks! > > > > include/linux/ioport.h | 12 +++++++++--- > > > kernel/resource.c | 28 ++++++++++++++++++++++++++++ > > > 2 files changed, 37 insertions(+), 3 deletions(-) > > > > > Leaving remaing text as a reference. > > > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > > > index da0ebaec25f0..d7b7e1e08291 100644 > > > --- a/include/linux/ioport.h > > > +++ b/include/linux/ioport.h > > > @@ -217,19 +217,25 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2) > > > > > > > > > /* Convenience shorthand with allocation */ > > > -#define request_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), 0) > > > -#define request_muxed_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED) > > > +#define request_region(start,n,name) __request_region_from_children(&ioport_resource, (start), (n), (name), 0) > > > +#define request_muxed_region(start,n,name) __request_region_from_children(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED) > > > #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl) > > > #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0) > > > #define request_mem_region_exclusive(start,n,name) \ > > > __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE) > > > #define rename_region(region, newname) do { (region)->name = (newname); } while (0) > > > > > > -extern struct resource * __request_region(struct resource *, > > > +extern struct resource *__request_region(struct resource *, > > > resource_size_t start, > > > resource_size_t n, > > > const char *name, int flags); > > > > > > +extern struct resource *__request_region_from_children(struct resource *, > > > + resource_size_t start, > > > + resource_size_t n, > > > + const char *name, int flags); > > > + > > > + > > > /* Compatibility cruft */ > > > #define release_region(start,n) __release_region(&ioport_resource, (start), (n)) > > > #define release_mem_region(start,n) __release_region(&iomem_resource, (start), (n)) > > > diff --git a/kernel/resource.c b/kernel/resource.c > > > index 92190f62ebc5..87ed200eda8b 100644 > > > --- a/kernel/resource.c > > > +++ b/kernel/resource.c > > > @@ -1097,6 +1097,34 @@ resource_size_t resource_alignment(struct resource *res) > > > > > > static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait); > > > > > > +/** > > > + * __request_region_from_children - create a new busy region from a child > > > + * @parent: parent resource descriptor > > > + * @start: resource start address > > > + * @n: resource region size > > > + * @name: reserving caller's ID string > > > + * @flags: IO resource flags > > > + */ > > > +struct resource *__request_region_from_children(struct resource *parent, > > > + resource_size_t start, > > > + resource_size_t n, > > > + const char *name, int flags) > > > +{ > > > + struct resource *res = __request_region(parent, start, n, name, flags); > > > + > > > + if (res && res->parent == parent) { > > > + /* > > > + * This is a direct descendent of the parent, which is > > > + * what we didn't want. > > > + */ > > > + __release_region(parent, start, n); > > > + res = NULL; > > > + } > > > + > > > + return res; > > > +} > > > +EXPORT_SYMBOL(__request_region_from_children); > > > + > > > /** > > > * __request_region - create a new busy resource region > > > * @parent: parent resource descriptor > > > -- > > > 2.17.1 > > > > > > > . > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource 2019-03-26 22:48 ` Bjorn Helgaas @ 2019-03-27 11:24 ` John Garry 2019-03-28 17:46 ` Lorenzo Pieralisi 1 sibling, 0 replies; 17+ messages in thread From: John Garry @ 2019-03-27 11:24 UTC (permalink / raw) To: Bjorn Helgaas Cc: rafael, arnd, lorenzo.pieralisi, bp, linux, linux-kernel, linux-pci, wangkefeng.wang, linuxarm, andy.shevchenko, Catalin Marinas, Will Deacon, linux-arm-kernel On 26/03/2019 22:48, Bjorn Helgaas wrote: > [+cc Catalin, Will, linux-arm-kernel] > >> From my checking, the f71882fg hwmon is accessed via the super-io interface >> on the PCH on x86. The super-io interface is at fixed addresses, those being >> 0x2e and 0x4e. >> >> Please see the following: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hwmon/f71805f.c?h=v5.1-rc2#n1621 >> >> and >> >> https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/8-series-chipset-pch-datasheet.pdf >> (Table 9.2). >> >> On x86 systems, these PCH IO ports will be mapped on a PCI bus, like: >> >> $more /proc/ioports >> 0000-0cf7 : PCI Bus 0000:00 >> 0000-001f : dma1 >> 0020-0021 : pic1 >> 0040-0043 : timer0 >> 0050-0053 : timer1 >> 0060-0060 : keyboard >> 0064-0064 : keyboard >> 0070-0077 : rtc0 >> 0080-008f : dma page reg >> 00a0-00a1 : pic2 >> 00c0-00df : dma2 >> 00f0-00ff : fpu >> >> So, the idea in the patch is that if PCI Bus 0000:00 does not exist because >> of no PCI host, then we should fail a request to an IO port region. > Hi Bjorn, > I'm not convinced about this last sentence. > > It's true that on most modern systems, including that Intel PCH, the > Super I/O controller is attached via an LPC bridge on a PCI bus. > > But I don't think it's an actual requirement that PCI be involved. > There certainly once were systems, e.g., PC/104, that had ISA devices > but no PCI. Maybe Super I/O attached via ISA is obsolete enough that > we don't care any more, but I really don't know. OK, fine. So if this is true, then this patch falls apart. However I don't know for sure either. I would still like to think that these legacy ISA system should still insert a bus resource under ioport_resource, from which devices on that bus should request resources. > >>> On x86, I think inb/inw/inl from a port where nothing responds >>> probably just returns ~0, and outb/outw/outl just get dropped. >>> Shouldn't arm64 do the same, without crashing? >> >> That would be ideal and we're doing something similar in patch 2/3. >> >> So on ARM64 we have to IO remap the PCI IO resource. If this mapping is not >> done (due to no PCI host), then any inb/inw/inl calls will crash the system. > > My take is that ARM64 is responsible for implementing inb/inw/inl in > such a way that they don't crash. I don't think it's practical to > update all the old ISA drivers or even the core code to work around > that. As I mentioned below, I was actually also fixing up inb/inw/inl et al for arm64 such that they don't crash the system in this case. This was in patch 2/3. So on arm64 - which defines PCI_IOBASE - we need to IO remap the PCI IO space resource. If this is not done and we access PCI IO space, then we crash. However with the introduction of logical PIO space in commit 031e3601869c, we can test this mapping by ensuring that we have a logical PIO region registered. If there is none, then we can discard the access. However this would only be for when INDIRECT_PIO is defined. Maybe I can make it work for when INDIRECT_PIO is not defined, or even drop !INDIRECT_PIO support. A final note on hwmon f71882fg: even with the change in 2/3, this driver still accesses IO ports 0x2e and 0x4e, which would not be a PCH fixed IO port on !x86 systems, so far from ideal. I saw that in commit 746cdfbf01c0 ("hwmon: Avoid building drivers for powerpc that read/write ISA addresses"), PPC would not build these drivers, as, like arm, it has no native ISA. > >> So in patch 2/3, I am also making the change to the logical PIO inb/inw/inl >> accessors to discard accesses when no PCI MMIO regions are registered in >> logical PIO space. >> >> This is really a second line of defense (this patch being the first). >> >>>> root@(none)$root@(none)$ insmod f71882fg.ko >>>> [ 152.215377] Unable to handle kernel paging request at virtual address ffff7dfffee0002e >>>> [ 152.231299] Mem abort info: >>>> [ 152.236898] ESR = 0x96000046 >>>> [ 152.243019] Exception class = DABT (current EL), IL = 32 bits >>>> [ 152.254905] SET = 0, FnV = 0 >>>> [ 152.261024] EA = 0, S1PTW = 0 >>>> [ 152.267320] Data abort info: >>>> [ 152.273091] ISV = 0, ISS = 0x00000046 >>>> [ 152.280784] CM = 0, WnR = 1 >>>> [ 152.286730] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) >>>> [ 152.300537] [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000 >>>> [ 152.318016] Internal error: Oops: 96000046 [#1] PREEMPT SMP >>>> [ 152.329199] Modules linked in: f71882fg(+) >>>> [ 152.337415] CPU: 8 PID: 2732 Comm: insmod Not tainted 5.1.0-rc1-00002-gab1a0e9200b8-dirty #102 >>>> [ 152.354712] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 >>>> [ 152.373058] pstate: 80000005 (Nzcv daif -PAN -UAO) >>>> [ 152.382675] pc : logic_outb+0x54/0xb8 >>>> [ 152.390017] lr : f71882fg_find+0x64/0x390 [f71882fg] >>>> [ 152.399977] sp : ffff000013393aa0 >>>> [ 152.406618] x29: ffff000013393aa0 x28: ffff000008b98b10 >>>> [ 152.417278] x27: ffff000013393df0 x26: 0000000000000100 >>>> [ 152.427938] x25: ffff801f8c872d30 x24: ffff000011420000 >>>> [ 152.438598] x23: ffff801fb49d2940 x22: ffff000011291000 >>>> [ 152.449257] x21: 000000000000002e x20: 0000000000000087 >>>> [ 152.459917] x19: ffff000013393b44 x18: ffffffffffffffff >>>> [ 152.470577] x17: 0000000000000000 x16: 0000000000000000 >>>> [ 152.481236] x15: ffff00001127d6c8 x14: ffff801f8cfd691c >>>> [ 152.491896] x13: 0000000000000000 x12: 0000000000000000 >>>> [ 152.502555] x11: 0000000000000003 x10: 0000801feace2000 >>>> [ 152.513215] x9 : 0000000000000000 x8 : ffff841fa654f280 >>>> [ 152.523874] x7 : 0000000000000000 x6 : 0000000000ffc0e3 >>>> [ 152.534534] x5 : ffff000011291360 x4 : ffff801fb4949f00 >>>> [ 152.545194] x3 : 0000000000ffbffe x2 : 76e767a63713d500 >>>> [ 152.555853] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000 >>>> [ 152.566514] Process insmod (pid: 2732, stack limit = 0x(____ptrval____)) >>>> [ 152.579968] Call trace: >>>> [ 152.584863] logic_outb+0x54/0xb8 >>>> [ 152.591506] f71882fg_find+0x64/0x390 [f71882fg] >>>> [ 152.600768] f71882fg_init+0x38/0xc70 [f71882fg] >>>> [ 152.610031] do_one_initcall+0x5c/0x198 >>>> [ 152.617723] do_init_module+0x54/0x1b0 >>>> [ 152.625237] load_module+0x1dc4/0x2158 >>>> [ 152.632752] __se_sys_init_module+0x14c/0x1e8 >>>> [ 152.641490] __arm64_sys_init_module+0x18/0x20 >>>> [ 152.650404] el0_svc_common+0x5c/0x100 >>>> [ 152.657919] el0_svc_handler+0x2c/0x80 >>>> [ 152.665433] el0_svc+0x8/0xc >>>> [ 152.671202] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034) >>>> [ 152.683434] ---[ end trace fd4f35b610829a48 ]--- >>>> Segmentation fault >>>> root@(none)$ >>> >>>> Note that the f71882fg driver correctly calls request_muxed_region(). >>>> >>>> This issue was originally reported in [1]. >>>> >>>> This patch changes the functionality of request{muxed_}_region() to >>>> request a region from a direct child descendent of the top >>>> ioport_resource. >>>> >>>> In this, if the IO port region has not been mapped for a particular IO >>>> region, the PCI IO resource would also not have been inserted, and so a >>>> suitable child region will not exist. As such, >>>> request_{muxed_}region() calls will fail. >>>> >>>> A side note: there are many drivers in the kernel which fail to even call >>>> request_{muxed_}region() prior to IO port accesses, and they also need to >>>> be fixed (to call request_{muxed_}region(), as appropriate) separately. >>>> >>>> [1] https://www.spinics.net/lists/linux-pci/msg49821.html >>> >>> Please use a https://lore.kernel.org/ URL instead of spinics.net. >> >> ok, I hope that I can find this old thread. > > The beauty of lore.kernel.org is that the URL contains the Message-ID, so > it's easy build the URL and it would contain useful information even if > lore.kernel.org disappeared: > > https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com > ok, great. Thanks again, John > Bjorn > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource 2019-03-26 22:48 ` Bjorn Helgaas 2019-03-27 11:24 ` John Garry @ 2019-03-28 17:46 ` Lorenzo Pieralisi 2019-03-29 10:42 ` John Garry 1 sibling, 1 reply; 17+ messages in thread From: Lorenzo Pieralisi @ 2019-03-28 17:46 UTC (permalink / raw) To: Bjorn Helgaas Cc: John Garry, rafael, arnd, bp, linux, linux-kernel, linux-pci, wangkefeng.wang, linuxarm, andy.shevchenko, Catalin Marinas, Will Deacon, linux-arm-kernel On Tue, Mar 26, 2019 at 05:48:10PM -0500, Bjorn Helgaas wrote: [...] > I'm not convinced about this last sentence. > > It's true that on most modern systems, including that Intel PCH, the > Super I/O controller is attached via an LPC bridge on a PCI bus. > > But I don't think it's an actual requirement that PCI be involved. > There certainly once were systems, e.g., PC/104, that had ISA devices > but no PCI. Maybe Super I/O attached via ISA is obsolete enough that > we don't care any more, but I really don't know. > > > > On x86, I think inb/inw/inl from a port where nothing responds > > > probably just returns ~0, and outb/outw/outl just get dropped. > > > Shouldn't arm64 do the same, without crashing? > > > > That would be ideal and we're doing something similar in patch 2/3. > > > > So on ARM64 we have to IO remap the PCI IO resource. If this mapping is not > > done (due to no PCI host), then any inb/inw/inl calls will crash the system. > > My take is that ARM64 is responsible for implementing inb/inw/inl in > such a way that they don't crash. I don't think it's practical to > update all the old ISA drivers or even the core code to work around > that. The problem is that those drivers are accessing a resource that does not exist in practice, it is taken for granted on x86 systems (and on IA64) because that was an actual bus (actual or emulated) and was made part of the architecture. The ISA space is not necessarily tied to PCI, at least not always. Side note: these drivers can't be compiled on PPC, it would be good to understand why, I have a hunch it can be related. > > So in patch 2/3, I am also making the change to the logical PIO inb/inw/inl > > accessors to discard accesses when no PCI MMIO regions are registered in > > logical PIO space. > > > > This is really a second line of defense (this patch being the first). > > > > > > root@(none)$root@(none)$ insmod f71882fg.ko > > > > [ 152.215377] Unable to handle kernel paging request at virtual address ffff7dfffee0002e > > > > [ 152.231299] Mem abort info: > > > > [ 152.236898] ESR = 0x96000046 > > > > [ 152.243019] Exception class = DABT (current EL), IL = 32 bits > > > > [ 152.254905] SET = 0, FnV = 0 > > > > [ 152.261024] EA = 0, S1PTW = 0 > > > > [ 152.267320] Data abort info: > > > > [ 152.273091] ISV = 0, ISS = 0x00000046 > > > > [ 152.280784] CM = 0, WnR = 1 > > > > [ 152.286730] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) > > > > [ 152.300537] [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000 > > > > [ 152.318016] Internal error: Oops: 96000046 [#1] PREEMPT SMP > > > > [ 152.329199] Modules linked in: f71882fg(+) > > > > [ 152.337415] CPU: 8 PID: 2732 Comm: insmod Not tainted 5.1.0-rc1-00002-gab1a0e9200b8-dirty #102 > > > > [ 152.354712] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 > > > > [ 152.373058] pstate: 80000005 (Nzcv daif -PAN -UAO) > > > > [ 152.382675] pc : logic_outb+0x54/0xb8 > > > > [ 152.390017] lr : f71882fg_find+0x64/0x390 [f71882fg] > > > > [ 152.399977] sp : ffff000013393aa0 > > > > [ 152.406618] x29: ffff000013393aa0 x28: ffff000008b98b10 > > > > [ 152.417278] x27: ffff000013393df0 x26: 0000000000000100 > > > > [ 152.427938] x25: ffff801f8c872d30 x24: ffff000011420000 > > > > [ 152.438598] x23: ffff801fb49d2940 x22: ffff000011291000 > > > > [ 152.449257] x21: 000000000000002e x20: 0000000000000087 > > > > [ 152.459917] x19: ffff000013393b44 x18: ffffffffffffffff > > > > [ 152.470577] x17: 0000000000000000 x16: 0000000000000000 > > > > [ 152.481236] x15: ffff00001127d6c8 x14: ffff801f8cfd691c > > > > [ 152.491896] x13: 0000000000000000 x12: 0000000000000000 > > > > [ 152.502555] x11: 0000000000000003 x10: 0000801feace2000 > > > > [ 152.513215] x9 : 0000000000000000 x8 : ffff841fa654f280 > > > > [ 152.523874] x7 : 0000000000000000 x6 : 0000000000ffc0e3 > > > > [ 152.534534] x5 : ffff000011291360 x4 : ffff801fb4949f00 > > > > [ 152.545194] x3 : 0000000000ffbffe x2 : 76e767a63713d500 > > > > [ 152.555853] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000 > > > > [ 152.566514] Process insmod (pid: 2732, stack limit = 0x(____ptrval____)) > > > > [ 152.579968] Call trace: > > > > [ 152.584863] logic_outb+0x54/0xb8 > > > > [ 152.591506] f71882fg_find+0x64/0x390 [f71882fg] > > > > [ 152.600768] f71882fg_init+0x38/0xc70 [f71882fg] > > > > [ 152.610031] do_one_initcall+0x5c/0x198 > > > > [ 152.617723] do_init_module+0x54/0x1b0 > > > > [ 152.625237] load_module+0x1dc4/0x2158 > > > > [ 152.632752] __se_sys_init_module+0x14c/0x1e8 > > > > [ 152.641490] __arm64_sys_init_module+0x18/0x20 > > > > [ 152.650404] el0_svc_common+0x5c/0x100 > > > > [ 152.657919] el0_svc_handler+0x2c/0x80 > > > > [ 152.665433] el0_svc+0x8/0xc > > > > [ 152.671202] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034) > > > > [ 152.683434] ---[ end trace fd4f35b610829a48 ]--- > > > > Segmentation fault > > > > root@(none)$ > > > > > > > Note that the f71882fg driver correctly calls request_muxed_region(). > > > > > > > > This issue was originally reported in [1]. > > > > > > > > This patch changes the functionality of request{muxed_}_region() to > > > > request a region from a direct child descendent of the top > > > > ioport_resource. > > > > > > > > In this, if the IO port region has not been mapped for a particular IO > > > > region, the PCI IO resource would also not have been inserted, and so a > > > > suitable child region will not exist. As such, > > > > request_{muxed_}region() calls will fail. > > > > > > > > A side note: there are many drivers in the kernel which fail to even call > > > > request_{muxed_}region() prior to IO port accesses, and they also need to > > > > be fixed (to call request_{muxed_}region(), as appropriate) separately. > > > > > > > > [1] https://www.spinics.net/lists/linux-pci/msg49821.html > > > > > > Please use a https://lore.kernel.org/ URL instead of spinics.net. > > > > ok, I hope that I can find this old thread. > > The beauty of lore.kernel.org is that the URL contains the Message-ID, so > it's easy build the URL and it would contain useful information even if > lore.kernel.org disappeared: > > https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com Yes, the bottom line is what Arnd outlined in the thread above. ISA IO port space is not necessarily PCI but it does not exist architecturally on ARM systems. Taking the example of IA64, the ISA space is memory mapped (like any other arch except for x86) but IIUC the virtual mapping for the ISA port space _always_ exists on IA64 so this issue won't happen. Arnd pointed out a solution in the thread above but I need to check if that's feasible. Lorenzo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource 2019-03-28 17:46 ` Lorenzo Pieralisi @ 2019-03-29 10:42 ` John Garry 2019-03-29 12:22 ` Lorenzo Pieralisi 0 siblings, 1 reply; 17+ messages in thread From: John Garry @ 2019-03-29 10:42 UTC (permalink / raw) To: Lorenzo Pieralisi, Bjorn Helgaas Cc: wangkefeng.wang, arnd, rafael, linux-pci, Will Deacon, linux-kernel, linuxarm, andy.shevchenko, linux-arm-kernel, Catalin Marinas, bp, linux On 28/03/2019 17:46, Lorenzo Pieralisi wrote: > On Tue, Mar 26, 2019 at 05:48:10PM -0500, Bjorn Helgaas wrote: > > [...] > Hi Lorenzo, >> I'm not convinced about this last sentence. >> >> It's true that on most modern systems, including that Intel PCH, the >> Super I/O controller is attached via an LPC bridge on a PCI bus. >> >> But I don't think it's an actual requirement that PCI be involved. >> There certainly once were systems, e.g., PC/104, that had ISA devices >> but no PCI. Maybe Super I/O attached via ISA is obsolete enough that >> we don't care any more, but I really don't know. >> >>>> On x86, I think inb/inw/inl from a port where nothing responds >>>> probably just returns ~0, and outb/outw/outl just get dropped. >>>> Shouldn't arm64 do the same, without crashing? >>> >>> That would be ideal and we're doing something similar in patch 2/3. >>> >>> So on ARM64 we have to IO remap the PCI IO resource. If this mapping is not >>> done (due to no PCI host), then any inb/inw/inl calls will crash the system. >> >> My take is that ARM64 is responsible for implementing inb/inw/inl in >> such a way that they don't crash. I don't think it's practical to >> update all the old ISA drivers or even the core code to work around >> that. > > The problem is that those drivers are accessing a resource that does not > exist in practice, it is taken for granted on x86 systems (and on IA64) > because that was an actual bus (actual or emulated) and was made part of > the architecture. The ISA space is not necessarily tied to PCI, > at least not always. > > Side note: these drivers can't be compiled on PPC, it would be > good to understand why, I have a hunch it can be related. I mentioned this earlier: I saw that in commits like 746cdfbf01c0 ("hwmon: Avoid building drivers forpowerpc that read/write ISA addresses"), PPC would not build these drivers, as, like arm, it has no native ISA. However I still don't think just avoiding compiling these drivers for certain archs solves the problem. [...] >>>>> [1] https://www.spinics.net/lists/linux-pci/msg49821.html >>>> >>>> Please use a https://lore.kernel.org/ URL instead of spinics.net. >>> >>> ok, I hope that I can find this old thread. >> >> The beauty of lore.kernel.org is that the URL contains the Message-ID, so >> it's easy build the URL and it would contain useful information even if >> lore.kernel.org disappeared: >> >> https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com > > Yes, the bottom line is what Arnd outlined in the thread above. > > ISA IO port space is not necessarily PCI but it does not exist > architecturally on ARM systems. > > Taking the example of IA64, the ISA space is memory mapped (like any > other arch except for x86) but IIUC the virtual mapping for the ISA > port space _always_ exists on IA64 so this issue won't happen. > > Arnd pointed out a solution in the thread above but I need to check > if that's feasible. I doubt that it can work now. Since we when introduced the concept of logical PIO space, this IO space became sparely populated by 2 regions - MMIO and indirect IO - so we cannot grow it as we map in regions. I also don't think it works for when we IO unmap regions. Thanks, John > > Lorenzo > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > . > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource 2019-03-29 10:42 ` John Garry @ 2019-03-29 12:22 ` Lorenzo Pieralisi 2019-04-02 9:46 ` John Garry 0 siblings, 1 reply; 17+ messages in thread From: Lorenzo Pieralisi @ 2019-03-29 12:22 UTC (permalink / raw) To: John Garry Cc: Bjorn Helgaas, wangkefeng.wang, arnd, rafael, linux-pci, Will Deacon, linux-kernel, linuxarm, andy.shevchenko, linux-arm-kernel, Catalin Marinas, bp, linux On Fri, Mar 29, 2019 at 10:42:17AM +0000, John Garry wrote: > On 28/03/2019 17:46, Lorenzo Pieralisi wrote: > >On Tue, Mar 26, 2019 at 05:48:10PM -0500, Bjorn Helgaas wrote: > > > >[...] > > > > Hi Lorenzo, > > > >>I'm not convinced about this last sentence. > >> > >>It's true that on most modern systems, including that Intel PCH, the > >>Super I/O controller is attached via an LPC bridge on a PCI bus. > >> > >>But I don't think it's an actual requirement that PCI be involved. > >>There certainly once were systems, e.g., PC/104, that had ISA devices > >>but no PCI. Maybe Super I/O attached via ISA is obsolete enough that > >>we don't care any more, but I really don't know. > >> > >>>>On x86, I think inb/inw/inl from a port where nothing responds > >>>>probably just returns ~0, and outb/outw/outl just get dropped. > >>>>Shouldn't arm64 do the same, without crashing? > >>> > >>>That would be ideal and we're doing something similar in patch 2/3. > >>> > >>>So on ARM64 we have to IO remap the PCI IO resource. If this mapping is not > >>>done (due to no PCI host), then any inb/inw/inl calls will crash the system. > >> > >>My take is that ARM64 is responsible for implementing inb/inw/inl in > >>such a way that they don't crash. I don't think it's practical to > >>update all the old ISA drivers or even the core code to work around > >>that. > > > >The problem is that those drivers are accessing a resource that does not > >exist in practice, it is taken for granted on x86 systems (and on IA64) > >because that was an actual bus (actual or emulated) and was made part of > >the architecture. The ISA space is not necessarily tied to PCI, > >at least not always. > > > >Side note: these drivers can't be compiled on PPC, it would be > >good to understand why, I have a hunch it can be related. > > I mentioned this earlier: > > I saw that in commits like 746cdfbf01c0 ("hwmon: Avoid building drivers > forpowerpc that read/write ISA addresses"), PPC would not build these > drivers, as, like arm, it has no native ISA. > > However I still don't think just avoiding compiling these drivers for > certain archs solves the problem. No it does not but I would like to understand how relevant is fixing those drivers (that should not use ISA IO space without first claiming their resources, for the records) given that PPC did not even try and apparently that's not a problem. > > [...] > > >>>>>[1] https://www.spinics.net/lists/linux-pci/msg49821.html > >>>> > >>>>Please use a https://lore.kernel.org/ URL instead of spinics.net. > >>> > >>>ok, I hope that I can find this old thread. > >> > >>The beauty of lore.kernel.org is that the URL contains the Message-ID, so > >>it's easy build the URL and it would contain useful information even if > >>lore.kernel.org disappeared: > >> > >>https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com > > > >Yes, the bottom line is what Arnd outlined in the thread above. > > > >ISA IO port space is not necessarily PCI but it does not exist > >architecturally on ARM systems. > > > >Taking the example of IA64, the ISA space is memory mapped (like any > >other arch except for x86) but IIUC the virtual mapping for the ISA > >port space _always_ exists on IA64 so this issue won't happen. > > > >Arnd pointed out a solution in the thread above but I need to check > >if that's feasible. > > I doubt that it can work now. > > Since we when introduced the concept of logical PIO space, this IO space > became sparely populated by 2 regions - MMIO and indirect IO - so we cannot > grow it as we map in regions. I also don't think it works for when we IO > unmap regions. I do not have the full picture but I suspect that, apart from x86/IA64, this is a common issue across architectures, I am trying to untangle how ARM 32-bit deals with this (if it does). Lorenzo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource 2019-03-29 12:22 ` Lorenzo Pieralisi @ 2019-04-02 9:46 ` John Garry 0 siblings, 0 replies; 17+ messages in thread From: John Garry @ 2019-04-02 9:46 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Bjorn Helgaas, wangkefeng.wang, arnd, rafael, linux-pci, Will Deacon, linux-kernel, linuxarm, andy.shevchenko, linux-arm-kernel, Catalin Marinas, bp, linux On 29/03/2019 12:22, Lorenzo Pieralisi wrote: >>> > >Side note: these drivers can't be compiled on PPC, it would be >>> > >good to understand why, I have a hunch it can be related. >> > >> > I mentioned this earlier: >> > >> > I saw that in commits like 746cdfbf01c0 ("hwmon: Avoid building drivers >> > forpowerpc that read/write ISA addresses"), PPC would not build these >> > drivers, as, like arm, it has no native ISA. >> > >> > However I still don't think just avoiding compiling these drivers for >> > certain archs solves the problem. > No it does not but I would like to understand how relevant is fixing > those drivers (that should not use ISA IO space without first claiming > their resources, for the records) given that PPC did not even try and > apparently that's not a problem. > Hi Lorenzo, Those drivers should still be fixed up separately. The tricky part in this series is making the resource claim fail if there is no IO space mapped/accessible at the addresses requested. However I would still like to fix up the low level IO port accessors to discard accesses when no IO space is mapped. Thanks, John >> > >> > [...] >> > >>>>>>> > >>>>>[1] https://www.spinics.net/lists/linux-pci ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/3] lib: logic_pio: Reject access to unregistered CPU MMIO regions 2019-03-20 18:14 [PATCH v2 0/3] Fix system crash for accessing unmapped IO port regions John Garry 2019-03-20 18:14 ` [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource John Garry @ 2019-03-20 18:14 ` John Garry 2019-03-23 19:15 ` Andy Shevchenko 2019-03-20 18:14 ` [PATCH v2 3/3] lib: logic_pio: Make some prints explicitly hex John Garry 2 siblings, 1 reply; 17+ messages in thread From: John Garry @ 2019-03-20 18:14 UTC (permalink / raw) To: bhelgaas, rafael, arnd, lorenzo.pieralisi, bp Cc: linux, linux-kernel, linux-pci, wangkefeng.wang, linuxarm, agraf, andy.shevchenko, John Garry Currently when accessing logical indirect PIO addresses in logic_{in, out}{,s}, we first ensure that the region is registered. However, no such check exists for CPU MMIO regions. The CPU MMIO regions would be registered by the PCI host (when PCI_IOBASE is defined) in pci_register_io_range(). We have seen scenarios when systems which don't have a PCI host or, they do, and the PCI host probe fails, that certain devices attempts to still attempt to access PCI IO ports; examples are in [1] and [2]. And even though we would protect against this by ensuring the driver call request_{muxed_}region(), some don't do this: root@(none)$ insmod hwmon/f71805f.ko [ 98.623872] Unable to handle kernel paging request at virtual address ffff7dfffee0002e [ 98.639804] Mem abort info: [ 98.645404] ESR = 0x96000046 [ 98.651525] Exception class = DABT (current EL), IL = 32 bits [ 98.663410] SET = 0, FnV = 0 [ 98.669530] EA = 0, S1PTW = 0 [ 98.675826] Data abort info: [ 98.681597] ISV = 0, ISS = 0x00000046 [ 98.689290] CM = 0, WnR = 1 [ 98.695237] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____) [ 98.709044] [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000 [ 98.726523] Internal error: Oops: 96000046 [#1] PREEMPT SMP [ 98.737706] Modules linked in: f71805f(+) [ 98.745748] CPU: 20 PID: 2736 Comm: insmod Not tainted 5.1.0-rc1-00003-g6f1bfec2a620-dirty #99 [ 98.763045] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018 [ 98.781390] pstate: 80000005 (Nzcv daif -PAN -UAO) [ 98.791008] pc : logic_outb+0x54/0xb8 [ 98.798351] lr : f71805f_find+0x2c/0x1b8 [f71805f] [ 98.807962] sp : ffff000025fbba90 [ 98.814603] x29: ffff000025fbba90 x28: ffff000008b944d0 [ 98.825263] x27: ffff000025fbbdf0 x26: 0000000000000100 [ 98.835922] x25: ffff801f8c270580 x24: ffff000011420000 [ 98.846582] x23: ffff000025fbbb3e x22: ffff000025fbbb40 [ 98.857242] x21: ffff000008b991b8 x20: 0000000000000087 [ 98.867901] x19: 000000000000002e x18: ffffffffffffffff [ 98.878561] x17: 0000000000000000 x16: 0000000000000000 [ 98.889220] x15: ffff00001127d6c8 x14: 0000000000000000 [ 98.899880] x13: 0000000000000000 x12: 0000000000000000 [ 98.910539] x11: 0000000000010820 x10: 0000841fdac40000 [ 98.921199] x9 : 0000000000000001 x8 : 0000000040000000 [ 98.931858] x7 : 0000000000210d00 x6 : 0000000000000000 [ 98.942517] x5 : ffff801fb6a46040 x4 : ffff841febeaeda0 [ 98.953177] x3 : 0000000000ffbffe x2 : ffff000025fbbb40 [ 98.963837] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000 [ 98.974497] Process insmod (pid: 2736, stack limit = 0x(____ptrval____)) [ 98.987950] Call trace: [ 98.992846] logic_outb+0x54/0xb8 [ 98.999489] f71805f_find+0x2c/0x1b8 [f71805f] [ 99.008403] f71805f_init+0x38/0xe48 [f71805f] [ 99.017317] do_one_initcall+0x5c/0x198 [ 99.025008] do_init_module+0x54/0x1b0 [ 99.032522] load_module+0x1dc4/0x2158 [ 99.040037] __se_sys_init_module+0x14c/0x1e8 [ 99.048774] __arm64_sys_init_module+0x18/0x20 [ 99.057688] el0_svc_common+0x5c/0x100 [ 99.065203] el0_svc_handler+0x2c/0x80 [ 99.072717] el0_svc+0x8/0xc [ 99.078486] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034) [ 99.090719] ---[ end trace 10ea80bde051bbfc ]--- root@(none)$ Driver f71805f does not call request_{muxed_}region(), as it should do. This patch adds a check to ensure that the CPU MMIO region is registered prior to accessing the PCI IO ports. [1] https://lkml.org/lkml/2019/3/14/630 [2] https://www.spinics.net/lists/arm-kernel/msg694702.html Signed-off-by: John Garry <john.garry@huawei.com> --- lib/logic_pio.c | 70 ++++++++++++++++++++++++++++--------------------- 1 file changed, 40 insertions(+), 30 deletions(-) diff --git a/lib/logic_pio.c b/lib/logic_pio.c index feea48fd1a0d..026a4fb4b944 100644 --- a/lib/logic_pio.c +++ b/lib/logic_pio.c @@ -192,70 +192,80 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr) } #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE) +#define INVALID_RANGE(range) (!range || \ + (range->flags == LOGIC_PIO_CPU_MMIO && !range->ops)) + #define BUILD_LOGIC_IO(bw, type) \ type logic_in##bw(unsigned long addr) \ { \ type ret = (type)~0; \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ + if (INVALID_RANGE(range)) { \ + WARN_ON_ONCE(1); \ + return ret; \ + } \ \ if (addr < MMIO_UPPER_LIMIT) { \ ret = read##bw(PCI_IOBASE + addr); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *entry = find_io_range(addr); \ - \ - if (entry && entry->ops) \ - ret = entry->ops->in(entry->hostdata, \ - addr, sizeof(type)); \ - else \ - WARN_ON_ONCE(1); \ + ret = range->ops->in(range->hostdata, \ + addr, sizeof(type)); \ } \ return ret; \ } \ \ void logic_out##bw(type value, unsigned long addr) \ { \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ + if (INVALID_RANGE(range)) { \ + WARN_ON_ONCE(1); \ + return; \ + } \ + \ if (addr < MMIO_UPPER_LIMIT) { \ write##bw(value, PCI_IOBASE + addr); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *entry = find_io_range(addr); \ - \ - if (entry && entry->ops) \ - entry->ops->out(entry->hostdata, \ - addr, value, sizeof(type)); \ - else \ - WARN_ON_ONCE(1); \ + if (range->ops && range->ops->out) \ + range->ops->out(range->hostdata, addr, \ + value, sizeof(type)); \ } \ } \ \ -void logic_ins##bw(unsigned long addr, void *buffer, \ +void logic_ins##bw(unsigned long addr, void *buffer, \ unsigned int count) \ { \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ + if (INVALID_RANGE(range)) { \ + WARN_ON_ONCE(1); \ + return; \ + } \ + \ if (addr < MMIO_UPPER_LIMIT) { \ reads##bw(PCI_IOBASE + addr, buffer, count); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *entry = find_io_range(addr); \ - \ - if (entry && entry->ops) \ - entry->ops->ins(entry->hostdata, \ - addr, buffer, sizeof(type), count); \ - else \ - WARN_ON_ONCE(1); \ + range->ops->ins(range->hostdata, addr, buffer, \ + sizeof(type), count); \ } \ - \ } \ \ void logic_outs##bw(unsigned long addr, const void *buffer, \ unsigned int count) \ { \ + struct logic_pio_hwaddr *range = find_io_range(addr); \ + \ + if (INVALID_RANGE(range)) { \ + WARN_ON_ONCE(1); \ + return; \ + } \ + \ if (addr < MMIO_UPPER_LIMIT) { \ writes##bw(PCI_IOBASE + addr, buffer, count); \ } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \ - struct logic_pio_hwaddr *entry = find_io_range(addr); \ - \ - if (entry && entry->ops) \ - entry->ops->outs(entry->hostdata, \ - addr, buffer, sizeof(type), count); \ - else \ - WARN_ON_ONCE(1); \ + range->ops->outs(range->hostdata, addr, buffer, \ + sizeof(type), count); \ } \ } -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] lib: logic_pio: Reject access to unregistered CPU MMIO regions 2019-03-20 18:14 ` [PATCH v2 2/3] lib: logic_pio: Reject access to unregistered CPU MMIO regions John Garry @ 2019-03-23 19:15 ` Andy Shevchenko 2019-03-25 9:59 ` John Garry 0 siblings, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2019-03-23 19:15 UTC (permalink / raw) To: John Garry Cc: Bjorn Helgaas, Rafael J. Wysocki, Arnd Bergmann, Lorenzo Pieralisi, Borislav Petkov, Guenter Roeck, Linux Kernel Mailing List, linux-pci, Kefeng Wang, Linuxarm, Alexander Graf On Wed, Mar 20, 2019 at 8:14 PM John Garry <john.garry@huawei.com> wrote: > > Currently when accessing logical indirect PIO addresses in > logic_{in, out}{,s}, we first ensure that the region is registered. > > However, no such check exists for CPU MMIO regions. The CPU MMIO regions > would be registered by the PCI host (when PCI_IOBASE is defined) in > pci_register_io_range(). > > We have seen scenarios when systems which don't have a PCI host or, they > do, and the PCI host probe fails, that certain devices attempts to still > attempt to access PCI IO ports; examples are in [1] and [2]. > > And even though we would protect against this by ensuring the driver call > request_{muxed_}region(), some don't do this: > > #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE) > +#define INVALID_RANGE(range) (!range || \ > + (range->flags == LOGIC_PIO_CPU_MMIO && !range->ops)) It would be better to read in a form #define foo(x) \ (...) > + ret = range->ops->in(range->hostdata, \ > + addr, sizeof(type)); \ Can it fit one line? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] lib: logic_pio: Reject access to unregistered CPU MMIO regions 2019-03-23 19:15 ` Andy Shevchenko @ 2019-03-25 9:59 ` John Garry 0 siblings, 0 replies; 17+ messages in thread From: John Garry @ 2019-03-25 9:59 UTC (permalink / raw) To: Andy Shevchenko Cc: Bjorn Helgaas, Rafael J. Wysocki, Arnd Bergmann, Lorenzo Pieralisi, Borislav Petkov, Guenter Roeck, Linux Kernel Mailing List, linux-pci, Kefeng Wang, Linuxarm, Alexander Graf On 23/03/2019 19:15, Andy Shevchenko wrote: > On Wed, Mar 20, 2019 at 8:14 PM John Garry <john.garry@huawei.com> wrote: >> >> Currently when accessing logical indirect PIO addresses in >> logic_{in, out}{,s}, we first ensure that the region is registered. >> >> However, no such check exists for CPU MMIO regions. The CPU MMIO regions >> would be registered by the PCI host (when PCI_IOBASE is defined) in >> pci_register_io_range(). >> >> We have seen scenarios when systems which don't have a PCI host or, they >> do, and the PCI host probe fails, that certain devices attempts to still >> attempt to access PCI IO ports; examples are in [1] and [2]. >> >> And even though we would protect against this by ensuring the driver call >> request_{muxed_}region(), some don't do this: >> > Hi Andy, > >> #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE) >> +#define INVALID_RANGE(range) (!range || \ >> + (range->flags == LOGIC_PIO_CPU_MMIO && !range->ops)) > > It would be better to read in a form > #define foo(x) \ Sure, I can change that if you think it reads better. > (...) > >> + ret = range->ops->in(range->hostdata, \ >> + addr, sizeof(type)); \ > > Can it fit one line? It should be ok. I can shorten some variable names. > Thanks for checking, John ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] lib: logic_pio: Make some prints explicitly hex 2019-03-20 18:14 [PATCH v2 0/3] Fix system crash for accessing unmapped IO port regions John Garry 2019-03-20 18:14 ` [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource John Garry 2019-03-20 18:14 ` [PATCH v2 2/3] lib: logic_pio: Reject access to unregistered CPU MMIO regions John Garry @ 2019-03-20 18:14 ` John Garry 2019-03-23 19:12 ` Andy Shevchenko 2 siblings, 1 reply; 17+ messages in thread From: John Garry @ 2019-03-20 18:14 UTC (permalink / raw) To: bhelgaas, rafael, arnd, lorenzo.pieralisi, bp Cc: linux, linux-kernel, linux-pci, wangkefeng.wang, linuxarm, agraf, andy.shevchenko, John Garry Some prints in the code are for a hex number, but don't prefix "0x". Add the prefix, as is the norm. Signed-off-by: John Garry <john.garry@huawei.com> --- lib/logic_pio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/logic_pio.c b/lib/logic_pio.c index 026a4fb4b944..6b37cc79a0bf 100644 --- a/lib/logic_pio.c +++ b/lib/logic_pio.c @@ -126,7 +126,7 @@ static struct logic_pio_hwaddr *find_io_range(unsigned long pio) if (in_range(pio, range->io_start, range->size)) return range; } - pr_err("PIO entry token %lx invalid\n", pio); + pr_err("PIO entry token 0x%lx invalid\n", pio); return NULL; } @@ -186,7 +186,7 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr) if (in_range(addr, range->hw_start, range->size)) return addr - range->hw_start + range->io_start; } - pr_err("addr %llx not registered in io_range_list\n", + pr_err("addr 0x%llx not registered in io_range_list\n", (unsigned long long) addr); return ~0UL; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] lib: logic_pio: Make some prints explicitly hex 2019-03-20 18:14 ` [PATCH v2 3/3] lib: logic_pio: Make some prints explicitly hex John Garry @ 2019-03-23 19:12 ` Andy Shevchenko 2019-03-25 9:48 ` John Garry 0 siblings, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2019-03-23 19:12 UTC (permalink / raw) To: John Garry Cc: Bjorn Helgaas, Rafael J. Wysocki, Arnd Bergmann, Lorenzo Pieralisi, Borislav Petkov, Guenter Roeck, Linux Kernel Mailing List, linux-pci, Kefeng Wang, Linuxarm, Alexander Graf On Wed, Mar 20, 2019 at 8:14 PM John Garry <john.garry@huawei.com> wrote: > > Some prints in the code are for a hex number, but don't prefix "0x". Add > the prefix, as is the norm. > - pr_err("addr %llx not registered in io_range_list\n", > + pr_err("addr 0x%llx not registered in io_range_list\n", > (unsigned long long) addr); Can we use %pa at the same time? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] lib: logic_pio: Make some prints explicitly hex 2019-03-23 19:12 ` Andy Shevchenko @ 2019-03-25 9:48 ` John Garry 2019-03-25 15:03 ` Andy Shevchenko 0 siblings, 1 reply; 17+ messages in thread From: John Garry @ 2019-03-25 9:48 UTC (permalink / raw) To: Andy Shevchenko Cc: Bjorn Helgaas, Rafael J. Wysocki, Arnd Bergmann, Lorenzo Pieralisi, Borislav Petkov, Guenter Roeck, Linux Kernel Mailing List, linux-pci, Kefeng Wang, Linuxarm, Alexander Graf On 23/03/2019 19:12, Andy Shevchenko wrote: > On Wed, Mar 20, 2019 at 8:14 PM John Garry <john.garry@huawei.com> wrote: >> >> Some prints in the code are for a hex number, but don't prefix "0x". Add >> the prefix, as is the norm. > >> - pr_err("addr %llx not registered in io_range_list\n", >> + pr_err("addr 0x%llx not registered in io_range_list\n", >> (unsigned long long) addr); > > Can we use %pa at the same time? > Hi Andy, OK, I think that we can use %pa and drop the casting to unsigned long long. Thanks for checking, John ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] lib: logic_pio: Make some prints explicitly hex 2019-03-25 9:48 ` John Garry @ 2019-03-25 15:03 ` Andy Shevchenko 0 siblings, 0 replies; 17+ messages in thread From: Andy Shevchenko @ 2019-03-25 15:03 UTC (permalink / raw) To: John Garry Cc: Bjorn Helgaas, Rafael J. Wysocki, Arnd Bergmann, Lorenzo Pieralisi, Borislav Petkov, Guenter Roeck, Linux Kernel Mailing List, linux-pci, Kefeng Wang, Linuxarm, Alexander Graf On Mon, Mar 25, 2019 at 11:49 AM John Garry <john.garry@huawei.com> wrote: > On 23/03/2019 19:12, Andy Shevchenko wrote: > > On Wed, Mar 20, 2019 at 8:14 PM John Garry <john.garry@huawei.com> wrote: > >> > >> Some prints in the code are for a hex number, but don't prefix "0x". Add > >> the prefix, as is the norm. > > > >> - pr_err("addr %llx not registered in io_range_list\n", > >> + pr_err("addr 0x%llx not registered in io_range_list\n", > >> (unsigned long long) addr); > > > > Can we use %pa at the same time? > OK, I think that we can use %pa and drop the casting to unsigned long long. Yes, that it is the point. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-04-02 9:51 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-20 18:14 [PATCH v2 0/3] Fix system crash for accessing unmapped IO port regions John Garry 2019-03-20 18:14 ` [RFC PATCH v2 1/3] resource: Request IO port regions from children of ioport_resource John Garry 2019-03-25 23:32 ` Bjorn Helgaas 2019-03-26 16:33 ` John Garry 2019-03-26 22:48 ` Bjorn Helgaas 2019-03-27 11:24 ` John Garry 2019-03-28 17:46 ` Lorenzo Pieralisi 2019-03-29 10:42 ` John Garry 2019-03-29 12:22 ` Lorenzo Pieralisi 2019-04-02 9:46 ` John Garry 2019-03-20 18:14 ` [PATCH v2 2/3] lib: logic_pio: Reject access to unregistered CPU MMIO regions John Garry 2019-03-23 19:15 ` Andy Shevchenko 2019-03-25 9:59 ` John Garry 2019-03-20 18:14 ` [PATCH v2 3/3] lib: logic_pio: Make some prints explicitly hex John Garry 2019-03-23 19:12 ` Andy Shevchenko 2019-03-25 9:48 ` John Garry 2019-03-25 15:03 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).