Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1302511 > unrolled thread

[PATCH v2 1/2] kexec: Introduce a protection mechanism for the crashkernel reserved memory

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/2] kexec: Introduce a protection mechanism for the crashkernel reserved memory Xunlei Pang <xlpang@redhat.com> - 2016-01-06 11:00 +0100
    [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres() Xunlei Pang <xlpang@redhat.com> - 2016-01-06 11:00 +0100
      Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Minfei Huang <mhuang@redhat.com> - 2016-01-06 18:10 +0100
        Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Xunlei Pang <xlpang@redhat.com> - 2016-01-07 03:20 +0100
          Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Xunlei Pang <xlpang@redhat.com> - 2016-01-07 03:30 +0100
          Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Minfei Huang <mhuang@redhat.com> - 2016-01-07 03:40 +0100
            Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Xunlei Pang <xlpang@redhat.com> - 2016-01-07 06:10 +0100
              Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Petr Tesarik <ptesarik@suse.cz> - 2016-01-07 10:30 +0100
                Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Xunlei Pang <xlpang@redhat.com> - 2016-01-07 12:20 +0100

#1302511 — [PATCH v2 1/2] kexec: Introduce a protection mechanism for the crashkernel reserved memory

For the cases that some kernel (module) path stamps the crash
reserved memory(already mapped by the kernel) where has been
loaded the second kernel data, the kdump kernel will probably
fail to boot when panic happens (or even not happens) leaving
the culprit at large, this is unacceptable.

The patch introduces a mechanism for detecting such cases:
1) After each crash kexec loading, it simply marks the reserved
memory regions readonly since we no longer access it after that.
When someone stamps the region, the first kernel will panic and
trigger the kdump. The weak arch_kexec_protect_crashkres() is
introduced to do the actual protection.

2) To allow multiple loading, once 1) was done we also need to
remark the reserved memory to readwrite each time a system call
related to kdump is made. The weak arch_kexec_unprotect_crashkres()
is introduced to undo the actual protection.

The architecture can make its specific implementation by overriding
arch_kexec_protect_crashkres() and arch_kexec_unprotect_crashkres().

Signed-off-by: Xunlei Pang <xlpang@redhat.com>
---
 include/linux/kexec.h | 2 ++
 kernel/kexec.c        | 9 ++++++++-
 kernel/kexec_core.c   | 6 ++++++
 kernel/kexec_file.c   | 8 +++++++-
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 7b68d27..ebd6950 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -329,6 +329,8 @@ int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr,
 					Elf_Shdr *sechdrs, unsigned int relsec);
 int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
 					unsigned int relsec);
+void arch_kexec_protect_crashkres(void);
+void arch_kexec_unprotect_crashkres(void);
 
 #else /* !CONFIG_KEXEC_CORE */
 struct pt_regs;
diff --git a/kernel/kexec.c b/kernel/kexec.c
index d873b64..3680f9c 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -167,8 +167,12 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 		return -EBUSY;
 
 	dest_image = &kexec_image;
-	if (flags & KEXEC_ON_CRASH)
+	if (flags & KEXEC_ON_CRASH) {
 		dest_image = &kexec_crash_image;
+		if (kexec_crash_image)
+			arch_kexec_unprotect_crashkres();
+	}
+
 	if (nr_segments > 0) {
 		unsigned long i;
 
@@ -211,6 +215,9 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 	image = xchg(dest_image, image);
 
 out:
+	if ((flags & KEXEC_ON_CRASH) && kexec_crash_image)
+		arch_kexec_protect_crashkres();
+
 	mutex_unlock(&kexec_mutex);
 	kimage_free(image);
 
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index c823f30..de4dd80 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -1560,3 +1560,9 @@ void __weak crash_map_reserved_pages(void)
 
 void __weak crash_unmap_reserved_pages(void)
 {}
+
+void __weak arch_kexec_protect_crashkres(void)
+{}
+
+void __weak arch_kexec_unprotect_crashkres(void)
+{}
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index b70ada0..5892d6a 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -327,8 +327,11 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 		return -EBUSY;
 
 	dest_image = &kexec_image;
-	if (flags & KEXEC_FILE_ON_CRASH)
+	if (flags & KEXEC_FILE_ON_CRASH) {
 		dest_image = &kexec_crash_image;
+		if (kexec_crash_image)
+			arch_kexec_unprotect_crashkres();
+	}
 
 	if (flags & KEXEC_FILE_UNLOAD)
 		goto exchange;
@@ -377,6 +380,9 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 exchange:
 	image = xchg(dest_image, image);
 out:
+	if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image)
+		arch_kexec_protect_crashkres();
+
 	mutex_unlock(&kexec_mutex);
 	kimage_free(image);
 	return ret;
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1302512 — [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres()

Implement the protection method for the crash kernel memory
reservation for the 64-bit x86 kdump.

Signed-off-by: Xunlei Pang <xlpang@redhat.com>
---
 arch/x86/kernel/machine_kexec_64.c | 41 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 819ab3f..cda867d 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -536,3 +536,44 @@ overflow:
 	return -ENOEXEC;
 }
 #endif /* CONFIG_KEXEC_FILE */
