Re: [PATCHv2 2/2] [fs] proc/vmcore: check the dummy place holder for offline cpu to avoid warning

[Liu ping fan]

On Wed, Dec 21, 2016 at 11:57 AM, Pratyush Anand  wrote:
>
>
> On Wednesday 21 December 2016 08:56 AM, Xunlei Pang wrote:
>>
>> On 12/20/2016 at 11:38 PM, Pratyush Anand wrote:
>>>
>>>
>>>
>>> On Monday 19 December 2016 08:10 AM, Dave Young wrote:

 Hi, Pingfan

 On 12/19/16 at 10:08am, Pingfan Liu wrote:
>>
>> kexec-tools always allocates program headers for present cpus. But
>> when crashing, offline cpus have dummy headers. We do not copy these
>> dummy notes into ELF file, also have no need of warning on them.

 I still think it is not worth such a fix, if you feel a lot of warnings
 in case large cpu numbers, I think you can change the pr_warn to
 pr_warn_once, we do not care the null cpu notes if it has nothing bad
 to the vmcore.

>>>
>>> I agree. Warning is more like information here. May be, we can count the
>>> number of times real_sz was 0, and then can print an info at the end in
>>> stead of warning, like..."N number of CPUs would have been offline, PT_NOTE
>>> entries was absent for them."
>>
>>
>> Well, OTOH the warning may also be due to some user-space misuse, we can't
>> distinguish that without extra information added.
>
>
> Yes, yes..I agree, I meant that the above info is just indicative. May be
> "might have been" could be better word than "would have been" in the above
> info print message.
>
>
>>
>> Another possible user-space fix would be: Firstly fix kexec-tools to add
>> notes only for online cpus,
>> then utilize udev rules(cpu online/offline events) to automatically
>> trigger kdump kernel reload.
>
>
> Hummm..this is certainly possible. But can we do much even when we get the
> info that the PT_NOTE was compromised by user space?
>
If doing this, kexec should be re-forked each time when
online/offline. I think it is not worth to do so just for suppressing
some warning.

> Therefore, I am of the view that if at all we are concerned about number of
> warning messages in case of multiple offline cpu, then we can just print the
> total number of NULL PT_NOTE at the end of loop.
>
Yes, I think it is a good idea. NULL PT_NOTES are only caused by
offlne cpu, it is better to change the waring to info

Thx,
Pingfan

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv2 2/2] [fs] proc/vmcore: check the dummy place holder for offline cpu to avoid warning

[Xunlei Pang]

On 12/21/2016 at 11:57 AM, Pratyush Anand wrote:
>
>
> On Wednesday 21 December 2016 08:56 AM, Xunlei Pang wrote:
>> On 12/20/2016 at 11:38 PM, Pratyush Anand wrote:
>>>
>>>
>>> On Monday 19 December 2016 08:10 AM, Dave Young wrote:
 Hi, Pingfan

 On 12/19/16 at 10:08am, Pingfan Liu wrote:
>> kexec-tools always allocates program headers for present cpus. But
>> when crashing, offline cpus have dummy headers. We do not copy these
>> dummy notes into ELF file, also have no need of warning on them.
 I still think it is not worth such a fix, if you feel a lot of warnings
 in case large cpu numbers, I think you can change the pr_warn to
 pr_warn_once, we do not care the null cpu notes if it has nothing bad
 to the vmcore.

>>>
>>> I agree. Warning is more like information here. May be, we can count the 
>>> number of times real_sz was 0, and then can print an info at the end in 
>>> stead of warning, like..."N number of CPUs would have been offline, PT_NOTE 
>>> entries was absent for them."
>>
>> Well, OTOH the warning may also be due to some user-space misuse, we can't 
>> distinguish that without extra information added.
>
> Yes, yes..I agree, I meant that the above info is just indicative. May be 
> "might have been" could be better word than "would have been" in the above 
> info print message.
>
>
>>
>> Another possible user-space fix would be: Firstly fix kexec-tools to add 
>> notes only for online cpus,
>> then utilize udev rules(cpu online/offline events) to automatically trigger 
>> kdump kernel reload.
>
> Hummm..this is certainly possible. But can we do much even when we get the 
> info that the PT_NOTE was compromised by user space?
>
> Therefore, I am of the view that if at all we are concerned about number of 
> warning messages in case of multiple offline cpu, then we can just print the 
> total number of NULL PT_NOTE at the end of loop.

