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@xxxxxxxxxxx>
>> ---
>> 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
>>
>>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
