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.

Sign up for GitHub

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

Jump to bottom

tty: Only store the stdin fd when it corresponds to a tty #1455

Open
wants to merge 128 commits into
base: criu-dev
Choose a base branch
from
Open

tty: Only store the stdin fd when it corresponds to a tty #1455

wants to merge 128 commits into from

Conversation

nviennot
Copy link
Member

Doing otherwise can lead to problems when using inherit-fd on stdin as
it gets closed.

rst0git
rst0git reviewed Apr 24, 2021
View reviewed changes
criu/tty.c Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2021 •
edited
Loading

Codecov Report

Patch coverage: 66.66% and project coverage change: -0.01% ⚠️

Comparison is base (2df6ec5) 70.68% compared to head (5195d92) 70.67%.

❗ Current head 5195d92 differs from pull request most recent head 2b71063. Consider uploading reports for the commit 2b71063 to get more accurate results

Additional details and impacted files 
@@             Coverage Diff              @@
##           criu-dev    #1455      +/-   ##
============================================
- Coverage     70.68%   70.67%   -0.01%     
============================================
  Files           133      133              
  Lines         33321    33317       -4     
============================================
- Hits          23552    23548       -4     
  Misses         9769     9769
Files Changed Coverage Δ
criu/tty.c 77.41% <60.00%> (-0.03%) ⬇️
criu/files.c 80.83% <100.00%> (ø)

... and 5 files with indirect coverage changes

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

Snorch
Snorch reviewed May 20, 2021
View reviewed changes
Copy link
Member

@Snorch Snorch left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like the code. I think that there is no point to put an fd into fdstore if it would not be used from fdstore, as self_stdin_fdid is currently only used if stdin_isatty == true.

Doing otherwise can lead to problems when using inherit-fd on stdin as
it gets closed.

But, can you please explain how adding the fd to fdstore can results in the original fd been closed?

@nviennot
Copy link
Member Author

nviennot commented May 20, 2021 •
edited by Snorch
Loading

When using --inherit-fd fd[0]:XXX (for example, to replace resource XXX with the file provided via stdin), the following gets invoked:

criu/criu/files.c

Line 1599 in f1cc40c

close_safe(&inh->inh_fd);
(upd: permalink)
This sends stdin into the fdstore, and closes stdin. From that point, fd=0 is now closed, and the tty code tried to use it again.

@Snorch
Copy link
Member

Snorch commented May 20, 2021 •
edited
Loading

When using --inherit-fd fd[0]:XXX (for example, to replace resource XXX with the file provided via stdin), the following gets invoked: https://github.com/checkpoint-restore/criu/blob/criu-dev/criu/files.c#L1599
This sends stdin into the fdstore, and closes stdin. From that point, fd=0 is now closed, and the tty code tried to use it again.

Probably we can instead don't allow --inherit-fd on 0-2 standard io fds?

upd: not instead but also

@rst0git
Copy link
Member

rst0git commented May 20, 2021

Probably we can instead don't allow --inherit-fd on 0-2 standard io fds?

I remember having a similar discussion in #681

@Snorch
Copy link
Member

Snorch commented May 21, 2021

Probably we can instead don't allow --inherit-fd on 0-2 standard io fds?

I remember having a similar discussion in #681

Nice note. Seems like the problem of reusing 0-2 fds was already there with opts.ps_socket, and we've had "keep_fd" logic to protect from it, but we've removed it, relying that opts.ps_socket just would not use 0-2.

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

prakritigoyal19 and others added 8 commits June 11, 2023 23:30
@prakritigoyal19 @avagin
Add flog to CRIU
823db1b 
Change made through this commit:
- Include copy of flog as a seperate tree.
- Modify the makefile to add and compile flog code.

Signed-off-by: prakritigoyal19 <prakritigoyal19@gmail.com>
@adrianreber @avagin
flog: Missing varargs init or cleanup (VARARGS)
1444b72 
CID 302713 (checkpoint-restore#1 of 1): Missing varargs init or cleanup (VARARGS)
 va_end was not called for argptr.

Signed-off-by: Adrian Reber <areber@redhat.com>
@adrianreber @avagin
Run 'make indent' on 'flog/'
cc193dc 
Separate commit for easier criu-dev <-> master transfer.

Acked-by: Mike Rapoport <rppt@linux.ibm.com>
Signed-off-by: Adrian Reber <areber@redhat.com>
@kolyshkin @avagin
flog: typo: mmaped -> mmapped
1fa21a0 
It is mapped, not maped. Same applies for mmap I guess.

Found by codespell, except it wants to change it to mapped,
which will make it less specific.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin @avagin
flog: fix some codespell warnings
7eaf7a3 
Brought to you by

    codespell -w

(using codespell v2.1.0).

[v2: use "make indent" on the result]

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@Daz-3ux @avagin
limit the field width of 'scanf'
aed3f34 
Fixes: checkpoint-restore#2121

Signed-off-by: Pengda Yang <daz-3ux@proton.me>
@h0lyalg0rithm @avagin
sk-inet: Add IP TOS socket option
d6860d0 
The TOS(type of service) field in the ip header allows you specify the
priority of the socket data.

Signed-off-by: Suraj Shirvankar <surajshirvankar@gmail.com>
@h0lyalg0rithm @avagin
zdtm: Add tests for ip tos restore
0dd4668 
Signed-off-by: Suraj Shirvankar <surajshirvankar@gmail.com>
hdzhoujie and others added 8 commits June 11, 2023 23:32
@hdzhoujie @avagin
dump: increase fcntl call failure judgment
13eb876 
The pipe_size type is unsigned int, when the fcntl call fails and
return -1, it will cause a negative rollover problem.

Signed-off-by: zhoujie <zhoujie133@huawei.com>
@adrianreber @avagin
compel: support XSAVE on newer Intel CPUs
970c4ab 
Newer Intel CPUs (Sapphire Rapids) have a much larger xsave area than
before. Looking at older CPUs I see 2440 bytes.

    # cpuid -1 -l 0xd -s 0
    ...
        bytes required by XSAVE/XRSTOR area     = 0x00000988 (2440)

On newer CPUs (Sapphire Rapids) it grows to 11008 bytes.

    # cpuid -1 -l 0xd -s 0
    ...
        bytes required by XSAVE/XRSTOR area     = 0x00002b00 (11008)

This increase the xsave area from one page to four pages.

Without this patch the fpu03 test fails, with this patch it works again.

Signed-off-by: Adrian Reber <areber@redhat.com>
@adrianreber @avagin
ci: fix new codespell errors
1d4c5ed 
Signed-off-by: Adrian Reber <areber@redhat.com>
@adrianreber @avagin
scripts: make newer versions of shellcheck happy
d98c533 
Signed-off-by: Adrian Reber <areber@redhat.com>
@Snorch @avagin
criu-ns: make --pidfile option show pid in caller pidns
8e0697d 
Using the fact that we know criu_pid and criu is a parent of restored
process we can create pidfile with pid on caller pidns level.

We need to move mount namespace creation to child so that criu-ns can
see caller pidns proc.

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
@rst0git @avagin
docs: rename amdgpu_plugin.txt to criu-amdgpu-plugin.txt
806ee35 
By default, the file name 'amdgpu_plugin.txt' is used also as the name
for the corresponding man page (`man amdgpu_plugin`). However, when
this man page is installed system-wide it would be more appropriate
to have a prefix 'criu-' (e.g., `man criu-amdgpu-plugin`).

Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
@adrianreber @avagin
lib/c: add empty_ns interfaces to libcriu
f57bda4 
crun wants to set empty_ns and this interface is missing from the
library. This adds it to libcriu.

Signed-off-by: Adrian Reber <areber@redhat.com>
@warusadura @avagin
criu-ns: Add --criu-binary argument to run_criu()
f308272 
--criu-binary argument provides a way to supply the CRIU binary
location to run_criu().

Related to: checkpoint-restore#1909

Signed-off-by: Dhanuka Warusadura <csx@tuta.io>
osctobe and others added 18 commits August 3, 2023 23:37
@osctobe @avagin
page-xfer: Pull tcp_cork,nodelay().
1db922f 
Move tcp_cork() and tcp_nodelay() to the only user: page-xfer.c. While
at it, fix error messages (as they do not refer to restoring the sockopt
values) and demote them as they are not fatal to the page transfer.

Signed-off-by: Michał Mirosław <emmir@google.com>
@osctobe @avagin
irmap: scan user-provided paths in order
72494ed 
Make the scan use the order of paths that came from the user.

Fixes: 4f2e4ab ("irmap: add --irmap-scan-path option"; 2015-09-16)
Signed-off-by: Michał Mirosław <emmir@google.com>
@rst0git @avagin
amdgpu_plugin: remove duplicated log prefix
242de4e 
The log prefix "amdgpu_plugin:" is defined with `LOG_PREFIX` in
`amdgpu_plugin.c`.  However, the prefix is also included in each
log message. As a result it appears duplicated in the log messages:

(00.044324) amdgpu_plugin: amdgpu_plugin: devices:1 bos:58 objects:148 priv_data:45696
(00.045376) amdgpu_plugin: amdgpu_plugin: Thread[0x5589] started
(00.167172) amdgpu_plugin: amdgpu_plugin: img_path = amdgpu-kfd-62.img
(00.083739) amdgpu_plugin: amdgpu_plugin : amdgpu_plugin_dump_file() called for fd = 235

Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
@avagin
scripts/apt: don't hide apt output
6fc5bc6 
It is required to investigate issues.

Signed-off-by: Andrei Vagin <avagin@google.com>
@avagin
ci/docker: install all required packages
2199220 
This change fixes the issue:
```
The following packages have unmet dependencies:
 docker-ce : Depends: containerd.io (>= 1.6.4)
E: Unable to correct problems, you have held broken packages.
```

Signed-off-by: Andrei Vagin <avagin@google.com>
@rst0git @avagin
lib/py: add VMA_AREA_MEMFD constant
e1cda9f 
The VMA_AREA_MEMFD constant was introduced with commit

29a1a88
memfd: add memory mapping support

This patch extends the status map used in CRIT and coredump with the
value of this constant to recognize it.

Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
@avagin
loongarch64: reformat syscall_64.tbl for 8-wide tabs
288d6a6 
Signed-off-by: Andrei Vagin <avagin@gmail.com>
@osctobe @avagin
dump+restore: Implement membarrier() registration c/r.
2f50da4 
Note: Silently drops MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED as it's
not currently detectable. This is still better than silently dropping
all membarrier() registrations.

Signed-off-by: Michał Mirosław <emmir@google.com>
@osctobe @avagin
zdtm: membarrier: test migration of membarrier() registration
b5c3ccc 
Signed-off-by: Michał Mirosław <emmir@google.com>
@osctobe @avagin
Put a cap on the size of single preadv in restore operation.
5fedcaa 
While each preadv() is followed by a fallocate() that removes the data
range from image files on tmpfs, temporarily (between preadv() and
fallocate()) the same data is in two places; this increases the memory
overhead of restore operation by the size of a single preadv.
Uncapped preadv() would read up to 2 GiB of data, thus we limit that to
a smaller block size (128 MiB).

Based-on-work-by: Paweł Stradomski <pstradomski@google.com>
Signed-off-by: Michał Mirosław <emmir@google.com>
@avagin
github: auto-remove changes requested and awaiting reply labels
649292c 
Labels are removed when new comments are posted.

Signed-off-by: Andrei Vagin <avagin@gmail.com>
@avagin
loongarch64: fix syscall_64.tbl
942b5fd 
The 288d6a6 change broke all the syscall numbers.

Reported-by: Michał Mirosław <emmir@google.com>
Fixes: (288d6a6 "loongarch64: reformat syscall_64.tbl for 8-wide tabs")
Signed-off-by: Andrei Vagin <avagin@gmail.com>
@osctobe @avagin
memfd: don't set fd attributes not needed for vma mapping
359b257 
There is only one user of memfd_open() outside of memfd.c: open_filemap().
It is restoring a file-backed mapping and doesn't need nor expect to
update F_SETOWN nor the fd's position.  Check the inherited_fd() handling
in the callers to simplify the code.

Signed-off-by: Michał Mirosław <emmir@google.com>
@avagin
ci/loongarch64: compile tests before running zdtm.py
675c5e4 
Otherwise tests fail by timeout.

Signed-off-by: Andrei Vagin <avagin@gmail.com>
@osctobe @avagin
kerndat: Make pagemap check more robust against swapped out pages.
af31e8e 
Fix test of whether the kernel exposes page frame numbers to cope with the
possibility that the top of the stack is swapped out, which was happening
in about one 1 out of 3 million runs.  This lead to a later failure when
trying to read the PFN of the zero page, after which criu would exit with
no error message.

Original-From: Ambrose Feinstein <ambrose@google.com>
Signed-off-by: Michał Mirosław <emmir@google.com>
@osctobe @avagin
compel/infect: include the relevant pid in "no-breakpoints restore" d…
38baf73 
…ebug message

Signed-off-by: Michał Mirosław <emmir@google.com>
@osctobe @avagin
proc_parse: remove trivial goto from vma_get_mapfile_user()
ba27d27 
Signed-off-by: Michał Mirosław <emmir@google.com>
@rst0git @avagin
test/other: add test for action-script
2df6ec5 
This commit is introducing a test for the action-script functionality
of CRIU to verify that pre-dump, post-dump, pre-restore, pre-resume,
post-restore, post-resume hooks are executed during dump/restore.

Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
@avagin avagin marked this pull request as draft August 25, 2023 22:07
@nviennot @Snorch
tty: Only store the stdin fd when it corresponds to a tty
8488934 
Let's save some place in fdstore by not adding fd when it is unused.
Also let's remove now excess stdin_isatty.

Co-authored-by: Nicolas Viennot <Nicolas.Viennot@twosigma.com>
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
@Snorch
Copy link
Member

Snorch commented Aug 28, 2023

@avagin I reworked it:

  • removed stdin_isatty
  • added patch to prohibit --inherit-fd on std fds

@Snorch Snorch marked this pull request as ready for review August 28, 2023 04:42
rst0git
rst0git reviewed Aug 28, 2023
View reviewed changes
Documentation/criu.txt Outdated Show resolved Hide resolved
@Snorch
files: prohibit --inherit-fd via std fds
1ed709c 
Else inherit_fd_move_to_fdstore will close std fds and options like
--shell-job will be broken.

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
rst0git
rst0git approved these changes Aug 28, 2023
View reviewed changes
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snorch Snorch Snorch left review comments

@rst0git rst0git rst0git approved these changes

Assignees

@Snorch Snorch

Labels
stale-pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.