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

[David Miller <davem@davemloft.net>]

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.