utils: suppress errors on missing legacy iptables #1976
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rst0git
force-pushed
the
iptables
branch
3 times, most recently
from
f1fad20 to
dd1ca2c
Compare
|
This looks like a correct change. The output says |
avagin
reviewed
Oct 2, 2022
rst0git
force-pushed
the
iptables
branch
2 times, most recently
from
0827c6e to
a0bed14
Compare
avagin
reviewed
Oct 20, 2022
criu/util.c
Outdated
| int exit_status = WEXITSTATUS(status); | ||
| /* Don't print error message for ENOENT (No such file or directory) | ||
| * when CRS_CAN_ENOENT flag has been set. */ | ||
| if (exit_status != ENOENT || !(flags & CRS_CAN_ENOENT)) |
There was a problem hiding this comment.
This isn't reliable. ENOENT is 2 and the tool that is executed can return 2, it isn't so unusual. I don't like all these jumps with iptable tools, can we use linux API to check whether iptable-s are empty or not? I don't ask to dump iptables, I mean just to check whether we need to dump them or not.
There was a problem hiding this comment.
@mihalicyn could you help with this?
There was a problem hiding this comment.
@mihalicyn @avagin Do you have any advice on what changes I should make to address this comment, or should I close this pull request?
There was a problem hiding this comment.
@rst0git you should not close this pr.
@mihalicyn, pls try to find time to look at this.
There was a problem hiding this comment.
yep, friends, I've started looking into this.
There was a problem hiding this comment.
@avagin @mihalicyn It seems that reading /proc/net/ip(6)_tables_names is enough to get info about existance of legacy rules in current netns:
in kernel: https://github.com/torvalds/linux/blob/0326074ff4652329f2a1a9c8685104576bd8d131/net/netfilter/x_tables.c#L1832
in iptables: https://git.netfilter.org/iptables/tree/iptables/nft-shared.c#n1460
@rst0git So we basically need to go over all netns-es of container and read these procfs files and check they are empty, to be sure that we don't need legacy iptables binary to save them.
There was a problem hiding this comment.
Good catch @Snorch! It works as a detector if tables are present or not, but unfortunately, it doesn't allow us to check if these tables are empty or not. Example:
root# cat /proc/net/ip_tables_names
root# iptables-legacy -L
Chain INPUT (policy ACCEPT)
target prot opt source destination
Chain FORWARD (policy ACCEPT)
target prot opt source destination
Chain OUTPUT (policy ACCEPT)
target prot opt source destination
root# cat /proc/net/ip_tables_names
filter
So, iptables-legacy even with the -L flag created the default struct xt_table with the name filter.
BTW, we can use getsockopt(fd, SOL_IP, {IPT,IP6T,EBT}_SO_GET_ENTRIES, ...) to check if these tables are empty or not.
I'm not sure if we want to be so precise here or if we just want to fail migration if the file /proc/net/ip(6)_tables_names is not empty and the iptables-legacy binary isn't present. What do you think?
There was a problem hiding this comment.
We've discussed this with Pavel and agreed that it seems sufficient just to check that the {ip,ip6}_tables_names files are empty in all net namespaces for now.
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>
CID 302713 (#1 of 1): Missing varargs init or cleanup (VARARGS) va_end was not called for argptr. Signed-off-by: Adrian Reber <areber@redhat.com>
Separate commit for easier criu-dev <-> master transfer. Acked-by: Mike Rapoport <rppt@linux.ibm.com> Signed-off-by: Adrian Reber <areber@redhat.com>
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>
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>
Fixes: checkpoint-restore#2121 Signed-off-by: Pengda Yang <daz-3ux@proton.me>
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>
Signed-off-by: Suraj Shirvankar <surajshirvankar@gmail.com>
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>
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>
Signed-off-by: Adrian Reber <areber@redhat.com>
Signed-off-by: Adrian Reber <areber@redhat.com>
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>
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>
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>
--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>
These changes remove and update the changes introduced in 7177938 in favor of the Python version in CI. os.waitstatus_to_exitcode() function appeared in Python 3.9 Related to: checkpoint-restore#1909 Signed-off-by: Dhanuka Warusadura <csx@tuta.io>
The is_native field is a boolean. Therefore, else if() should can be
changed to a simple else{}.
Signed-off-by: Taemin Ha <taeminha@cs.utexas.edu>
Signed-off-by: Andrei Vagin <avagin@gmail.com>
The condition meant to check fd2 instead of fd1, which is checked in line 24. Signed-off-by: Taemin Ha <taeminha@cs.utexas.edu> Signed-off-by: Andrei Vagin <avagin@gmail.com>
line 131 checks if (ret >= 0). line 133 could be replaced by a simple else statement Signed-off-by: Taemin Ha <taeminha@cs.utexas.edu> Signed-off-by: Andrei Vagin <avagin@gmail.com>
Eventpollentry's fields are set only when ret == 3 or ret == 6. The remaining cases can be grouped together to an error Signed-off-by: Taemin Ha <taemin.ha@utexas.edu> Signed-off-by: Andrei Vagin <avagin@gmail.com>
At this point the correct position is already restored, so reading from the fd results in the position being moved forward by 5 bytes. Fixes: 9191f87 ("criu/files-reg.c: add build-id validation functionality") Signed-off-by: Michal Clapinski <mclapinski@google.com>
Signed-off-by: Michal Clapinski <mclapinski@google.com>
Signed-off-by: Michal Clapinski <mclapinski@google.com>
cgroup_ifpriomap test needs net_prio cgroup, which might not be available. Make the .checkskip script check it. Signed-off-by: Michał Mirosław <emmir@google.com>
Newer versions of pip use an isolated virtual environment when building Python projects. However, when the source code of CRIT is copied into the isolated environment, the symlink for `../lib/py` (pycriu) becomes invalid. As a workaround, we used the `--no-build-isolation` option for `pip install`. However, this functionality has issues in some versions of PIP [1, 2]. To fix this problem, this patch adds separate packages for pycriu and crit, and each package is installed independently. [1] pypa/pip#8221 [2] pypa/pip#8165 (comment) Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Do not use $(USERCFLAGS) for anything other than what the user provide. Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
In the compel/arch/arm/plugins/std/syscalls/syscall.def, the syscall number of bind on ARM64 should be 200 instead of 235 Signed-off-by: Sally Kang <snapekang@gmail.com>
The rawhide netlink errors are fixed with a newer kernel than the default 6.2 available in Fedora 38. Signed-off-by: Adrian Reber <areber@redhat.com>
The old test was checking if '/' is btrfs but we should check if the current directory is btrfs. Signed-off-by: Adrian Reber <areber@redhat.com>
Signed-off-by: Adrian Reber <areber@redhat.com>
Signed-off-by: Andrei Vagin <avagin@gmail.com>
Replace a recursive call with a loop. Reported-by: Andrei Vagin <avagin@gmail.com> Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Checkpoint/restore with version 25.0.0-beta.1 fails with the following error: $ docker start --checkpoint=c1 cr Error response from daemon: failed to create task for container: content digest fdb1054b00a8c07f08574ce52198c5501d1f552b6a5fb46105c688c70a9acb45: not found: unknown Release notes: moby/moby#46816 Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
WARNINGS variable should be amended, not redefined. We still need, e.g., `-Wno-dangling-pointer` to build criu on loongarch64 with gcc13. Signed-off-by: Ivan A. Melnikov <iv@altlinux.org>
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>
Newer versions of 'tail' rely on inotify and after a restore 'tail' is unhappy with the state of inotify and just stops. This replaces 'tail' with a minimal shell based test (thanks Andrei). Signed-off-by: Adrian Reber <areber@redhat.com>
The image has a too old version of nettle which does not work with gnutls. Just upgrade to the latest to make the error go away. Signed-off-by: Adrian Reber <areber@redhat.com>
Signed-off-by: Adrian Reber <areber@redhat.com>
In commit [1] was introduced a mechanism to auto-generate the files: sys-exec-tbl*.c, syscalls*.S, syscall-codes*.h, and syscall*.h. This commit also updated the gitignore rules to ignore auto-generated files. However, after commit [2], the path for these files has changed and the patterns specified in gitignore are no longer needed. [1] bbc2f13 (x86/build: generate syscalls-{64,32}.built-in.o) [2] 19fadee (compel: plugins,std -- Implement syscalls in std plugin) Reported-by: @felicitia Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Starting with the musl v1.2.4~69, _GNU_SOURCE doesn't set _LARGEFILE64_SOURCE. Fixes checkpoint-restore#2313 Signed-off-by: Andrei Vagin <avagin@gmail.com>
When the legacy iptables backend is not installed, iptables-legacy-save
and ip6tables-legacy-save binary files are missing and this results in
the following error messages:
(00.062021) iptables has nft backend: iptables-save v1.8.8 (nf_tables)
Error (criu/util.c:626): execvp("iptables-legacy-save", ...) failed: No such file or directory
(00.062793) Error (criu/util.c:641): exited, status=1
(00.062800) Error (criu/util.c:1566): iptables-legacy-save -V failed
(00.069758) iptables has nft backend: ip6tables-save v1.8.8 (nf_tables)
Error (criu/util.c:626): execvp("ip6tables-legacy-save", ...) failed: No such file or directory
(00.070615) Error (criu/util.c:641): exited, status=1
(00.070624) Error (criu/util.c:1566): ip6tables-legacy-save -V failed
(00.070632) skipping iptables dump - no legacy version present
(00.070635) skipping ip6tables dump - no legacy version present
The error messages "No such file or directory" can be ignored.
This patch updates the get_legacy_iptables_bin() to check if the
/proc/net/ip(6)_tables_names file is empty before trying to run
iptables-legacy.
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the legacy iptables backend is not installed, iptables-legacy-save and ip6tables-legacy-save binaries are missing. This results in the following error messages:
However, these messages should not be errors and can be ignored. With the changes in this pull request, the following messages will be included in the logs instead.