netfilter: xt_TEE: Fix potential deadlock when TEE target is inserted

PATCH In review Taehee Yoo 2 participants 3 messages Sep 3, 2017

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Taehee Yoo

author

2017-09-03 14:30 UTC #0 lore raw

PATCH 1/1 netfilter: xt_TEE: Fix potential deadlock when TEE target is inserted

When xt_TEE target is inserted, lockdep warns about possible DEADLOCK situation. to avoid deadlock situation
the register_netdevice_notifier() should be called by only init routine.

reproduce command is :

   # iptables -I INPUT -j TEE --oif enp3s0 --gateway 192.168.0.1

warning message is :

[  115.182917] WARNING: possible circular locking dependency detected
[  115.189846] 4.13.0-rc1+ #68 Not tainted
[  115.194141] ------------------------------------------------------
[  115.201065] iptables/1283 is trying to acquire lock:
[  115.206627]  (rtnl_mutex){+.+.+.}, at: [<ffffffff8236f0d7>] rtnl_lock+0x17/0x20
[  115.214842]
[  115.214842] but task is already holding lock:
[  115.221378]  (sk_lock-AF_INET){+.+.+.}, at: [<ffffffff8273ab0d>] ip_setsockopt+0x6d/0xb0
[  115.230462]
[  115.230462] which lock already depends on the new lock.
[  115.230462]
[  115.239627]
[  115.239627] the existing dependency chain (in reverse order) is:
[  115.248012]
[  115.248012] -> #1 (sk_lock-AF_INET){+.+.+.}:
[  115.254472]        lock_acquire+0x190/0x370
[  115.259165]        lock_sock_nested+0xb8/0x100
[  115.264148]        do_ip_setsockopt.isra.16+0x140/0x24f0
[  115.270125]        ip_setsockopt+0x34/0xb0
[  115.274742]        udp_setsockopt+0x1b/0x30
[  115.279455]        sock_common_setsockopt+0x78/0xf0
[  115.284937]        SyS_setsockopt+0x11c/0x220
[  115.289835]        do_syscall_64+0x187/0x410
[  115.294638]        return_from_SYSCALL_64+0x0/0x7a
[  115.300025]
[  115.300025] -> #0 (rtnl_mutex){+.+.+.}:
[  115.306030]        __lock_acquire+0x4114/0x47c0
[  115.311132]        lock_acquire+0x190/0x370
[  115.315844]        __mutex_lock+0xef/0x1460
[  115.320555]        mutex_lock_nested+0x1b/0x20
[  115.325558]        rtnl_lock+0x17/0x20
[  115.329785]        register_netdevice_notifier+0x6f/0x4f0
[  115.335851]        tee_tg_check+0x19b/0x260
[  115.340562]        xt_check_target+0x1f5/0x6c0
[  115.345569]        find_check_entry.isra.7+0x62f/0x960
[  115.351353]        translate_table+0xcf2/0x1830
[  115.356454]        do_ipt_set_ctl+0x1ff/0x3a0
[  115.361362]        nf_setsockopt+0x61/0xc0
[  115.365977]        ip_setsockopt+0x82/0xb0
[  115.370592]        raw_setsockopt+0x73/0xa0
[  115.375304]        sock_common_setsockopt+0x78/0xf0
[  115.380793]        SyS_setsockopt+0x11c/0x220
[  115.385701]        entry_SYSCALL_64_fastpath+0x1c/0xb1
[  115.391478]
[  115.391478] other info that might help us debug this:
[  115.391478]
[  115.400511]  Possible unsafe locking scenario:
[  115.400511]
[  115.407176]        CPU0                    CPU1
[  115.412270]        ----                    ----
[  115.417364]   lock(sk_lock-AF_INET);
[  115.421394]                                lock(rtnl_mutex);
[  115.427760]                                lock(sk_lock-AF_INET);
[  115.434723]   lock(rtnl_mutex);
[  115.438267]
[  115.438267]  *** DEADLOCK ***

[ ... ]

Signed-off-by Taehee Yoo <[email protected]>

---
 include/uapi/linux/netfilter/xt_TEE.h |  3 +-
 net/netfilter/xt_TEE.c                | 90 ++++++++++++++++++++++-------------
 2 files changed, 59 insertions(+), 34 deletions(-)

include/uapi/linux/netfilter/xt_TEE.h +2 -1

index 0109202..4b7eae4 100644
@@ -2,10 +2,11 @@
 #define _XT_TEE_TARGET_H
 
 #include <linux/netfilter.h>
+#include <linux/if.h>
 
 struct xt_tee_tginfo {
 	union nf_inet_addr gw;
-	char oif[16];
+	char oif[IFNAMSIZ];
 
 	/* used internally by the kernel */
 	struct xt_tee_priv *priv __attribute__((aligned(8)));

net/netfilter/xt_TEE.c +57 ~~-33~~

154 lines not on this page · raw message

Signature · 1 line

2.9.3

Jan Engelhardt

2017-09-03 15:32 UTC #1 lore raw

include/uapi/linux/netfilter/xt_TEE.h line 9

Quoted 10 lines · Taehee Yoo

When xt_TEE target is inserted, lockdep warns about possible DEADLOCK situation. to avoid deadlock situation
the register_netdevice_notifier() should be called by only init routine.
+#include <linux/if.h>
struct xt_tee_tginfo {
	union nf_inet_addr gw;
include/uapi/linux/netfilter/xt_TEE.h +1 -1
-	char oif[16];
+	char oif[IFNAMSIZ];

This should not be done, as xt_tee_tginfo is exported to userspace. (It also has nothing to do with fixing the deadlock, really.)

net/netfilter/xt_TEE.c static int tee_netdev_event(struct notifier_block *this, unsigned long event, · line 71

Quoted 2 lines

net/netfilter/xt_TEE.c +2 -0

+		case NETDEV_UNREGISTER:
+			if ((dev->ifindex == priv->oif) &&

redundant new parenthesis group

Taehee Yoo

author

2017-09-03 15:51 UTC #2 lore raw

2017-09-04 0:32 GMT+09:00 Jan Engelhardt <[email protected]>:

Quoted 14 lines · Taehee Yoo

include/uapi/linux/netfilter/xt_TEE.h line 9
Quoted 11 lines
When xt_TEE target is inserted, lockdep warns about possible DEADLOCK situation. to avoid deadlock situation
the register_netdevice_notifier() should be called by only init routine.
+#include <linux/if.h>
struct xt_tee_tginfo {
      union nf_inet_addr gw;
include/uapi/linux/netfilter/xt_TEE.h +1 -1
-      char oif[16];
+      char oif[IFNAMSIZ];

Quoted 7 lines

This should not be done, as xt_tee_tginfo is exported to userspace. (It also has nothing to do with fixing the deadlock, really.)
net/netfilter/xt_TEE.c static int tee_netdev_event(struct notifier_block *this, unsigned long event, · line 71
Quoted 2 lines
net/netfilter/xt_TEE.c +2 -0
+              case NETDEV_UNREGISTER:
+                      if ((dev->ifindex == priv->oif) &&
redundant new parenthesis group

Thank you for your review!

I will send v2 patch.

Thanks