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

tty: skip ioctl(TIOCSLCKTRMIOS) if possible #2311

Conversation

mihalicyn
Copy link
Member

@mihalicyn mihalicyn commented Dec 8, 2023

If ioctl(TIOCSLCKTRMIOS) fails with EPERM it means that a CRIU process lacks of CAP_SYS_ADMIN capability. But we can use ioctl(TIOCGLCKTRMIOS) to read current ->termios_locked value from the kernel and if it's the same as we already have we can skip failing ioctl(TIOCSLCKTRMIOS) safely.

Adrian has recently posted [1] a very good patch to allow ioctl(TIOCSLCKTRMIOS) for processes that have CAP_CHECKPOINT_RESTORE (right now it requires CAP_SYS_ADMIN).

[1] https://lore.kernel.org/all/20231206134340.7093-1-areber@redhat.com/

Suggested-by: Andrei Vagin

If ioctl(TIOCSLCKTRMIOS) fails with EPERM it means that a CRIU
process lacks of CAP_SYS_ADMIN capability. But we can use
ioctl(TIOCGLCKTRMIOS) to *read* current ->termios_locked
value from the kernel and if it's the same as we already have
we can skip failing ioctl(TIOCSLCKTRMIOS) safely.

Adrian has recently posted [1] a very good patch to allow ioctl(TIOCSLCKTRMIOS)
for processes that have CAP_CHECKPOINT_RESTORE (right now it requires CAP_SYS_ADMIN).

[1] https://lore.kernel.org/all/20231206134340.7093-1-areber@redhat.com/

Suggested-by: Andrei Vagin <avagin@gmail.com>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
@mihalicyn
Copy link
Member Author

mihalicyn commented Dec 8, 2023

I have tested this with the example from our wiki (https://criu.org/Simple_loop#A_shell_job):

echo 0 > /proc/sys/kernel/yama/ptrace_scope
setcap cap_checkpoint_restore+eip ./criu/criu

./criu/criu dump -vvvv -o dump.log -t 184826 --shell-job --unprivileged
./criu/criu restore -vvvv -o restore.log --shell-job --unprivileged

And it works perfectly fine for this case.

@codecov-commenter
Copy link

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (0da1ab2) 70.86% compared to head (14d9b45) 70.85%.

❗ Current head 14d9b45 differs from pull request most recent head 6448805. Consider uploading reports for the commit 6448805 to get more accurate results

Files Patch % Lines
criu/tty.c 10.00% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           criu-dev    #2311      +/-   ##
============================================
- Coverage     70.86%   70.85%   -0.02%     
============================================
  Files           133      133              
  Lines         32725    32733       +8     
============================================
+ Hits          23191    23192       +1     
- Misses         9534     9541       +7     

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

@avagin
Copy link
Member

avagin commented Dec 8, 2023

lgtm. Thanks.

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

@avagin avagin merged commit dc49eb4 into checkpoint-restore:criu-dev Dec 11, 2023
34 of 39 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.

None yet

4 participants