As part of adding STRICT_DEVMEM support to the RISC-V port, Zong provided an
implementation of devmem_is_allowed() that's exactly the same as the version in
a handful of other ports. Rather than duplicate code, I've put a generic
version of this in lib/ and used it for the RISC-V port.
I've put those first two patches on riscv/for-next, which I'm targeting for 5.9
(though this is the first version, so they're unreviewed). The other three
obviously depend on the first one going on, but I'm not putting them in the
RISC-V tree as I don't want to step on anyone's toes. If you want me to take
yours along with the others then please say something, as otherwise I'll
probably forget.
I've put the whole thing at
ssh://gitolite.kernel.org/pub/scm/linux/kernel/git/palmer/linux.git -b
generic-devmem .
From: Palmer Dabbelt <[email protected]>
This allows us to enable STRICT_DEVMEM.
Signed-off-by: Palmer Dabbelt <[email protected]>
---
arch/riscv/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 089293e4ad46..8ff368a65a07 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -37,6 +37,7 @@ config RISCV
select GENERIC_IOREMAP
select GENERIC_IRQ_MULTI_HANDLER
select GENERIC_IRQ_SHOW
+ select GENERIC_LIB_DEVMEM_IS_ALLOWED
select GENERIC_PCI_IOMAP
select GENERIC_PTDUMP if MMU
select GENERIC_SCHED_CLOCK
--
2.27.0.383.g050319c2ae-goog
From: Palmer Dabbelt <[email protected]>
Aside from being inlineable, this is exactly the same as the arm64
version, which I recently copied into lib/ for use by the RISC-V port.
[I haven't even build tested this. The lib/ patch is on riscv/for-next,
which I'm targeting for 5.9, so this won't work alone. See the cover
letter for more details.]
Signed-off-by: Palmer Dabbelt <[email protected]>
---
arch/unicore32/Kconfig | 1 +
arch/unicore32/include/asm/io.h | 23 -----------------------
2 files changed, 1 insertion(+), 23 deletions(-)
diff --git a/arch/unicore32/Kconfig b/arch/unicore32/Kconfig
index 11ba1839d198..5334f51614a5 100644
--- a/arch/unicore32/Kconfig
+++ b/arch/unicore32/Kconfig
@@ -12,6 +12,7 @@ config UNICORE32
select HAVE_KERNEL_LZO
select HAVE_KERNEL_LZMA
select HAVE_PCI
+ select GENERIC_LIB_DEVMEM_IS_ALLOWED
select VIRT_TO_BUS
select ARCH_HAVE_CUSTOM_GPIO_H
select GENERIC_FIND_FIRST_BIT
diff --git a/arch/unicore32/include/asm/io.h b/arch/unicore32/include/asm/io.h
index bd4e7c332f85..4560d2531655 100644
--- a/arch/unicore32/include/asm/io.h
+++ b/arch/unicore32/include/asm/io.h
@@ -42,28 +42,5 @@ extern void __uc32_iounmap(volatile void __iomem *addr);
#define PIO_MASK (unsigned int)(IO_SPACE_LIMIT)
#define PIO_RESERVED (PIO_OFFSET + PIO_MASK + 1)
-#ifdef CONFIG_STRICT_DEVMEM
-
-#include <linux/ioport.h>
-#include <linux/mm.h>
-
-/*
- * devmem_is_allowed() checks to see if /dev/mem access to a certain
- * address is valid. The argument is a physical page number.
- * We mimic x86 here by disallowing access to system RAM as well as
- * device-exclusive MMIO regions. This effectively disable read()/write()
- * on /dev/mem.
- */
-static inline int devmem_is_allowed(unsigned long pfn)
-{
- if (iomem_is_exclusive(pfn << PAGE_SHIFT))
- return 0;
- if (!page_is_ram(pfn))
- return 1;
- return 0;
-}
-
-#endif /* CONFIG_STRICT_DEVMEM */
-
#endif /* __KERNEL__ */
#endif /* __UNICORE_IO_H__ */
--
2.27.0.383.g050319c2ae-goog
From: Palmer Dabbelt <[email protected]>
I recently copied this into lib/ for use by the RISC-V port.
[I haven't even build tested this. The lib/ patch is on riscv/for-next,
which I'm targeting for 5.9, so this won't work alone. See the cover
letter for more details.]
Signed-off-by: Palmer Dabbelt <[email protected]>
---
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/io.h | 2 --
arch/arm64/mm/mmap.c | 21 ---------------------
3 files changed, 1 insertion(+), 23 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 66dc41fd49f2..0770ed21a8c4 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -110,6 +110,7 @@ config ARM64
select GENERIC_IRQ_PROBE
select GENERIC_IRQ_SHOW
select GENERIC_IRQ_SHOW_LEVEL
+ select GENERIC_LIB_DEVMEM_IS_ALLOWED
select GENERIC_PCI_IOMAP
select GENERIC_PTDUMP
select GENERIC_SCHED_CLOCK
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index ff50dd731852..c53eba1a7fd2 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -200,6 +200,4 @@ extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
extern int valid_phys_addr_range(phys_addr_t addr, size_t size);
extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t size);
-extern int devmem_is_allowed(unsigned long pfn);
-
#endif /* __ASM_IO_H */
diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
index 3028bacbc4e9..07937b49cb88 100644
--- a/arch/arm64/mm/mmap.c
+++ b/arch/arm64/mm/mmap.c
@@ -47,24 +47,3 @@ int valid_mmap_phys_addr_range(unsigned long pfn, size_t size)
{
return !(((pfn << PAGE_SHIFT) + size) & ~PHYS_MASK);
}
-
-#ifdef CONFIG_STRICT_DEVMEM
-
-#include <linux/ioport.h>
-
-/*
- * devmem_is_allowed() checks to see if /dev/mem access to a certain address
- * is valid. The argument is a physical page number. We mimic x86 here by
- * disallowing access to system RAM as well as device-exclusive MMIO regions.
- * This effectively disable read()/write() on /dev/mem.
- */
-int devmem_is_allowed(unsigned long pfn)
-{
- if (iomem_is_exclusive(pfn << PAGE_SHIFT))
- return 0;
- if (!page_is_ram(pfn))
- return 1;
- return 0;
-}
-
-#endif
--
2.27.0.383.g050319c2ae-goog
From: Palmer Dabbelt <[email protected]>
This is exactly the same as the arm64 version, which I recently copied
into lib/ for use by the RISC-V port.
[I haven't even build tested this. The lib/ patch is on riscv/for-next,
which I'm targeting for 5.9, so this won't work alone. See the cover
letter for more details.]
Signed-off-by: Palmer Dabbelt <[email protected]>
---
arch/arm/Kconfig | 1 +
arch/arm/include/asm/io.h | 1 -
arch/arm/mm/mmap.c | 22 ----------------------
3 files changed, 1 insertion(+), 23 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 2ac74904a3ce..0c9da68835c2 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -54,6 +54,7 @@ config ARM
select GENERIC_IRQ_PROBE
select GENERIC_IRQ_SHOW
select GENERIC_IRQ_SHOW_LEVEL
+ select GENERIC_LIB_DEVMEM_IS_ALLOWED
select GENERIC_PCI_IOMAP
select GENERIC_SCHED_CLOCK
select GENERIC_SMP_IDLE_THREAD
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index ab2b654084fa..fc748122f1e0 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -441,7 +441,6 @@ extern void pci_iounmap(struct pci_dev *dev, void __iomem *addr);
#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
extern int valid_phys_addr_range(phys_addr_t addr, size_t size);
extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t size);
-extern int devmem_is_allowed(unsigned long pfn);
#endif
/*
diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index b8d912ac9e61..a0f8a0ca0788 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -165,25 +165,3 @@ int valid_mmap_phys_addr_range(unsigned long pfn, size_t size)
{
return (pfn + (size >> PAGE_SHIFT)) <= (1 + (PHYS_MASK >> PAGE_SHIFT));
}
-
-#ifdef CONFIG_STRICT_DEVMEM
-
-#include <linux/ioport.h>
-
-/*
- * devmem_is_allowed() checks to see if /dev/mem access to a certain
- * address is valid. The argument is a physical page number.
- * We mimic x86 here by disallowing access to system RAM as well as
- * device-exclusive MMIO regions. This effectively disable read()/write()
- * on /dev/mem.
- */
-int devmem_is_allowed(unsigned long pfn)
-{
- if (iomem_is_exclusive(pfn << PAGE_SHIFT))
- return 0;
- if (!page_is_ram(pfn))
- return 1;
- return 0;
-}
-
-#endif
--
2.27.0.383.g050319c2ae-goog
From: Palmer Dabbelt <[email protected]>
As part of adding support for STRICT_DEVMEM to the RISC-V port, Zong
provided a devmem_is_allowed() implementation that's exactly the same as
all the others I checked. Instead I'm adding a generic version, which
will soon be used.
Signed-off-by: Palmer Dabbelt <[email protected]>
---
include/asm-generic/io.h | 4 ++++
lib/Kconfig | 4 ++++
lib/Makefile | 2 ++
lib/devmem_is_allowed.c | 27 +++++++++++++++++++++++++++
4 files changed, 37 insertions(+)
create mode 100644 lib/devmem_is_allowed.c
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 8b1e020e9a03..69e3db65fba0 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1122,6 +1122,10 @@ static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
}
#endif
+#ifndef CONFIG_GENERIC_DEVMEM_IS_ALLOWED
+extern int devmem_is_allowed(unsigned long pfn);
+#endif
+
#endif /* __KERNEL__ */
#endif /* __ASM_GENERIC_IO_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index df3f3da95990..3b1b6481e073 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -676,3 +676,7 @@ config GENERIC_LIB_CMPDI2
config GENERIC_LIB_UCMPDI2
bool
+
+config GENERIC_LIB_DEVMEM_IS_ALLOWED
+ bool
+ select ARCH_HAS_DEVMEM_IS_ALLOWED
diff --git a/lib/Makefile b/lib/Makefile
index b1c42c10073b..554ef14f9be5 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -318,3 +318,5 @@ obj-$(CONFIG_OBJAGG) += objagg.o
# KUnit tests
obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
+
+obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
diff --git a/lib/devmem_is_allowed.c b/lib/devmem_is_allowed.c
new file mode 100644
index 000000000000..c0d67c541849
--- /dev/null
+++ b/lib/devmem_is_allowed.c
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * A generic version of devmem_is_allowed.
+ *
+ * Based on arch/arm64/mm/mmap.c
+ *
+ * Copyright (C) 2020 Google, Inc.
+ * Copyright (C) 2012 ARM Ltd.
+ */
+
+#include <linux/mm.h>
+#include <linux/ioport.h>
+
+/*
+ * devmem_is_allowed() checks to see if /dev/mem access to a certain address
+ * is valid. The argument is a physical page number. We mimic x86 here by
+ * disallowing access to system RAM as well as device-exclusive MMIO regions.
+ * This effectively disable read()/write() on /dev/mem.
+ */
+int devmem_is_allowed(unsigned long pfn)
+{
+ if (iomem_is_exclusive(pfn << PAGE_SHIFT))
+ return 0;
+ if (!page_is_ram(pfn))
+ return 1;
+ return 0;
+}
--
2.27.0.383.g050319c2ae-goog
Hi Palmer,
On Thu, Jul 09, 2020 at 01:05:48PM -0700, Palmer Dabbelt wrote:
> From: Palmer Dabbelt <[email protected]>
>
> As part of adding support for STRICT_DEVMEM to the RISC-V port, Zong
> provided a devmem_is_allowed() implementation that's exactly the same as
> all the others I checked. Instead I'm adding a generic version, which
> will soon be used.
>
> Signed-off-by: Palmer Dabbelt <[email protected]>
> ---
> include/asm-generic/io.h | 4 ++++
> lib/Kconfig | 4 ++++
> lib/Makefile | 2 ++
> lib/devmem_is_allowed.c | 27 +++++++++++++++++++++++++++
> 4 files changed, 37 insertions(+)
> create mode 100644 lib/devmem_is_allowed.c
>
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index 8b1e020e9a03..69e3db65fba0 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -1122,6 +1122,10 @@ static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
> }
> #endif
>
> +#ifndef CONFIG_GENERIC_DEVMEM_IS_ALLOWED
> +extern int devmem_is_allowed(unsigned long pfn);
> +#endif
> +
> #endif /* __KERNEL__ */
>
> #endif /* __ASM_GENERIC_IO_H */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index df3f3da95990..3b1b6481e073 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -676,3 +676,7 @@ config GENERIC_LIB_CMPDI2
>
> config GENERIC_LIB_UCMPDI2
> bool
> +
> +config GENERIC_LIB_DEVMEM_IS_ALLOWED
> + bool
> + select ARCH_HAS_DEVMEM_IS_ALLOWED
This seems to work the other way around from the usual Kconfig chains.
In the most cases ARCH_HAS_SOMETHING selects GENERIC_SOMETHING.
I believe nicer way would be to make
config STRICT_DEVMEM
bool "Filter access to /dev/mem"
depends on MMU && DEVMEM
depends on ARCH_HAS_DEVMEM_IS_ALLOWED || GENERIC_LIB_DEVMEM_IS_ALLOWED
config GENERIC_LIB_DEVMEM_IS_ALLOWED
bool
and then s/select ARCH_HAS_DEVMEM_IS_ALLOWED/select GENERIC_LIB_DEVMEM_IS_ALLOWED/
in the arch Kconfigs and drop ARCH_HAS_DEVMEM_IS_ALLOWED in the end.
> diff --git a/lib/Makefile b/lib/Makefile
> index b1c42c10073b..554ef14f9be5 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -318,3 +318,5 @@ obj-$(CONFIG_OBJAGG) += objagg.o
> # KUnit tests
> obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> +
> +obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
> diff --git a/lib/devmem_is_allowed.c b/lib/devmem_is_allowed.c
> new file mode 100644
> index 000000000000..c0d67c541849
> --- /dev/null
> +++ b/lib/devmem_is_allowed.c
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * A generic version of devmem_is_allowed.
> + *
> + * Based on arch/arm64/mm/mmap.c
> + *
> + * Copyright (C) 2020 Google, Inc.
> + * Copyright (C) 2012 ARM Ltd.
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/ioport.h>
> +
> +/*
> + * devmem_is_allowed() checks to see if /dev/mem access to a certain address
> + * is valid. The argument is a physical page number. We mimic x86 here by
> + * disallowing access to system RAM as well as device-exclusive MMIO regions.
> + * This effectively disable read()/write() on /dev/mem.
> + */
> +int devmem_is_allowed(unsigned long pfn)
> +{
> + if (iomem_is_exclusive(pfn << PAGE_SHIFT))
> + return 0;
> + if (!page_is_ram(pfn))
> + return 1;
> + return 0;
> +}
> --
> 2.27.0.383.g050319c2ae-goog
>
--
Sincerely yours,
Mike.
On Thu, 09 Jul 2020 13:49:21 PDT (-0700), [email protected] wrote:
> Hi Palmer,
>
> On Thu, Jul 09, 2020 at 01:05:48PM -0700, Palmer Dabbelt wrote:
>> From: Palmer Dabbelt <[email protected]>
>>
>> As part of adding support for STRICT_DEVMEM to the RISC-V port, Zong
>> provided a devmem_is_allowed() implementation that's exactly the same as
>> all the others I checked. Instead I'm adding a generic version, which
>> will soon be used.
>>
>> Signed-off-by: Palmer Dabbelt <[email protected]>
>> ---
>> include/asm-generic/io.h | 4 ++++
>> lib/Kconfig | 4 ++++
>> lib/Makefile | 2 ++
>> lib/devmem_is_allowed.c | 27 +++++++++++++++++++++++++++
>> 4 files changed, 37 insertions(+)
>> create mode 100644 lib/devmem_is_allowed.c
>>
>> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
>> index 8b1e020e9a03..69e3db65fba0 100644
>> --- a/include/asm-generic/io.h
>> +++ b/include/asm-generic/io.h
>> @@ -1122,6 +1122,10 @@ static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
>> }
>> #endif
>>
>> +#ifndef CONFIG_GENERIC_DEVMEM_IS_ALLOWED
>> +extern int devmem_is_allowed(unsigned long pfn);
>> +#endif
>> +
>> #endif /* __KERNEL__ */
>>
>> #endif /* __ASM_GENERIC_IO_H */
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index df3f3da95990..3b1b6481e073 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -676,3 +676,7 @@ config GENERIC_LIB_CMPDI2
>>
>> config GENERIC_LIB_UCMPDI2
>> bool
>> +
>> +config GENERIC_LIB_DEVMEM_IS_ALLOWED
>> + bool
>> + select ARCH_HAS_DEVMEM_IS_ALLOWED
>
> This seems to work the other way around from the usual Kconfig chains.
> In the most cases ARCH_HAS_SOMETHING selects GENERIC_SOMETHING.
Ya, it seemed kind of odd.
>
> I believe nicer way would be to make
>
> config STRICT_DEVMEM
> bool "Filter access to /dev/mem"
> depends on MMU && DEVMEM
> depends on ARCH_HAS_DEVMEM_IS_ALLOWED || GENERIC_LIB_DEVMEM_IS_ALLOWED
>
> config GENERIC_LIB_DEVMEM_IS_ALLOWED
> bool
>
> and then s/select ARCH_HAS_DEVMEM_IS_ALLOWED/select GENERIC_LIB_DEVMEM_IS_ALLOWED/
> in the arch Kconfigs and drop ARCH_HAS_DEVMEM_IS_ALLOWED in the end.
There's some arches that can't be converted to the generic version, at least
not trivially, so we wouldn't drop it. I think it's still cleaner, though.
I'll send a v2.
>
>> diff --git a/lib/Makefile b/lib/Makefile
>> index b1c42c10073b..554ef14f9be5 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -318,3 +318,5 @@ obj-$(CONFIG_OBJAGG) += objagg.o
>> # KUnit tests
>> obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
>> obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
>> +
>> +obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
>> diff --git a/lib/devmem_is_allowed.c b/lib/devmem_is_allowed.c
>> new file mode 100644
>> index 000000000000..c0d67c541849
>> --- /dev/null
>> +++ b/lib/devmem_is_allowed.c
>> @@ -0,0 +1,27 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * A generic version of devmem_is_allowed.
>> + *
>> + * Based on arch/arm64/mm/mmap.c
>> + *
>> + * Copyright (C) 2020 Google, Inc.
>> + * Copyright (C) 2012 ARM Ltd.
>> + */
>> +
>> +#include <linux/mm.h>
>> +#include <linux/ioport.h>
>> +
>> +/*
>> + * devmem_is_allowed() checks to see if /dev/mem access to a certain address
>> + * is valid. The argument is a physical page number. We mimic x86 here by
>> + * disallowing access to system RAM as well as device-exclusive MMIO regions.
>> + * This effectively disable read()/write() on /dev/mem.
>> + */
>> +int devmem_is_allowed(unsigned long pfn)
>> +{
>> + if (iomem_is_exclusive(pfn << PAGE_SHIFT))
>> + return 0;
>> + if (!page_is_ram(pfn))
>> + return 1;
>> + return 0;
>> +}
>> --
>> 2.27.0.383.g050319c2ae-goog
>>
On Thu, Jul 09, 2020 at 11:49:21PM +0300, Mike Rapoport wrote:
> > +#ifndef CONFIG_GENERIC_DEVMEM_IS_ALLOWED
> > +extern int devmem_is_allowed(unsigned long pfn);
> > +#endif
Nit: no need for the extern here.
> > +config GENERIC_LIB_DEVMEM_IS_ALLOWED
> > + bool
> > + select ARCH_HAS_DEVMEM_IS_ALLOWED
>
> This seems to work the other way around from the usual Kconfig chains.
> In the most cases ARCH_HAS_SOMETHING selects GENERIC_SOMETHING.
>
> I believe nicer way would be to make
>
> config STRICT_DEVMEM
> bool "Filter access to /dev/mem"
> depends on MMU && DEVMEM
> depends on ARCH_HAS_DEVMEM_IS_ALLOWED || GENERIC_LIB_DEVMEM_IS_ALLOWED
>
> config GENERIC_LIB_DEVMEM_IS_ALLOWED
> bool
>
> and then s/select ARCH_HAS_DEVMEM_IS_ALLOWED/select GENERIC_LIB_DEVMEM_IS_ALLOWED/
> in the arch Kconfigs and drop ARCH_HAS_DEVMEM_IS_ALLOWED in the end.
To take a step back: Is there any reason to not just always
STRICT_DEVMEM? Maybe for a few architectures that don't currently
support a strict /dev/mem the generic version isn't quite correct, but
someone selecting the option and finding the issue is the best way to
figure that out..
On Fri, Jul 10, 2020 at 08:48:17AM +0300, Nick Kossifidis wrote:
> ???????? 2020-07-10 08:38, Christoph Hellwig ????????????:
> > On Thu, Jul 09, 2020 at 11:49:21PM +0300, Mike Rapoport wrote:
> > > > +#ifndef CONFIG_GENERIC_DEVMEM_IS_ALLOWED
> > > > +extern int devmem_is_allowed(unsigned long pfn);
> > > > +#endif
> >
> > Nit: no need for the extern here.
> >
> > > > +config GENERIC_LIB_DEVMEM_IS_ALLOWED
> > > > + bool
> > > > + select ARCH_HAS_DEVMEM_IS_ALLOWED
> > >
> > > This seems to work the other way around from the usual Kconfig chains.
> > > In the most cases ARCH_HAS_SOMETHING selects GENERIC_SOMETHING.
> > >
> > > I believe nicer way would be to make
> > >
> > > config STRICT_DEVMEM
> > > bool "Filter access to /dev/mem"
> > > depends on MMU && DEVMEM
> > > depends on ARCH_HAS_DEVMEM_IS_ALLOWED ||
> > > GENERIC_LIB_DEVMEM_IS_ALLOWED
> > >
> > > config GENERIC_LIB_DEVMEM_IS_ALLOWED
> > > bool
> > >
> > > and then s/select ARCH_HAS_DEVMEM_IS_ALLOWED/select
> > > GENERIC_LIB_DEVMEM_IS_ALLOWED/
> > > in the arch Kconfigs and drop ARCH_HAS_DEVMEM_IS_ALLOWED in the end.
> >
> > To take a step back: Is there any reason to not just always
> > STRICT_DEVMEM? Maybe for a few architectures that don't currently
> > support a strict /dev/mem the generic version isn't quite correct, but
> > someone selecting the option and finding the issue is the best way to
> > figure that out..
> >
>
> During prototyping / testing having full access to all physical memory
> through /dev/mem is very useful. We should have it enabled by default but
> leave the config option there so that users / developers can disable it if
> needed IMHO.
I did not suggest to take the config option away. Just to
unconditionally allow enabling the option on all architectures.
Στις 2020-07-10 08:38, Christoph Hellwig έγραψε:
> On Thu, Jul 09, 2020 at 11:49:21PM +0300, Mike Rapoport wrote:
>> > +#ifndef CONFIG_GENERIC_DEVMEM_IS_ALLOWED
>> > +extern int devmem_is_allowed(unsigned long pfn);
>> > +#endif
>
> Nit: no need for the extern here.
>
>> > +config GENERIC_LIB_DEVMEM_IS_ALLOWED
>> > + bool
>> > + select ARCH_HAS_DEVMEM_IS_ALLOWED
>>
>> This seems to work the other way around from the usual Kconfig chains.
>> In the most cases ARCH_HAS_SOMETHING selects GENERIC_SOMETHING.
>>
>> I believe nicer way would be to make
>>
>> config STRICT_DEVMEM
>> bool "Filter access to /dev/mem"
>> depends on MMU && DEVMEM
>> depends on ARCH_HAS_DEVMEM_IS_ALLOWED ||
>> GENERIC_LIB_DEVMEM_IS_ALLOWED
>>
>> config GENERIC_LIB_DEVMEM_IS_ALLOWED
>> bool
>>
>> and then s/select ARCH_HAS_DEVMEM_IS_ALLOWED/select
>> GENERIC_LIB_DEVMEM_IS_ALLOWED/
>> in the arch Kconfigs and drop ARCH_HAS_DEVMEM_IS_ALLOWED in the end.
>
> To take a step back: Is there any reason to not just always
> STRICT_DEVMEM? Maybe for a few architectures that don't currently
> support a strict /dev/mem the generic version isn't quite correct, but
> someone selecting the option and finding the issue is the best way to
> figure that out..
>
During prototyping / testing having full access to all physical memory
through /dev/mem is very useful. We should have it enabled by default
but leave the config option there so that users / developers can disable
it if needed IMHO.