Yes, agree.

Regards,
Xunlei

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv2 2/2] [fs] proc/vmcore: check the dummy place holder for offline cpu to avoid warning

[Xunlei Pang]

On 12/20/2016 at 11:38 PM, Pratyush Anand wrote:
>
>
> On Monday 19 December 2016 08:10 AM, Dave Young wrote:
>> Hi, Pingfan
>>
>> On 12/19/16 at 10:08am, Pingfan Liu wrote:
>>> > kexec-tools always allocates program headers for present cpus. But
>>> > when crashing, offline cpus have dummy headers. We do not copy these
>>> > dummy notes into ELF file, also have no need of warning on them.
>> I still think it is not worth such a fix, if you feel a lot of warnings
>> in case large cpu numbers, I think you can change the pr_warn to
>> pr_warn_once, we do not care the null cpu notes if it has nothing bad
>> to the vmcore.
>>
>
> I agree. Warning is more like information here. May be, we can count the 
> number of times real_sz was 0, and then can print an info at the end in stead 
> of warning, like..."N number of CPUs would have been offline, PT_NOTE entries 
> was absent for them."

Well, OTOH the warning may also be due to some user-space misuse, we can't 
distinguish that without extra information added.

Another possible user-space fix would be: Firstly fix kexec-tools to add notes 
only for online cpus,
then utilize udev rules(cpu online/offline events) to automatically trigger 
kdump kernel reload.

Regards,
Xunlei

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv2 2/2] [fs] proc/vmcore: check the dummy place holder for offline cpu to avoid warning

[Pratyush Anand]

On Monday 19 December 2016 08:10 AM, Dave Young wrote:

Hi, Pingfan

On 12/19/16 at 10:08am, Pingfan Liu wrote:

> kexec-tools always allocates program headers for present cpus. But
> when crashing, offline cpus have dummy headers. We do not copy these
> dummy notes into ELF file, also have no need of warning on them.

I still think it is not worth such a fix, if you feel a lot of warnings
in case large cpu numbers, I think you can change the pr_warn to
pr_warn_once, we do not care the null cpu notes if it has nothing bad
to the vmcore.



I agree. Warning is more like information here. May be, we can count the 
number of times real_sz was 0, and then can print an info at the end in 
stead of warning, like..."N number of CPUs would have been offline, 
PT_NOTE entries was absent for them."


~Pratyush

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv2 2/2] [fs] proc/vmcore: check the dummy place holder for offline cpu to avoid warning

[Dave Young]

Hi, Pingfan

On 12/19/16 at 10:08am, Pingfan Liu wrote:
> kexec-tools always allocates program headers for present cpus. But
> when crashing, offline cpus have dummy headers. We do not copy these
> dummy notes into ELF file, also have no need of warning on them.

I still think it is not worth such a fix, if you feel a lot of warnings
in case large cpu numbers, I think you can change the pr_warn to
pr_warn_once, we do not care the null cpu notes if it has nothing bad
to the vmcore.

