Re: [PATCH 0/4] kaslr: extend movable_node to movable_node=nn[KMG]@ss[KMG]

[Dou Liyang]

Hi Chao,

At 10/20/2017 10:53 AM, Chao Fan wrote:
> On Fri, Oct 20, 2017 at 10:37:52AM +0800, Dou Liyang wrote:
> > Hi Chao,
> Hi Dou-san,
>
> > Cheer! I have some concerns below.
>
> Thanks for your reply.
>
> > At 10/19/2017 06:02 PM, Chao Fan wrote:
> > > Here is a problem:
> > > Here is a machine with several NUMA nodes and some of them are hot-pluggable.
> > > It's not good for kernel to be extracted in the memory region of movable node.
> > > But in current code, I print the address choosen by kaslr and found it may be
> > > placed in movable node sometimes. To solve this problem, it's better to limit
> > > the memory region choosen by kaslr to immovable node in kaslr.c. But the memory
> > > infomation about if it's hot-pluggable is stored in ACPI SRAT table, which is
> > > parsed after kernel is extracted. So we can't get the detail memory infomation
> > > before extracting kernel.
> > >
> > > So extend the movable_node to movable_node=nn@ss, in which nn means
> > > the size of memory in *immovable* node, and ss means the start position of
> > > this memory region. Then limit kaslr choose memory in these regions.
> >
> > Yes, great. Here we should remember that the situation of
> > 'movable_node=nn@ss' is rare, normal situation is 'movable_node=nn'.
> >
> > So, we should consider our code tendencies for normal situation. ;-)
>
> Yes, it's normal. But you can not make sure the special situation will
> never happen,. If it happens, we can make sure codes work well, right?
>
> We can not make sure that the movable nodes are continuous, or even if
> the movable nodes are continuous, we can not make sure the memory
> address are continuous.
>
> It is easy to avoid the memory region in movable node.
> But if we can handle more special situations, and at the same time,
> make kernel more safe, why not?

You misunderstand my opinions, I means that
when we code, we need to know the problem clearly and which part of
problem will often be executed.

Make our code more suitable for the normal situation without affecting 
the function of the problem.
Just like:

likely() and unlikely()

Here I guess you don't consider that. so I said that.

> > > There are two policies:
> > > 1. Specify the memory region in *movable* node to avoid:
> > >    Then we can use the existing mem_avoid to handle. But if the memory
> > >    one movable node was separated by memory hole or different movable nodes
> > >    are discontinuous, we don't know how many regions need to avoid.
> >
> > It is not a problem.
> >
> > As you said, we should provide an interface for users later, like that:
> >
> > # cat /sys/device/system/memory/movable_node
> > nn@ss
>
> Both are OK. I think outputing the memory region in movable_node or
> immovable_node are both reasonable. So the interface of both methods
> will be useful. And after we decided which policy used in kaslr, then
> add the interface of /sys.

Actually, I prefer the first one, are you ready to post the patches
for the first policy?

Thanks,
	dou.
> Thanks,
> Chao Fan
>
> > Thanks,
> > 	dou.
> > >    OTOH, we must avoid all of the movable memory, otherwise, kaslr may
> > >    choose the wrong place.
> > > 2. Specify the memory region in "immovable* node to select:
> > >    Only support 4 regions in this parameter. Then user can use two nodes
> > >    at least for kaslr to choose, it's enough for the kernel to extract.
> > >    At the same time, because we need only 4 new mem_vector, the usage
> > >    of memory here is not too big.
> > >
> > > PATCH 1/4 parse the extended movable_node=nn[KMG]@ss[KMG], then
> > > 	  store the memory regions.
> > > PATCH 2/4 selects the memory region in immovable node when process
> > > 	  memmap.
> > > PATCH 3/4 is the change of document.
> > > PATCH 4/4 cleans up some little problems.
> > >
> > > Chao Fan (4):
> > >   kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]
> > >   kaslr: select the memory region in immovable node to process
> > >   document: change the document for the extended movable_node
> > >   kaslr: clean up a useless variable and some usless space
> > >
> > >  Documentation/admin-guide/kernel-parameters.txt |   9 ++
> > >  arch/x86/boot/compressed/kaslr.c                | 140 +++++++++++++++++++++---
> > >  2 files changed, 131 insertions(+), 18 deletions(-)