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

[Daniel Borkmann <dborkman@redhat.com>]

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?