Re: Error on loading some network Kernel modules

[Alex Ghiti <alex@ghiti.fr>]

Hi Vincent,

On 2/10/20 4:53 AM, Vincent Chen wrote:
> On Mon, Feb 10, 2020 at 2:37 PM Alex Ghiti <alex@ghiti.fr> wrote:
>>
>> Hi Vincent,
>>
>> On 2/7/20 9:51 AM, Vincent Chen wrote:
>>> On Fri, Feb 7, 2020 at 10:39 PM Vincent Chen <vincent.chen@sifive.com> wrote:
>>>>
>>>>>> For point1, we don't need a dedicated region as long as we are allocating
>>>>>> modules from VMALLOC area. Let's avoid another virtual memory region if
>>>>>> possible.
>>>>>>
>>>>>
>>>>> Your comments inspire me if we can know the end of the percpu data
>>>>> region, we can dynamically calculate the start address in each module
>>>>> allocation by "<end of the percpu data region> - 2GB" if needed. I am
>>>>> finding a way to derive the <end of the percpu data region>.
>>>>>
>>>>
>>>> In the implementation of the idea, I found that I did have some
>>>> misunderstandings about the mechanism of accessing static percpu
>>>> symbol. Currently, I think this issue can be solved by modifying the
>>>> module_allocate() to the following.
>>>> #define MODULE_START max(PFN_ALIGN(&_end - 2 *
>>>> SZ_1G), PFN_ALIGN(&_text + 8 * SZ_1K - 2 * SZ_1G))
>>>> void *module_alloc(unsigned long size)
>>>> {
>>>> return __vmalloc_node_range(size, 1, MODULE_START,
>>>> VMALLOC_END, GFP_KERNEL,
>>>> PAGE_KERNEL_EXEC, 0,
>>>> NUMA_NO_NODE,
>>>> __builtin_return_address(0));
>>>> }
>>>> After applying a similar modification above, the kernel can
>>>> successfully insert the test module provided by Romain. Any feedback
>>>> is welcome. If someone is curious about the reason, I roughly
>>>> described it at the end of this mail. I am sorry if someone was misled
>>>> by early discussion.
>>>>
>>>>
>>>> As mentioned early, kernel pre-allocates a memory region for each CPU
>>>> to place the instances of the percpu data. According to the
>>>> declaration type, each memory region can be divided into three chunks.
>>>> These three chunks from low memory to high memory are static chunk,
>>>> reserved chunk, and dynamic chunk.
>>>> a. The static chunk is used for the static percpu symbols declared in the kernel
>>>> b. The reserved chunk is used for the static percpu symbols declared
>>>> in the kernel module
>>>> c. The dynamic chunk is used for all dynamic percpu symbols.
>>>>
>>>> The order of the percpu symbols in each memory region is the same and
>>>> is recorded in a symbol list maintained by the kernel. Therefore, the
>>>> address of a specific CPU's percpu symbol is "The address of this
>>>> percpu symbol in the symbol list" + "The offset from symbols list to
>>>> the specific CPU region".
>>>> a. The symbol list is created based on the ".data..percpu" section. In
>>>> other words, the start address of this symbol list is the start
>>>> address of ".data..percpu". When a dynamic percpu symbol is created or
>>>> a kernel module import a static percpu symbol, the kernel will follow
>>>> the above chunk rule to add this percpu symbol to its suitable region.
>>>> b. When each memory area was created, "The offset from symbols list to
>>>> the specific CPU region" has been recorded.
>>>>
>>>> Back to the issue, according to the rules above, the kernel will
>>>> relocate the ".data..percpu" section of the loaded module to the end
>>>> of the ".data..percpu" section of the kernel image. The end of the
>>>> ".data..percpu" section of the kernel image almost equals _text, and
>>>> the current size of the reserved chunk is 8KB. Therefore, the
>>>> appropriate range to place the loaded module is [max(PFN_ALIGN(&_end -
>>>> 2 *SZ_1G), PFN_ALIGN(&_text + 8 * SZ_1K - 2 * SZ_1G)),  VMALLOC_END].
>>>
>>> I did not notice that the size of the kernel .text section is
>>> impossible below 2 PAGES. Therefore, the modification of module_alloc
>>> function can be reduced to the following.
>>> void *module_alloc(unsigned long size)
>>>    {
>>>    return __vmalloc_node_range(size, 1, PFN_ALIGN(&_end - 2 *SZ_1G),
>>>    VMALLOC_END, GFP_KERNEL,
>>>    PAGE_KERNEL_EXEC, 0,
>>>    NUMA_NO_NODE,
>>>    __builtin_return_address(0));
>>>    }
>>> It is the same as Alex's suggestion. Is Alex willing to send this
>>> patch to resolve this bug?
>>>
>>
>> You did all the work, please send a patch explaining what you learnt and
>> feel free to add a Suggested-By. Anyway, I'll add a Reviewed-by when you
>> propose something.
>>
>> Thank you Vincent for all your research,
>>
>> Alex
> 
> OK, I understood. Thank you and Anup for giving me some suggestions
> from different viewpoints. I will add Suggested-By for you all if
> possible :)
> 
> Vincent
> 

Are you going to propose something soon regarding this issue ? I 
strongly believe we need to fix modules loading for 5.6.

Thanks,

Alex