GNOME Bugzilla – Bug 692673
[review] bg/iptables-sharing-rules-bgo692673 - NM adds duplicate iptables MASQUERADE rules
Last modified: 2016-01-23 09:28:12 UTC
If you set up a connection sharing to your other devices, eg usb reverse tethering on your mobile phone and restart network manager while the device is connected, network manager keeps adding duplicate packet forwarding rule to iptables.. % sudo iptables -t nat -nvL POSTROUTING Chain POSTROUTING (policy ACCEPT 14 packets, 1112 bytes) pkts bytes target prot opt in out source destination 0 0 MASQUERADE all -- * * 10.42.0.0/24 !10.42.0.0/24 % sudo service network-manager restart network-manager stop/waiting network-manager start/running, process 21778 % sudo iptables -t nat -nvL POSTROUTING Chain POSTROUTING (policy ACCEPT 0 packets, 0 bytes) pkts bytes target prot opt in out source destination 0 0 MASQUERADE all -- * * 10.42.0.0/24 !10.42.0.0/24 2 435 MASQUERADE all -- * * 10.42.0.0/24 !10.42.0.0/24 % sudo service network-manager restart network-manager stop/waiting network-manager start/running, process 21947 % sudo iptables -t nat -nvL POSTROUTING Chain POSTROUTING (policy ACCEPT 0 packets, 0 bytes) pkts bytes target prot opt in out source destination 0 0 MASQUERADE all -- * * 10.42.0.0/24 !10.42.0.0/24 1 272 MASQUERADE all -- * * 10.42.0.0/24 !10.42.0.0/24 2 435 MASQUERADE all -- * * 10.42.0.0/24 !10.42.0.0/24 It's desirable if network manager checks for existing rule before adding a new one so it wouldn't clutter up iptables with duplicate rules
Could be done by trying to delete the rule first: -D, --delete chain rule-specification
(In reply to comment #1) > Could be done by trying to delete the rule first: > > -D, --delete chain rule-specification Don't tell that to me..add it into nm.
(In reply to comment #2) > Don't tell that to me..add it into nm. This is why we have bugzilla. For communication.
(In reply to comment #3) > (In reply to comment #2) > > Don't tell that to me..add it into nm. > > This is why we have bugzilla. For communication. Yes and for filing bug report too.
(In reply to comment #4) > Yes and for filing bug report too. Misunderstanding sorted out off-list. (In reply to comment #1) > Could be done by trying to delete the rule first: > > -D, --delete chain rule-specification Just to be clear, this was a suggestion for NetworkManager code, as we are using the iptables command directly for handling MASQUERADE rules.
Update: When looking into the code and re-reading the bug report, I see the --delete is actually being used in the code (otherwise we could not switch back from shared connection). It may look like restating the obvious but... The deletion is only omitted when stopping NetworkManager. Therefore we must always call nm_act_request_set_shared(act_request, FALSE) for all shared connections when exitting NetworkManager. Also I believe we should always log firewall changes (iptables actions) we perform (I suggest INFO level).
Just found a pretty amusing thing about the iptables rule code in NetworkManager: 1) The rules are added in the reverse order for no reason, as they are added with g_slist_append(). That means the slower function is being used only to get the rules in the wrong order. Later before the rules are applied, the list is copied and the new copy is reserved to fix it *once again*. 2) The rules are *always* the same. Therefore the list of rules is not needed at all as they can be easily constructed from the interface name, address and prefix length.
Proposing for 1.2.
Pushed branch bg/iptables-sharing-rules-bgo692673.
LGTM; worth backporting to 1.0.x too.
Merged to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=0ba500d13174e103e3d35ce6a278928f874b7d9a and nm-1-0: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=nm-1-0&id=936a70bf4aa8608779b7e2ba2097d00698b15ef6