* [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
@ 2014-04-14 10:35 Jingbai Ma
2014-04-15 1:15 ` HATAYAMA Daisuke
0 siblings, 1 reply; 5+ messages in thread
From: Jingbai Ma @ 2014-04-14 10:35 UTC (permalink / raw)
To: kumagai-atsushi; +Cc: d.hatayama, kexec, lisa.mitchell
This patch intend to fix a segmentation fault when pfn exceeds 2G boundary.
In function is_on(), if the pfn (i) is greater than 2G, it will be a negative
value and will cause a segmentation fault.
is_on(char *bitmap, int i)
{
return bitmap[i>>3] & (1 << (i & 7));
}
Signed-off-by: Jingbai Ma <jingbai.ma@hp.com>
---
makedumpfile.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/makedumpfile.h b/makedumpfile.h
index 3d270c6..03d35a8 100644
--- a/makedumpfile.h
+++ b/makedumpfile.h
@@ -1591,7 +1591,7 @@ int get_xen_info_ia64(void);
#endif /* s390x */
static inline int
-is_on(char *bitmap, int i)
+is_on(char *bitmap, unsigned long long i)
{
return bitmap[i>>3] & (1 << (i & 7));
}
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
2014-04-14 10:35 [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary Jingbai Ma
@ 2014-04-15 1:15 ` HATAYAMA Daisuke
2014-04-15 8:07 ` HATAYAMA Daisuke
0 siblings, 1 reply; 5+ messages in thread
From: HATAYAMA Daisuke @ 2014-04-15 1:15 UTC (permalink / raw)
To: jingbai.ma; +Cc: kexec, kumagai-atsushi, lisa.mitchell
From: Jingbai Ma <jingbai.ma@hp.com>
Subject: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
Date: Mon, 14 Apr 2014 18:35:44 +0800
> This patch intend to fix a segmentation fault when pfn exceeds 2G boundary.
>
> In function is_on(), if the pfn (i) is greater than 2G, it will be a negative
> value and will cause a segmentation fault.
> is_on(char *bitmap, int i)
> {
> return bitmap[i>>3] & (1 << (i & 7));
> }
>
>
> Signed-off-by: Jingbai Ma <jingbai.ma@hp.com>
> ---
> makedumpfile.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/makedumpfile.h b/makedumpfile.h
> index 3d270c6..03d35a8 100644
> --- a/makedumpfile.h
> +++ b/makedumpfile.h
> @@ -1591,7 +1591,7 @@ int get_xen_info_ia64(void);
> #endif /* s390x */
>
> static inline int
> -is_on(char *bitmap, int i)
> +is_on(char *bitmap, unsigned long long i)
> {
> return bitmap[i>>3] & (1 << (i & 7));
> }
>
is_on is used at the following two places.
static inline int
is_dumpable(struct dump_bitmap *bitmap, unsigned long long pfn)
{
off_t offset;
if (pfn == 0 || bitmap->no_block != pfn/PFN_BUFBITMAP) {
offset = bitmap->offset + BUFSIZE_BITMAP*(pfn/PFN_BUFBITMAP);
lseek(bitmap->fd, offset, SEEK_SET);
read(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP);
if (pfn == 0)
bitmap->no_block = 0;
else
bitmap->no_block = pfn/PFN_BUFBITMAP;
}
return is_on(bitmap->buf, pfn%PFN_BUFBITMAP);
}
PFN_BUFBTIMAP is constant 32 K. So, it seems that the 4 byte byte
length came here.
But right shift to signed integer is implementation defined. We should
not use right shift to signed integer. it looks gcc performs
arithmetic shift and this bahaviour is buggy in case of is_on().
static inline int
is_dumpable_cyclic(char *bitmap, unsigned long long pfn, struct cycle *cycle)
{
if (pfn < cycle->start_pfn || cycle->end_pfn <= pfn)
return FALSE;
else
return is_on(bitmap, pfn - cycle->start_pfn);
}
Simply, (pfn - cycle->start_pfn) could be (info->max_mapnr - 0). It's
possible to pass more than 2 Gi by using system with more than 8 TiB
physical memory space.
So,
- i must be unsigned in order to make right shift operation
meaningful, and
- i must have 8 byte for systems with more than 8 TiB physical memory
space.
BTW, to make this situation happen in cyclic mode, there needs to be
16 GiB free memory for bitmap to become large enough. I guess you
don't see this segmentation in the kdump 2nd kernel. Is this right?
Thanks.
HATAYAMA, Daisuke
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
2014-04-15 1:15 ` HATAYAMA Daisuke
@ 2014-04-15 8:07 ` HATAYAMA Daisuke
2014-04-15 10:32 ` Jingbai Ma
0 siblings, 1 reply; 5+ messages in thread
From: HATAYAMA Daisuke @ 2014-04-15 8:07 UTC (permalink / raw)
To: jingbai.ma; +Cc: kumagai-atsushi, kexec, lisa.mitchell
From: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
Subject: Re: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
Date: Tue, 15 Apr 2014 10:15:21 +0900
> From: Jingbai Ma <jingbai.ma@hp.com>
> Subject: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
> Date: Mon, 14 Apr 2014 18:35:44 +0800
>
>> This patch intend to fix a segmentation fault when pfn exceeds 2G boundary.
>>
>> In function is_on(), if the pfn (i) is greater than 2G, it will be a negative
>> value and will cause a segmentation fault.
>> is_on(char *bitmap, int i)
>> {
>> return bitmap[i>>3] & (1 << (i & 7));
>> }
>>
>>
>> Signed-off-by: Jingbai Ma <jingbai.ma@hp.com>
>> ---
>> makedumpfile.h | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/makedumpfile.h b/makedumpfile.h
>> index 3d270c6..03d35a8 100644
>> --- a/makedumpfile.h
>> +++ b/makedumpfile.h
>> @@ -1591,7 +1591,7 @@ int get_xen_info_ia64(void);
>> #endif /* s390x */
>>
>> static inline int
>> -is_on(char *bitmap, int i)
>> +is_on(char *bitmap, unsigned long long i)
>> {
>> return bitmap[i>>3] & (1 << (i & 7));
>> }
>>
>
> is_on is used at the following two places.
>
> static inline int
> is_dumpable(struct dump_bitmap *bitmap, unsigned long long pfn)
> {
> off_t offset;
> if (pfn == 0 || bitmap->no_block != pfn/PFN_BUFBITMAP) {
> offset = bitmap->offset + BUFSIZE_BITMAP*(pfn/PFN_BUFBITMAP);
> lseek(bitmap->fd, offset, SEEK_SET);
> read(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP);
> if (pfn == 0)
> bitmap->no_block = 0;
> else
> bitmap->no_block = pfn/PFN_BUFBITMAP;
> }
> return is_on(bitmap->buf, pfn%PFN_BUFBITMAP);
> }
>
> PFN_BUFBTIMAP is constant 32 K. So, it seems that the 4 byte byte
> length came here.
>
> But right shift to signed integer is implementation defined. We should
> not use right shift to signed integer. it looks gcc performs
> arithmetic shift and this bahaviour is buggy in case of is_on().
>
> static inline int
> is_dumpable_cyclic(char *bitmap, unsigned long long pfn, struct cycle *cycle)
> {
> if (pfn < cycle->start_pfn || cycle->end_pfn <= pfn)
> return FALSE;
> else
> return is_on(bitmap, pfn - cycle->start_pfn);
> }
>
> Simply, (pfn - cycle->start_pfn) could be (info->max_mapnr - 0). It's
> possible to pass more than 2 Gi by using system with more than 8 TiB
> physical memory space.
>
> So,
>
> - i must be unsigned in order to make right shift operation
> meaningful, and
>
> - i must have 8 byte for systems with more than 8 TiB physical memory
> space.
>
> BTW, to make this situation happen in cyclic mode, there needs to be
> 16 GiB free memory for bitmap to become large enough. I guess you
> don't see this segmentation in the kdump 2nd kernel. Is this right?
>
This is my terrible careless miss... 256 MiB is correct. 16 GiB is
obviously too large. Then, the amount seems typical.
Thanks.
HATAYAMA, Daisuke
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
2014-04-15 8:07 ` HATAYAMA Daisuke
@ 2014-04-15 10:32 ` Jingbai Ma
2014-04-17 4:53 ` Atsushi Kumagai
0 siblings, 1 reply; 5+ messages in thread
From: Jingbai Ma @ 2014-04-15 10:32 UTC (permalink / raw)
To: HATAYAMA Daisuke; +Cc: kumagai-atsushi, kexec, lisa.mitchell, jingbai.ma
On 04/15/2014 04:07 PM, HATAYAMA Daisuke wrote:
> From: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
> Subject: Re: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
> Date: Tue, 15 Apr 2014 10:15:21 +0900
>
>> From: Jingbai Ma <jingbai.ma@hp.com>
>> Subject: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
>> Date: Mon, 14 Apr 2014 18:35:44 +0800
>>
>>> This patch intend to fix a segmentation fault when pfn exceeds 2G boundary.
>>>
>>> In function is_on(), if the pfn (i) is greater than 2G, it will be a negative
>>> value and will cause a segmentation fault.
>>> is_on(char *bitmap, int i)
>>> {
>>> return bitmap[i>>3] & (1 << (i & 7));
>>> }
>>>
>>>
>>> Signed-off-by: Jingbai Ma <jingbai.ma@hp.com>
>>> ---
>>> makedumpfile.h | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/makedumpfile.h b/makedumpfile.h
>>> index 3d270c6..03d35a8 100644
>>> --- a/makedumpfile.h
>>> +++ b/makedumpfile.h
>>> @@ -1591,7 +1591,7 @@ int get_xen_info_ia64(void);
>>> #endif /* s390x */
>>>
>>> static inline int
>>> -is_on(char *bitmap, int i)
>>> +is_on(char *bitmap, unsigned long long i)
>>> {
>>> return bitmap[i>>3] & (1 << (i & 7));
>>> }
>>>
>>
>> is_on is used at the following two places.
>>
>> static inline int
>> is_dumpable(struct dump_bitmap *bitmap, unsigned long long pfn)
>> {
>> off_t offset;
>> if (pfn == 0 || bitmap->no_block != pfn/PFN_BUFBITMAP) {
>> offset = bitmap->offset + BUFSIZE_BITMAP*(pfn/PFN_BUFBITMAP);
>> lseek(bitmap->fd, offset, SEEK_SET);
>> read(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP);
>> if (pfn == 0)
>> bitmap->no_block = 0;
>> else
>> bitmap->no_block = pfn/PFN_BUFBITMAP;
>> }
>> return is_on(bitmap->buf, pfn%PFN_BUFBITMAP);
>> }
>>
>> PFN_BUFBTIMAP is constant 32 K. So, it seems that the 4 byte byte
>> length came here.
>>
>> But right shift to signed integer is implementation defined. We should
>> not use right shift to signed integer. it looks gcc performs
>> arithmetic shift and this bahaviour is buggy in case of is_on().
>>
>> static inline int
>> is_dumpable_cyclic(char *bitmap, unsigned long long pfn, struct cycle *cycle)
>> {
>> if (pfn < cycle->start_pfn || cycle->end_pfn <= pfn)
>> return FALSE;
>> else
>> return is_on(bitmap, pfn - cycle->start_pfn);
>> }
>>
>> Simply, (pfn - cycle->start_pfn) could be (info->max_mapnr - 0). It's
>> possible to pass more than 2 Gi by using system with more than 8 TiB
>> physical memory space.
>>
>> So,
>>
>> - i must be unsigned in order to make right shift operation
>> meaningful, and
>>
>> - i must have 8 byte for systems with more than 8 TiB physical memory
>> space.
>>
Yes, all your comments are correct. :)
So would you like I send out a v2 patch with all your comments in it?
>> BTW, to make this situation happen in cyclic mode, there needs to be
>> 16 GiB free memory for bitmap to become large enough. I guess you
>> don't see this segmentation in the kdump 2nd kernel. Is this right?
>>
>
> This is my terrible careless miss... 256 MiB is correct. 16 GiB is
> obviously too large. Then, the amount seems typical.
>
Correct, 2G >> 3 = 256M.
> Thanks.
> HATAYAMA, Daisuke
>
--
Thanks,
Jingbai Ma
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary
2014-04-15 10:32 ` Jingbai Ma
@ 2014-04-17 4:53 ` Atsushi Kumagai
0 siblings, 0 replies; 5+ messages in thread
From: Atsushi Kumagai @ 2014-04-17 4:53 UTC (permalink / raw)
To: jingbai.ma@hp.com, d.hatayama@jp.fujitsu.com
Cc: kexec@lists.infradead.org, lisa.mitchell@hp.com
>>>> This patch intend to fix a segmentation fault when pfn exceeds 2G boundary.
>>>>
>>>> In function is_on(), if the pfn (i) is greater than 2G, it will be a negative
>>>> value and will cause a segmentation fault.
>>>> is_on(char *bitmap, int i)
>>>> {
>>>> return bitmap[i>>3] & (1 << (i & 7));
>>>> }
>>>>
>>>>
>>>> Signed-off-by: Jingbai Ma <jingbai.ma@hp.com>
>>>> ---
>>>> makedumpfile.h | 2 +-
>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/makedumpfile.h b/makedumpfile.h
>>>> index 3d270c6..03d35a8 100644
>>>> --- a/makedumpfile.h
>>>> +++ b/makedumpfile.h
>>>> @@ -1591,7 +1591,7 @@ int get_xen_info_ia64(void);
>>>> #endif /* s390x */
>>>>
>>>> static inline int
>>>> -is_on(char *bitmap, int i)
>>>> +is_on(char *bitmap, unsigned long long i)
>>>> {
>>>> return bitmap[i>>3] & (1 << (i & 7));
>>>> }
>>>>
>>>
>>> is_on is used at the following two places.
>>>
>>> static inline int
>>> is_dumpable(struct dump_bitmap *bitmap, unsigned long long pfn)
>>> {
>>> off_t offset;
>>> if (pfn == 0 || bitmap->no_block != pfn/PFN_BUFBITMAP) {
>>> offset = bitmap->offset + BUFSIZE_BITMAP*(pfn/PFN_BUFBITMAP);
>>> lseek(bitmap->fd, offset, SEEK_SET);
>>> read(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP);
>>> if (pfn == 0)
>>> bitmap->no_block = 0;
>>> else
>>> bitmap->no_block = pfn/PFN_BUFBITMAP;
>>> }
>>> return is_on(bitmap->buf, pfn%PFN_BUFBITMAP);
>>> }
>>>
>>> PFN_BUFBTIMAP is constant 32 K. So, it seems that the 4 byte byte
>>> length came here.
>>>
>>> But right shift to signed integer is implementation defined. We should
>>> not use right shift to signed integer. it looks gcc performs
>>> arithmetic shift and this bahaviour is buggy in case of is_on().
>>>
>>> static inline int
>>> is_dumpable_cyclic(char *bitmap, unsigned long long pfn, struct cycle *cycle)
>>> {
>>> if (pfn < cycle->start_pfn || cycle->end_pfn <= pfn)
>>> return FALSE;
>>> else
>>> return is_on(bitmap, pfn - cycle->start_pfn);
>>> }
>>>
>>> Simply, (pfn - cycle->start_pfn) could be (info->max_mapnr - 0). It's
>>> possible to pass more than 2 Gi by using system with more than 8 TiB
>>> physical memory space.
>>>
>>> So,
>>>
>>> - i must be unsigned in order to make right shift operation
>>> meaningful, and
>>>
>>> - i must have 8 byte for systems with more than 8 TiB physical memory
>>> space.
>>>
>
>Yes, all your comments are correct. :)
>So would you like I send out a v2 patch with all your comments in it?
I'd like to merge this patch into v1.5.6 since it's a critical issue,
so could you send the v2 patch?
Thanks
Atsushi Kumagai
>>> BTW, to make this situation happen in cyclic mode, there needs to be
>>> 16 GiB free memory for bitmap to become large enough. I guess you
>>> don't see this segmentation in the kdump 2nd kernel. Is this right?
>>>
>>
>> This is my terrible careless miss... 256 MiB is correct. 16 GiB is
>> obviously too large. Then, the amount seems typical.
>>
>
>Correct, 2G >> 3 = 256M.
>
>
>> Thanks.
>> HATAYAMA, Daisuke
>>
>
>
>--
>Thanks,
>Jingbai Ma
>
>_______________________________________________
>kexec mailing list
>kexec@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/kexec
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-04-17 4:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-14 10:35 [PATCH] makedumpfile: fix a segmentation fault when pfn exceeds 2G boundary Jingbai Ma
2014-04-15 1:15 ` HATAYAMA Daisuke
2014-04-15 8:07 ` HATAYAMA Daisuke
2014-04-15 10:32 ` Jingbai Ma
2014-04-17 4:53 ` Atsushi Kumagai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox