[RFC PATCH 0/5] netlink: mmap kernel panic and some issues

[RFC PATCH 0/5] netlink: mmap kernel panic and some issues

[Ken-ichirou MATSUZAWA]

 Hello,

I got a kernel panic below when I dumped using mmaped netlink socket
while monitoring it by nlmon tap device. I realized it is because
mmaped netlink skb does not have skb_shared_info but don't know how
to fix it in sane. This patch series seems to work fine for me but
I'm not sure it's right or not.

Patch 1/5 added helper functions for mmaped netlink skb and applied
these at 2/5. I'm not sure I embed helper functions like this or add
skb functions and wrap it like alloc_skb_head() in
netlink_alloc_skb(). Patch 3/5 fixes nm_state for skb which is
allocated but not sent.

I noticed I can not send netlink message by using mmaped netlink
socket since:

    commit: a8866ff6a5bce7d0ec465a63bc482a85c09b0d39
    netlink: make the check for "send from tx_ring" deterministic

I found a msg->msg_iter.type was set to 1 (WRITE). It seems that we
need to accept it but reject KERNEL_DS. Patch 4/5 may fix it.

Talking about Patch 5/5, I receive many notifications which frame
status is NL_MMAP_STATUS_RESERVED from mmaped nflog poll() when I
specified QTHRESH or TIMEOUT nflog config option. This behavior
seems to be different from normal socket. And I don't need to be
notified that there is a frame I'm processing - SKIP in the ring
too.

It would be appreciate if someone consolidate patches or tell me how
to fix it.

Thanks,

[  196.691844] Netfilter messages via NETLINK v0.30.
[  196.742847] nf_conntrack version 0.5.0 (2943 buckets, 11772 max)
[  196.787119] ctnetlink v0.93: registering with nfnetlink.
[  211.177865] device eth1 entered promiscuous mode
[  211.314466] bridge: automatic filtering via arp/ip/ip6tables has been deprecated. Update your scripts to load br_netfilter if you need this.
[  211.319998] br0: port 1(eth1) entered forwarding state
[  211.320419] br0: port 1(eth1) entered forwarding state
[  211.466591] Ebtables v2.0 registered
[  226.336171] br0: port 1(eth1) entered forwarding state
[  300.957103] BUG: unable to handle kernel NULL pointer dereference at 0000000000000002
[  300.958740] IP: [<ffffffff81482b48>] kfree_skb_list+0x18/0x30
[  300.959814] PGD 177ae067 PUD 177c6067 PMD 0 
[  300.960958] Oops: 0000 [#1] SMP 
[  300.960958] Modules linked in: nlmon nf_conntrack_ipv4 nf_defrag_ipv4 ebt_redirect ebtable_broute ebtables x_tables bridge stp llc dummy nf_conntrack_netlink nf_conntrack nfnetlink netconsole binfmt_misc ttm drm_kms_helper drm ppdev snd_pcm snd_timer parport_pc snd parport soundcore acpi_cpufreq psmouse pcspkr i2c_piix4 evdev i2c_core processor button thermal_sys serio_raw configfs loop autofs4 ext4 crc16 mbcache jbd2 sg sr_mod cdrom ata_generic virtio_blk virtio_net ata_piix virtio_pci virtio_ring virtio libata scsi_mod floppy [last unloaded: netconsole]
[  300.960958] CPU: 0 PID: 890 Comm: ulogd Not tainted 4.1.1 #3
[  300.960958] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  300.960958] task: ffff8800129963d0 ti: ffff880017254000 task.ti: ffff880017254000
[  300.960958] RIP: 0010:[<ffffffff81482b48>]  [<ffffffff81482b48>] kfree_skb_list+0x18/0x30
[  300.960958] RSP: 0018:ffff8800172577e8  EFLAGS: 00010202
[  300.960958] RAX: 0000000000000000 RBX: ffff88001513c000 RCX: 000000005fb50000
[  300.960958] RDX: 00000000ffffffff RSI: ffff88000012e000 RDI: 0000000000000002
[  300.960958] RBP: ffff8800172577f8 R08: 0000000000000020 R09: 0000000000000578
[  300.960958] R10: ffffffff818c4cc0 R11: 0000000000000000 R12: ffff88001747d800
[  300.960958] R13: 0000000000000000 R14: 0000000000001000 R15: ffff8800157ed400
[  300.960958] FS:  00007f92e6dc1700(0000) GS:ffff880017c00000(0000) knlGS:0000000000000000
[  300.960958] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  300.960958] CR2: 0000000000000002 CR3: 0000000015100000 CR4: 00000000000006f0
[  300.960958] Stack:
[  300.960958]  ffff880017666600 ffff88001513c000 ffff880017257828 ffffffff81482be5
[  300.960958]  ffff880017257828 ffff88001747d800 0000000000000000 ffff88000012e000
[  300.960958]  ffff880017257848 ffffffff81482cc6 ffff88001747d800 ffff88001747d800
[  300.960958] Call Trace:
[  300.960958]  [<ffffffff81482be5>] ? skb_release_data+0x85/0xd0
[  300.960958]  [<ffffffff81482cc6>] ? __kfree_skb+0x16/0x90
[  300.960958]  [<ffffffffa033b16c>] ? nlmon_xmit+0x2c/0x30 [nlmon]
[  300.960958]  [<ffffffff81494043>] ? dev_hard_start_xmit+0x233/0x3e0
[  300.960958]  [<ffffffff8149442e>] ? netif_skb_features+0xfe/0x200
[  300.960958]  [<ffffffff81494770>] ? validate_xmit_skb+0x40/0x330
[  300.960958]  [<ffffffff81494f59>] ? __dev_queue_xmit+0x489/0x590
[  300.960958]  [<ffffffff814c2e26>] ? netlink_deliver_tap+0xe6/0x170
[  300.960958]  [<ffffffff814c2eeb>] ? __netlink_sendskb+0x3b/0x240
[  300.960958]  [<ffffffff814c57c6>] ? netlink_dump+0x1c6/0x2d0
[  300.960958]  [<ffffffff814c769a>] ? __netlink_dump_start+0x19a/0x1d0
[  300.960958]  [<ffffffffa02f4d20>] ? ctnetlink_get_conntrack+0xc0/0x25c [nf_conntrack_netlink]
[  300.960958]  [<ffffffffa02f2b20>] ? ctnetlink_dump_dying+0x20/0x20 [nf_conntrack_netlink]
[  300.960958]  [<ffffffffa02f0a40>] ? ctnetlink_nfqueue_attach_expect+0x170/0x170 [nf_conntrack_netlink]
[  300.960958]  [<ffffffff8131a15e>] ? __nla_reserve+0x4e/0x70
[  300.960958]  [<ffffffff8131a15e>] ? __nla_reserve+0x4e/0x70
[  300.960958]  [<ffffffffa02f4c60>] ? ctnetlink_nfqueue_parse+0x2e0/0x2e0 [nf_conntrack_netlink]
[  300.960958]  [<ffffffffa0056b7b>] ? nfnetlink_rcv_msg+0x28b/0x2a0 [nfnetlink]
[  300.960958]  [<ffffffff81494770>] ? validate_xmit_skb+0x40/0x330
[  300.960958]  [<ffffffffa00568f0>] ? nfnetlink_rcv+0xe0/0xe0 [nfnetlink]
[  300.960958]  [<ffffffff814c65d9>] ? netlink_rcv_skb+0xa9/0xd0
[  300.960958]  [<ffffffff814c6266>] ? netlink_unicast+0x126/0x1c0
[  300.960958]  [<ffffffff814c6ea6>] ? netlink_sendmsg+0x556/0x660
[  300.960958]  [<ffffffff8147770d>] ? sock_sendmsg+0x4d/0x60
[  300.960958]  [<ffffffff814791b4>] ? SYSC_sendto+0x104/0x180
[  300.960958]  [<ffffffff811d7eb9>] ? vfs_read+0xa9/0xe0
[  300.960958]  [<ffffffff811d87fc>] ? SyS_read+0x9c/0xd0
[  300.960958]  [<ffffffff81596bae>] ? system_call_fastpath+0x12/0x71
[  300.960958] Code: 48 83 c4 08 5b c9 c3 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 53 48 83 ec 08 0f 1f 44 00 00 48 85 ff 74 15 0f 1f 44 00 00 <48> 8b 1f e8 f0 fc ff ff 48 85 db 48 89 df 75 f0 48 83 c4 08 5b 
[  300.960958] RIP  [<ffffffff81482b48>] kfree_skb_list+0x18/0x30
[  300.960958]  RSP <ffff8800172577e8>
[  300.960958] CR2: 0000000000000002
[  300.960958] ---[ end trace fa655a8b26512358 ]---
[  300.960958] Kernel panic - not syncing: Fatal exception in interrupt
[  300.960958] Kernel Offset: disabled
[  300.960958] ---[ end Kernel panic - not syncing: Fatal exception in interrupt

----- End forwarded message -----

[PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Ken-ichirou MATSUZAWA]

 Hi,

It would be better if someone else is working on this.
Or could you help me out? It's a tough work for me.

Changes from RFC:
  * remove netlink_skb_zerocopy()
    I made a big mistake, and ``len <= skb_tailroom(to)'' for mmaped
    skb is always true in nfqnl_build_packet_message()

  * copy portid in netlink_skb_copy()
    for netlink_lookup() in __netlink_dump_start()

[PATCHv1 net-next 1/5] netlink: mmap: introduce mmaped skb helper functions

[Ken-ichirou MATSUZAWA]

It seems that we need helper functions for skb which is allocated
at netlink_alloc_skb() since it does not have skb_shared_info.

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 include/linux/netlink.h  |   21 +++++-------------
 net/netlink/af_netlink.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 9120edb..60492bf 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -70,6 +70,11 @@ extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
 extern int netlink_has_listeners(struct sock *sk, unsigned int group);
 extern struct sk_buff *netlink_alloc_skb(struct sock *ssk, unsigned int size,
 					 u32 dst_portid, gfp_t gfp_mask);
+extern struct sk_buff *netlink_skb_copy(const struct sk_buff *skb, gfp_t gfp_mask);
+extern struct sk_buff *netlink_skb_clone(struct sk_buff *skb, gfp_t gfp_mask);
+extern void netlink_free_skb(struct sk_buff *skb);
+void netlink_consume_skb(struct sk_buff *skb);
+
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 portid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 portid,
 			     __u32 group, gfp_t allocation);
@@ -88,22 +93,6 @@ int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
 void netlink_detachskb(struct sock *sk, struct sk_buff *skb);
 int netlink_sendskb(struct sock *sk, struct sk_buff *skb);
 
-static inline struct sk_buff *
-netlink_skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
-{
-	struct sk_buff *nskb;
-
-	nskb = skb_clone(skb, gfp_mask);
-	if (!nskb)
-		return NULL;
-
-	/* This is a large skb, set destructor callback to release head */
-	if (is_vmalloc_addr(skb->head))
-		nskb->destructor = skb->destructor;
-
-	return nskb;
-}
-
 /*
  *	skb should fit one page. This choice is good for headerless malloc.
  *	But we should limit to 8K so that userspace does not have to
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index d8e2e39..98ed579 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1894,6 +1894,61 @@ out:
 }
 EXPORT_SYMBOL_GPL(netlink_alloc_skb);
 
+struct sk_buff *netlink_skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
+{
+#ifdef CONFIG_NETLINK_MMAP
+	if (netlink_skb_is_mmaped(skb)) {
+		struct sk_buff *n = alloc_skb(skb->len, gfp_mask);
+		if (!n)
+			return NULL;
+
+		skb_put(n, skb->len);
+		NETLINK_CB(n).portid = NETLINK_CB(skb).portid;
+		memcpy(n->data, skb->data, skb->len);
+		return n;
+	} else
+#endif
+		return skb_copy(skb, gfp_mask);
+}
+EXPORT_SYMBOL_GPL(netlink_skb_copy);
+
+struct sk_buff *netlink_skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
+{
+	struct sk_buff *nskb;
+
+#ifdef CONFIG_NETLINK_MMAP
+	if (netlink_skb_is_mmaped(skb))
+		return netlink_skb_copy(skb, gfp_mask);
+#endif
+	nskb = skb_clone(skb, gfp_mask);
+	if (!nskb)
+		return NULL;
+
+	/* This is a large skb, set destructor callback to release head */
+	if (is_vmalloc_addr(skb->head))
+		nskb->destructor = skb->destructor;
+
+	return nskb;
+}
+EXPORT_SYMBOL_GPL(netlink_skb_clone);
+
+void netlink_free_skb(struct sk_buff *skb)
+{
+	kfree_skb_partial(skb, netlink_skb_is_mmaped(skb));
+}
+EXPORT_SYMBOL_GPL(netlink_free_skb);
+
+void netlink_consume_skb(struct sk_buff *skb)
+{
+#ifdef CONFIG_NETLINK_MMAP
+	if (netlink_skb_is_mmaped(skb))
+		kfree_skb_partial(skb, true);
+	else
+#endif
+		consume_skb(skb);
+}
+EXPORT_SYMBOL_GPL(netlink_consume_skb);
+
 int netlink_has_listeners(struct sock *sk, unsigned int group)
 {
 	int res = 0;
-- 
1.7.10.4

[PATCHv1 net-next 2/5] netlink: mmap: apply mmaped skb helper functions

[Ken-ichirou MATSUZAWA]

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 net/netfilter/nfnetlink_log.c        |    2 +-
 net/netfilter/nfnetlink_queue_core.c |    6 +++---
 net/netlink/af_netlink.c             |   26 +++++++++++++-------------
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 4670821..cca2c50 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -357,7 +357,7 @@ __nfulnl_send(struct nfulnl_instance *inst)
 						 0);
 		if (WARN_ONCE(!nlh, "bad nlskb size: %u, tailroom %d\n",
 			      inst->skb->len, skb_tailroom(inst->skb))) {
-			kfree_skb(inst->skb);
+			netlink_free_skb(inst->skb);
 			goto out;
 		}
 	}
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 685cc6a..8d07036 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -389,7 +389,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			sizeof(struct nfgenmsg), 0);
 	if (!nlh) {
 		skb_tx_error(entskb);
-		kfree_skb(skb);
+		netlink_free_skb(skb);
 		return NULL;
 	}
 	nfmsg = nlmsg_data(nlh);
@@ -536,7 +536,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 
 nla_put_failure:
 	skb_tx_error(entskb);
-	kfree_skb(skb);
+	netlink_free_skb(skb);
 	net_err_ratelimited("nf_queue: error creating packet message\n");
 	return NULL;
 }
@@ -584,7 +584,7 @@ __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
 	return 0;
 
 err_out_free_nskb:
