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

sk-inet: Add support for checkpoint/restore of ICMP sockets #2558

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

Conversation

ss141309
Copy link

Currently there is no option to checkpoint/restore programs that use ICMP sockets, such as ping. This patch adds support for the same.

Fixes #2557

@rst0git
Copy link
Member

rst0git commented Dec 27, 2024

@ss141309 Thank you for opening this pull request! Would you be able to add a ZDTM test for this functionality?

Example:

@ss141309
Copy link
Author

@rst0git oops, it looks like I forgot to add an IP6 version of the test, do I need to create it?

@rst0git
Copy link
Member

rst0git commented Dec 28, 2024

I forgot to add an IP6 version of the test, do I need to create it?

It would be good to have test for this. CRIU is used in some production environments where only IPv6 addresses are being used.

@avagin
Copy link
Member

avagin commented Dec 30, 2024

As far as I remember, ICMP sockets can have attached filters and we need to dump them. Pls take a look at c2cbcaf, maybe some code can be reused.

@ss141309
Copy link
Author

ss141309 commented Jan 1, 2025

it seems that the tests are failing because of the GIDs being set in the ping_group_range variable. What should I set them to in the test/zdtm_ct.c and test/zdtm/lib/ns.c files? setting them to 0 2147483647 causes some of the other tests like socket_udp to fail. Also in which flavours should I run the tests?

@ss141309 ss141309 requested review from rst0git and avagin January 2, 2025 09:57
@ss141309
Copy link
Author

ss141309 commented Jan 7, 2025

ping! @avagin @rst0git

@avagin
Copy link
Member

avagin commented Jan 8, 2025

it seems that the tests are failing because of the GIDs being set in the ping_group_range variable. What should I set them to in the test/zdtm_ct.c and test/zdtm/lib/ns.c files? setting them to 0 2147483647 causes some of the other tests like socket_udp to fail. Also in which flavours should I run the tests?

The test gid is 58467:

env['ZDTM_GID'] = "58467"

0 2147483647 doesn't work, because some of these gids are not mapped in test user namespaces:

#define GID_MAP "0 400000 50000\n50000 500000 100000"

I think "58467 58468" is the right range in this case.

@ss141309
Copy link
Author

ss141309 commented Jan 8, 2025

As far as I remember, ICMP sockets can have attached filters and we need to dump them. Pls take a look at c2cbcaf, maybe some code can be reused.

ICMP filters are only attached when using SOCK_RAW, since unprivileged ICMP sockets only accept ICMP_ECHO and ICMP_ECHOREPLY type messages

@ss141309 ss141309 force-pushed the criu-dev branch 2 times, most recently from 6f97c64 to 9c54c86 Compare January 9, 2025 08:33
@ss141309
Copy link
Author

ss141309 commented Jan 11, 2025

@avagin @rst0git I believe the patch is now complete, could you please review it?

criu/sk-inet.c Outdated Show resolved Hide resolved
@avagin
Copy link
Member

avagin commented Jan 13, 2025

Overall, it looks good to me. We need to move C/R of the sysctl to the proper place and resort patches. I will do all of that this week. Thanks for the contribution.

criu/sockets.c Outdated Show resolved Hide resolved
@rst0git
Copy link
Member

rst0git commented Jan 14, 2025

@ss141309 Would you be able update the pull request to apply the fixup changes into previous commits?
See CONTRIBUTING.md for more information.

@ss141309
Copy link
Author

@ss141309 Would you be able update the pull request to apply the fixup changes into previous commits? See CONTRIBUTING.md for more information.

@rst0git I did the changes, is it now alright?

@rst0git
Copy link
Member

rst0git commented Jan 15, 2025

@ss141309 Would you be able to apply the change from test: Actually fail and return when icmp_reply type is not ECHOREPLY into test: add a static test for ICMP socket? (This commit is considered as a fixup change)

@Snorch
Copy link
Member

Snorch commented Jan 15, 2025

sk-inet: Checkpoint/Restore net.ipv4.ping_group_range

We need to integrate it into dump_netns_conf/restore_netns_conf, probably taking as an example ebe3b52353c

This value belongs to namespace, not to socket.

@ss141309
Copy link
Author

sk-inet: Checkpoint/Restore net.ipv4.ping_group_range

We need to integrate it into dump_netns_conf/restore_netns_conf, probably taking as an example ebe3b52353c

This value belongs to namespace, not to socket.

Should I make a new commit or edit the existing one and force push the changes?

@Snorch
Copy link
Member

Snorch commented Jan 16, 2025

@ss141309 I did proper handling of ping_group_range c/r here #2565, you can rebase on top of it when/if it is merged. Machinery of sysctls in CRIU is a bit too complex, I must admit. And so I helped you a bit here, as you can see there is a lot of code to do one more sysctl in the directory which is not yet handled.

test/zdtm/lib/ns.c Outdated Show resolved Hide resolved
Currently there is no option to checkpoint/restore programs that use
ICMP sockets, such as `ping`. This patch adds support for the same.

Fixes checkpoint-restore#2557

Signed-off-by: समीर सिंह Sameer Singh <[email protected]>
ZDTM test suite creates separate network namespaces to run
tests. These namespaces do not preserve the value of the sysctl
variable `net.ipv4.ping_group_range` which allows the creation of
unprivileged ICMP sockets. This commit modifies the variable after
the namespaces have been created to allow GIDs 0-58468 to create
unprivileged ICMP sockets, since the zdtm test GID is in this range.

Signed-off-by: समीर सिंह Sameer Singh <[email protected]>
Add ZDTM static tests for IP4/ICMP and IP6/ICMP
socket feature.

Signed-off-by: समीर सिंह Sameer Singh <[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

@@ -0,0 +1 @@
{'flags': 'suid', 'flavor': 'h ns'}
Copy link
Member

Choose a reason for hiding this comment

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

why is it suid? Why uns is excluded?

Copy link
Member

@Snorch Snorch Feb 4, 2025

Choose a reason for hiding this comment

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

  1. If we remove suid we get:
04:09:54.090:     4: ERR: sysctl.c:34: Can't open /proc/sys/net/ipv4/ping_group_range (errno = 13 (Permission denied))

Which is EACCES as file have permissions "0644", and non-root is not allowed to write it. But we want to avoid suid to check non-root users too. So we can probably just remove setting ping_group_range from test and rely on it being set in second patch for all tests.

So please remove suid together with setting ping_group_range from test/zdtm/static/socket*_icmp.c and let's see if it works.

  1. If we add uns we get:
b'(00.004760)      1: Error (criu/net.c:2180): net: unix: Failed to write net/ipv4/<sysctls>'

I see that we get EINVAL when writing to ping_group_range on criu restore.

Likely the low/high uids we try to set got considered invalid in the userns of the caller (CRIU). We should probably just enter proper userns at that point of setting ping_group_range, I will try to fix this.

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.

Checkpoint/restore of ICMP sockets is not supported
4 participants