Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1515465 > unrolled thread

[PATCH net-next 1/5] net: l2tp: fix L2TP_ATTR_UDP_CSUM attribute type

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next 1/5] net: l2tp: fix L2TP_ATTR_UDP_CSUM attribute type Asbjoern Sloth Toennesen <asbjorn@asbjorn.st> - 2016-11-05 00:00 +0100
    [PATCH net-next 4/5] net: l2tp: cleanup: remove redundant condition Asbjoern Sloth Toennesen <asbjorn@asbjorn.st> - 2016-11-05 00:00 +0100
    [PATCH net-next 3/5] net: l2tp: netlink: l2tp_nl_tunnel_send: set UDP6 checksum flags Asbjoern Sloth Toennesen <asbjorn@asbjorn.st> - 2016-11-05 00:00 +0100
    Re: [PATCH net-next 1/5] net: l2tp: fix L2TP_ATTR_UDP_CSUM  attribute type David Miller <davem@davemloft.net> - 2016-11-07 19:10 +0100
      Re: [PATCH net-next 1/5] net: l2tp: fix L2TP_ATTR_UDP_CSUM  attribute type Asbjørn Sloth Tønnesen <asbjorn@asbjorn.st> - 2016-11-07 22:10 +0100

#1515465 — [PATCH net-next 1/5] net: l2tp: fix L2TP_ATTR_UDP_CSUM attribute type

L2TP_ATTR_UDP_CSUM is a flag, and gets read with
nla_get_flag, but it is defined as NLA_U8 in
the nla_policy.

It appears that this is only publicly used in
iproute2, where it's broken, because it's used as
a NLA_FLAG, and fails validation as a NLA_U8.

The only place it's used as a NLA_U8 is in
l2tp_nl_tunnel_send(), but iproute2 again reads that
as a flag, it's therefore always set. Fortunately
it is never used for anything, just read.

CC: Miao Wang <shankerwangmiao@gmail.com>
Signed-off-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
---
 include/uapi/linux/l2tp.h |  2 +-
 net/l2tp/l2tp_netlink.c   | 12 +++++++++---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/l2tp.h b/include/uapi/linux/l2tp.h
index 4bd27d0..73e3a23 100644
--- a/include/uapi/linux/l2tp.h
+++ b/include/uapi/linux/l2tp.h
@@ -104,7 +104,7 @@ enum {
 	L2TP_ATTR_PEER_CONN_ID,		/* u32 */
 	L2TP_ATTR_SESSION_ID,		/* u32 */
 	L2TP_ATTR_PEER_SESSION_ID,	/* u32 */
-	L2TP_ATTR_UDP_CSUM,		/* u8 */
+	L2TP_ATTR_UDP_CSUM,		/* flag */
 	L2TP_ATTR_VLAN_ID,		/* u16 */
 	L2TP_ATTR_COOKIE,		/* 0, 4 or 8 bytes */
 	L2TP_ATTR_PEER_COOKIE,		/* 0, 4 or 8 bytes */
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 59aa2d2..1fe05da 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -379,9 +379,15 @@ static int l2tp_nl_tunnel_send(struct sk_buff *skb, u32 portid, u32 seq, int fla
 
 	switch (tunnel->encap) {
 	case L2TP_ENCAPTYPE_UDP:
+		switch (sk->sk_family) {
+		case AF_INET:
+			if ((!sk->sk_no_check_tx) &&
+			    nla_put_flag(skb, L2TP_ATTR_UDP_CSUM))
+				goto nla_put_failure;
+			break;
+		}
 		if (nla_put_u16(skb, L2TP_ATTR_UDP_SPORT, ntohs(inet->inet_sport)) ||
-		    nla_put_u16(skb, L2TP_ATTR_UDP_DPORT, ntohs(inet->inet_dport)) ||
-		    nla_put_u8(skb, L2TP_ATTR_UDP_CSUM, !sk->sk_no_check_tx))
+		    nla_put_u16(skb, L2TP_ATTR_UDP_DPORT, ntohs(inet->inet_dport)))
 			goto nla_put_failure;
 		/* NOBREAK */
 	case L2TP_ENCAPTYPE_IP:
@@ -873,7 +879,7 @@ static const struct nla_policy l2tp_nl_policy[L2TP_ATTR_MAX + 1] = {
 	[L2TP_ATTR_PEER_CONN_ID]	= { .type = NLA_U32, },
 	[L2TP_ATTR_SESSION_ID]		= { .type = NLA_U32, },
 	[L2TP_ATTR_PEER_SESSION_ID]	= { .type = NLA_U32, },
-	[L2TP_ATTR_UDP_CSUM]		= { .type = NLA_U8, },
+	[L2TP_ATTR_UDP_CSUM]		= { .type = NLA_FLAG, },
 	[L2TP_ATTR_VLAN_ID]		= { .type = NLA_U16, },
 	[L2TP_ATTR_DEBUG]		= { .type = NLA_U32, },
 	[L2TP_ATTR_RECV_SEQ]		= { .type = NLA_U8, },
-- 
2.10.1

[toc] | [next] | [standalone]

#1515467 — [PATCH net-next 4/5] net: l2tp: cleanup: remove redundant condition

These assignments follow this pattern:

	unsigned int foo:1;
	struct nlattr *nla = info->attrs[bar];

	if (nla)
		foo = nla_get_flag(nla); /* expands to: foo = !!nla */

This could be simplified to: if (nla) foo = 1;
but lets just remove the condition and use the macro,

	foo = nla_get_flag(nla);

Signed-off-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
---
 net/l2tp/l2tp_netlink.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 1b3fcde..abf6bf1 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -220,14 +220,14 @@ static int l2tp_nl_cmd_tunnel_create(struct sk_buff *skb, struct genl_info *info
 			cfg.local_udp_port = nla_get_u16(info->attrs[L2TP_ATTR_UDP_SPORT]);
 		if (info->attrs[L2TP_ATTR_UDP_DPORT])
 			cfg.peer_udp_port = nla_get_u16(info->attrs[L2TP_ATTR_UDP_DPORT]);
-		if (info->attrs[L2TP_ATTR_UDP_CSUM])
-			cfg.use_udp_checksums = nla_get_flag(info->attrs[L2TP_ATTR_UDP_CSUM]);
+		cfg.use_udp_checksums = nla_get_flag(
+			info->attrs[L2TP_ATTR_UDP_CSUM]);
 
 #if IS_ENABLED(CONFIG_IPV6)
-		if (info->attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX])
-			cfg.udp6_zero_tx_checksums = nla_get_flag(info->attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
-		if (info->attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX])
-			cfg.udp6_zero_rx_checksums = nla_get_flag(info->attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
+		cfg.udp6_zero_tx_checksums = nla_get_flag(
+			info->attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
+		cfg.udp6_zero_rx_checksums = nla_get_flag(
+			info->attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
 #endif
 	}
 
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1515469 — [PATCH net-next 3/5] net: l2tp: netlink: l2tp_nl_tunnel_send: set UDP6 checksum flags

This patch causes the proper attribute flags to be set,
in the case that IPv6 UDP checksums are disabled, so that
userspace ie. `ip l2tp show tunnel` knows about it.

Signed-off-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
---
 net/l2tp/l2tp_netlink.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index e45c5409..1b3fcde 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -385,6 +385,16 @@ static int l2tp_nl_tunnel_send(struct sk_buff *skb, u32 portid, u32 seq, int fla
 			    nla_put_flag(skb, L2TP_ATTR_UDP_CSUM))
 				goto nla_put_failure;
 			break;
+#if IS_ENABLED(CONFIG_IPV6)
+		case AF_INET6:
+			if (udp_get_no_check6_tx(sk) &&
+			    nla_put_flag(skb, L2TP_ATTR_UDP_ZERO_CSUM6_TX))
+				goto nla_put_failure;
+			if (udp_get_no_check6_rx(sk) &&
+			    nla_put_flag(skb, L2TP_ATTR_UDP_ZERO_CSUM6_RX))
+				goto nla_put_failure;
+			break;
+#endif
 		}
 		if (nla_put_u16(skb, L2TP_ATTR_UDP_SPORT, ntohs(inet->inet_sport)) ||
 		    nla_put_u16(skb, L2TP_ATTR_UDP_DPORT, ntohs(inet->inet_dport)))
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1516406 — Re: [PATCH net-next 1/5] net: l2tp: fix L2TP_ATTR_UDP_CSUM attribute type

From: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
Date: Fri,  4 Nov 2016 22:48:34 +0000

> L2TP_ATTR_UDP_CSUM is a flag, and gets read with
> nla_get_flag, but it is defined as NLA_U8 in
> the nla_policy.
> 
> It appears that this is only publicly used in
> iproute2, where it's broken, because it's used as
> a NLA_FLAG, and fails validation as a NLA_U8.
> 
> The only place it's used as a NLA_U8 is in
> l2tp_nl_tunnel_send(), but iproute2 again reads that
> as a flag, it's therefore always set. Fortunately
> it is never used for anything, just read.
> 
> CC: Miao Wang <shankerwangmiao@gmail.com>
> Signed-off-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>

This is definitely the wrong way to go about this.

The kernel is everywhere and updating iproute2 is infinitely
easier for users to do than updating the kernel.

And in any event, once exported we really should never change
the API of anything shown to userspace like this.  Just because
you can't find a user out there doesn't mean it doesn't exist.

Please instead fix iproute2 to use u8 attributes for this.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1516592 — Re: [PATCH net-next 1/5] net: l2tp: fix L2TP_ATTR_UDP_CSUM attribute type

[Multipart message — attachments visible in raw view] — view raw

Hi David,

Thanks for the review.

On Mon, 07 Nov 2016 13:08:45 -0500 (EST), David Miller <davem@davemloft.net> wrote:
> From: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
> Date: Fri,  4 Nov 2016 22:48:34 +0000
> 
> > L2TP_ATTR_UDP_CSUM is a flag, and gets read with
> > nla_get_flag, but it is defined as NLA_U8 in
> > the nla_policy.
> > 
> > It appears that this is only publicly used in
> > iproute2, where it's broken, because it's used as
> > a NLA_FLAG, and fails validation as a NLA_U8.
> > 
> > The only place it's used as a NLA_U8 is in
> > l2tp_nl_tunnel_send(), but iproute2 again reads that
> > as a flag, it's therefore always set. Fortunately
> > it is never used for anything, just read.
> > 
> > CC: Miao Wang <shankerwangmiao@gmail.com>
> > Signed-off-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
> 
> This is definitely the wrong way to go about this.
> 
> The kernel is everywhere and updating iproute2 is infinitely
> easier for users to do than updating the kernel.
> 
> And in any event, once exported we really should never change
> the API of anything shown to userspace like this.  Just because
> you can't find a user out there doesn't mean it doesn't exist.

Sure, I have submitted a v2 of the patchset, that keeps the
current netlink API intact.

Was unsure how frozen the API was in these outlying corners,
also only tried changing the cases where the kernel side is inconsistently
implemented, ie. kept L2TP_ATTR_{SEND,RECV}_SEQ as u8-flags since it was
used consitently.


> Please instead fix iproute2 to use u8 attributes for this.

Will do (set with u8-flag, read as u8).

-- 
Best regards
Asbjørn Sloth Tønnesen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web