[PATCH net-next 0/2] nlmon updates

[PATCH net-next 0/2] nlmon updates

[Daniel Borkmann]

Daniel Borkmann (2):
  netlink: only do not deliver to tap when both sides are kernel sks
  netlink: specify netlink packet direction for nlmon

 include/uapi/linux/if_packet.h |  3 +++
 net/netlink/af_netlink.c       | 15 ++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/2] netlink: only do not deliver to tap when both sides are kernel sks

[Daniel Borkmann]

We should also deliver packets to nlmon devices when we are in
netlink_unicast_kernel(), and only one of the {src,dst} sockets
is user sk and the other one kernel sk. That's e.g. the case in
netlink diag, netlink route, etc. Still, forbid to deliver messages
from kernel to kernel sks.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Jakub Zawadzki <darkjames-ws@darkjames.pl>
---
 net/netlink/af_netlink.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index bca50b9..56e09d8 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -239,6 +239,13 @@ static void netlink_deliver_tap(struct sk_buff *skb)
 	rcu_read_unlock();
 }
 
+static void netlink_deliver_tap_kernel(struct sock *dst, struct sock *src,
+				       struct sk_buff *skb)
+{
+	if (!(netlink_is_kernel(dst) && netlink_is_kernel(src)))
+		netlink_deliver_tap(skb);
+}
+
 static void netlink_overrun(struct sock *sk)
 {
 	struct netlink_sock *nlk = nlk_sk(sk);
@@ -1697,14 +1704,10 @@ static int netlink_unicast_kernel(struct sock *sk, struct sk_buff *skb,
 
 	ret = -ECONNREFUSED;
 	if (nlk->netlink_rcv != NULL) {
-		/* We could do a netlink_deliver_tap(skb) here as well
-		 * but since this is intended for the kernel only, we
-		 * should rather let it stay under the hood.
-		 */
-
 		ret = skb->len;
 		netlink_skb_set_owner_r(skb, sk);
 		NETLINK_CB(skb).sk = ssk;
+		netlink_deliver_tap_kernel(sk, ssk, skb);
 		nlk->netlink_rcv(skb);
 		consume_skb(skb);
 	} else {
-- 
1.8.3.1

[PATCH net-next 2/2] netlink: specify netlink packet direction for nlmon

[Daniel Borkmann]

In order to facilitate development for netlink protocol dissector,
fill the unused field skb->pkt_type of the cloned skb with a hint
of the address space of the new owner (receiver) socket in the
notion of "to kernel" resp. "to user".

At the time we invoke __netlink_deliver_tap_skb(), we already have
set the new skb owner via netlink_skb_set_owner_r(), so we can use
that for netlink_is_kernel() probing.

In normal PF_PACKET network traffic, this field denotes if the
packet is destined for us (PACKET_HOST), if it's broadcast
(PACKET_BROADCAST), etc.

As we only have 3 bit reserved, "overload" the meaning of these
flags for netlink skbs on nlmon devices, thus it can be picked up
via sll_pkttype in struct sockaddr_ll. We have now:

 - PACKET_USER    ->  to user space
 - PACKET_KERNEL  ->  to kernel space

Partial `ip a` example strace for sa_family=AF_NETLINK with detected
nl msg direction:

syscall:                     direction:
sendto(3,  ...) = 40         /* to kernel */
recvmsg(3, ...) = 3404       /* to user */
recvmsg(3, ...) = 1120       /* to user */
recvmsg(3, ...) = 20         /* to user */
sendto(3,  ...) = 40         /* to kernel */
recvmsg(3, ...) = 168        /* to user */
recvmsg(3, ...) = 144        /* to user */
recvmsg(3, ...) = 20         /* to user */

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Jakub Zawadzki <darkjames-ws@darkjames.pl>
---
 include/uapi/linux/if_packet.h | 3 +++
 net/netlink/af_netlink.c       | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
index e9d844c..a8d4ff1 100644
--- a/include/uapi/linux/if_packet.h
+++ b/include/uapi/linux/if_packet.h
@@ -29,6 +29,9 @@ struct sockaddr_ll {
 /* These ones are invisible by user level */
 #define PACKET_LOOPBACK		5		/* MC/BRD frame looped back */
 #define PACKET_FASTROUTE	6		/* Fastrouted frame	*/
+/* These ones are for nlmon devices */
+#define PACKET_USER		0		/* To user space */
+#define PACKET_KERNEL		1		/* To kernel space */
 
 /* Packet socket options */
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 56e09d8..3f75f1c 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -204,6 +204,8 @@ static int __netlink_deliver_tap_skb(struct sk_buff *skb,
 	if (nskb) {
 		nskb->dev = dev;
 		nskb->protocol = htons((u16) sk->sk_protocol);
+		nskb->pkt_type = netlink_is_kernel(sk) ?
+				 PACKET_KERNEL : PACKET_USER;
 
 		ret = dev_queue_xmit(nskb);
 		if (unlikely(ret > 0))
-- 
1.8.3.1

Re: [PATCH net-next 2/2] netlink: specify netlink packet direction for nlmon

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Thu, 19 Dec 2013 02:29:39 +0100

> @@ -29,6 +29,9 @@ struct sockaddr_ll {
>  /* These ones are invisible by user level */
>  #define PACKET_LOOPBACK		5		/* MC/BRD frame looped back */
>  #define PACKET_FASTROUTE	6		/* Fastrouted frame	*/
> +/* These ones are for nlmon devices */
> +#define PACKET_USER		0		/* To user space */
> +#define PACKET_KERNEL		1		/* To kernel space */

I know it is tempting to do so, but please do not reuse values
like this.

THanks.

Re: [PATCH net-next 2/2] netlink: specify netlink packet direction for nlmon

[Daniel Borkmann]

On 12/23/2013 12:56 AM, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Thu, 19 Dec 2013 02:29:39 +0100
>
>> @@ -29,6 +29,9 @@ struct sockaddr_ll {
>>   /* These ones are invisible by user level */
>>   #define PACKET_LOOPBACK		5		/* MC/BRD frame looped back */
>>   #define PACKET_FASTROUTE	6		/* Fastrouted frame	*/
>> +/* These ones are for nlmon devices */
>> +#define PACKET_USER		0		/* To user space */
>> +#define PACKET_KERNEL		1		/* To kernel space */
>
> I know it is tempting to do so, but please do not reuse values
> like this.

Hm, ok. As we only have 3 bits for all this, are you okay with
doing the following ...

#define PACKET_USER	6	/* To user space   */
#define PACKET_KERNEL	7	/* To kernel space */

PACKET_FASTROUTE isn't used anywhere in the tree, only defined
in this header file. Then, by doing this, we would still fit.

> THanks.