-	kfree_skb(nskb);
+	netlink_free_skb(nskb);
 err_out_unlock:
 	spin_unlock_bh(&queue->lock);
 	if (failopen)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 98ed579..45c8502 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -205,7 +205,7 @@ static int __netlink_deliver_tap_skb(struct sk_buff *skb,
 	int ret = -ENOMEM;
 
 	dev_hold(dev);
-	nskb = skb_clone(skb, GFP_ATOMIC);
+	nskb = netlink_skb_clone(skb, GFP_ATOMIC);
 	if (nskb) {
 		nskb->dev = dev;
 		nskb->protocol = htons((u16) sk->sk_protocol);
@@ -764,7 +764,7 @@ static int netlink_mmap_sendmsg(struct sock *sk, struct msghdr *msg,
 
 		err = security_netlink_send(sk, skb);
 		if (err) {
-			kfree_skb(skb);
+			kfree_skb_partial(skb, true);
 			goto out;
 		}
 
@@ -804,7 +804,7 @@ static void netlink_queue_mmaped_skb(struct sock *sk, struct sk_buff *skb)
 	netlink_set_status(hdr, NL_MMAP_STATUS_VALID);
 
 	NETLINK_CB(skb).flags |= NETLINK_SKB_DELIVERED;
-	kfree_skb(skb);
+	kfree_skb_partial(skb, true);
 }
 
 static void netlink_ring_set_copied(struct sock *sk, struct sk_buff *skb)
@@ -1804,7 +1804,7 @@ int netlink_unicast(struct sock *ssk, struct sk_buff *skb,
 retry:
 	sk = netlink_getsockbyportid(ssk, portid);
 	if (IS_ERR(sk)) {
-		kfree_skb(skb);
+		netlink_free_skb(skb);
 		return PTR_ERR(sk);
 	}
 	if (netlink_is_kernel(sk))
@@ -1812,7 +1812,7 @@ retry:
 
 	if (sk_filter(sk, skb)) {
 		err = skb->len;
-		kfree_skb(skb);
+		netlink_free_skb(skb);
 		sock_put(sk);
 		return err;
 	}
@@ -1876,7 +1876,7 @@ struct sk_buff *netlink_alloc_skb(struct sock *ssk, unsigned int size,
 	return skb;
 
 err2:
-	kfree_skb(skb);
+	kfree_skb_partial(skb, true);
 	spin_unlock_bh(&sk->sk_receive_queue.lock);
 	netlink_overrun(sk);
 err1:
@@ -1884,7 +1884,7 @@ err1:
 	return NULL;
 
 out_free:
-	kfree_skb(skb);
+	kfree_skb_partial(skb, true);
 	spin_unlock_bh(&sk->sk_receive_queue.lock);
 out_put:
 	sock_put(sk);
@@ -2029,7 +2029,7 @@ static void do_one_broadcast(struct sock *sk,
 	sock_hold(sk);
 	if (p->skb2 == NULL) {
 		if (skb_shared(p->skb)) {
-			p->skb2 = skb_clone(p->skb, p->allocation);
+			p->skb2 = netlink_skb_clone(p->skb, p->allocation);
 		} else {
 			p->skb2 = skb_get(p->skb);
 			/*
@@ -2105,7 +2105,7 @@ int netlink_broadcast_filtered(struct sock *ssk, struct sk_buff *skb, u32 portid
 	sk_for_each_bound(sk, &nl_table[ssk->sk_protocol].mc_list)
 		do_one_broadcast(sk, &info);
 
-	consume_skb(skb);
+	netlink_consume_skb(skb);
 
 	netlink_unlock_table();
 
@@ -2810,7 +2810,7 @@ static int netlink_dump(struct sock *sk)
 		mutex_unlock(nlk->cb_mutex);
 
 		if (sk_filter(sk, skb))
-			kfree_skb(skb);
+			netlink_free_skb(skb);
 		else
 			__netlink_sendskb(sk, skb);
 		return 0;
@@ -2825,7 +2825,7 @@ static int netlink_dump(struct sock *sk)
 	memcpy(nlmsg_data(nlh), &len, sizeof(len));
 
 	if (sk_filter(sk, skb))
-		kfree_skb(skb);
+		netlink_free_skb(skb);
 	else
 		__netlink_sendskb(sk, skb);
 
@@ -2840,7 +2840,7 @@ static int netlink_dump(struct sock *sk)
 
 errout_skb:
 	mutex_unlock(nlk->cb_mutex);
-	kfree_skb(skb);
+	netlink_free_skb(skb);
 	return err;
 }
 
@@ -2858,7 +2858,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 	 * a reference to the skb.
 	 */
 	if (netlink_skb_is_mmaped(skb)) {
-		skb = skb_copy(skb, GFP_KERNEL);
+		skb = netlink_skb_copy(skb, GFP_KERNEL);
 		if (skb == NULL)
 			return -ENOBUFS;
 	} else
-- 
1.7.10.4

[PATCHv1 net-next 3/5] netlink: mmap: fix status for not delivered skb

[Ken-ichirou MATSUZAWA]

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 net/netlink/af_netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 45c8502..c03fad0 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -864,7 +864,7 @@ static void netlink_skb_destructor(struct sk_buff *skb)
 		} else {
 			if (!(NETLINK_CB(skb).flags & NETLINK_SKB_DELIVERED)) {
 				hdr->nm_len = 0;
-				netlink_set_status(hdr, NL_MMAP_STATUS_VALID);
+				netlink_set_status(hdr, NL_MMAP_STATUS_UNUSED);
 			}
 			ring = &nlk_sk(sk)->rx_ring;
 		}
-- 
1.7.10.4

[PATCHv1 net-next 4/5] netlink: mmap: update tx type check

[Ken-ichirou MATSUZAWA]

We need to accept msg_iter.type 1(WRITE) which is set in sendto/sendmsg.

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 net/netlink/af_netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c03fad0..d8f5151 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2451,7 +2451,7 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	 * sendmsg(), but that's what we've got...
 	 */
 	if (netlink_tx_is_mmaped(sk) &&
-	    msg->msg_iter.type == ITER_IOVEC &&
+	    !(msg->msg_iter.type & (ITER_KVEC | ITER_BVEC)) &&
 	    msg->msg_iter.nr_segs == 1 &&
 	    msg->msg_iter.iov->iov_base == NULL) {
 		err = netlink_mmap_sendmsg(sk, msg, dst_portid, dst_group,
-- 
1.7.10.4

[PATCHv1 net-next 5/5] netlink: mmap: notify only when NL_MMAP_STATUS_VALID frame exists

[Ken-ichirou MATSUZAWA]

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 net/netlink/af_netlink.c |   28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index d8f5151..9f9e9ac 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -593,16 +593,6 @@ netlink_current_frame(const struct netlink_ring *ring,
 	return netlink_lookup_frame(ring, ring->head, status);
 }
 
-static struct nl_mmap_hdr *
-netlink_previous_frame(const struct netlink_ring *ring,
-		       enum nl_mmap_status status)
-{
-	unsigned int prev;
-
-	prev = ring->head ? ring->head - 1 : ring->frame_max;
-	return netlink_lookup_frame(ring, prev, status);
-}
-
 static void netlink_increment_head(struct netlink_ring *ring)
 {
 	ring->head = ring->head != ring->frame_max ? ring->head + 1 : 0;
@@ -623,6 +613,21 @@ static void netlink_forward_ring(struct netlink_ring *ring)
 	} while (ring->head != head);
 }
 
+static bool netlink_has_valid_frame(struct netlink_ring *ring)
+{
+	unsigned int head = ring->head, pos = head;
+	const struct nl_mmap_hdr *hdr;
+
+	do {
+		hdr = __netlink_lookup_frame(ring, pos);
+		if (hdr->nm_status == NL_MMAP_STATUS_VALID)
+			return true;
+		pos = pos != ring->frame_max ? pos + 1 : 0;
+	} while (pos != head);
+
+	return false;
+}
+
 static bool netlink_dump_space(struct netlink_sock *nlk)
 {
 	struct netlink_ring *ring = &nlk->rx_ring;
@@ -670,8 +675,7 @@ static unsigned int netlink_poll(struct file *file, struct socket *sock,
 
 	spin_lock_bh(&sk->sk_receive_queue.lock);
 	if (nlk->rx_ring.pg_vec) {
-		netlink_forward_ring(&nlk->rx_ring);
-		if (!netlink_previous_frame(&nlk->rx_ring, NL_MMAP_STATUS_UNUSED))
+		if (netlink_has_valid_frame(&nlk->rx_ring))
 			mask |= POLLIN | POLLRDNORM;
 	}
 	spin_unlock_bh(&sk->sk_receive_queue.lock);
-- 
1.7.10.4

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[David Miller]

From: Ken-ichirou MATSUZAWA <chamaken@gmail.com>
Date: Wed, 12 Aug 2015 17:28:24 +0900

> It would be better if someone else is working on this.
> Or could you help me out? It's a tough work for me.

If you are fixing a panic, you must show that panic message
and describe in detail the exact sequence of events and state
that lead to that crash.  And then also exactly why your fix
takes care of the problem, and why it is the proper way to
approach the issue.

As-is, your commit message and this top-level introductory
posting are way too terse and I myself can't even figure out
what exactly the purpose of this series is, and what bug it
is precisely fixing.

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Ken-ichirou MATSUZAWA]

 Hi,

Thank you for taking your time.
Please let me explain these with code samples on gist.
I can not describe and arrange it well, sorry.
 
    normal socket nflog sample:
    https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/nflog.c

set iptables

    iptables -A INPUT -p icmp --icmp-type echo-request \
        -j NFLOG --nflog-group 2 --nflog-threshold 4

monitor nlmon (like netsniff-ng), run this sample and
ping -i 0.2 -c 10 from another hosts. This sample only shows receive
size and nlmsg_type. Same things can be done with rx mmaped socket.

    rx only mmaped nflog sample:
    https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/rxring-nflog.c

This sample gets a panic if monitoring nlmon.

    panic message:
    https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/mmaped_netlink_panic

I think it's because of accessing a skb_shared_info when releasing
skb, although mmaped netlink skb does not have a skb_shared_info. I
tried to fix this at patch 1 and 2 by introducing helper function
which will not access a skb_shared_info.

And I think nm_status should be set to UNUSED when releasing it so
also tried to fix it patch 3.

----

With both tx/rx mmaped,

    both tx/rx mmaped nflog sample:
    https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/ring-nflog.c

This sample will not work, since msg->msg_iter.type in
netlink_sendmsg() is set to 1 (WRITE) when this sample calls
sendto(). patch 4 fix this by accepting it.

----

After applying patch 1 and 2, rx only sample can work but it behaves
differ from normal one. patch 5 may fix this.

And it also works well with my another code which set frame
nm_status to SKIP and passes it to worker threads and the worker
threads set status to UNUSED, even though ring becomes full.

That my another code may set UNUSED status in random, not
sequensially, so that it seems I need to check whole ring.

Thanks,

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Daniel Borkmann]

On 08/14/2015 10:58 AM, Ken-ichirou MATSUZAWA wrote:
>   Hi,
>
> Thank you for taking your time.
> Please let me explain these with code samples on gist.
> I can not describe and arrange it well, sorry.
>
>      normal socket nflog sample:
>      https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/nflog.c
>
> set iptables
>
>      iptables -A INPUT -p icmp --icmp-type echo-request \
>          -j NFLOG --nflog-group 2 --nflog-threshold 4
>
> monitor nlmon (like netsniff-ng), run this sample and
> ping -i 0.2 -c 10 from another hosts. This sample only shows receive
> size and nlmsg_type. Same things can be done with rx mmaped socket.
>
>      rx only mmaped nflog sample:
>      https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/rxring-nflog.c
>
> This sample gets a panic if monitoring nlmon.
>
>      panic message:
>      https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/mmaped_netlink_panic
>
> I think it's because of accessing a skb_shared_info when releasing
> skb, although mmaped netlink skb does not have a skb_shared_info. I
> tried to fix this at patch 1 and 2 by introducing helper function
> which will not access a skb_shared_info.
>
> And I think nm_status should be set to UNUSED when releasing it so
> also tried to fix it patch 3.

Ok, I'm trying to understand the issue: you are saying that whenever
there's an skb_clone on an netlink mmaped skb, we have the situation
that skb->data, skb->head etc points to the mmaped user space buffer
slot, and thus _must_ have no shared info.

Currently, what happens is that the shared info accesses whatever
memory is there in the mmaped region. So when you already do an
skb_clone() you should already get into trouble right there f.e. when
we test for orphaning frags etc (if at the right offset in the mmap
buffer, the tx_flags member would contain a SKBTX_DEV_ZEROCOPY bit).

> ----
>
> With both tx/rx mmaped,
>
>      both tx/rx mmaped nflog sample:
>      https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/ring-nflog.c
>
> This sample will not work, since msg->msg_iter.type in
> netlink_sendmsg() is set to 1 (WRITE) when this sample calls
> sendto(). patch 4 fix this by accepting it.
>
> ----
>
> After applying patch 1 and 2, rx only sample can work but it behaves
> differ from normal one. patch 5 may fix this.
>
> And it also works well with my another code which set frame
> nm_status to SKIP and passes it to worker threads and the worker
> threads set status to UNUSED, even though ring becomes full.
>
> That my another code may set UNUSED status in random, not
> sequensially, so that it seems I need to check whole ring.
>
> Thanks,
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Daniel Borkmann]

On 08/14/2015 12:01 PM, Daniel Borkmann wrote:
> On 08/14/2015 10:58 AM, Ken-ichirou MATSUZAWA wrote:
>>   Hi,
>>
>> Thank you for taking your time.
>> Please let me explain these with code samples on gist.
>> I can not describe and arrange it well, sorry.
>>
>>      normal socket nflog sample:
>>      https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/nflog.c
>>
>> set iptables
>>
>>      iptables -A INPUT -p icmp --icmp-type echo-request \
>>          -j NFLOG --nflog-group 2 --nflog-threshold 4
>>
>> monitor nlmon (like netsniff-ng), run this sample and
>> ping -i 0.2 -c 10 from another hosts. This sample only shows receive
>> size and nlmsg_type. Same things can be done with rx mmaped socket.
>>
>>      rx only mmaped nflog sample:
>>      https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/rxring-nflog.c
>>
>> This sample gets a panic if monitoring nlmon.
>>
>>      panic message:
>>      https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/mmaped_netlink_panic
>>
>> I think it's because of accessing a skb_shared_info when releasing
>> skb, although mmaped netlink skb does not have a skb_shared_info. I
>> tried to fix this at patch 1 and 2 by introducing helper function
>> which will not access a skb_shared_info.
>>
>> And I think nm_status should be set to UNUSED when releasing it so
>> also tried to fix it patch 3.
>
> Ok, I'm trying to understand the issue: you are saying that whenever
> there's an skb_clone on an netlink mmaped skb, we have the situation
> that skb->data, skb->head etc points to the mmaped user space buffer
> slot, and thus _must_ have no shared info.
>
> Currently, what happens is that the shared info accesses whatever
> memory is there in the mmaped region. So when you already do an
> skb_clone() you should already get into trouble right there f.e. when
> we test for orphaning frags etc (if at the right offset in the mmap
> buffer, the tx_flags member would contain a SKBTX_DEV_ZEROCOPY bit).

Ken-ichirou, have you observed this issue only in relation to nlmon?
Haven't checked yet if there are any upper layer netlink consumers that
would call for some reason into skb_clone() as well. I am thinking that
if taps are indeed the only ones affected, it might probably not be
worth adding that much complexity for a fix itself, but to keep it simple
instead. I don't know if there are any real users of netlink mmap, but
if it would really be needed, we could think about it on a net-next basis?
It seems you have some other, separate fixes in your series, so you might
want to submit them separately against the net tree, instead?

  include/linux/netlink.h  |  4 ++++
  net/netlink/af_netlink.c | 12 +++++++-----
  2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 9120edb..42cdcd8 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -35,6 +35,10 @@ struct netlink_skb_parms {
  #define NETLINK_CB(skb)		(*(struct netlink_skb_parms*)&((skb)->cb))
  #define NETLINK_CREDS(skb)	(&NETLINK_CB((skb)).creds)

+static inline bool netlink_skb_is_mmaped(const struct sk_buff *skb)
+{
+	return NETLINK_CB(skb).flags & NETLINK_SKB_MMAPED;
+}

  extern void netlink_table_grab(void);
  extern void netlink_table_ungrab(void);
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 67d2104..4307446 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -238,6 +238,13 @@ static void __netlink_deliver_tap(struct sk_buff *skb)

  static void netlink_deliver_tap(struct sk_buff *skb)
  {
+	/* Netlink mmaped skbs must not access shared info, and thus
+	 * are not allowed to be cloned. For now, just don't allow
+	 * them to get inspected by taps.
+	 */
+	if (netlink_skb_is_mmaped(skb))
+		return;
+
  	rcu_read_lock();

  	if (unlikely(!list_empty(&netlink_tap_all)))
@@ -278,11 +285,6 @@ static void netlink_rcv_wake(struct sock *sk)
  }

  #ifdef CONFIG_NETLINK_MMAP
-static bool netlink_skb_is_mmaped(const struct sk_buff *skb)
-{
-	return NETLINK_CB(skb).flags & NETLINK_SKB_MMAPED;
-}
-
  static bool netlink_rx_is_mmaped(struct sock *sk)
  {
  	return nlk_sk(sk)->rx_ring.pg_vec != NULL;
-- 
1.9.3




>> ----
>>
>> With both tx/rx mmaped,
>>
>>      both tx/rx mmaped nflog sample:
>>      https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/2d6da8fff31ef61af77e68713fdb1d71978746a6/ring-nflog.c
>>
>> This sample will not work, since msg->msg_iter.type in
>> netlink_sendmsg() is set to 1 (WRITE) when this sample calls
>> sendto(). patch 4 fix this by accepting it.
>>
>> ----
>>
>> After applying patch 1 and 2, rx only sample can work but it behaves
>> differ from normal one. patch 5 may fix this.
>>
>> And it also works well with my another code which set frame
>> nm_status to SKIP and passes it to worker threads and the worker
>> threads set status to UNUSED, even though ring becomes full.
>>
>> That my another code may set UNUSED status in random, not
>> sequensially, so that it seems I need to check whole ring.
>>
>> Thanks,
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Ken-ichirou MATSUZAWA]

 Hi,

Thank you for taking your time and trying to understand, even though
one of samples is wrong. correct one is:

    rx only mmaped nflog sample:
    https://gist.github.com/chamaken/dc0f80c14862e8061c06/raw/365c8a106840368f313a3791958da9be0f5fbed0/rxring-nflog.c

> >Currently, what happens is that the shared info accesses whatever
> >memory is there in the mmaped region. So when you already do an
> >skb_clone() you should already get into trouble right there f.e. when
> >we test for orphaning frags etc (if at the right offset in the mmap
> >buffer, the tx_flags member would contain a SKBTX_DEV_ZEROCOPY bit).

And I'm afraid of a skb which does not have shared info can be released
by kfree_skb or not if the next frame is valid. i.e. the current
skb->end, shared info points to the next frame's nm_status, say
NL_MMAP_STATUS_SKIP, and handle it as shared info pointer.

> Ken-ichirou, have you observed this issue only in relation to nlmon?

Yes,

> if taps are indeed the only ones affected, it might probably not be
> worth adding that much complexity for a fix itself, but to keep it simple
> instead. I don't know if there are any real users of netlink mmap, but

You mean mmaped skb can not be monitored by nlmon for a while?
I'll follow you, it's tough for me to fix this issue.

> It seems you have some other, separate fixes in your series, so you might
> want to submit them separately against the net tree, instead?

I'll follow you too.
Thank you, I appreciate.

>  include/linux/netlink.h  |  4 ++++
>  net/netlink/af_netlink.c | 12 +++++++-----
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 9120edb..42cdcd8 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -35,6 +35,10 @@ struct netlink_skb_parms {
>  #define NETLINK_CB(skb)		(*(struct netlink_skb_parms*)&((skb)->cb))
>  #define NETLINK_CREDS(skb)	(&NETLINK_CB((skb)).creds)
> 
> +static inline bool netlink_skb_is_mmaped(const struct sk_buff *skb)
> +{
> +	return NETLINK_CB(skb).flags & NETLINK_SKB_MMAPED;
> +}
> 
>  extern void netlink_table_grab(void);
>  extern void netlink_table_ungrab(void);
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 67d2104..4307446 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -238,6 +238,13 @@ static void __netlink_deliver_tap(struct sk_buff *skb)
> 
>  static void netlink_deliver_tap(struct sk_buff *skb)
>  {
> +	/* Netlink mmaped skbs must not access shared info, and thus
> +	 * are not allowed to be cloned. For now, just don't allow
> +	 * them to get inspected by taps.
> +	 */
> +	if (netlink_skb_is_mmaped(skb))
> +		return;
> +
>  	rcu_read_lock();
> 
>  	if (unlikely(!list_empty(&netlink_tap_all)))
> @@ -278,11 +285,6 @@ static void netlink_rcv_wake(struct sock *sk)
>  }
> 
>  #ifdef CONFIG_NETLINK_MMAP
> -static bool netlink_skb_is_mmaped(const struct sk_buff *skb)
> -{
> -	return NETLINK_CB(skb).flags & NETLINK_SKB_MMAPED;
> -}
> -
>  static bool netlink_rx_is_mmaped(struct sock *sk)
>  {
>  	return nlk_sk(sk)->rx_ring.pg_vec != NULL;
> -- 
> 1.9.3

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Fri, 14 Aug 2015 12:38:21 +0200

> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 67d2104..4307446 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -238,6 +238,13 @@ static void __netlink_deliver_tap(struct sk_buff
> *skb)
> 
>  static void netlink_deliver_tap(struct sk_buff *skb)
>  {
> +	/* Netlink mmaped skbs must not access shared info, and thus
> +	 * are not allowed to be cloned. For now, just don't allow
> +	 * them to get inspected by taps.
> +	 */
> +	if (netlink_skb_is_mmaped(skb))
> +		return;
> +

I would seriously rather see us do an expensive full copy of the SKB
than to have traffic which is unexpectedly invisible to taps.

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Daniel Borkmann]

On 08/17/2015 11:02 PM, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Fri, 14 Aug 2015 12:38:21 +0200
>
>> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
>> index 67d2104..4307446 100644
>> --- a/net/netlink/af_netlink.c
>> +++ b/net/netlink/af_netlink.c
>> @@ -238,6 +238,13 @@ static void __netlink_deliver_tap(struct sk_buff
>> *skb)
>>
>>   static void netlink_deliver_tap(struct sk_buff *skb)
>>   {
>> +	/* Netlink mmaped skbs must not access shared info, and thus
>> +	 * are not allowed to be cloned. For now, just don't allow
>> +	 * them to get inspected by taps.
>> +	 */
>> +	if (netlink_skb_is_mmaped(skb))
>> +		return;
>> +
>
> I would seriously rather see us do an expensive full copy of the SKB
> than to have traffic which is unexpectedly invisible to taps.

Do you mean generically as we do in TX path, or only in this
particular scenario?

Thanks,
Daniel

[PATCH net] netlink: mmap: fix tx type check

[Ken-ichirou MATSUZAWA]

I can't send netlink message via mmaped netlink socket since

    commit: a8866ff6a5bce7d0ec465a63bc482a85c09b0d39
    netlink: make the check for "send from tx_ring" deterministic

msg->msg_iter.type is set to WRITE (1) at

    SYSCALL_DEFINE6(sendto, ...
        import_single_range(WRITE, ...
            iov_iter_init(1, WRITE, ...

call path, so that we need to check the type by iter_is_iovec()
to accept the WRITE.

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 net/netlink/af_netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 85ccd8b..4444687 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2456,7 +2456,7 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	 * sendmsg(), but that's what we've got...
 	 */
 	if (netlink_tx_is_mmaped(sk) &&
-	    msg->msg_iter.type == ITER_IOVEC &&
+	    iter_is_iovec(&msg->msg_iter) &&
 	    msg->msg_iter.nr_segs == 1 &&
 	    msg->msg_iter.iov->iov_base == NULL) {
 		err = netlink_mmap_sendmsg(sk, msg, dst_portid, dst_group,
-- 
1.7.10.4

[PATCH net] netlink: rx mmap: fix POLLIN condition

[Ken-ichirou MATSUZAWA]

Now poll() returns immediately after setting kernel current frame
(ring->head) to SKIP from user space even if there are no new
frames. And in a case of all frames is VALID, user space program
unintensionally sets (only) kernel current frame to UNUSED, then
calls poll(), it will not return immediately even though there are
VALID frames.

To avoid situations like above, I think we need to scan all frames
to find a VALID frame at poll() like netlink_alloc_skb(),
netlink_forward_ring() finding an UNUSED frame at skb allocation.

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 net/netlink/af_netlink.c |   28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 4444687..c65ec2e 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -593,16 +593,6 @@ netlink_current_frame(const struct netlink_ring *ring,
 	return netlink_lookup_frame(ring, ring->head, status);
 }
 
-static struct nl_mmap_hdr *
-netlink_previous_frame(const struct netlink_ring *ring,
-		       enum nl_mmap_status status)
-{
-	unsigned int prev;
-
-	prev = ring->head ? ring->head - 1 : ring->frame_max;
-	return netlink_lookup_frame(ring, prev, status);
-}
-
 static void netlink_increment_head(struct netlink_ring *ring)
 {
 	ring->head = ring->head != ring->frame_max ? ring->head + 1 : 0;
@@ -623,6 +613,21 @@ static void netlink_forward_ring(struct netlink_ring *ring)
 	} while (ring->head != head);
 }
 
+static bool netlink_has_valid_frame(struct netlink_ring *ring)
+{
+	unsigned int head = ring->head, pos = head;
+	const struct nl_mmap_hdr *hdr;
+
+	do {
+		hdr = __netlink_lookup_frame(ring, pos);
+		if (hdr->nm_status == NL_MMAP_STATUS_VALID)
+			return true;
+		pos = pos != 0 ? pos - 1 : ring->frame_max;
+	} while (pos != head);
+
+	return false;
+}
+
 static bool netlink_dump_space(struct netlink_sock *nlk)
 {
 	struct netlink_ring *ring = &nlk->rx_ring;
@@ -670,8 +675,7 @@ static unsigned int netlink_poll(struct file *file, struct socket *sock,
 
 	spin_lock_bh(&sk->sk_receive_queue.lock);
 	if (nlk->rx_ring.pg_vec) {
-		netlink_forward_ring(&nlk->rx_ring);
-		if (!netlink_previous_frame(&nlk->rx_ring, NL_MMAP_STATUS_UNUSED))
+		if (netlink_has_valid_frame(&nlk->rx_ring))
 			mask |= POLLIN | POLLRDNORM;
 	}
 	spin_unlock_bh(&sk->sk_receive_queue.lock);
-- 
1.7.10.4

[PATCH net] netlink: mmap: fix status setting in skb destructor

[Ken-ichirou MATSUZAWA]

I don't know the intension of setting VALID status in the skb
destructor. But I think it need to be set UNUSED status in case of
error then release skb, or rx ring might be filled with RESERVED
frames.

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 net/netlink/af_netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index e6134f4..85ccd8b 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -864,7 +864,7 @@ static void netlink_skb_destructor(struct sk_buff *skb)
 		} else {
 			if (!(NETLINK_CB(skb).flags & NETLINK_SKB_DELIVERED)) {
 				hdr->nm_len = 0;
-				netlink_set_status(hdr, NL_MMAP_STATUS_VALID);
+				netlink_set_status(hdr, NL_MMAP_STATUS_UNUSED);
 			}
 			ring = &nlk_sk(sk)->rx_ring;
 		}
-- 
1.7.10.4

Re: [PATCH net] netlink: mmap: fix tx type check

[David Miller]

From: Ken-ichirou MATSUZAWA <chamaken@gmail.com>
Date: Thu, 20 Aug 2015 12:43:53 +0900

> I can't send netlink message via mmaped netlink socket since
> 
>     commit: a8866ff6a5bce7d0ec465a63bc482a85c09b0d39
>     netlink: make the check for "send from tx_ring" deterministic
> 
> msg->msg_iter.type is set to WRITE (1) at
> 
>     SYSCALL_DEFINE6(sendto, ...
>         import_single_range(WRITE, ...
>             iov_iter_init(1, WRITE, ...
> 
> call path, so that we need to check the type by iter_is_iovec()
> to accept the WRITE.
> 
> Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>

Applied, thanks.

Re: [PATCH net] netlink: rx mmap: fix POLLIN condition

[David Miller]

From: Ken-ichirou MATSUZAWA <chamaken@gmail.com>
Date: Thu, 20 Aug 2015 14:54:47 +0900

> Now poll() returns immediately after setting kernel current frame
> (ring->head) to SKIP from user space even if there are no new
> frames. And in a case of all frames is VALID, user space program
> unintensionally sets (only) kernel current frame to UNUSED, then
> calls poll(), it will not return immediately even though there are
> VALID frames.
> 
> To avoid situations like above, I think we need to scan all frames
> to find a VALID frame at poll() like netlink_alloc_skb(),
> netlink_forward_ring() finding an UNUSED frame at skb allocation.
> 
> Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>

There seems to be a few issues here.

Taking a look at netlink_forward_ring(), it appears buggy.

	static void netlink_forward_ring(struct netlink_ring *ring)
	{
		unsigned int head = ring->head, pos = head;
		const struct nl_mmap_hdr *hdr;

		do {
			hdr = __netlink_lookup_frame(ring, pos);
			if (hdr->nm_status == NL_MMAP_STATUS_UNUSED)
				break;
			if (hdr->nm_status != NL_MMAP_STATUS_SKIP)
				break;
			netlink_increment_head(ring);
		} while (ring->head != head);
	}

No matter what any of this code does, __netlink_lookup_frame() is always
called with the same "pos" value.

So, as far as I can tell, it will look at the same ring entry header over
and over again, every time through this loop.

netlink_increment_head() changes ring->head, but this has no influence
upon the calculations made inside of __netlink_lookup_frame().

So if netlink_forward_ring() _actually_ sees an entry that we should
advance past, it will cycle through the whole ring, advancing ring->head
until it equals the "ring->head != head" loop test fails.

We should definitely fix this bug first.

As per your patch, I wonder if a backwards scan would be faster.

Re: [PATCH net] netlink: mmap: fix status setting in skb destructor

[David Miller]

From: Ken-ichirou MATSUZAWA <chamaken@gmail.com>
Date: Thu, 20 Aug 2015 16:07:33 +0900

> I don't know the intension of setting VALID status in the skb
> destructor. But I think it need to be set UNUSED status in case of
> error then release skb, or rx ring might be filled with RESERVED
> frames.
> 
> Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>

I think the idea is to have the user process this "zero length" frame
and advance the status itself.

I think it is probably racy and problematic to have the kernel set a
frame's state to UNUSED.  It is not a valid state transition for the
kernel side of RX ring processing.

Only the user can safely release ring entries back to the kernel.

Re: [PATCH net] netlink: rx mmap: fix POLLIN condition

[Ken-ichirou MATSUZAWA]

 Thank you for the reply.
 
On Tue, Aug 25, 2015 at 08:17:12PM -0700, David Miller wrote:
> So if netlink_forward_ring() _actually_ sees an entry that we should
> advance past, it will cycle through the whole ring, advancing ring->head
> until it equals the "ring->head != head" loop test fails.
> 
> We should definitely fix this bug first.

I should have realized it, sorry. I think the following patch will
fix it, would you review it?

> As per your patch, I wonder if a backwards scan would be faster.

I think so, thanks. I will resend it after netlink_forward_ring()
fix is applied.

Thanks,
Ken

[PATCH net] netlink: mmap: fix lookup frame position

[Ken-ichirou MATSUZAWA]

__netlink_lookup_frame() was always called with the same "pos"
value in netlink_forward_ring(). It will look at the same ring entry
header over and over again, every time through this loop. Then cycle
through the whole ring, advancing ring->head, not "pos" until it
equals the "ring->head != head" loop test fails.

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 net/netlink/af_netlink.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index a774985..39fa91f 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -610,11 +610,11 @@ static void netlink_increment_head(struct netlink_ring *ring)
 
 static void netlink_forward_ring(struct netlink_ring *ring)
 {
-	unsigned int head = ring->head, pos = head;
+	unsigned int head = ring->head;
 	const struct nl_mmap_hdr *hdr;
 
 	do {
-		hdr = __netlink_lookup_frame(ring, pos);
+		hdr = __netlink_lookup_frame(ring, ring->head);
 		if (hdr->nm_status == NL_MMAP_STATUS_UNUSED)
 			break;
 		if (hdr->nm_status != NL_MMAP_STATUS_SKIP)
-- 
1.7.10.4

Re: [PATCH net] netlink: mmap: fix status setting in skb destructor

[Ken-ichirou MATSUZAWA]

On Tue, Aug 25, 2015 at 08:22:03PM -0700, David Miller wrote:
> From: Ken-ichirou MATSUZAWA <chamaken@gmail.com>
> > I don't know the intension of setting VALID status in the skb
> > destructor. But I think it need to be set UNUSED status in case of
> 
> I think the idea is to have the user process this "zero length" frame
> and advance the status itself.
> 
> I think it is probably racy and problematic to have the kernel set a
> frame's state to UNUSED.  It is not a valid state transition for the
> kernel side of RX ring processing.
> 
> Only the user can safely release ring entries back to the kernel.

I will just update the frame status to UNUSED and advance ring
position in user space in case of nm_len == 0. Thank you.

Re: [PATCH net] netlink: mmap: fix lookup frame position

[David Miller]

From: Ken-ichirou MATSUZAWA <chamaken@gmail.com>
Date: Fri, 28 Aug 2015 16:05:20 +0900

> __netlink_lookup_frame() was always called with the same "pos"
> value in netlink_forward_ring(). It will look at the same ring entry
> header over and over again, every time through this loop. Then cycle
> through the whole ring, advancing ring->head, not "pos" until it
> equals the "ring->head != head" loop test fails.
> 
> Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>

Applied.

[PATCH net] netlink: rx mmap: fix POLLIN condition

[Ken-ichirou MATSUZAWA]

Poll() returns immediately after setting the kernel current frame
(ring->head) to SKIP from user space even though there is no new
frame. And in a case of all frames is VALID, user space program
unintensionally sets (only) kernel current frame to UNUSED, then
calls poll(), it will not return immediately even though there are
VALID frames.

To avoid situations like above, I think we need to scan all frames
to find VALID frames at poll() like netlink_alloc_skb(),
netlink_forward_ring() finding an UNUSED frame at skb allocation.

Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>
---
 net/netlink/af_netlink.c |   28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7965ca7..50889be 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -594,16 +594,6 @@ netlink_current_frame(const struct netlink_ring *ring,
 	return netlink_lookup_frame(ring, ring->head, status);
 }
 
-static struct nl_mmap_hdr *
-netlink_previous_frame(const struct netlink_ring *ring,
-		       enum nl_mmap_status status)
-{
-	unsigned int prev;
-
-	prev = ring->head ? ring->head - 1 : ring->frame_max;
-	return netlink_lookup_frame(ring, prev, status);
-}
-
 static void netlink_increment_head(struct netlink_ring *ring)
 {
 	ring->head = ring->head != ring->frame_max ? ring->head + 1 : 0;
@@ -624,6 +614,21 @@ static void netlink_forward_ring(struct netlink_ring *ring)
 	} while (ring->head != head);
 }
 
+static bool netlink_has_valid_frame(struct netlink_ring *ring)
+{
+	unsigned int head = ring->head, pos = head;
+	const struct nl_mmap_hdr *hdr;
+
+	do {
+		hdr = __netlink_lookup_frame(ring, pos);
+		if (hdr->nm_status == NL_MMAP_STATUS_VALID)
+			return true;
+		pos = pos != 0 ? pos - 1 : ring->frame_max;
+	} while (pos != head);
+
+	return false;
+}
+
 static bool netlink_dump_space(struct netlink_sock *nlk)
 {
 	struct netlink_ring *ring = &nlk->rx_ring;
@@ -671,8 +676,7 @@ static unsigned int netlink_poll(struct file *file, struct socket *sock,
 
 	spin_lock_bh(&sk->sk_receive_queue.lock);
 	if (nlk->rx_ring.pg_vec) {
-		netlink_forward_ring(&nlk->rx_ring);
-		if (!netlink_previous_frame(&nlk->rx_ring, NL_MMAP_STATUS_UNUSED))
+		if (netlink_has_valid_frame(&nlk->rx_ring))
 			mask |= POLLIN | POLLRDNORM;
 	}
 	spin_unlock_bh(&sk->sk_receive_queue.lock);
-- 
1.7.10.4

Re: [PATCH net] netlink: rx mmap: fix POLLIN condition

[David Miller]

From: Ken-ichirou MATSUZAWA <chamaken@gmail.com>
Date: Mon, 31 Aug 2015 07:54:49 +0900

> Poll() returns immediately after setting the kernel current frame
> (ring->head) to SKIP from user space even though there is no new
> frame. And in a case of all frames is VALID, user space program
> unintensionally sets (only) kernel current frame to UNUSED, then
> calls poll(), it will not return immediately even though there are
> VALID frames.
> 
> To avoid situations like above, I think we need to scan all frames
> to find VALID frames at poll() like netlink_alloc_skb(),
> netlink_forward_ring() finding an UNUSED frame at skb allocation.
> 
> Signed-off-by: Ken-ichirou MATSUZAWA <chamas@h4.dion.ne.jp>

Applied, thanks.

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Ken-ichirou MATSUZAWA]

On Wed, Aug 19, 2015 at 04:29:32PM +0200, Daniel Borkmann wrote:
> On 08/17/2015 11:02 PM, David Miller wrote:
> >From: Daniel Borkmann <daniel@iogearbox.net>
> >Date: Fri, 14 Aug 2015 12:38:21 +0200
> >
> >>diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> >>index 67d2104..4307446 100644
> >>--- a/net/netlink/af_netlink.c
> >>+++ b/net/netlink/af_netlink.c
> >>@@ -238,6 +238,13 @@ static void __netlink_deliver_tap(struct sk_buff
> >>*skb)
> >>
> >>  static void netlink_deliver_tap(struct sk_buff *skb)
> >>  {
> >>+	/* Netlink mmaped skbs must not access shared info, and thus
> >>+	 * are not allowed to be cloned. For now, just don't allow
> >>+	 * them to get inspected by taps.
> >>+	 */
> >>+	if (netlink_skb_is_mmaped(skb))
> >>+		return;
> >>+
> >
> >I would seriously rather see us do an expensive full copy of the SKB
> >than to have traffic which is unexpectedly invisible to taps.
> 
> Do you mean generically as we do in TX path, or only in this
> particular scenario?

It seems that we only handle this particular scenario.

I think I can understand the cause of the panic. The original mmaped
skb, allocated by netlink_alloc_skb, has a skb destructor which set
skb head NULL. This prevents from accessing shared info in
skb_release_all, so my concerns that the shared info of original skb
may be accessed in kfree_skb and may cause a panic, is unnecessary.

But since skb_clone does not copy the destructor, skb_release_all
calls skb_release_data for cloned skb, accessing shared info and
causes the panic, I think. Setting the destructor after clone could
not fix the problem since __dev_queue_xmit call path,
netif_skb_features?, accesses shared info.

Talking about skb_copy path, original skb's shared info is accessed
only in copy_skb_header, to get gso related field. As a result of
those, we can avoid the panic by:

  @@ -205,7 +205,10 @@ static int __netlink_deliver_tap_skb(struct sk_buff *skb,
        int ret = -ENOMEM;

        dev_hold(dev);
        -       nskb = skb_clone(skb, GFP_ATOMIC);
        +       if (netlink_skb_is_mmaped(skb))
        +               nskb = skb_copy(skb, GFP_ATOMIC);
        +       else
        +               nskb = skb_clone(skb, GFP_ATOMIC);
        
Thanks to you, my question become clear:
Should we set gso_size to 0 after copying to make skb_is_gso
returning false?

Thanks,

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Daniel Borkmann]

On 09/02/2015 02:04 AM, Ken-ichirou MATSUZAWA wrote:
> On Wed, Aug 19, 2015 at 04:29:32PM +0200, Daniel Borkmann wrote:
>> On 08/17/2015 11:02 PM, David Miller wrote:
>>> From: Daniel Borkmann <daniel@iogearbox.net>
>>> Date: Fri, 14 Aug 2015 12:38:21 +0200
>>>
>>>> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
>>>> index 67d2104..4307446 100644
>>>> --- a/net/netlink/af_netlink.c
>>>> +++ b/net/netlink/af_netlink.c
>>>> @@ -238,6 +238,13 @@ static void __netlink_deliver_tap(struct sk_buff
>>>> *skb)
>>>>
>>>>   static void netlink_deliver_tap(struct sk_buff *skb)
>>>>   {
>>>> +	/* Netlink mmaped skbs must not access shared info, and thus
>>>> +	 * are not allowed to be cloned. For now, just don't allow
>>>> +	 * them to get inspected by taps.
>>>> +	 */
>>>> +	if (netlink_skb_is_mmaped(skb))
>>>> +		return;
>>>> +
>>>
>>> I would seriously rather see us do an expensive full copy of the SKB
>>> than to have traffic which is unexpectedly invisible to taps.
>>
>> Do you mean generically as we do in TX path, or only in this
>> particular scenario?
>
> It seems that we only handle this particular scenario.
>
> I think I can understand the cause of the panic. The original mmaped
> skb, allocated by netlink_alloc_skb, has a skb destructor which set
> skb head NULL. This prevents from accessing shared info in
> skb_release_all, so my concerns that the shared info of original skb
> may be accessed in kfree_skb and may cause a panic, is unnecessary.
>
> But since skb_clone does not copy the destructor, skb_release_all
> calls skb_release_data for cloned skb, accessing shared info and
> causes the panic, I think. Setting the destructor after clone could
> not fix the problem since __dev_queue_xmit call path,
> netif_skb_features?, accesses shared info.
>
> Talking about skb_copy path, original skb's shared info is accessed
> only in copy_skb_header, to get gso related field. As a result of
> those, we can avoid the panic by:
>
>    @@ -205,7 +205,10 @@ static int __netlink_deliver_tap_skb(struct sk_buff *skb,
>          int ret = -ENOMEM;
>
>          dev_hold(dev);
>          -       nskb = skb_clone(skb, GFP_ATOMIC);
>          +       if (netlink_skb_is_mmaped(skb))
>          +               nskb = skb_copy(skb, GFP_ATOMIC);
>          +       else
>          +               nskb = skb_clone(skb, GFP_ATOMIC);
>
> Thanks to you, my question become clear:
> Should we set gso_size to 0 after copying to make skb_is_gso
> returning false?

It's still not correct. The thing is you can neither call skb_copy() nor
skb_clone() on netlink mmaped skbs. For example, skb_copy_bits() would
look at frags[] in the shared info and tries to copy them. I think adding
extra logic to skb_copy() would be very ugly just to accommodate for this
special case. We need an own netlink_mmap_to_full_skb() handler for this,
that copies/transforms this into a "normal" skb. I'll have a look it this
week.

Thanks,
Daniel

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Ken-ichirou MATSUZAWA]

Thank you for the reply.

On Wed, Sep 02, 2015 at 11:47:26AM +0200, Daniel Borkmann wrote:
> On 09/02/2015 02:04 AM, Ken-ichirou MATSUZAWA wrote:
> >Talking about skb_copy path, original skb's shared info is accessed
> >only in copy_skb_header, to get gso related field. As a result of
> 
> It's still not correct. The thing is you can neither call skb_copy() nor
> skb_clone() on netlink mmaped skbs. For example, skb_copy_bits() would

I am sorry for the lack of explanation.
And I am afraid I misunderstand...

Updated pointers to its data area in a mmaped netlink skb is only
its tail. Head, data and end will not be updated. skb_copy() calls

    int skb_copy_bits(const struct sk_buff *skb, int offset, void *to, int len)

as its argument, "offset" is always 0 and "len" is skb->len. In
skb_copy_bits() both "start" and "copy" are skb->len, which means
"len - copy" is always 0 so that retuns 0 before accessing shared
info.

I don't know the situation is intended or not, it seems that
skb_copy() for a mmaped skb will not access its shared info.

After that, copy_skb_header() will set newly allocate skb's (wrong)
gso fields, I asked we should clear it or not.

> special case. We need an own netlink_mmap_to_full_skb() handler for this,
> that copies/transforms this into a "normal" skb. I'll have a look it this

If the above situation is an unintentional, we need it to avoid a
future confusion.

Thanks,

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Daniel Borkmann]

On 09/02/2015 01:35 PM, Ken-ichirou MATSUZAWA wrote:
> Thank you for the reply.
>
> On Wed, Sep 02, 2015 at 11:47:26AM +0200, Daniel Borkmann wrote:
>> On 09/02/2015 02:04 AM, Ken-ichirou MATSUZAWA wrote:
>>> Talking about skb_copy path, original skb's shared info is accessed
>>> only in copy_skb_header, to get gso related field. As a result of
>>
>> It's still not correct. The thing is you can neither call skb_copy() nor
>> skb_clone() on netlink mmaped skbs. For example, skb_copy_bits() would
>
> I am sorry for the lack of explanation.
> And I am afraid I misunderstand...
>
> Updated pointers to its data area in a mmaped netlink skb is only
> its tail. Head, data and end will not be updated. skb_copy() calls
>
>      int skb_copy_bits(const struct sk_buff *skb, int offset, void *to, int len)
>
> as its argument, "offset" is always 0 and "len" is skb->len. In
> skb_copy_bits() both "start" and "copy" are skb->len, which means
> "len - copy" is always 0 so that retuns 0 before accessing shared
> info.
>
> I don't know the situation is intended or not, it seems that
> skb_copy() for a mmaped skb will not access its shared info.

Okay, right, since it's all linear, but ...

> After that, copy_skb_header() will set newly allocate skb's (wrong)
> gso fields, I asked we should clear it or not.

... here still we access skb_shinfo() from the mmap'ed skb, which we
are simply not allowed (despite whether resetting fields later on as
you suggest or not), for two reasons: I think (will start experimenting
more with it tomorrow), you would get an out of bounds access here in
case the skb->data is the last slot in the ring buffer and reaches
exactly to the ring buffer end. And (despite that), it's also hard
to maintain - the next one adding a new shared info member will very
likely oversee this special case in netlink here, thus the issue would
then simply be reintroduced over and over.

Thanks,
Daniel

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Ken-ichirou MATSUZAWA]

On Wed, Sep 02, 2015 at 05:56:36PM +0200, Daniel Borkmann wrote:
> you suggest or not), for two reasons: I think (will start experimenting
> more with it tomorrow), you would get an out of bounds access here in
> case the skb->data is the last slot in the ring buffer and reaches
> exactly to the ring buffer end. And (despite that), it's also hard

I thought accessing as a value, not a pointer, in thats wrong shared
info will not be a big problem, but

> to maintain - the next one adding a new shared info member will very
> likely oversee this special case in netlink here, thus the issue would
> then simply be reintroduced over and over.

I agree with you.
Thank you for taking your time. I think I have learned a lot.

Thanks,

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Daniel Borkmann]

On 08/17/2015 11:02 PM, David Miller wrote:
...
> I would seriously rather see us do an expensive full copy of the SKB
> than to have traffic which is unexpectedly invisible to taps.

I've been looking into this issue a bit further, so the copy for the
tap seems doable, but while further going through the code to find similar
issues elsewhere, and doing some experiments, it looks like we write
shared info also in some edge-cases of upcalls such as nfqueue or ovs
when mmaped netlink is used for rx. I did a test with nfqueue using
the libmnl mmap branch [1].

My test case reduced the hard-coded frame size in libmnl to 4096. I
then crafted via pktgen in receive mode skbs that f.e. have a MTU
of 9000, and which do contain page frags. Redirecting that traffic
into nfqueue with a listener that is rx mmaped, I can see that in case
of nfqnl_build_packet_message(), the kernel is, when invoking skb_zerocopy(),
failing the (len <= skb_tailroom(to)) condition, writing some header
part, and filling the rest with frags[] in the for loop (thus writing/
leaking into the user mmap buffer), so given the right preconditions,
the kernel wouldn't prevent us from doing that.

One way to fix it would be to change netlink_alloc_skb() and all its
call-sites to add another size param, say 'ldiff', that would contain
the size difference that would be needed to copy the non-linear parts
from the network skb into the linear ring buffer slot of the netlink skb.
Thus, that also in case it exceeds the slot size, the kernel will just
fall back to alloc_skb() inside of netlink_alloc_skb() and fill the
slot with status NL_MMAP_STATUS_COPY. With this, no doubt it's not
super pretty to add that additional size param, but the kernel could
still work with mmap receivers.

Other than that, I found an skb_copy() in __netlink_dump_start(), but
that is a left-over from tx mmap in netlink, so not an issue here.

Thanks,
Daniel

   [1] http://git.netfilter.org/libmnl/log/?h=nl-mmap

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Mon, 07 Sep 2015 16:54:46 +0200

> On 08/17/2015 11:02 PM, David Miller wrote:
> ...
>> I would seriously rather see us do an expensive full copy of the SKB
>> than to have traffic which is unexpectedly invisible to taps.
> 
> I've been looking into this issue a bit further, so the copy for the
> tap seems doable, but while further going through the code to find
> similar
> issues elsewhere, and doing some experiments, it looks like we write
> shared info also in some edge-cases of upcalls such as nfqueue or ovs
> when mmaped netlink is used for rx. I did a test with nfqueue using
> the libmnl mmap branch [1].

Honestly if it's something isolated to something like nf_queue it can
be contained to just being a special fix there.

nf_queue is usually very special and needs hacks to handle things
properly since it acts as an "escape" point for various SKB things.

But if it's in OVS too....

I guess we need a more generic fix.

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Thomas Graf]

On 09/07/15 at 04:54pm, Daniel Borkmann wrote:
> On 08/17/2015 11:02 PM, David Miller wrote:
> ...
> >I would seriously rather see us do an expensive full copy of the SKB
> >than to have traffic which is unexpectedly invisible to taps.
> 
> I've been looking into this issue a bit further, so the copy for the
> tap seems doable, but while further going through the code to find similar
> issues elsewhere, and doing some experiments, it looks like we write
> shared info also in some edge-cases of upcalls such as nfqueue or ovs
> when mmaped netlink is used for rx. I did a test with nfqueue using
> the libmnl mmap branch [1].

Note that OVS does not utilize mmaped netlink even though it has been
considered for a while. It is theoretically possible that non-OVS user
space user of the OVS netlink API is using it although I'm not aware
somebody actually does. We can probably fix this specifically for nfqueue.

Re: [PATCHv1 net-next 0/5] netlink: mmap: kernel panic and some issues

[Daniel Borkmann]

On 09/09/2015 10:53 AM, Thomas Graf wrote:
> On 09/07/15 at 04:54pm, Daniel Borkmann wrote:
>> On 08/17/2015 11:02 PM, David Miller wrote:
>> ...
>>> I would seriously rather see us do an expensive full copy of the SKB
>>> than to have traffic which is unexpectedly invisible to taps.
>>
>> I've been looking into this issue a bit further, so the copy for the
>> tap seems doable, but while further going through the code to find similar
>> issues elsewhere, and doing some experiments, it looks like we write
>> shared info also in some edge-cases of upcalls such as nfqueue or ovs
>> when mmaped netlink is used for rx. I did a test with nfqueue using
>> the libmnl mmap branch [1].
>
> Note that OVS does not utilize mmaped netlink even though it has been
> considered for a while. It is theoretically possible that non-OVS user
> space user of the OVS netlink API is using it although I'm not aware
> somebody actually does. We can probably fix this specifically for nfqueue.

Sure, I know, it's not included in OVS user space upstream. I meant the
kernel parts of these subsystems where it could be possible /iff/ there's
someone running a netlink socket in rx ring mode against it (but I have
no overview whether someone is doing this in the wild); sorry, should have
been more specific.

Netlink mmap is also not officially upstream in libmnl and neither in
libnetfilter_queue. It looks like it's sitting in the libmnl branch that
I mentioned, but didn't get merged so far. Afaik, Ken-ichirou was doing
work related to adapting this into netfilter in the past.

Anyway, I'll get the stuff ready tonight that I have so far wrt fixes.

Cheers,
Daniel