When CRIU restore failed, iptable rules could be leaked when using network namespace

[alexjinghn]

Hi,

We have a setup where we first try to restore a container using criu. If for any reason criu restore failed, we will launch another container normally as fallback. These two containers will belong to the same pod, sharing the same network namespace of the pod.

One issue we observed is that when the first container fails to be restored from a checkpoint due to TCP issues, these iptable rules would be leaked to the second fallback container because of shared network namespace, thus preventing it from functioning normally.

Chain INPUT (policy ACCEPT 3 packets, 180 bytes)
 pkts bytes target     prot opt in     out     source               destination         
 1086 65160 CRIU       0    --  *      *       0.0.0.0/0            0.0.0.0/0           

Chain FORWARD (policy ACCEPT 0 packets, 0 bytes)
 pkts bytes target     prot opt in     out     source               destination         

Chain OUTPUT (policy ACCEPT 3 packets, 120 bytes)
 pkts bytes target     prot opt in     out     source               destination         
  293 17580 CRIU       0    --  *      *       0.0.0.0/0            0.0.0.0/0           

Chain CRIU (2 references)
 pkts bytes target     prot opt in     out     source               destination         
    0     0 ACCEPT     0    --  *      *       0.0.0.0/0            0.0.0.0/0            mark match 0xc114
 1379 82740 DROP       0    --  *      *       0.0.0.0/0            0.0.0.0/0       

I believe these iptable rules are introduced during network_lock_internal

criu/criu/cr-restore.c

Line 2123 in 2cf8f13

ret = network_lock_internal(/* restore = */ true);

However, when restore failed, the control flow jumps to out_kill without invoking network_unlock

criu/criu/cr-restore.c

Line 2201 in 2cf8f13

network_unlock();

which seems to be only invoked on successfully restoring the checkpoint.

I'm wondering if we should consider clean up the iptable rules even on failure. Thanks for your attention!

[rst0git]

@alexjinghn Have you tried using CRIU with --network-lock skip? For example, you can add network-lock=skip in /etc/criu/runc.conf.

[alexjinghn]

thanks @rst0git based on the conf name, i believe that would have addressed this problem. I'm wondering the consequence of disabling network lock. would it interfere with TCP connection repairing?

Taking a step back, if network lock feature is default on, I would like to explore the best way to fix/improve it regarding this behavior.

[rst0git]

@alexjinghn "network locking" is mainly for live migration. This mechanism is used to "drop" all incoming packets for established TCP connections while the checkpointed processes are not running. This time period is also known as downtime during migration. IIRC the reason we drop these packets is to prevent the kernel from sending back TCP RSTs that would close the established connections.

CRIU also supports the --tcp-close option, which can be used for other use cases where TCP connections don't need to be preserved:

$ man criu

dump
...
--tcp-close
  Don't dump the state of, or block, established tcp connections (including the connection is once established but now closed).
  This is useful when tcp connections are not going to be restored.
...
--network-lock [mode]
  Set the method to be used for network locking/unlocking. Locking is done to ensure that tcp packets are dropped between dump and restore.
  This is done to avoid the kernel sending RST when a packet arrives destined for the dumped process.

  The mode may be one of the following:
    iptables
      Use iptables rules to drop the packets. This is the default if mode is not specified.
    nftables
      Use nftables rules to drop the packets.
    skip
      Don't lock the network. If --tcp-close is not used, the network must be locked externally to allow CRIU to dump TCP connections.
...
restore
...
--tcp-close
  Restore connected TCP sockets in closed state.

would it interfere with TCP connection repairing?

If the iptables rules are not set, the established TCP connections might be closed when CRIU tries to repair them. You can use --tcp-close or manually "lock/unlock" the network connections by adding or removing similar iptables rules.

[alexjinghn]

thanks @rst0git for explaining the purpose of network locking. In our use case, we don't require any external TCP connections (in fact, we have checks to fail checkpoint if any is found), so i believe disable network locking would work for us barring any unknown stuff that relies on network locking to function till now.

Speaking of which, from a stability perspective, I'd like to make minimal changes and avoid dealing with the unknown. In terms of blast radius, disabling network locking will impact the happy path whereas this issue of iptable rules leak is currently only impacting the sad path where criu restore failed, which happens much more rarely compared to happy path.

as such, i'd still like to hear your opinion on callling network_unlock() right after out_kill to clean up the iptable rules:

criu/criu/cr-restore.c

Line 2298 in 2cf8f13

out_kill:

do you see any problem with this approach?

[avagin]

@alexjinghn Have you tried using CRIU with --network-lock skip? For example, you can add network-lock=skip in /etc/criu/runc.conf.

I strongly suggest you think twice before doing that. The network lock doesn't just block external packets; it locks all internal packets as well. In many cases, processes use local TCP connections for inter-process communication. Without the network lock, these connections will not be restored properly.

[avagin]

@alexjinghn The network_unlock function isn't called on error paths by design. First, the user might diagnose the failure and attempt to restore the workload again. Second, if a failure occurs at a very early stage or due to a crash, the network lock rules wouldn't be cleaned up anyway. Third, there are other C/R resources such as ghost or remapped files that also require cleanup. I think the proper solution is to introduce the clean command to clean up all these C/R-related leftovers.

[github-actions]

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