Re: [RFC PATCH 04/15] Fix verify_module() and next_module_vaddr()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2023/05/25 22:59, lijiang wrote:
> On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx>
> wrote:
> 
>> fix verify_module() and next_module_vaddr()
>>
>> Signed-off-by: Kazuhito Hagio <k-hagio-ab@xxxxxxx>
>> ---
>>   kernel.c | 19 ++++++++++++++++---
>>   memory.c | 36 ++++++++++++++++++++++++++++++++++--
>>   2 files changed, 50 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel.c b/kernel.c
>> index 62afff25bca8..013a9a4d08f4 100644
>> --- a/kernel.c
>> +++ b/kernel.c
>> @@ -3913,8 +3913,13 @@ verify_modules(void)
>>                                  mod_base = mod;
>>                                  break;
>>                          case KMOD_V2:
>> -                               mod_base = ULONG(modbuf +
>> -                                       MODULE_OFFSET2(module_module_core,
>> rx));
>> +                               if (MODULE_MEMORY())
>> +                                       /* mem[MOD_TEXT].base */
>>
> 
> Here, because it only stored the mem[MOD_TEXT].base to lm->mod_base.

Right, lm->mod_base is now equal to mem[MOD_TEXT].base.

> 
> 
>> +                                       mod_base = ULONG(modbuf +
>> OFFSET(module_mem) +
>> +
>>   OFFSET(module_memory_base));
>> +                               else
>> +                                       mod_base = ULONG(modbuf +
>> +
>>   MODULE_OFFSET2(module_module_core, rx));
>>                                  break;
>>                          }
>>
>> @@ -3936,7 +3941,15 @@ verify_modules(void)
>>                                  case KMOD_V2:
>>                                          module_name = modbuf +
>>                                                  OFFSET(module_name);
>> -                                       if (THIS_KERNEL_VERSION >=
>> LINUX(2,6,27))
>> +                                       if (MODULE_MEMORY()) {
>> +                                               int j;
>> +                                               mod_size = 0;
>> +                                               for (j = MOD_TEXT; j <
>> MOD_INIT_TEXT; j++) {
>> +                                                       mod_size +=
>> UINT(modbuf + OFFSET(module_mem) +
>> +
>>   SIZE(module_memory) * i +
>>
> 
>                                         ^^^
> 
> Here it should be  "SIZE(module_memory) * j", not variable i, otherwise it
> will always print the following NOTE information for live debugging when
> executing a mod command.
> crash> mod
> mod: NOTE: modules have changed on this system -- reinitializing

Thanks, I didn't notice it because I'm using a vmcore to test so far.

(btw, isn't it possible for you to send emails only with plain text 
part?  Your emails have two parts, html and plain text, but the html one 
is shown with proportional font and indents are fuzzy.  The plain text 
one looks auto-generated(?) and is wrapped excessively..  Or is my 
Thunderbird setting not good?)

> 
> 
> +
>>   OFFSET(module_memory_size));
>> +                                               }
>> +                                       } else if (THIS_KERNEL_VERSION >=
>> LINUX(2,6,27))
>>                                                  mod_size = UINT(modbuf +
>>
>> MODULE_OFFSET2(module_core_size, rx));
>>                                          else
>> diff --git a/memory.c b/memory.c
>> index 0568f18eb9b7..d8a37e7a0c4f 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -15687,10 +15687,13 @@ in_vmlist_segment(ulong vaddr)
>>   static int
>>   next_module_vaddr(ulong vaddr, ulong *nextvaddr)
>>   {
>> -       int i;
>> -       ulong start, end;
>> +       int i, j;
>> +       ulong start, end, min = (ulong)-1;
>>          struct load_module *lm;
>>
>> +       if (MODULE_MEMORY())
>> +               goto module_memory;
>> +
>>          for (i = 0; i < st->mods_installed; i++) {
>>                  lm = &st->load_modules[i];
>>                  start = lm->mod_base;
>> @@ -15707,6 +15710,35 @@ next_module_vaddr(ulong vaddr, ulong *nextvaddr)
>>                  return TRUE;
>>          }
>>
>> +       return FALSE;
>> +
>> +module_memory: /* TODO: test this */
>> +       for (i = 0; i < st->mods_installed; i++) {
>> +               lm = &st->load_modules[i];
>> +
>> +               for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) {
>> +                       start = lm->mem[j].base;
>> +                       end = start + lm->mem[j].size;
>> +
>> +                       if (vaddr >= end)
>> +                               continue;
>> +
>> +                       if (vaddr < start) {
>> +                               if (start < min) /* replace candidate */
>> +                                       min = start;
>>
> 
> If so,  wouldn't it be overwritten in each module memory type loop? And
> later it only checked them one time(see the code at the end of this
> function).
> 
> In addition, why does it need to deal with this one here? Could you please
> explain more details?

sorry, I don't understand your questions.

This function searches for the next lowest module address from the 
vaddr.  Now the regions of "a module" are not one block and even not 
sorted, this function needs to search all regions of all modules.  min 
is a candidate, so when a lower start address than min is found, min is 
replaced with the start address.

Thanks,
Kazu
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux