checkpoint-restore / criu

Checkpoint/Restore tool
criu.org
Other
2.79k stars 565 forks source link

uffd: Fix page fault address #2270

Closed yota9 closed 10 months ago

yota9 commented 10 months ago

The page_size() returns unsigned int value that is after "bitwise not" is promoted to unsigned long (msg->arg.pagefault.address) value. Sinc e the value is unsigned promotion is done with 0 MSB that results in lost of MSB pagefault address bits. Cast page_size to unsigned long first to avoid such situation.

adrianreber commented 10 months ago

@rppt PTAL

codecov-commenter commented 10 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (5e37ccf) 70.43% compared to head (75a7da0) 70.53%.

:exclamation: Current head 75a7da0 differs from pull request most recent head 5a31b86. Consider uploading reports for the commit 5a31b86 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## criu-dev #2270 +/- ## ============================================ + Coverage 70.43% 70.53% +0.10% ============================================ Files 133 132 -1 Lines 33518 33507 -11 ============================================ + Hits 23607 23634 +27 + Misses 9911 9873 -38 ``` | [Files](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2270?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore) | Coverage Δ | | |---|---|---| | [criu/uffd.c](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2270?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore#diff-Y3JpdS91ZmZkLmM=) | `78.41% <ø> (+5.84%)` | :arrow_up: | ... and [6 files with indirect coverage changes](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2270/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

minhbq-99 commented 10 months ago

It's a nice catch. page_size in defined in criu/include/common/arch/*/asm/page.h. Interestingly, on x86, s390, arm, it returns unsigned long which does not cause the above issue. However, on aarch64, mips, ppc64, it returns unsigned int. I wonder if changing the page_size to return unsigned long in all archs is a good solution.

rppt commented 10 months ago

It's a nice catch. page_size in defined in criu/include/common/arch/*/asm/page.h. Interestingly, on x86, s390, arm, it returns unsigned long which does not cause the above issue. However, on aarch64, mips, ppc64, it returns unsigned int. I wonder if changing the page_size to return unsigned long in all archs is a good solution.

I think it is. Adding a cast in uffd.c is more of a band aid and making page_size() return unsigned long is the proper solution IMO.

yota9 commented 10 months ago

It's a nice catch. page_size in defined in criu/include/common/arch/*/asm/page.h. Interestingly, on x86, s390, arm, it returns unsigned long which does not cause the above issue. However, on aarch64, mips, ppc64, it returns unsigned int. I wonder if changing the page_size to return unsigned long in all archs is a good solution.

Thanks for further investigations, I've made page_size() to return unsigned long in all the arches and couple of other places.

rppt commented 10 months ago

It's a nice catch. page_size in defined in criu/include/common/arch/*/asm/page.h. Interestingly, on x86, s390, arm, it returns unsigned long which does not cause the above issue. However, on aarch64, mips, ppc64, it returns unsigned int. I wonder if changing the page_size to return unsigned long in all archs is a good solution.

Thanks for further investigations, I've made page_size() to return unsigned long in all the arches and couple of other places.

It looks like you've missed loongarch64 :)

yota9 commented 10 months ago

True, done. Thanks!

It's a nice catch. page_size in defined in criu/include/common/arch/*/asm/page.h. Interestingly, on x86, s390, arm, it returns unsigned long which does not cause the above issue. However, on aarch64, mips, ppc64, it returns unsigned int. I wonder if changing the page_size to return unsigned long in all archs is a good solution.

Thanks for further investigations, I've made page_size() to return unsigned long in all the arches and couple of other places.

It looks like you've missed loongarch64 :)

yota9 commented 10 months ago

I'm not authorized to merge this pull request, so please do it for me, thanks!