Re: [PATCH 1/5] lib: Add a generic version of devmem_is_allowed()

[Nick Kossifidis]

ÎÏÎÏ 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.