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: fixed for exiting exiting program as user #1483

Merged
merged 1 commit into from
May 30, 2021
Merged

uffd: fixed for exiting exiting program as user #1483

merged 1 commit into from
May 30, 2021

Conversation

nithin-jaikar
Copy link
Contributor

While Dumping the process, it fails and exits as a user because of kerndat_uffd() returns -1 and due permission error
uffd = syscall(SYS_userfaultfd, flags); this function fails inturn kerndat_uffd() fails.
In the change it ignores the permission error and proceeds further.

@rppt rppt requested a review from xemul May 21, 2021 09:31
Copy link
Member

@xemul xemul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nithin-jaikar rework the PR, leave only the EPERM checking of uffd opening and write a proper patch comment.

criu/uffd.c Outdated Show resolved Hide resolved
@xemul
Copy link
Member

xemul commented May 21, 2021

@nithin-jaikar , don't add 5th commit, please. Remove all commits and leave only one.

Copy link
Member

@xemul xemul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch (not PR, but the very commit) must have a descriptive comment

criu/uffd.c Outdated Show resolved Hide resolved
criu/kerndat.c Outdated Show resolved Hide resolved
criu/kerndat.c Outdated Show resolved Hide resolved
@xemul
Copy link
Member

xemul commented May 21, 2021

@nithin-jaikar , you've somehow pulled in the Adrian's patch, then merged yours with his and then added the fix I requested as another commit. Please, be more attentive and make a proper PR.

@xemul
Copy link
Member

xemul commented May 21, 2021

Ok, @nithin-jaikar , now let's add the comment to the commit (the text that accompanies the pull-request can be just copied)

criu/kerndat.c Outdated Show resolved Hide resolved
@adrianreber
Copy link
Member

From the PR description it is not clear to me what this tries to solve. Some more details how this could happen would be interesting.

@xemul
Copy link
Member

xemul commented May 21, 2021

@adrianreber , this is the part of "dump shared memory as user" task.

The "criu dump" used to work from non-root some time ago, but then this mode testing stopped and the functionality got broken. On of the found problems is that non-root is not allowed to open uffd, so criu cannot even check the kerndat stuff.

@adrianreber
Copy link
Member

Ah, thanks. I saw similar errors when doing the non-root changes in #1155

Anyway, that information would be good to have in the commit and/or PR.

@xemul
Copy link
Member

xemul commented May 21, 2021

Whee, comment commit message is mastered!
@nithin-jaikar , now please read https://criu.org/How_to_submit_patches and make the commit message look the way it must. E.g. like this

kerndat: Handle non-root mode when checking uffd

When criu is run as user it fails and exits because of kerndat_uffd() returning -1.
This, in turn, happens after uffd = syscall(SYS_userfaultfd, flags); which only works
for root.

In the change it ignores the permission error and proceeds further just like it's done
for e.g. pagemap checking.

Signed-off-by: Nithin Jaikar J <[email protected]>

When criu is run as user it fails and exits because of kerndat_uffd() returning -1.
This, in turn, happens after uffd = syscall(SYS_userfaultfd, flags); which only works
for root.

In the change it ignores the permission error and proceeds further just like it's done
for e.g. pagemap checking.

Signed-off-by: Nithin Jaikar J <[email protected]>
Copy link
Member

@rst0git rst0git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@Snorch Snorch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#define EPERM 1

Should not we differentiate errno == EPERM from syscall SYS_userfaultfd and later errors in uffd_open() ?

@avagin avagin merged commit cfdeac4 into checkpoint-restore:criu-dev May 30, 2021
@Snorch
Copy link
Member

Snorch commented May 31, 2021

I still think that my objection is valid, so #1493

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.

6 participants