Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uffd: Fix page fault address #2270

Merged
merged 1 commit into from
Sep 28, 2023
Merged

Conversation

yota9
Copy link
Contributor

@yota9 yota9 commented Sep 27, 2023

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
Copy link
Member

@rppt PTAL

@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

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

❗ 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
@@             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 Coverage Δ
criu/uffd.c 78.41% <ø> (+5.84%) ⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@minhbq-99
Copy link
Member

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
Copy link
Member

rppt commented Sep 27, 2023

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
Copy link
Contributor Author

yota9 commented Sep 27, 2023

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
Copy link
Member

rppt commented Sep 28, 2023

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 :)

Currently page_size() returns unsigned int value that is after "bitwise
not" is promoted to unsigned long value e.g. in uffd.c
handle_page_fault. Since the value is unsigned promotion is done with 0
MSB that results in lost of MSB pagefault address bits. So make
page_size to return  unsigned long to avoid such situation.

Signed-off-by: Vladislav Khmelevsky <och95@yandex.ru>
@yota9
Copy link
Contributor Author

yota9 commented Sep 28, 2023

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
Copy link
Contributor Author

yota9 commented Sep 28, 2023

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

@avagin avagin merged commit 1e4f5fb into checkpoint-restore:criu-dev Sep 28, 2023
33 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants