Re: [Crash-utility] [PATCH] diskdump: fix read_diskdump() returns successfully for illegal 0-size page descriptors
1:22 a.m.
Thank you for the patch, HATAYAMA.
On Thu, Sep 7, 2023 at 6:55 PM <crash-utility-request(a)redhat.com> wrote:
Date: Thu, 7 Sep 2023 18:38:25 +0900
From: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
To: crash-utility(a)redhat.com
Subject: [Crash-utility] [PATCH] diskdump: fix read_diskdump() returns
successfully for illegal 0-size page descriptors
Message-ID: <20230907093825.515-1-d.hatayama(a)fujitsu.com>
Content-Type: text/plain; charset="US-ASCII"; x-default=true
read_diskdump() returns successfully for illegal 0-size page
descriptors. Page descriptors are illegal if their size member holds 0
because makedumpfile never puts 0 there because any data never result
in 0 byte by compression. If page descriptors hold 0 in size member,
it means the crash dump file is corrupted for some reason.
The root cause of this is that sanity check of function cache_page()
doesn't focus on such 0-size page descriptors. Then, the 0-size page
descriptor is passed to pread(), pread() immediately returns 0
successfully because read data is 0 byte, and then read_diskdump()
returns successfully.
To fix this issue, let the sanity check take into account such 0-size
page descriptors and read_diskdump() result in READ_ERROR.
Signed-off-by: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
---
diskdump.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/diskdump.c b/diskdump.c
index 2c284ff..2be7cc7 100644
--- a/diskdump.c
+++ b/diskdump.c
@@ -1210,7 +1210,7 @@ cache_page(physaddr_t paddr)
return ret;
/* sanity check */
- if (pd.size > block_size)
+ if (pd.size > block_size || !pd.size)
Actually this may not be a real read error instead of an incomplete page
due to corruption or other reasons. Given that, I would suggest adding the
sanity check as below:
- } else if (0 == pd.offset) {
+ } else if (0 == pd.offset || !pd.size) {
It can help to print more information according to the code when fails on
the page:
/*
* First check whether zero_excluded has been set.
*/
if (*diskdump_flags & ZERO_EXCLUDED) {
if (CRASHDEBUG(8))
fprintf(fp,
"read_diskdump/cache_page: zero-fill: "
"paddr/pfn: %llx/%lx\n",
(ulonglong)paddr, pfn);
memset(dd->compressed_page, 0, dd->block_size);
} else {
if (CRASHDEBUG(8))
fprintf(fp,
"read_diskdump/cache_page: "
"descriptor with zero offset found
at "
"paddr/pfn/pos: %llx/%lx/%lx\n",
(ulonglong)paddr, pfn, desc_pos);
return PAGE_INCOMPLETE;
}
I don't have such vmcores, and can not test it, just an idea.
Thanks.
Lianbo
return READ_ERROR;
/* read page data */
--
2.25.1
3:43 a.m.
New subject: [PATCH] diskdump: fix read_diskdump() returns successfully for illegal 0-size page descriptors
Hi Hatayama-san,
sorry for the delay, exceptionally busy last month..
On 2023/09/19 15:22, lijiang wrote:
> read_diskdump() returns successfully for illegal 0-size page
> descriptors. Page descriptors are illegal if their size member holds 0
> because makedumpfile never puts 0 there because any data never result
> in 0 byte by compression. If page descriptors hold 0 in size member,
> it means the crash dump file is corrupted for some reason.
>
> The root cause of this is that sanity check of function cache_page()
> doesn't focus on such 0-size page descriptors. Then, the 0-size page
> descriptor is passed to pread(), pread() immediately returns 0
> successfully because read data is 0 byte, and then read_diskdump()
> returns successfully.
>
> To fix this issue, let the sanity check take into account such 0-size
> page descriptors and read_diskdump() result in READ_ERROR.
>
> Signed-off-by: HATAYAMA Daisuke <d.hatayama(a)fujitsu.com>
> ---
> diskdump.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/diskdump.c b/diskdump.c
> index 2c284ff..2be7cc7 100644
> --- a/diskdump.c
> +++ b/diskdump.c
> @@ -1210,7 +1210,7 @@ cache_page(physaddr_t paddr)
> return ret;
>
> /* sanity check */
> - if (pd.size > block_size)
> + if (pd.size > block_size || !pd.size)
>
Actually this may not be a real read error instead of an incomplete page
due to corruption or other reasons. Given that, I would suggest adding the
sanity check as below:
- } else if (0 == pd.offset) {
+ } else if (0 == pd.offset || !pd.size) {
Agree to Lianbo here.
I think this might be a bit better, because makedumpfile may write zeros
to page descriptor when ENOSPC and it looks like this pd.offset == 0
check already implies that pd.size is also zero.
It can help to print more information according to the code when fails on
the page:
/*
* First check whether zero_excluded has been set.
*/
if (*diskdump_flags & ZERO_EXCLUDED) {
if (CRASHDEBUG(8))
fprintf(fp,
"read_diskdump/cache_page: zero-fill: "
"paddr/pfn: %llx/%lx\n",
(ulonglong)paddr, pfn);
memset(dd->compressed_page, 0, dd->block_size);
} else {
if (CRASHDEBUG(8))
fprintf(fp,
"read_diskdump/cache_page: "
"descriptor with zero offset found
at "
So I suggest
- "descriptor with zero offset found at "
+ "descriptor with zero offset or size found at "
with the above.
Thanks,
Kazu
> "paddr/pfn/pos: %llx/%lx/%lx\n",
> (ulonglong)paddr, pfn, desc_pos);
> return PAGE_INCOMPLETE;
> }
>
> I don't have such vmcores, and can not test it, just an idea.
>
> Thanks.
> Lianbo
>
> return READ_ERROR;
>>
>> /* read page data */
>> --
>> 2.25.1
>>
>>
349
days inactive
363
days old
devel@lists.crash-utility.osci.io
1 comments
2 participants
participants (2)
-
HAGIO KAZUHITO(萩尾 一仁) -
lijiang