[PATCH net-next v2 0/2] nlmon updates

[PATCH net-next v2 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 |  4 +++-
 net/netlink/af_netlink.c       | 15 ++++++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

-- 
1.7.11.7

[PATCH net-next v2 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>
---
 v1->v2:
  - unchanged

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

[PATCH net-next v2 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, we can use the value (= 6) of
PACKET_FASTROUTE as it's _not used_ anywhere in the whole kernel
and packets of such type were never exposed to user space, so
there are no overlapping users of such kind. Thus, as wished,
that seems the only way to make both PACKET_* values non-overlapping
and therefore device agnostic.

By using those two flags for netlink skbs on nlmon devices, they
can be made available and picked up via sll_pkttype (previously
unused in netlink context) in struct sockaddr_ll. We now have
these two directions:

 - PACKET_USER (= 6)    ->  to user space
 - PACKET_KERNEL (= 7)  ->  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>
---
 v1->v2:
  - let PACKET_* values not overlap as wished by Dave

 include/uapi/linux/if_packet.h | 4 +++-
 net/netlink/af_netlink.c       | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
index e9d844c..06e2a28 100644
--- a/include/uapi/linux/if_packet.h
+++ b/include/uapi/linux/if_packet.h
@@ -26,8 +26,10 @@ struct sockaddr_ll {
 #define PACKET_MULTICAST	2		/* To group		*/
 #define PACKET_OTHERHOST	3		/* To someone else 	*/
 #define PACKET_OUTGOING		4		/* Outgoing of any type */
-/* These ones are invisible by user level */
 #define PACKET_LOOPBACK		5		/* MC/BRD frame looped back */
+#define PACKET_USER		6		/* To user space	*/
+#define PACKET_KERNEL		7		/* To kernel space	*/
+/* Unused, PACKET_FASTROUTE and PACKET_LOOPBACK are invisble to user space */
 #define PACKET_FASTROUTE	6		/* Fastrouted frame	*/
 
 /* 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.7.11.7

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

[Nicolas Dichtel]

Le 23/12/2013 09:48, Daniel Borkmann a écrit :
> 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, we can use the value (= 6) of
> PACKET_FASTROUTE as it's _not used_ anywhere in the whole kernel
> and packets of such type were never exposed to user space, so
> there are no overlapping users of such kind. Thus, as wished,
> that seems the only way to make both PACKET_* values non-overlapping
> and therefore device agnostic.
>
> By using those two flags for netlink skbs on nlmon devices, they
> can be made available and picked up via sll_pkttype (previously
> unused in netlink context) in struct sockaddr_ll. We now have
> these two directions:
>
>   - PACKET_USER (= 6)    ->  to user space
>   - PACKET_KERNEL (= 7)  ->  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>
> ---
>   v1->v2:
>    - let PACKET_* values not overlap as wished by Dave
>
>   include/uapi/linux/if_packet.h | 4 +++-
>   net/netlink/af_netlink.c       | 2 ++
>   2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
> index e9d844c..06e2a28 100644
> --- a/include/uapi/linux/if_packet.h
> +++ b/include/uapi/linux/if_packet.h
> @@ -26,8 +26,10 @@ struct sockaddr_ll {
>   #define PACKET_MULTICAST	2		/* To group		*/
>   #define PACKET_OTHERHOST	3		/* To someone else 	*/
>   #define PACKET_OUTGOING		4		/* Outgoing of any type */
> -/* These ones are invisible by user level */
>   #define PACKET_LOOPBACK		5		/* MC/BRD frame looped back */
> +#define PACKET_USER		6		/* To user space	*/
Reusing this value is like changing the API. If some userland apps and external
modules rely on it, this patch may break them.

> +#define PACKET_KERNEL		7		/* To kernel space	*/
> +/* Unused, PACKET_FASTROUTE and PACKET_LOOPBACK are invisble to user space */
nitpicking: s/invisble/invisible

>   #define PACKET_FASTROUTE	6		/* Fastrouted frame	*/
>
>   /* 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))
>

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

[Daniel Borkmann]

On 12/23/2013 11:43 AM, Nicolas Dichtel wrote:
> Le 23/12/2013 09:48, Daniel Borkmann a écrit :
>> 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, we can use the value (= 6) of
>> PACKET_FASTROUTE as it's _not used_ anywhere in the whole kernel
>> and packets of such type were never exposed to user space, so
>> there are no overlapping users of such kind. Thus, as wished,
>> that seems the only way to make both PACKET_* values non-overlapping
>> and therefore device agnostic.
>>
>> By using those two flags for netlink skbs on nlmon devices, they
>> can be made available and picked up via sll_pkttype (previously
>> unused in netlink context) in struct sockaddr_ll. We now have
>> these two directions:
>>
>>   - PACKET_USER (= 6)    ->  to user space
>>   - PACKET_KERNEL (= 7)  ->  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>
>> ---
>>   v1->v2:
>>    - let PACKET_* values not overlap as wished by Dave
>>
>>   include/uapi/linux/if_packet.h | 4 +++-
>>   net/netlink/af_netlink.c       | 2 ++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
>> index e9d844c..06e2a28 100644
>> --- a/include/uapi/linux/if_packet.h
>> +++ b/include/uapi/linux/if_packet.h
>> @@ -26,8 +26,10 @@ struct sockaddr_ll {
>>   #define PACKET_MULTICAST    2        /* To group        */
>>   #define PACKET_OTHERHOST    3        /* To someone else     */
>>   #define PACKET_OUTGOING        4        /* Outgoing of any type */
>> -/* These ones are invisible by user level */
>>   #define PACKET_LOOPBACK        5        /* MC/BRD frame looped back */
>> +#define PACKET_USER        6        /* To user space    */
> Reusing this value is like changing the API. If some userland apps and external
> modules rely on it, this patch may break them.

Sorry, but I thought I made it clear in the commit message that
PACKET_FASTROUTE is *not* used anywhere in the whole kernel tree.
And as the comment said as well, this type was never exposed to
user land.

>> +#define PACKET_KERNEL        7        /* To kernel space    */
>> +/* Unused, PACKET_FASTROUTE and PACKET_LOOPBACK are invisble to user space */
> nitpicking: s/invisble/invisible
>
>>   #define PACKET_FASTROUTE    6        /* Fastrouted frame    */
>>
>>   /* 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))
>>
> --
> 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: [PATCH net-next v2 2/2] netlink: specify netlink packet direction for nlmon

[Nicolas Dichtel]

Le 23/12/2013 11:46, Daniel Borkmann a écrit :
> On 12/23/2013 11:43 AM, Nicolas Dichtel wrote:
>> Le 23/12/2013 09:48, Daniel Borkmann a écrit :
>>> 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, we can use the value (= 6) of
>>> PACKET_FASTROUTE as it's _not used_ anywhere in the whole kernel
>>> and packets of such type were never exposed to user space, so
>>> there are no overlapping users of such kind. Thus, as wished,
>>> that seems the only way to make both PACKET_* values non-overlapping
>>> and therefore device agnostic.
>>>
>>> By using those two flags for netlink skbs on nlmon devices, they
>>> can be made available and picked up via sll_pkttype (previously
>>> unused in netlink context) in struct sockaddr_ll. We now have
>>> these two directions:
>>>
>>>   - PACKET_USER (= 6)    ->  to user space
>>>   - PACKET_KERNEL (= 7)  ->  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>
>>> ---
>>>   v1->v2:
>>>    - let PACKET_* values not overlap as wished by Dave
>>>
>>>   include/uapi/linux/if_packet.h | 4 +++-
>>>   net/netlink/af_netlink.c       | 2 ++
>>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
>>> index e9d844c..06e2a28 100644
>>> --- a/include/uapi/linux/if_packet.h
>>> +++ b/include/uapi/linux/if_packet.h
>>> @@ -26,8 +26,10 @@ struct sockaddr_ll {
>>>   #define PACKET_MULTICAST    2        /* To group        */
>>>   #define PACKET_OTHERHOST    3        /* To someone else     */
>>>   #define PACKET_OUTGOING        4        /* Outgoing of any type */
>>> -/* These ones are invisible by user level */
>>>   #define PACKET_LOOPBACK        5        /* MC/BRD frame looped back */
>>> +#define PACKET_USER        6        /* To user space    */
>> Reusing this value is like changing the API. If some userland apps and external
>> modules rely on it, this patch may break them.
>
> Sorry, but I thought I made it clear in the commit message that
> PACKET_FASTROUTE is *not* used anywhere in the whole kernel tree.
Yes, it's why I talk about *external* modules, which in fact are allowed
to use existing API.

> And as the comment said as well, this type was never exposed to
> user land.
The fact is that the value is in include/uapi/*, hence it's exposed to userland.

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

[Daniel Borkmann]

On 12/23/2013 12:03 PM, Nicolas Dichtel wrote:
> Le 23/12/2013 11:46, Daniel Borkmann a écrit :
>> On 12/23/2013 11:43 AM, Nicolas Dichtel wrote:
>>> Le 23/12/2013 09:48, Daniel Borkmann a écrit :
>>>> 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, we can use the value (= 6) of
>>>> PACKET_FASTROUTE as it's _not used_ anywhere in the whole kernel
>>>> and packets of such type were never exposed to user space, so
>>>> there are no overlapping users of such kind. Thus, as wished,
>>>> that seems the only way to make both PACKET_* values non-overlapping
>>>> and therefore device agnostic.
>>>>
>>>> By using those two flags for netlink skbs on nlmon devices, they
>>>> can be made available and picked up via sll_pkttype (previously
>>>> unused in netlink context) in struct sockaddr_ll. We now have
>>>> these two directions:
>>>>
>>>>   - PACKET_USER (= 6)    ->  to user space
>>>>   - PACKET_KERNEL (= 7)  ->  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>
>>>> ---
>>>>   v1->v2:
>>>>    - let PACKET_* values not overlap as wished by Dave
>>>>
>>>>   include/uapi/linux/if_packet.h | 4 +++-
>>>>   net/netlink/af_netlink.c       | 2 ++
>>>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
>>>> index e9d844c..06e2a28 100644
>>>> --- a/include/uapi/linux/if_packet.h
>>>> +++ b/include/uapi/linux/if_packet.h
>>>> @@ -26,8 +26,10 @@ struct sockaddr_ll {
>>>>   #define PACKET_MULTICAST    2        /* To group        */
>>>>   #define PACKET_OTHERHOST    3        /* To someone else     */
>>>>   #define PACKET_OUTGOING        4        /* Outgoing of any type */
>>>> -/* These ones are invisible by user level */
>>>>   #define PACKET_LOOPBACK        5        /* MC/BRD frame looped back */
>>>> +#define PACKET_USER        6        /* To user space    */
>>> Reusing this value is like changing the API. If some userland apps and external
>>> modules rely on it, this patch may break them.
>>
>> Sorry, but I thought I made it clear in the commit message that
>> PACKET_FASTROUTE is *not* used anywhere in the whole kernel tree.
> Yes, it's why I talk about *external* modules, which in fact are allowed
> to use existing API.

Sorry, but we *never* cared about external out-of-tree modules! If
out-of-tree modules want to use kernel APIs and stay updated then
people should start submitting them to the kernel. I thought that
this is clear as this is the default policy here on netdev!

>> And as the comment said as well, this type was never exposed to
>> user land.
> The fact is that the value is in include/uapi/*, hence it's exposed to userland.

Ok, let me explain once more ... no packet *what-so-ever* will ever
go up to user space with PACKET_FASTROUTE in sll_pkttype. 1) because
if you grep the kernel tree then you'll see that this is _not used
anywhere_, 2) as the comment says, skbs of such type were invisible
to user land, hence _never_ exposed through PF_PACKET in user space.

Thanks !

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

[Nicolas Dichtel]

Le 23/12/2013 12:11, Daniel Borkmann a écrit :
> On 12/23/2013 12:03 PM, Nicolas Dichtel wrote:
>> Le 23/12/2013 11:46, Daniel Borkmann a écrit :
>>> On 12/23/2013 11:43 AM, Nicolas Dichtel wrote:
>>>> Le 23/12/2013 09:48, Daniel Borkmann a écrit :
>>>>> 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, we can use the value (= 6) of
>>>>> PACKET_FASTROUTE as it's _not used_ anywhere in the whole kernel
>>>>> and packets of such type were never exposed to user space, so
>>>>> there are no overlapping users of such kind. Thus, as wished,
>>>>> that seems the only way to make both PACKET_* values non-overlapping
>>>>> and therefore device agnostic.
>>>>>
>>>>> By using those two flags for netlink skbs on nlmon devices, they
>>>>> can be made available and picked up via sll_pkttype (previously
>>>>> unused in netlink context) in struct sockaddr_ll. We now have
>>>>> these two directions:
>>>>>
>>>>>   - PACKET_USER (= 6)    ->  to user space
>>>>>   - PACKET_KERNEL (= 7)  ->  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>
>>>>> ---
>>>>>   v1->v2:
>>>>>    - let PACKET_* values not overlap as wished by Dave
>>>>>
>>>>>   include/uapi/linux/if_packet.h | 4 +++-
>>>>>   net/netlink/af_netlink.c       | 2 ++
>>>>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
>>>>> index e9d844c..06e2a28 100644
>>>>> --- a/include/uapi/linux/if_packet.h
>>>>> +++ b/include/uapi/linux/if_packet.h
>>>>> @@ -26,8 +26,10 @@ struct sockaddr_ll {
>>>>>   #define PACKET_MULTICAST    2        /* To group        */
>>>>>   #define PACKET_OTHERHOST    3        /* To someone else     */
>>>>>   #define PACKET_OUTGOING        4        /* Outgoing of any type */
>>>>> -/* These ones are invisible by user level */
>>>>>   #define PACKET_LOOPBACK        5        /* MC/BRD frame looped back */
>>>>> +#define PACKET_USER        6        /* To user space    */
>>>> Reusing this value is like changing the API. If some userland apps and external
>>>> modules rely on it, this patch may break them.
>>>
>>> Sorry, but I thought I made it clear in the commit message that
>>> PACKET_FASTROUTE is *not* used anywhere in the whole kernel tree.
>> Yes, it's why I talk about *external* modules, which in fact are allowed
>> to use existing API.
>
> Sorry, but we *never* cared about external out-of-tree modules! If
> out-of-tree modules want to use kernel APIs and stay updated then
> people should start submitting them to the kernel. I thought that
> this is clear as this is the default policy here on netdev!
Yes, this is perfectly clear. But I was thinking not changing/breaking an API
was a MUST too.

>
>>> And as the comment said as well, this type was never exposed to
>>> user land.
>> The fact is that the value is in include/uapi/*, hence it's exposed to userland.
>
> Ok, let me explain once more ... no packet *what-so-ever* will ever
> go up to user space with PACKET_FASTROUTE in sll_pkttype. 1) because
> if you grep the kernel tree then you'll see that this is _not used
> anywhere_, 2) as the comment says, skbs of such type were invisible
> to user land, hence _never_ exposed through PF_PACKET in user space.
Why keeping PACKET_FASTROUTE then?

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

[Daniel Borkmann]

On 12/23/2013 02:08 PM, Nicolas Dichtel wrote:
> Le 23/12/2013 12:11, Daniel Borkmann a écrit :
>> On 12/23/2013 12:03 PM, Nicolas Dichtel wrote:
>>> Le 23/12/2013 11:46, Daniel Borkmann a écrit :
>>>> On 12/23/2013 11:43 AM, Nicolas Dichtel wrote:
>>>>> Le 23/12/2013 09:48, Daniel Borkmann a écrit :
>>>>>> 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, we can use the value (= 6) of
>>>>>> PACKET_FASTROUTE as it's _not used_ anywhere in the whole kernel
>>>>>> and packets of such type were never exposed to user space, so
>>>>>> there are no overlapping users of such kind. Thus, as wished,
>>>>>> that seems the only way to make both PACKET_* values non-overlapping
>>>>>> and therefore device agnostic.
>>>>>>
>>>>>> By using those two flags for netlink skbs on nlmon devices, they
>>>>>> can be made available and picked up via sll_pkttype (previously
>>>>>> unused in netlink context) in struct sockaddr_ll. We now have
>>>>>> these two directions:
>>>>>>
>>>>>>   - PACKET_USER (= 6)    ->  to user space
>>>>>>   - PACKET_KERNEL (= 7)  ->  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>
>>>>>> ---
>>>>>>   v1->v2:
>>>>>>    - let PACKET_* values not overlap as wished by Dave
>>>>>>
>>>>>>   include/uapi/linux/if_packet.h | 4 +++-
>>>>>>   net/netlink/af_netlink.c       | 2 ++
>>>>>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
>>>>>> index e9d844c..06e2a28 100644
>>>>>> --- a/include/uapi/linux/if_packet.h
>>>>>> +++ b/include/uapi/linux/if_packet.h
>>>>>> @@ -26,8 +26,10 @@ struct sockaddr_ll {
>>>>>>   #define PACKET_MULTICAST    2        /* To group        */
>>>>>>   #define PACKET_OTHERHOST    3        /* To someone else     */
>>>>>>   #define PACKET_OUTGOING        4        /* Outgoing of any type */
>>>>>> -/* These ones are invisible by user level */
>>>>>>   #define PACKET_LOOPBACK        5        /* MC/BRD frame looped back */
>>>>>> +#define PACKET_USER        6        /* To user space    */
>>>>> Reusing this value is like changing the API. If some userland apps and external
>>>>> modules rely on it, this patch may break them.
>>>>
>>>> Sorry, but I thought I made it clear in the commit message that
>>>> PACKET_FASTROUTE is *not* used anywhere in the whole kernel tree.
>>> Yes, it's why I talk about *external* modules, which in fact are allowed
>>> to use existing API.
>>
>> Sorry, but we *never* cared about external out-of-tree modules! If
>> out-of-tree modules want to use kernel APIs and stay updated then
>> people should start submitting them to the kernel. I thought that
>> this is clear as this is the default policy here on netdev!
> Yes, this is perfectly clear. But I was thinking not changing/breaking an API
> was a MUST too.

Please *explicitly* point out to me where I break something in
user space!

With this patch, on nlmon devices there'll be only skbs send up
for PF_PACKET to user space that *either* have PACKET_USER *or*
PACKET_KERNEL set as sll_pkttype, _nothing_ else, and netlink
is the only user of this!

The rest (e.g. traditional PF_PACKET path of network traffic) is
unchanged, plus *nobody ever* sets PACKET_FASTROUTE WHAT-SO-EVER!

Nothing breaks ...

>>>> And as the comment said as well, this type was never exposed to
>>>> user land.
>>> The fact is that the value is in include/uapi/*, hence it's exposed to userland.
>>
>> Ok, let me explain once more ... no packet *what-so-ever* will ever
>> go up to user space with PACKET_FASTROUTE in sll_pkttype. 1) because
>> if you grep the kernel tree then you'll see that this is _not used
>> anywhere_, 2) as the comment says, skbs of such type were invisible
>> to user land, hence _never_ exposed through PF_PACKET in user space.
> Why keeping PACKET_FASTROUTE then?

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

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Mon, 23 Dec 2013 11:43:52 +0100

> Le 23/12/2013 09:48, Daniel Borkmann a écrit :
>> 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, we can use the value (= 6) of
>> PACKET_FASTROUTE as it's _not used_ anywhere in the whole kernel
>> and packets of such type were never exposed to user space, so
>> there are no overlapping users of such kind. Thus, as wished,
>> that seems the only way to make both PACKET_* values non-overlapping
>> and therefore device agnostic.
>>
>> By using those two flags for netlink skbs on nlmon devices, they
>> can be made available and picked up via sll_pkttype (previously
>> unused in netlink context) in struct sockaddr_ll. We now have
>> these two directions:
>>
>>   - PACKET_USER (= 6)    ->  to user space
>>   - PACKET_KERNEL (= 7)  ->  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>
>> ---
>>   v1->v2:
>>    - let PACKET_* values not overlap as wished by Dave
>>
>>   include/uapi/linux/if_packet.h | 4 +++-
>>   net/netlink/af_netlink.c       | 2 ++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/if_packet.h
>> b/include/uapi/linux/if_packet.h
>> index e9d844c..06e2a28 100644
>> --- a/include/uapi/linux/if_packet.h
>> +++ b/include/uapi/linux/if_packet.h
>> @@ -26,8 +26,10 @@ struct sockaddr_ll {
>>   #define PACKET_MULTICAST	2		/* To group		*/
>>   #define PACKET_OTHERHOST	3		/* To someone else 	*/
>>   #define PACKET_OUTGOING 4 /* Outgoing of any type */
>> -/* These ones are invisible by user level */
>>   #define PACKET_LOOPBACK 5 /* MC/BRD frame looped back */
>> +#define PACKET_USER 6 /* To user space */
> Reusing this value is like changing the API. If some userland apps and
> external
> modules rely on it, this patch may break them.

Interpretation of a value is contextual, please stop ignoring this
fact.

And in this context, Daniel's reappropriation of the PACKET_FASTROUTE
value causes no harm to anyone.

Values never used by us at any point in the past, yet nevertheless
were placed in a user visible header file, have to be kept around.  But
that doesn't mean that we can't reuse them for other meanings.

The worst thing that could exist, is that some code out there builds,
for example, string tables using PACKET_* values and whoever did them
decided to be complete enough to build entries even for effectively
unused values such as PACKET_FASTROUTE.

If you actually do searches for code making reference to
PACKET_FASTROUTE or other similarly unused defines, that's what you
find.  There is nothing looking semantically at this value at all.

Any objections to Daniel's changes will fall on deaf ears until
someone can really provide an actual existing way that things can
break.  Because I'm very sure that they cannot.