Hi Anshuman,

Thanks for taking a look at this.

On Fri, Jun 14, 2019 at 05:47:55PM +0530, Anshuman Khandual wrote:

Hello Steve,

On 06/12/2019 10:56 PM, Steve Capper wrote:

quoted

Put the direct linear map in the lower addresses of the kernel VA range
and everything else in the higher ranges.

The reason for this flip has to be more clear in the commit message.

Agreed, that's what I failed to do below ;-).

quoted

This allows us to make room for an inline KASAN shadow that operates
under both 48 and 52 bit kernel VA sizes. For example with a 52-bit VA,
if KASAN_SHADOW_END < 0xFFF8000000000000 (it is in the lower addresses
of the kernel VA range), this will be below the start of the minimum
48-bit kernel VA address of 0xFFFF000000000000.

Though this is true it does not convey to the effect of why the flip is
required. As you had mentioned previously KASAN_SHADOW_END is fixed because
it needs to support the highest possible VA (~0UL) in both 48 and 52 bits.

Hence KASAN_SHADOW_START will have to be variable in order to accommodate
both 48 bits or 52 bits effective virtual address. Hence not sure what the
above example based on KASAN_SHADOW_START is trying to convey.

KASAN_SHADOW_END cannot be in the (52 bit space - 48 bit space) exclusion VA
range as it would not work for the 48 bits case. But KASAN_SHADOW_START can
very well be in that space for 52 bits case.

The current definition

#define KASAN_SHADOW_START      (VA_START)
#define KASAN_SHADOW_END        (KASAN_SHADOW_START + KASAN_SHADOW_SIZE)

This wont work in the new scheme because VA_START is different for 48 bits
and 52 bits which will make KASAN_SHADOW_END variable as well. Hence we need
to change this arrangement.

Yes in a future patch? VA_BITS is still constant here.

What the commit message here does not try to convince is that there are no
other alternate arrangements where KASAN_SHADOW_END remains fixed (and also
accessible in 48 bits scheme), KASAN_SHADOW_FIRST is variable accommodating
both 48 bits and 52 bits case and flipping the kernel VA space is the only
viable option.

Ahh I see this in the cover letter.

" In order to allow for a KASAN shadow that changes size at boot time, one
must fix the KASAN_SHADOW_END for both 48 & 52-bit VAs and "grow" the
start address. Also, it is highly desirable to maintain the same
function addresses in the kernel .text between VA sizes. Both of these
requirements necessitate us to flip the kernel address space halves s.t.
the direct linear map occupies the lower addresses."

Though this is better (please add it to commit message).

Sure :-)

There were no alternate arrangements to achieve the above two objectives
without flipping the VA space ? The reasoning here in the commit message is
not convincing enough even with the above cover letter extract.

quoted

We need to adjust:
 *) KASAN shadow region placement logic,
 *) KASAN_SHADOW_OFFSET computation logic,
 *) virt_to_phys, phys_to_virt checks,
 *) page table dumper.

These are all small changes, that need to take place atomically, so they
are bundled into this commit.

It will be great to add a before patch and after patch view of the kernel
virtual address space enlisting different sections to make things apparent
about what and how the layout has changed.

Hmm, pondering this. I can introduce a documentation document in a new
first patch then modify it as the series changes?

Catalin, would that work for you?

Cheers,
-- 
Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel