[PATCH] net-netlink: Add a new attribute to expose TCLASS values via netlink

[PATCH] net-netlink: Add a new attribute to expose TCLASS values via netlink

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

commit 3ceca749668a52bd795585e0f71c6f0b04814f7b added a TOS attribute.

Unfortunately TOS and TCLASS are both present in a dual-stack v6 socket,
furthermore they can have different values.  As such one cannot in a
sane way expose both through a single attribute.

Signed-off-by: Maciej Żenczyowski <maze@google.com>
CC: Murali Raja <muralira@google.com>
CC: Stephen Hemminger <shemminger@vyatta.com>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: David S. Miller <davem@davemloft.net>
---
 include/linux/inet_diag.h |    3 ++-
 net/ipv4/inet_diag.c      |    4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/inet_diag.h b/include/linux/inet_diag.h
index 80b480c..abf5028 100644
--- a/include/linux/inet_diag.h
+++ b/include/linux/inet_diag.h
@@ -98,9 +98,10 @@ enum {
 	INET_DIAG_VEGASINFO,
 	INET_DIAG_CONG,
 	INET_DIAG_TOS,
+	INET_DIAG_TCLASS,
 };
 
-#define INET_DIAG_MAX INET_DIAG_TOS
+#define INET_DIAG_MAX INET_DIAG_TCLASS
 
 
 /* INET_DIAG_MEM */
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index f5e2bda..68e8ac5 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -133,8 +133,8 @@ static int inet_csk_diag_fill(struct sock *sk,
 			       &np->rcv_saddr);
 		ipv6_addr_copy((struct in6_addr *)r->id.idiag_dst,
 			       &np->daddr);
-		if (ext & (1 << (INET_DIAG_TOS - 1)))
-			RTA_PUT_U8(skb, INET_DIAG_TOS, np->tclass);
+		if (ext & (1 << (INET_DIAG_TCLASS - 1)))
+			RTA_PUT_U8(skb, INET_DIAG_TCLASS, np->tclass);
 	}
 #endif
 
-- 
1.7.3.1

Re: [PATCH] net-netlink: Add a new attribute to expose TCLASS values via netlink

[Maciej Żenczykowski]

FYI, This obviously requires a follow up patch which will add TOS and
TCLASS info to appropriate dual-stack sockets (for example listening
tcp v6 non-ipv6only).

On Mon, Nov 7, 2011 at 4:23 PM, Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
> From: Maciej Żenczykowski <maze@google.com>
>
> commit 3ceca749668a52bd795585e0f71c6f0b04814f7b added a TOS attribute.
>
> Unfortunately TOS and TCLASS are both present in a dual-stack v6 socket,
> furthermore they can have different values.  As such one cannot in a
> sane way expose both through a single attribute.
>
> Signed-off-by: Maciej Żenczyowski <maze@google.com>
> CC: Murali Raja <muralira@google.com>
> CC: Stephen Hemminger <shemminger@vyatta.com>
> CC: Eric Dumazet <eric.dumazet@gmail.com>
> CC: David S. Miller <davem@davemloft.net>
> ---
>  include/linux/inet_diag.h |    3 ++-
>  net/ipv4/inet_diag.c      |    4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/inet_diag.h b/include/linux/inet_diag.h
> index 80b480c..abf5028 100644
> --- a/include/linux/inet_diag.h
> +++ b/include/linux/inet_diag.h
> @@ -98,9 +98,10 @@ enum {
>        INET_DIAG_VEGASINFO,
>        INET_DIAG_CONG,
>        INET_DIAG_TOS,
> +       INET_DIAG_TCLASS,
>  };
>
> -#define INET_DIAG_MAX INET_DIAG_TOS
> +#define INET_DIAG_MAX INET_DIAG_TCLASS
>
>
>  /* INET_DIAG_MEM */
> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> index f5e2bda..68e8ac5 100644
> --- a/net/ipv4/inet_diag.c
> +++ b/net/ipv4/inet_diag.c
> @@ -133,8 +133,8 @@ static int inet_csk_diag_fill(struct sock *sk,
>                               &np->rcv_saddr);
>                ipv6_addr_copy((struct in6_addr *)r->id.idiag_dst,
>                               &np->daddr);
> -               if (ext & (1 << (INET_DIAG_TOS - 1)))
> -                       RTA_PUT_U8(skb, INET_DIAG_TOS, np->tclass);
> +               if (ext & (1 << (INET_DIAG_TCLASS - 1)))
> +                       RTA_PUT_U8(skb, INET_DIAG_TCLASS, np->tclass);
>        }
>  #endif
>
> --
> 1.7.3.1
>
>



-- 
Maciej A. Żenczykowski
Kernel Networking Developer @ Google
1600 Amphitheatre Parkway, Mountain View, CA 94043
tel: +1 (650) 253-0062

Re: [PATCH] net-netlink: Add a new attribute to expose TCLASS values via netlink

[Stephen Hemminger]

On Mon,  7 Nov 2011 16:23:11 -0800
Maciej Żenczykowski <zenczykowski@gmail.com> wrote:

> commit 3ceca749668a52bd795585e0f71c6f0b04814f7b added a TOS attribute.
> 
> Unfortunately TOS and TCLASS are both present in a dual-stack v6 socket,
> furthermore they can have different values.  As such one cannot in a
> sane way expose both through a single attribute.

Do we really want to continue to expose that as a supported
API. I would argue it was a mistake in the original implementation.

Re: [PATCH] net-netlink: Add a new attribute to expose TCLASS values via netlink

[Maciej Żenczykowski]

> Do we really want to continue to expose that as a supported
> API. I would argue it was a mistake in the original implementation.

Yes, that's an interesting question...

However, can this be changed at this point in time?

Theoretically network fabric hardware could interpret v4 and v6 dscp
code points differently
(don't think anyone sane would do really want to do this though...)

[PATCH] [RFC] net-netlink: fix tos/tclass for dual-stack ipv6 sockets

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

Something along the following lines would be needed.

Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 include/net/ipv6.h   |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/inet_diag.c |   11 +++++----
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 3b5ac1f..50c7a3b 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -465,6 +465,58 @@ static inline int ipv6_addr_diff(const struct in6_addr *a1, const struct in6_add
 
 extern void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt);
 
+/* Return true for:
+ *   - an IPv4 socket (listening or connected)
+ *   - an IPv4 connection on a dual-stack IPv6 socket
+ *   - an IPv6 dual-stack listening socket -> can later accept IPv4 connection
+ */
+static inline bool sk_might_be_ipv4(struct sock *sk) {
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	const struct ipv6_pinfo *np;
+
+	if (!sk) return false;
+	if (sk->sk_family == AF_INET) return true;
+	if (sk->sk_family != AF_INET6) return false;
+	np = inet6_sk(sk);
+	if (np->ipv6only) return false;
+
+	if (ipv6_addr_v4mapped(&np->rcv_saddr)) return true;
+	if (!ipv6_addr_any(&np->rcv_saddr)) return false;
+
+	if (sk->sk_state == TCP_LISTEN) return true;
+
+	if (ipv6_addr_v4mapped(&np->saddr)) return true;
+	return false;
+#else
+	return sk && (sk->sk_family == AF_INET);
+#endif
+}
+
+/* Return true for:
+ *   - a native IPv6 connection
+ *   - a listening IPv6 socket
+ */
+static inline bool sk_might_be_ipv6(struct sock *sk) {
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	const struct ipv6_pinfo *np;
+
+	if (!sk) return false;
+	if (sk->sk_family != AF_INET6) return false;
+	np = inet6_sk(sk);
+	if (np->ipv6only) return true;
+
+	if (ipv6_addr_v4mapped(&np->rcv_saddr)) return false;
+	if (!ipv6_addr_any(&np->rcv_saddr)) return true;
+
+	if (sk->sk_state == TCP_LISTEN) return true;
+
+	if (ipv6_addr_v4mapped(&np->saddr)) return false;
+	return true;
+#else
+	return false;
+#endif
+}
+
 /*
  *	Prototypes exported by ipv6
  */
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 68e8ac5..39bc97c 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -108,9 +108,6 @@ static int inet_csk_diag_fill(struct sock *sk,
 		       icsk->icsk_ca_ops->name);
 	}
 
-	if ((ext & (1 << (INET_DIAG_TOS - 1))) && (sk->sk_family != AF_INET6))
-		RTA_PUT_U8(skb, INET_DIAG_TOS, inet->tos);
-
 	r->idiag_family = sk->sk_family;
 	r->idiag_state = sk->sk_state;
 	r->idiag_timer = 0;
@@ -125,7 +122,13 @@ static int inet_csk_diag_fill(struct sock *sk,
 	r->id.idiag_src[0] = inet->inet_rcv_saddr;
 	r->id.idiag_dst[0] = inet->inet_daddr;
 
+	if ((ext & (1 << (INET_DIAG_TOS - 1))) && sk_might_be_ipv4(sk))
+		RTA_PUT_U8(skb, INET_DIAG_TOS, inet->tos);
+
 #if defined(CONFIG_IPV6) || defined (CONFIG_IPV6_MODULE)
+	if ((ext & (1 << (INET_DIAG_TCLASS - 1))) && sk_might_be_ipv6(sk))
+		RTA_PUT_U8(skb, INET_DIAG_TCLASS, inet6_sk(sk)->tclass);
+
 	if (r->idiag_family == AF_INET6) {
 		const struct ipv6_pinfo *np = inet6_sk(sk);
 
@@ -133,8 +136,6 @@ static int inet_csk_diag_fill(struct sock *sk,
 			       &np->rcv_saddr);
 		ipv6_addr_copy((struct in6_addr *)r->id.idiag_dst,
 			       &np->daddr);
-		if (ext & (1 << (INET_DIAG_TCLASS - 1)))
-			RTA_PUT_U8(skb, INET_DIAG_TCLASS, np->tclass);
 	}
 #endif
 
-- 
1.7.3.1

Re: [PATCH] net-netlink: Add a new attribute to expose TCLASS values via netlink

[David Miller]

From: Maciej Żenczykowski <zenczykowski@gmail.com>
Date: Mon,  7 Nov 2011 16:23:11 -0800

> From: Maciej Żenczykowski <maze@google.com>
> 
> commit 3ceca749668a52bd795585e0f71c6f0b04814f7b added a TOS attribute.
> 
> Unfortunately TOS and TCLASS are both present in a dual-stack v6 socket,
> furthermore they can have different values.  As such one cannot in a
> sane way expose both through a single attribute.
> 
> Signed-off-by: Maciej Żenczyowski <maze@google.com>

I can't see how an ipv6 mapped socket can even set the inet->tos value.

As far as I can see, only net/ipv4/ip_sockglue.c:ip_setsockopt() provides
the interface to change inet->tos.

And ipv6 sockets, of any type, are provided no such vector by which to
get at those interfaces.

So inet->tos is always left at it's default value for ipv6 mapped sockets,
and therefore I see no reason to report TCLASS vs. TOS separately.

In fact, what I would suggest is to do something about the lack of
ability to set inet->tos, and the best way to do that seems to be to
simply propagate the npinfo->tclass setting into inet->tos.  Performaing
any munging if necessary.

I'm not applying this patch.

Re: [PATCH] [RFC] net-netlink: fix tos/tclass for dual-stack ipv6 sockets

[David Miller]

From: Maciej Żenczykowski <zenczykowski@gmail.com>
Date: Mon,  7 Nov 2011 17:46:40 -0800

> From: Maciej Żenczykowski <maze@google.com>
> 
> Something along the following lines would be needed.
> 
> Signed-off-by: Maciej Żenczykowski <maze@google.com>

This is terrible, see my other email, inet->tos doesn't matter even
for mapped ipv6 sockets.

Re: [PATCH] net-netlink: Add a new attribute to expose TCLASS values via netlink

[Maciej Żenczykowski]

> I can't see how an ipv6 mapped socket can even set the inet->tos value.
>
> As far as I can see, only net/ipv4/ip_sockglue.c:ip_setsockopt() provides
> the interface to change inet->tos.

Yes, but an ipv6 socket is permitted to setsockopt SOL_IP in addition
to SOL_IPV6.
As such, one can simply setsockopt(ipv6_socket, SOL_IP, IP_TOS, value).
This is just as well, because inet->tos is what determines the ipv4 tos value
of any ipv4 packets generated by an ipv6 socket, ie. when connect(ed/ing)
to an ipv4 mapped ipv6 address.

> And ipv6 sockets, of any type, are provided no such vector by which to
> get at those interfaces.

> So inet->tos is always left at it's default value for ipv6 mapped sockets,
> and therefore I see no reason to report TCLASS vs. TOS separately.

> In fact, what I would suggest is to do something about the lack of
> ability to set inet->tos, and the best way to do that seems to be to
> simply propagate the npinfo->tclass setting into inet->tos.  Performaing
> any munging if necessary.

> I'm not applying this patch.

If people are in agreement that inet->tos vs ipv6->tclass being
separately settable is not desirable,
then I'm willing to go through and remove the tclass field (and send
out a patch to do that).

However, this is _QUITE DEFINITELY_ a user visible change in API and
application visible behaviour.

It is however an annoying corner case, so perhaps would be best fixed
by getting rid of it?
OTOH, I'm not sure if people aren't perhaps relying on the ability to
separately set ipv4 dscp vs ipv6 tclass due to some internal network
constraints...

Personally, I'd prefer the simplification of not having to deal with
IP_TOS vs IPV6_TCLASS discrepancies on ipv6 dual-stack sockets.

- Maciej

Re: [PATCH] [RFC] net-netlink: fix tos/tclass for dual-stack ipv6 sockets

[Maciej Żenczykowski]

2011/11/9 David Miller <davem@davemloft.net>:
> From: Maciej Żenczykowski <zenczykowski@gmail.com>
> Date: Mon,  7 Nov 2011 17:46:40 -0800
>
>> From: Maciej Żenczykowski <maze@google.com>
>>
>> Something along the following lines would be needed.
>>
>> Signed-off-by: Maciej Żenczykowski <maze@google.com>
>
> This is terrible, see my other email, inet->tos doesn't matter even
> for mapped ipv6 sockets.

Oh, yes, I definitely agree that this is terrible.
Can we just get rid of tclass and use inet->tos for ipv6 as well?

Re: [PATCH] net-netlink: Add a new attribute to expose TCLASS values via netlink

[David Miller]

From: Maciej Żenczykowski <zenczykowski@gmail.com>
Date: Wed, 9 Nov 2011 17:50:27 -0800

> Yes, but an ipv6 socket is permitted to setsockopt SOL_IP in addition
> to SOL_IPV6.
> As such, one can simply setsockopt(ipv6_socket, SOL_IP, IP_TOS, value).

That's my whole point, this operation isn't currently possible, the
socket ops for ipv4 setsockopt() aren't hooked up to ipv6 mapped
sockets in the kernel right now.

Re: [PATCH] net-netlink: Add a new attribute to expose TCLASS values via netlink

[Maciej Żenczykowski]

2011/11/13 David Miller <davem@davemloft.net>:
>>>> Yes, but an ipv6 socket is permitted to setsockopt SOL_IP in addition to SOL_IPV6.
>>>> As such, one can simply setsockopt(ipv6_socket, SOL_IP, IP_TOS, value).
>>>
>>> That's my whole point, this operation isn't currently possible, the
>>> socket ops for ipv4 setsockopt() aren't hooked up to ipv6 mapped
>>> sockets in the kernel right now.
>>
>> http://lxr.linux.no/linux+v3.1/net/ipv6/ipv6_sockglue.c#L822
>
> That definitely destroys the basis for all of my arguments :-)
>
> I'm going to add the TCLASS inet_diag patch, but can you find a cleaner
> way to make sure that all types of sockets have the value initialized
> to something sane?  The one you posted wasn't... optimal :-)
>
> Thanks.

I only realized just now that I had replied only to you and not to all.
I hope you don't mind that I'm adding everyone back into the loop.

(a) if we keep tos/tclass distinct.

We should make sure to get this patch (and the as yet unwritten
followup) into 3.2-final and not 3.3-rc1 (ie. net/master and not
net-next/master).
Since it does slightly change userspace visible API (thankfully, it
was just added so it's not yet too late to change it).
[FYI, I already see this patch in net/master - thanks!]

As for the followup, I can see three ways to proceed.
- The first involves figuring out what type of socket we're dealing
with when we look at it (ie. something like the RFC patch I posted - I
don't think it's going to be much prettier no matter what one does -
there's simply a lot of complexity...)
- The second involves keeping some sort of type-of-ip-socket
field/bitmap (none, ipv4 only, ipv4/ipv6 dual stack, ipv6 only)
directly in the socket.  This is probably much harder to get right
(changes all over the stack), but may have long term benefits???
- The third involves punting on the problem, we handle the simple
cases in kernel and sometimes return excess information to userspace
(ie. we take the RFC patch and strip out the address comparisons and
error on the side of returning 'true') -> it's userspace's problem to
sift through the excess garbage.

(b) personally I would still prefer to just merge tos and tclass and
get rid of the distinction... is this minor-but-userspace-visible-api
change out of the question?

- Maciej

[PATCH] net-netlink: fix diag to export IPv4 tos for dual-stack IPv6 sockets

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

Signed-off-by: Maciej Żenczykowski <maze@google.com>
CC: Murali Raja <muralira@google.com>
CC: Stephen Hemminger <shemminger@vyatta.com>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: David S. Miller <davem@davemloft.net>
---
 net/ipv4/inet_diag.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 68e8ac5..ccee270 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -108,9 +108,6 @@ static int inet_csk_diag_fill(struct sock *sk,
 		       icsk->icsk_ca_ops->name);
 	}
 
-	if ((ext & (1 << (INET_DIAG_TOS - 1))) && (sk->sk_family != AF_INET6))
-		RTA_PUT_U8(skb, INET_DIAG_TOS, inet->tos);
-
 	r->idiag_family = sk->sk_family;
 	r->idiag_state = sk->sk_state;
 	r->idiag_timer = 0;
@@ -125,16 +122,23 @@ static int inet_csk_diag_fill(struct sock *sk,
 	r->id.idiag_src[0] = inet->inet_rcv_saddr;
 	r->id.idiag_dst[0] = inet->inet_daddr;
 
+	/* IPv6 dual-stack sockets use inet->tos for IPv4 connections,
+	 * hence this needs to be included regardless of socket family.
+	 */
+	if (ext & (1 << (INET_DIAG_TOS - 1)))
+		RTA_PUT_U8(skb, INET_DIAG_TOS, inet->tos);
+
 #if defined(CONFIG_IPV6) || defined (CONFIG_IPV6_MODULE)
 	if (r->idiag_family == AF_INET6) {
 		const struct ipv6_pinfo *np = inet6_sk(sk);
 
+		if (ext & (1 << (INET_DIAG_TCLASS - 1)))
+			RTA_PUT_U8(skb, INET_DIAG_TCLASS, np->tclass);
+
 		ipv6_addr_copy((struct in6_addr *)r->id.idiag_src,
 			       &np->rcv_saddr);
 		ipv6_addr_copy((struct in6_addr *)r->id.idiag_dst,
 			       &np->daddr);
-		if (ext & (1 << (INET_DIAG_TCLASS - 1)))
-			RTA_PUT_U8(skb, INET_DIAG_TCLASS, np->tclass);
 	}
 #endif
 
-- 
1.7.3.1

Re: [PATCH] net-netlink: fix diag to export IPv4 tos for dual-stack IPv6 sockets

[Maciej Żenczykowski]

As you see, I've decided to go with the simplest 'punt the problem to
userspace' solution.

Hope this is acceptable...

- Maciej

Re: [PATCH] net-netlink: fix diag to export IPv4 tos for dual-stack IPv6 sockets

[David Miller]

From: Maciej Żenczykowski <zenczykowski@gmail.com>
Date: Mon, 21 Nov 2011 17:50:20 -0800

> From: Maciej Żenczykowski <maze@google.com>
> 
> Signed-off-by: Maciej Żenczykowski <maze@google.com>

Looks good, applied, thanks!

Re: [PATCH] net-netlink: fix diag to export IPv4 tos for dual-stack IPv6 sockets

[David Miller]

From: Maciej Żenczykowski <zenczykowski@gmail.com>
Date: Mon, 21 Nov 2011 17:52:09 -0800

> As you see, I've decided to go with the simplest 'punt the problem to
> userspace' solution.
> 
> Hope this is acceptable...

It's a good solution, especially for 3.2-rcX

Thanks.