sk-inet: Support IP_TRANSPARENT and IPV6_TRANSPARENT socket options

[juntongdeng]

Support for IP_TRANSPARENT and IPV6_TRANSPARENT socket options is useful for restoring transparent proxies.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.72%. Comparing base (cb39c62) to head (85842b2).

:exclamation: Current head 85842b2 differs from pull request most recent head 7c2c8a8. Consider uploading reports for the commit 7c2c8a8 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##           criu-dev    #2357      +/-   ##
============================================
+ Coverage     70.21%   70.72%   +0.50%     
============================================
  Files           132      135       +3     
  Lines         34372    32907    -1465     
============================================
- Hits          24134    23272     -862     
+ Misses        10238     9635     -603     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Snorch]

Please reorder patches, c/r support must be first patch and a test for it must be second patch. This way you would not break tests on bisect.

[Snorch]

This PR tries to do a good thing. But actually there it misses a bigger problem with IP_TRANSPARENT option, this option allows to bind to non-local address, so if we modify the test a bit to actually do the bind to non-local address:

--- a/test/zdtm/static/sock_ip_opts00.c
+++ b/test/zdtm/static/sock_ip_opts00.c
@@ -5,6 +5,7 @@
 #include <linux/in.h>
 #include <linux/ip.h>
 #include <linux/in6.h>
+#include <arpa/inet.h>
 
 #include "zdtmtst.h"
 
@@ -83,6 +84,21 @@ int main(int argc, char **argv)
                                goto close;
                        }
                }
+
+               if (sk_confs[i].domain == AF_INET) {
+                       socklen_t len = sizeof(struct sockaddr_in);
+                       struct sockaddr_in addr;
+
+                       memset(&addr, 0, sizeof(addr));
+                       addr.sin_family = AF_INET;
+                       addr.sin_addr.s_addr = inet_addr("5.5.5.5");
+                       addr.sin_port = htons(5555);
+
+                       if (bind(sk_confs[i].sk, (struct sockaddr *)&addr, len) < 0) {
+                               pr_perror("bind on 5.5.5.5:5555 failed");
+                               goto close;
+                       }
+               }
        }
 
        test_daemon();

We see that nothing works:

[root@turmoil criu]# ./test/zdtm.py run -t zdtm/static/sock_ip_opts00
userns is supported
Checking feature ipv6_freebind
ipv6_freebind is supported
=== Run 1/1 ================ zdtm/static/sock_ip_opts00
===================== Run zdtm/static/sock_ip_opts00 in ns =====================
 DEP       sock_ip_opts00.d
 DEP       sock_ip_opts01.d
 CC        sock_ip_opts00.o
 LINK      sock_ip_opts00
Start test
Test is SUID
./sock_ip_opts00 --pidfile=sock_ip_opts00.pid --outfile=sock_ip_opts00.out
Run criu dump
Run criu restore
=[log]=> dump/zdtm/static/sock_ip_opts00/104/1/restore.log
------------------------ grep Error ------------------------
b'(00.002868)      1: No ipcns-sem-11.img image'
b'(00.003590)      1: net: Try to restore a link 10:1:lo'
b'(00.003707)      1: net: Restoring link lo type 1'
b'(00.004127)      1: net: \tRunning ip addr restore'
b'Error: ipv4: Address already assigned.'
b'Error: ipv6: address already assigned.'
b'(00.030163)      4: \t\tCreate fd for 3'
b'(00.030170)      1: `- render 1 iovs (0x7f09c5a79000:8192...)'
b'(00.030177)      4: inet: \tRestore: family AF_INET    type SOCK_DGRAM     proto IPPROTO_UDP      port 5555 state TCP_CLOSE        src_addr 5.5.5.5'
b'(00.030182)      1: Restore via sigreturn'
b"(00.030217)      4: Error (criu/sk-inet.c:1033): inet: Can't bind inet socket (id 12): Cannot assign requested address"
b'(00.030229)      4: Error (criu/files.c:1213): Unable to open fd=4 id=0xc'
b'(00.030838)      1: Error (criu/cr-restore.c:1518): 4 exited, status=1'
b'(00.031589) mnt: Switching to new ns to clean ghosts'
b'(00.031695) Error (criu/cr-restore.c:2572): Restoring FAILED.'
------------------------ ERROR OVER ------------------------
############# Test zdtm/static/sock_ip_opts00 FAIL at CRIU restore #############
Test output: ================================

 <<< ================================
##################################### FAIL #####################################

Just dumping and restoring IP_TRANSPARENT is not enough, it should be properly integrated with the bind stage (I doubt that any app which uses it uses it without bind to non-local adress). Note that we have exactly the same problem with IP_FREEBIND (on ipv4) and to actually check IP_TRANSPARENT you need to:

--- a/test/zdtm/static/sock_ip_opts00.c
+++ b/test/zdtm/static/sock_ip_opts00.c
@@ -25,7 +25,6 @@ struct sk_opt {
 };
 
 struct sk_opt sk_opts_v4[] = {
-       { SOL_IP, IP_FREEBIND, IP_OPT_VAL },
        { SOL_IP, IP_PKTINFO, IP_OPT_VAL },
        { SOL_IP, IP_TTL, 32 },
        { SOL_IP, IP_TOS, IPTOS_TOS(IPTOS_THROUGHPUT) },
@@ -37,7 +36,6 @@ struct sk_opt sk_opts_v4[] = {
 #endif
 
 struct sk_opt sk_opts_v6[] = {
-       { SOL_IPV6, IPV6_FREEBIND, IP_OPT_VAL },
        { SOL_IPV6, IPV6_RECVPKTINFO, IP_OPT_VAL },
        { SOL_IPV6, IPV6_TRANSPARENT, IP_OPT_VAL },
 };

[juntongdeng]

This PR tries to do a good thing. But actually there it misses a bigger problem with IP_TRANSPARENT option, this option allows to bind to non-local address, so if we modify the test a bit to actually do the bind to non-local address:

Just dumping and restoring IP_TRANSPARENT is not enough, it should be properly integrated with the bind stage (I doubt that any app which uses it uses it without bind to non-local adress). Note that we have exactly the same problem with IP_FREEBIND (on ipv4) and to actually check IP_TRANSPARENT you need to:

I think the cause of this problem is that in the current open_inet_sk() of sk-inet.c, the socket options are restored at the end, and listen(), connect(), bind() are all called before setsockopt().

I think we should change this order and set all socket options after creating the socket, before listen(), connect(), bind().

What do you think? If you agree, I can create a new pull request.

[Snorch]

set all socket options after creating the socket, before listen(), connect(), bind(). What do you think? If you agree, I can create a new pull request.

~~Looks like a good idea.~~

Also one more possible complication: 1) set IP_TRANSPARENT 2) bind to non-local ip 3) unset IP_TRANSPARENT, this way we somehow need to guess to set this before bind and then unset it to restore properly.

So maybe we just need to extend 73a739b8d ("inet: set IP_FREEBIND before binding socket to an ipv6 address (v2)") to ipv4 to fix that.

[github-actions[bot]]

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