* [PATCH net-next v3 0/2] nlmon updates
@ 2013-12-23 13:35 Daniel Borkmann
2013-12-23 13:35 ` [PATCH net-next v3 1/2] netlink: only do not deliver to tap when both sides are kernel sks Daniel Borkmann
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Daniel Borkmann @ 2013-12-23 13:35 UTC (permalink / raw
To: davem; +Cc: netdev
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next v3 1/2] netlink: only do not deliver to tap when both sides are kernel sks
2013-12-23 13:35 [PATCH net-next v3 0/2] nlmon updates Daniel Borkmann
@ 2013-12-23 13:35 ` Daniel Borkmann
2013-12-23 13:35 ` [PATCH net-next v3 2/2] netlink: specify netlink packet direction for nlmon Daniel Borkmann
2013-12-31 19:32 ` [PATCH net-next v3 0/2] nlmon updates David Miller
2 siblings, 0 replies; 8+ messages in thread
From: Daniel Borkmann @ 2013-12-23 13:35 UTC (permalink / raw
To: davem; +Cc: netdev, Jakub Zawadzki
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->v3:
- 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next v3 2/2] netlink: specify netlink packet direction for nlmon
2013-12-23 13:35 [PATCH net-next v3 0/2] nlmon updates Daniel Borkmann
2013-12-23 13:35 ` [PATCH net-next v3 1/2] netlink: only do not deliver to tap when both sides are kernel sks Daniel Borkmann
@ 2013-12-23 13:35 ` Daniel Borkmann
2013-12-23 17:46 ` Nicolas Dichtel
2013-12-31 19:32 ` [PATCH net-next v3 0/2] nlmon updates David Miller
2 siblings, 1 reply; 8+ messages in thread
From: Daniel Borkmann @ 2013-12-23 13:35 UTC (permalink / raw
To: davem; +Cc: netdev, Jakub Zawadzki
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 not supported anywhere, 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 requested by Dave
v2->v3:
- fixed typo in comment spotted by Nicolas, thanks
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 invisible 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v3 2/2] netlink: specify netlink packet direction for nlmon
2013-12-23 13:35 ` [PATCH net-next v3 2/2] netlink: specify netlink packet direction for nlmon Daniel Borkmann
@ 2013-12-23 17:46 ` Nicolas Dichtel
2013-12-23 17:54 ` Daniel Borkmann
0 siblings, 1 reply; 8+ messages in thread
From: Nicolas Dichtel @ 2013-12-23 17:46 UTC (permalink / raw
To: Daniel Borkmann, davem; +Cc: netdev, Jakub Zawadzki
Le 23/12/2013 14:35, 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 not supported anywhere, 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 requested by Dave
> v2->v3:
> - fixed typo in comment spotted by Nicolas, thanks
>
> 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 invisible to user space */
> #define PACKET_FASTROUTE 6 /* Fastrouted frame */
Sorry to insist, I just try to understand. Why not removing the definition of
PACKET_FASTROUTE?
Or have a name like PACKET_NL_USER to document the difference between both
cases?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v3 2/2] netlink: specify netlink packet direction for nlmon
2013-12-23 17:46 ` Nicolas Dichtel
@ 2013-12-23 17:54 ` Daniel Borkmann
2013-12-31 18:50 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Borkmann @ 2013-12-23 17:54 UTC (permalink / raw
To: nicolas.dichtel; +Cc: davem, netdev, Jakub Zawadzki
On 12/23/2013 06:46 PM, Nicolas Dichtel wrote:
> Le 23/12/2013 14:35, 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 not supported anywhere, 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 requested by Dave
>> v2->v3:
>> - fixed typo in comment spotted by Nicolas, thanks
>>
>> 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 invisible to user space */
>> #define PACKET_FASTROUTE 6 /* Fastrouted frame */
> Sorry to insist, I just try to understand. Why not removing the definition of
> PACKET_FASTROUTE?
> Or have a name like PACKET_NL_USER to document the difference between both
> cases?
It's now used by nl, but as we have purely generic names, I simply wanted
to comply with that.
We could entirely remove it as it was e.g. proposed in 2008 [1] already if
you see any value in that. Eventually it's up to Dave and if he likes, I'll
be happy to send a patch that removes this define.
Best,
Daniel
[1] http://lists.openwall.net/netdev/2008/05/07/19
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v3 2/2] netlink: specify netlink packet direction for nlmon
2013-12-23 17:54 ` Daniel Borkmann
@ 2013-12-31 18:50 ` David Miller
2014-01-01 4:16 ` Daniel Borkmann
0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2013-12-31 18:50 UTC (permalink / raw
To: dborkman; +Cc: nicolas.dichtel, netdev, darkjames-ws
From: Daniel Borkmann <dborkman@redhat.com>
Date: Mon, 23 Dec 2013 18:54:31 +0100
> We could entirely remove it as it was e.g. proposed in 2008 [1]
> already if you see any value in that. Eventually it's up to Dave and
> if he likes, I'll be happy to send a patch that removes this define.
Removing user visible defines can break source builds, for example
someone building string tables or auto-generating things to facilitate
accessing these values from languages other than C.
It's harmless, since nobody semantically expects anything of it, but
we have to keep it around.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v3 0/2] nlmon updates
2013-12-23 13:35 [PATCH net-next v3 0/2] nlmon updates Daniel Borkmann
2013-12-23 13:35 ` [PATCH net-next v3 1/2] netlink: only do not deliver to tap when both sides are kernel sks Daniel Borkmann
2013-12-23 13:35 ` [PATCH net-next v3 2/2] netlink: specify netlink packet direction for nlmon Daniel Borkmann
@ 2013-12-31 19:32 ` David Miller
2 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2013-12-31 19:32 UTC (permalink / raw
To: dborkman; +Cc: netdev
From: Daniel Borkmann <dborkman@redhat.com>
Date: Mon, 23 Dec 2013 14:35:54 +0100
> Daniel Borkmann (2):
> netlink: only do not deliver to tap when both sides are kernel sks
> netlink: specify netlink packet direction for nlmon
Series applied, thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v3 2/2] netlink: specify netlink packet direction for nlmon
2013-12-31 18:50 ` David Miller
@ 2014-01-01 4:16 ` Daniel Borkmann
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Borkmann @ 2014-01-01 4:16 UTC (permalink / raw
To: David Miller; +Cc: nicolas.dichtel, netdev, darkjames-ws
On 12/31/2013 07:50 PM, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Mon, 23 Dec 2013 18:54:31 +0100
>
>> We could entirely remove it as it was e.g. proposed in 2008 [1]
>> already if you see any value in that. Eventually it's up to Dave and
>> if he likes, I'll be happy to send a patch that removes this define.
>
> Removing user visible defines can break source builds, for example
> someone building string tables or auto-generating things to facilitate
> accessing these values from languages other than C.
>
> It's harmless, since nobody semantically expects anything of it, but
> we have to keep it around.
Ok, that's fine by me.
Thanks for applying Dave and a happy new year!
Best,
Daniel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-01-01 4:16 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-23 13:35 [PATCH net-next v3 0/2] nlmon updates Daniel Borkmann
2013-12-23 13:35 ` [PATCH net-next v3 1/2] netlink: only do not deliver to tap when both sides are kernel sks Daniel Borkmann
2013-12-23 13:35 ` [PATCH net-next v3 2/2] netlink: specify netlink packet direction for nlmon Daniel Borkmann
2013-12-23 17:46 ` Nicolas Dichtel
2013-12-23 17:54 ` Daniel Borkmann
2013-12-31 18:50 ` David Miller
2014-01-01 4:16 ` Daniel Borkmann
2013-12-31 19:32 ` [PATCH net-next v3 0/2] nlmon updates David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.