netlink: mmap kernel panic and some issues

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 -----

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()

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

---
 include/linux/netlink.h  |   21 +++++-------------
 net/netlink/af_netlink.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 16 deletions(-)

index 9120edb..60492bf 100644
@@ -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

1.7.10.4

---
 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(-)

index 4670821..cca2c50 100644
@@ -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;
 		}
 	}

index 685cc6a..8d07036 100644
@@ -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)

index 98ed579..45c8502 100644
@@ -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

---
 net/netlink/af_netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 45c8502..c03fad0 100644
@@ -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

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

---
 net/netlink/af_netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c03fad0..d8f5151 100644
@@ -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

---
 net/netlink/af_netlink.c |   28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

index d8f5151..9f9e9ac 100644
@@ -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

From: Ken-ichirou MATSUZAWA <[email protected]>
Date: Wed, 12 Aug 2015 17:28:24 +0900

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.

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,

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(-)

index 9120edb..42cdcd8 100644
@@ -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);

index 67d2104..4307446 100644
@@ -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

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

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.

Yes,

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.

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

From: Daniel Borkmann <[email protected]>
Date: Fri, 14 Aug 2015 12:38:21 +0200

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

Thanks,
Daniel

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,

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

Thank you for the reply.

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.

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

Thanks,

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

... 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

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

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

Thanks,

On 08/17/2015 11:02 PM, David Miller wrote:
...

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

From: Daniel Borkmann <[email protected]>
Date: Mon, 07 Sep 2015 16:54:46 +0200

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.

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

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.

---
 net/netlink/af_netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 85ccd8b..4444687 100644
@@ -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

From: Ken-ichirou MATSUZAWA <[email protected]>
Date: Thu, 20 Aug 2015 12:43:53 +0900

Applied, thanks.

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.

---
 net/netlink/af_netlink.c |   28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

index 4444687..c65ec2e 100644
@@ -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

From: Ken-ichirou MATSUZAWA <[email protected]>
Date: Thu, 20 Aug 2015 14:54:47 +0900

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.

Thank you for the reply.

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

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

Thanks,
Ken

__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.

---
 net/netlink/af_netlink.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index a774985..39fa91f 100644
@@ -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

From: Ken-ichirou MATSUZAWA <[email protected]>
Date: Fri, 28 Aug 2015 16:05:20 +0900

Applied.

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.

---
 net/netlink/af_netlink.c |   28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

index 7965ca7..50889be 100644
@@ -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

From: Ken-ichirou MATSUZAWA <[email protected]>
Date: Mon, 31 Aug 2015 07:54:49 +0900

Applied, thanks.

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.

---
 net/netlink/af_netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e6134f4..85ccd8b 100644
@@ -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

From: Ken-ichirou MATSUZAWA <[email protected]>
Date: Thu, 20 Aug 2015 16:07:33 +0900

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.