+
+static int
+kexec_mark_range(unsigned long start, unsigned long end, bool protect)
+{
+	struct page *page;
+	unsigned int nr_pages;
+
+	/* For physical range: [start, end] */
+	if (!start || !end || start > end)
+		return 0;
+
+	page = pfn_to_page(start >> PAGE_SHIFT);
+	nr_pages = (end + PAGE_SIZE - start) >> PAGE_SHIFT;
+	if (protect)
+		return set_pages_ro(page, nr_pages);
+	else
+		return set_pages_rw(page, nr_pages);
+}
+
+static void kexec_mark_crashkres(bool protect)
+{
+	unsigned long control;
+
+	kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
+
+	/* Don't touch the control code page used in crash_kexec().*/
+	control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
+	/* Control code page is located in the 2nd page. */
+	kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
+	kexec_mark_range(control + 2 * PAGE_SIZE, crashk_res.end, protect);
+}
+
+void arch_kexec_protect_crashkres(void)
+{
+	kexec_mark_crashkres(true);
+}
+
+void arch_kexec_unprotect_crashkres(void)
+{
+	kexec_mark_crashkres(false);
+}
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302904 — Re: [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres()

On 01/06/16 at 05:50pm, Xunlei Pang wrote:
> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index 819ab3f..cda867d 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -536,3 +536,44 @@ overflow:
>  	return -ENOEXEC;
>  }
>  #endif /* CONFIG_KEXEC_FILE */
> +
> +static int
> +kexec_mark_range(unsigned long start, unsigned long end, bool protect)
> +{
> +	struct page *page;
> +	unsigned int nr_pages;
> +
> +	/* For physical range: [start, end] */
> +	if (!start || !end || start > end)
> +		return 0;

Hi, Xunlei.

        if (start > end)
                return 0;

See the below comment.
> +
> +	page = pfn_to_page(start >> PAGE_SHIFT);
> +	nr_pages = (end + PAGE_SIZE - start) >> PAGE_SHIFT;

As I commented in last version, it is better to cover the case if the
range from start to end acrosses two pages.

> +	if (protect)
> +		return set_pages_ro(page, nr_pages);
> +	else
> +		return set_pages_rw(page, nr_pages);
> +}
> +
> +static void kexec_mark_crashkres(bool protect)
> +{
> +	unsigned long control;
> +
> +	kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);

Adding the following if test to test crashk_low_res is better. Then we
do not need to test if start or end is equal to 0 in kexec_mark_range.

        if (crashk_low_res.start != crashk_low_res.end) {
                kexec_mark_range(crashk_low_res.start,
                                crashk_low_res.end, protect);
        }

> +
> +	/* Don't touch the control code page used in crash_kexec().*/
> +	control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
> +	/* Control code page is located in the 2nd page. */
> +	kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
> +	kexec_mark_range(control + 2 * PAGE_SIZE, crashk_res.end, protect);

I think it is more readable, if we use MACRO KEXEC_CONTROL_PAGE_SIZE,
instead of using 2*PAGE_SIZE directly.

