[PATCH v2] makedumpfile: fix a segmentation fault when physical address exceeds 8TB boundary

[Petr Tesarik]

On Thu, 17 Apr 2014 15:59:02 +0800
Jingbai Ma <jingbai.ma at hp.com> wrote:

> This patch intends to fix a segmentation fault when physical address exceeds
> 8TB boundary.
> 
> Changelog:
> v2:
> - Add more comments from Daisuke HATAYAMA.
> 
> 
> In function is_on(), if the physical address higher than 8T, pfn (i) will
> 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));
> }
> 
> Daisuke's detailed analysis:
> 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, in function is_on()
> 
> - 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.

While the patch is correct, the explanation is not precise, because it
does not explain why an unsigned long is not sufficient. In fact, it
would be enough on 64-bit systems, where an unsigned long is just as big
as an unsigned long long (both 64 bits). But on 32-bit systems, an
unsigned long is 32-bit, but physical memory can be larger (e.g. 36
bits with PAE).

Petr T