On Tue, Aug 27, 2019 at 5:43 AM Paul Walmsley <paul.walms...@sifive.com> wrote: > > Hello Anup, > > On Mon, 19 Aug 2019, Anup Patel wrote: > > > Currently, various virtual memory areas of Linux RISC-V are organized > > in increasing order of their virtual addresses is as follows: > > 1. User space area (This is lowest area and starts at 0x0) > > 2. FIXMAP area > > 3. VMALLOC area > > 4. Kernel area (This is highest area and starts at PAGE_OFFSET) > > > > The maximum size of user space aread is represented by TASK_SIZE. > > > > On RV32 systems, TASK_SIZE is defined as VMALLOC_START which causes the > > user space area to overlap the FIXMAP area. This allows user space apps > > to potentially corrupt the FIXMAP area and kernel OF APIs will crash > > whenever they access corrupted FDT in the FIXMAP area. > > > > On RV64 systems, TASK_SIZE is set to fixed 256GB and no other areas > > happen to overlap so we don't see any FIXMAP area corruptions. > > > > This patch fixes FIXMAP area corruption on RV32 systems by setting > > TASK_SIZE to FIXADDR_START. > > This part -- the TASK_SIZE change -- makes sense to me. > > However, the patch also changes FIXADDR_SIZE to be defined in terms of > page table-related constants. Previously, FIXADDR_SIZE was based on > __end_of_fixed_addresses, as it is for most other architectures. The part > of the patch that changes FIXADDR_SIZE seems unrelated to the actual fix. > > If that's indeed the case -- that the change to FIXADDR_SIZE is unrelated > from the fix -- could you please split that into a separate patch, with a > description of the rationale? I think I understand why you're proposing > it, but it seems odd to explicitly connect it to page table-related > constants, rather than the contents of "enum fixed_addresses", and I'm > reluctant to merge that part of this patch without a bit more discussion.
The FIXADDR_SIZE change is related to the TASK_SIZE requirement and it is not a separate change because: 1. TASK_SIZE must be evenly divisible by PGDIR_SIZE. The FIXADDR_START is defined as (FIXADDR_TOP - FIXADDR_SIZE). The original FIXADDR_SIZE defined in-terms of __end_of_fixed_addresses is not a multiple of PGDIR_SIZE hence it makes sense to make FIXADDR_SIZE as PGDIR_SIZE. 2. Let say we ignore point1 above then still we cannot continue to express FIXADDR_SIZE in-terms of __end_of_fixed_addresses because of cyclic header dependency where asm/fixmap.h includes asm/pgtable.h and __end_of_fixed_addresses is defined in asm/fixmap.h. We certainly need to move FIXADDR_TOP, FIXADDR_START, and FIXADDR_SIZE to asm/pgtable.h so that we can express TASK_SIZE as FIXADDR_START for RV32. If we don't simplify FIXADDR_SIZE then it will result in compile errors. Regards, Anup > > > > We also move FIXADDR_TOP, FIXADDR_SIZE, and FIXADDR_START defines to > > asm/pgtable.h so that we can avoid cyclic header includes. > > > > Signed-off-by: Anup Patel <anup.pa...@wdc.com> > > Tested-by: Alistair Francis <alistair.fran...@wdc.com> > > Reviewed-by: Christoph Hellwig <h...@lst.de> > > --- > > Changes since v1: > > - Drop braces from "#define FIXADDR_TOP" > > --- > > arch/riscv/include/asm/fixmap.h | 4 ---- > > arch/riscv/include/asm/pgtable.h | 12 ++++++++++-- > > 2 files changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/arch/riscv/include/asm/fixmap.h > > b/arch/riscv/include/asm/fixmap.h > > index 9c66033c3a54..161f28d04a07 100644 > > --- a/arch/riscv/include/asm/fixmap.h > > +++ b/arch/riscv/include/asm/fixmap.h > > @@ -30,10 +30,6 @@ enum fixed_addresses { > > __end_of_fixed_addresses > > }; > > > > -#define FIXADDR_SIZE (__end_of_fixed_addresses * PAGE_SIZE) > > -#define FIXADDR_TOP (VMALLOC_START) > > -#define FIXADDR_START (FIXADDR_TOP - FIXADDR_SIZE) > > - > > #define FIXMAP_PAGE_IO PAGE_KERNEL > > > > #define __early_set_fixmap __set_fixmap > > diff --git a/arch/riscv/include/asm/pgtable.h > > b/arch/riscv/include/asm/pgtable.h > > index a364aba23d55..c24a083b3e12 100644 > > --- a/arch/riscv/include/asm/pgtable.h > > +++ b/arch/riscv/include/asm/pgtable.h > > @@ -420,14 +420,22 @@ static inline void pgtable_cache_init(void) > > #define VMALLOC_END (PAGE_OFFSET - 1) > > #define VMALLOC_START (PAGE_OFFSET - VMALLOC_SIZE) > > > > +#define FIXADDR_TOP VMALLOC_START > > +#ifdef CONFIG_64BIT > > +#define FIXADDR_SIZE PMD_SIZE > > +#else > > +#define FIXADDR_SIZE PGDIR_SIZE > > +#endif > > +#define FIXADDR_START (FIXADDR_TOP - FIXADDR_SIZE) > > + > > /* > > - * Task size is 0x4000000000 for RV64 or 0xb800000 for RV32. > > + * Task size is 0x4000000000 for RV64 or 0x9fc00000 for RV32. > > * Note that PGDIR_SIZE must evenly divide TASK_SIZE. > > */ > > #ifdef CONFIG_64BIT > > #define TASK_SIZE (PGDIR_SIZE * PTRS_PER_PGD / 2) > > #else > > -#define TASK_SIZE VMALLOC_START > > +#define TASK_SIZE FIXADDR_START > > #endif > > > > #include <asm-generic/pgtable.h> > > -- > > 2.17.1 > > > > > - Paul