> 
> Signed-off-by: Pingfan Liu 
> ---
>  fs/proc/vmcore.c | 46 ++
>  1 file changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index 8ab782d..fc6e352 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -526,9 +526,10 @@ static u64 __init get_vmcore_size(size_t elfsz, size_t 
> elfnotesegsz,
>   */
>  static int __init update_note_header_size_elf64(const Elf64_Ehdr *ehdr_ptr)
>  {
> - int i, rc=0;
> + int i, j, rc = 0;
>   Elf64_Phdr *phdr_ptr;
> - Elf64_Nhdr *nhdr_ptr;
> + Elf64_Nhdr *nhdr_ptr, *prev_ptr;
> + bool warn;
>  
>   phdr_ptr = (Elf64_Phdr *)(ehdr_ptr + 1);
>   for (i = 0; i < ehdr_ptr->e_phnum; i++, phdr_ptr++) {
> @@ -536,6 +537,7 @@ static int __init update_note_header_size_elf64(const 
> Elf64_Ehdr *ehdr_ptr)
>   u64 offset, max_sz, sz, real_sz = 0;
>   if (phdr_ptr->p_type != PT_NOTE)
>   continue;
> + warn = true;
>   max_sz = phdr_ptr->p_memsz;
>   offset = phdr_ptr->p_offset;
>   notes_section = kmalloc(max_sz, GFP_KERNEL);
> @@ -556,14 +558,27 @@ static int __init update_note_header_size_elf64(const 
> Elf64_Ehdr *ehdr_ptr)
>   nhdr_ptr->n_namesz, nhdr_ptr->n_descsz);
>   break;
>   }
> + prev_ptr = nhdr_ptr;
>   real_sz += sz;
>   nhdr_ptr = (Elf64_Nhdr*)((char*)nhdr_ptr + sz);
> + if ((prev_ptr->n_type == NT_DUMMY)
> +   && !strncmp(KEXEC_CORE_NOTE_NAME,
> + (char *)prev_ptr + sizeof(Elf64_Nhdr),
> + strlen(KEXEC_CORE_NOTE_NAME))) {
> + if (nhdr_ptr->n_namesz == 0) {
> + /* do not copy this dummy note */
> + real_sz = 0;
> + warn = false;
> + } else
> + pr_warn("Warning: Dummy PT_NOTE not 
> overwritten\n");
> + }
>   }
> + if (real_sz != 0)
> + warn = false;
>   kfree(notes_section);
>   phdr_ptr->p_memsz = real_sz;
> - if (real_sz == 0) {
> + if (warn)
>   pr_warn("Warning: Zero PT_NOTE entries found\n");
> - }
>   }
>  
>   return 0;
> @@ -712,9 +727,10 @@ static int __init merge_note_headers_elf64(char *elfptr, 
> size_t *elfsz,
>   */
>  static int __init update_note_header_size_elf32(const Elf32_Ehdr *ehdr_ptr)
>  {
> - int i, rc=0;
> + int i, j, rc = 0;
>   Elf32_Phdr *phdr_ptr;
> - Elf32_Nhdr *nhdr_ptr;
> + Elf32_Nhdr *nhdr_ptr, *prev_ptr;
> + bool warn;
>  
>   phdr_ptr = (Elf32_Phdr *)(ehdr_ptr + 1);
>   for (i = 0; i < ehdr_ptr->e_phnum; i++, phdr_ptr++) {
> @@ -722,6 +738,7 @@ static int __init update_note_header_size_elf32(const 
> Elf32_Ehdr *ehdr_ptr)
>   u64 offset, max_sz, sz, real_sz = 0;
>   if (phdr_ptr->p_type != PT_NOTE)
>   continue;
> + warn = true;
>   max_sz = phdr_ptr->p_memsz;
>   offset = phdr_ptr->p_offset;
>   notes_section = kmalloc(max_sz, GFP_KERNEL);
> @@ -742,14 +759,27 @@ static int __init update_note_header_size_elf32(const 
> Elf32_Ehdr *ehdr_ptr)
>   nhdr_ptr->n_namesz, nhdr_ptr->n_descsz);
>   break;
>   }
> + prev_ptr = nhdr_ptr;
>   real_sz += sz;
>   nhdr_ptr = (Elf32_Nhdr*)((char*)nhdr_ptr + sz);
> + if ((prev_ptr->n_type == NT_DUMMY)
> +   && !strncmp(KEXEC_CORE_NOTE_NAME,
> + (char *)prev_ptr + sizeof(Elf32_Nhdr),
> + strlen(KEXEC_CORE_NOTE_NAME))) {
> + if (nhdr_ptr->n_namesz == 0) {
> + /* do not copy this dummy note */
> + real_sz = 0;
> + warn = false;
> + } else
> +