Thanks
Minfei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303243 — Re: [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres()

On 01/07/2016 at 01:08 AM, Minfei Huang wrote:
> On 01/06/16 at 05:50pm, Xunlei Pang wrote:
>> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
>> index 819ab3f..cda867d 100644
>> --- a/arch/x86/kernel/machine_kexec_64.c
>> +++ b/arch/x86/kernel/machine_kexec_64.c
>> @@ -536,3 +536,44 @@ overflow:
>>  	return -ENOEXEC;
>>  }
>>  #endif /* CONFIG_KEXEC_FILE */
>> +
>> +static int
>> +kexec_mark_range(unsigned long start, unsigned long end, bool protect)
>> +{
>> +	struct page *page;
>> +	unsigned int nr_pages;
>> +
>> +	/* For physical range: [start, end] */
>> +	if (!start || !end || start > end)
>> +		return 0;
> Hi, Xunlei.
>
>         if (start > end)
>                 return 0;

If both start and end are zero, we want to return directly, so the two
more check doesn't hurt.

> See the below comment.
>> +
>> +	page = pfn_to_page(start >> PAGE_SHIFT);
>> +	nr_pages = (end + PAGE_SIZE - start) >> PAGE_SHIFT;
> As I commented in last version, it is better to cover the case if the
> range from start to end acrosses two pages.

right.

>> +	if (protect)
>> +		return set_pages_ro(page, nr_pages);
>> +	else
>> +		return set_pages_rw(page, nr_pages);
>> +}
>> +
>> +static void kexec_mark_crashkres(bool protect)
>> +{
>> +	unsigned long control;
>> +
>> +	kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
> Adding the following if test to test crashk_low_res is better. Then we
> do not need to test if start or end is equal to 0 in kexec_mark_range.
>
>         if (crashk_low_res.start != crashk_low_res.end) {
>                 kexec_mark_range(crashk_low_res.start,
>                                 crashk_low_res.end, protect);
>         }

The checks in kexec_mark_range() will handle the case, it's not
performance-critical path and will make the code less clean.

>> +
>> +	/* Don't touch the control code page used in crash_kexec().*/
>> +	control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
>> +	/* Control code page is located in the 2nd page. */
>> +	kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
>> +	kexec_mark_range(control + 2 * PAGE_SIZE, crashk_res.end, protect);
> I think it is more readable, if we use MACRO KEXEC_CONTROL_PAGE_SIZE,
> instead of using 2*PAGE_SIZE directly.

OK.

How about the following update:

+static int
+kexec_mark_range(unsigned long start, unsigned long end, bool protect)
+{
+       struct page *page;
+       unsigned int nr_pages;
+
+       /* For physical range: [start, end] */
+       if (!start || !end || start > end)
+               return 0;
+
+       page = pfn_to_page(start >> PAGE_SHIFT);
+       nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1;
+       if (protect)
+               return set_pages_ro(page, nr_pages);
+       else
+               return set_pages_rw(page, nr_pages);
+}
+
+static void kexec_mark_crashkres(bool protect)
+{
+       unsigned long control;
+
+       kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
+
+       /* Don't touch the control code page used in crash_kexec().*/
+       control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
+       /* Control code page is located in the 2nd page. */
+       kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
+       control += KEXEC_CONTROL_PAGE_SIZE;
+       kexec_mark_range(control, crashk_res.end, protect);
+}
+
+void arch_kexec_protect_crashkres(void)
+{
+       kexec_mark_crashkres(true);
+}
+
+void arch_kexec_unprotect_crashkres(void)
+{
+       kexec_mark_crashkres(false);
+}

> Thanks
> Minfei
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303245 — Re: [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres()

On 01/07/2016 at 10:14 AM, Xunlei Pang wrote:
> On 01/07/2016 at 01:08 AM, Minfei Huang wrote:
>> On 01/06/16 at 05:50pm, Xunlei Pang wrote:
>>> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
>>> index 819ab3f..cda867d 100644
>>> --- a/arch/x86/kernel/machine_kexec_64.c
>>> +++ b/arch/x86/kernel/machine_kexec_64.c
>>> @@ -536,3 +536,44 @@ overflow:
>>>  	return -ENOEXEC;
>>>  }
>>>  #endif /* CONFIG_KEXEC_FILE */
>>> +
>>> +static int
>>> +kexec_mark_range(unsigned long start, unsigned long end, bool protect)
>>> +{
>>> +	struct page *page;
>>> +	unsigned int nr_pages;
>>> +
>>> +	/* For physical range: [start, end] */
>>> +	if (!start || !end || start > end)
>>> +		return 0;
>> Hi, Xunlei.
>>
>>         if (start > end)
>>                 return 0;
> If both start and end are zero, we want to return directly, so the two
> more check doesn't hurt.
>
>> See the below comment.
>>> +
>>> +	page = pfn_to_page(start >> PAGE_SHIFT);
>>> +	nr_pages = (end + PAGE_SIZE - start) >> PAGE_SHIFT;
>> As I commented in last version, it is better to cover the case if the
>> range from start to end acrosses two pages.
> right.
>
>>> +	if (protect)
>>> +		return set_pages_ro(page, nr_pages);
>>> +	else
>>> +		return set_pages_rw(page, nr_pages);
>>> +}
>>> +
>>> +static void kexec_mark_crashkres(bool protect)
>>> +{
>>> +	unsigned long control;
>>> +
>>> +	kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
>> Adding the following if test to test crashk_low_res is better. Then we
>> do not need to test if start or end is equal to 0 in kexec_mark_range.
>>
>>         if (crashk_low_res.start != crashk_low_res.end) {
>>                 kexec_mark_range(crashk_low_res.start,
>>                                 crashk_low_res.end, protect);
>>         }
> The checks in kexec_mark_range() will handle the case, it's not
> performance-critical path and will make the code less clean.
>
>>> +
>>> +	/* Don't touch the control code page used in crash_kexec().*/
>>> +	control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
>>> +	/* Control code page is located in the 2nd page. */
>>> +	kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
>>> +	kexec_mark_range(control + 2 * PAGE_SIZE, crashk_res.end, protect);
>> I think it is more readable, if we use MACRO KEXEC_CONTROL_PAGE_SIZE,
>> instead of using 2*PAGE_SIZE directly.
> OK.
>
> How about the following update:
>
> +static int
> +kexec_mark_range(unsigned long start, unsigned long end, bool protect)
> +{
> +       struct page *page;
> +       unsigned int nr_pages;
> +
> +       /* For physical range: [start, end] */
> +       if (!start || !end || start > end)
> +               return 0;
> +
> +       page = pfn_to_page(start >> PAGE_SHIFT);
> +       nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1;
> +       if (protect)
> +               return set_pages_ro(page, nr_pages);
> +       else
> +               return set_pages_rw(page, nr_pages);
> +}
> +
> +static void kexec_mark_crashkres(bool protect)
> +{
> +       unsigned long control;
> +
> +       kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
> +
> +       /* Don't touch the control code page used in crash_kexec().*/
> +       control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
> +       /* Control code page is located in the 2nd page. */
> +       kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
> +       control += KEXEC_CONTROL_PAGE_SIZE;

In fact, control code page is only 1 page, using control + 2*PAGE_SIZE is clearer.
For example, if we have more other type pages following it. Anyway this is not
that important.

Regards,
Xunlei

> +       kexec_mark_range(control, crashk_res.end, protect);
> +}
> +
> +void arch_kexec_protect_crashkres(void)
> +{
> +       kexec_mark_crashkres(true);
> +}
> +
> +void arch_kexec_unprotect_crashkres(void)
> +{
> +       kexec_mark_crashkres(false);
> +}
>
>> Thanks
>> Minfei
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303247 — Re: [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres()

On 01/07/16 at 10:14am, Xunlei Pang wrote:
> >> +static int
> >> +kexec_mark_range(unsigned long start, unsigned long end, bool protect)
> >> +{
> >> +	struct page *page;
> >> +	unsigned int nr_pages;
> >> +
> >> +	/* For physical range: [start, end] */
> >> +	if (!start || !end || start > end)
> >> +		return 0;
> > Hi, Xunlei.
> >
> >         if (start > end)
> >                 return 0;
> 
> If both start and end are zero, we want to return directly, so the two
> more check doesn't hurt.

How about if the start is equal to 0, and end is larger than 0? It is
better to make code more robust, although it never happen in currect
kexec code.

> 
> > See the below comment.
> >> +
> >> +	page = pfn_to_page(start >> PAGE_SHIFT);
> >> +	nr_pages = (end + PAGE_SIZE - start) >> PAGE_SHIFT;
> > As I commented in last version, it is better to cover the case if the
> > range from start to end acrosses two pages.
> 
> right.
> 
> >> +	if (protect)
> >> +		return set_pages_ro(page, nr_pages);
> >> +	else
> >> +		return set_pages_rw(page, nr_pages);
> >> +}
> >> +
> >> +static void kexec_mark_crashkres(bool protect)
> >> +{
> >> +	unsigned long control;
> >> +
> >> +	kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
> > Adding the following if test to test crashk_low_res is better. Then we
> > do not need to test if start or end is equal to 0 in kexec_mark_range.
> >
> >         if (crashk_low_res.start != crashk_low_res.end) {
> >                 kexec_mark_range(crashk_low_res.start,
> >                                 crashk_low_res.end, protect);
> >         }
> 
> The checks in kexec_mark_range() will handle the case, it's not
> performance-critical path and will make the code less clean.
> 
> >> +
> >> +	/* Don't touch the control code page used in crash_kexec().*/
> >> +	control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
> >> +	/* Control code page is located in the 2nd page. */
> >> +	kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
> >> +	kexec_mark_range(control + 2 * PAGE_SIZE, crashk_res.end, protect);
> > I think it is more readable, if we use MACRO KEXEC_CONTROL_PAGE_SIZE,
> > instead of using 2*PAGE_SIZE directly.
> 
> OK.
> 
> How about the following update:
> +static void kexec_mark_crashkres(bool protect)
> +{
> +       unsigned long control;
> +
> +       kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
> +
> +       /* Don't touch the control code page used in crash_kexec().*/
> +       control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
> +       /* Control code page is located in the 2nd page. */
> +       kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
> +       control += KEXEC_CONTROL_PAGE_SIZE;
> +       kexec_mark_range(control, crashk_res.end, protect);
> +}

I'm fine with this.

Thanks
Minfei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303289 — Re: [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres()

On 01/07/2016 at 10:36 AM, Minfei Huang wrote:
> On 01/07/16 at 10:14am, Xunlei Pang wrote:
>>>> +static int
>>>> +kexec_mark_range(unsigned long start, unsigned long end, bool protect)
>>>> +{
>>>> +	struct page *page;
>>>> +	unsigned int nr_pages;
>>>> +
>>>> +	/* For physical range: [start, end] */
>>>> +	if (!start || !end || start > end)
>>>> +		return 0;
>>> Hi, Xunlei.
>>>
>>>         if (start > end)
>>>                 return 0;
>> If both start and end are zero, we want to return directly, so the two
>> more check doesn't hurt.
> How about if the start is equal to 0, and end is larger than 0? It is
> better to make code more robust, although it never happen in currect
> kexec code.

Hmm, this will be better:

	if (!end || start > end)
		return 0;

it handles the common case not using crash_low_res(start and end are 0).

Regards,
Xunlei

>
>>> See the below comment.
>>>> +
>>>> +	page = pfn_to_page(start >> PAGE_SHIFT);
>>>> +	nr_pages = (end + PAGE_SIZE - start) >> PAGE_SHIFT;
>>> As I commented in last version, it is better to cover the case if the
>>> range from start to end acrosses two pages.
>> right.
>>
>>>> +	if (protect)
>>>> +		return set_pages_ro(page, nr_pages);
>>>> +	else
>>>> +		return set_pages_rw(page, nr_pages);
>>>> +}
>>>> +
>>>> +static void kexec_mark_crashkres(bool protect)
>>>> +{
>>>> +	unsigned long control;
>>>> +
>>>> +	kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
>>> Adding the following if test to test crashk_low_res is better. Then we
>>> do not need to test if start or end is equal to 0 in kexec_mark_range.
>>>
>>>         if (crashk_low_res.start != crashk_low_res.end) {
>>>                 kexec_mark_range(crashk_low_res.start,
>>>                                 crashk_low_res.end, protect);
>>>         }
>> The checks in kexec_mark_range() will handle the case, it's not
>> performance-critical path and will make the code less clean.
>>
>>>> +
>>>> +	/* Don't touch the control code page used in crash_kexec().*/
>>>> +	control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
>>>> +	/* Control code page is located in the 2nd page. */
>>>> +	kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
>>>> +	kexec_mark_range(control + 2 * PAGE_SIZE, crashk_res.end, protect);
>>> I think it is more readable, if we use MACRO KEXEC_CONTROL_PAGE_SIZE,
>>> instead of using 2*PAGE_SIZE directly.
>> OK.
>>
>> How about the following update:
>> +static void kexec_mark_crashkres(bool protect)
>> +{
>> +       unsigned long control;
>> +
>> +       kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
>> +
>> +       /* Don't touch the control code page used in crash_kexec().*/
>> +       control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
>> +       /* Control code page is located in the 2nd page. */
>> +       kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
>> +       control += KEXEC_CONTROL_PAGE_SIZE;
>> +       kexec_mark_range(control, crashk_res.end, protect);
>> +}
> I'm fine with this.
>
> Thanks
> Minfei
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303415 — Re: [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres()

On Thu, 7 Jan 2016 13:08:21 +0800
Xunlei Pang <xlpang@redhat.com> wrote:

> On 01/07/2016 at 10:36 AM, Minfei Huang wrote:
> > On 01/07/16 at 10:14am, Xunlei Pang wrote:
> >>>> +static int
> >>>> +kexec_mark_range(unsigned long start, unsigned long end, bool protect)
> >>>> +{
> >>>> +	struct page *page;
> >>>> +	unsigned int nr_pages;
> >>>> +
> >>>> +	/* For physical range: [start, end] */
> >>>> +	if (!start || !end || start > end)
> >>>> +		return 0;
> >>> Hi, Xunlei.
> >>>
> >>>         if (start > end)
> >>>                 return 0;
> >> If both start and end are zero, we want to return directly, so the two
> >> more check doesn't hurt.
> > How about if the start is equal to 0, and end is larger than 0? It is
> > better to make code more robust, although it never happen in currect
> > kexec code.
> 
> Hmm, this will be better:
> 
> 	if (!end || start > end)
> 		return 0;
> 
> it handles the common case not using crash_low_res(start and end are 0).

Hm, if both start and end are 0, then what about using this condition:

 	if (start >= end)
 		return 0;

I think it's good enough, because if start is equal to end, then
there's nothing to protect anyway.

Regards,
Petr Tesarik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303487 — Re: [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres()

On 01/07/2016 at 05:20 PM, Petr Tesarik wrote:
> On Thu, 7 Jan 2016 13:08:21 +0800
> Xunlei Pang <xlpang@redhat.com> wrote:
>
>> On 01/07/2016 at 10:36 AM, Minfei Huang wrote:
>>> On 01/07/16 at 10:14am, Xunlei Pang wrote:
>>>>>> +static int
>>>>>> +kexec_mark_range(unsigned long start, unsigned long end, bool protect)
>>>>>> +{
>>>>>> +	struct page *page;
>>>>>> +	unsigned int nr_pages;
>>>>>> +
>>>>>> +	/* For physical range: [start, end] */
>>>>>> +	if (!start || !end || start > end)
>>>>>> +		return 0;
>>>>> Hi, Xunlei.
>>>>>
>>>>>         if (start > end)
>>>>>                 return 0;
>>>> If both start and end are zero, we want to return directly, so the two
>>>> more check doesn't hurt.
>>> How about if the start is equal to 0, and end is larger than 0? It is
>>> better to make code more robust, although it never happen in currect
>>> kexec code.
>> Hmm, this will be better:
>>
>> 	if (!end || start > end)
>> 		return 0;
>>
>> it handles the common case not using crash_low_res(start and end are 0).
> Hm, if both start and end are 0, then what about using this condition:
>
>  	if (start >= end)
>  		return 0;
>
> I think it's good enough, because if start is equal to end, then
> there's nothing to protect anyway.

In theory, start==end(not 0) still means we have 1B to protect :-)
But in practice there are no such cases, so I think this is ok.

Regards,
Xunlei

>
> Regards,
> Petr Tesarik
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web