* Re: [PATCH] ip: add udp_csum, udp6_csum_tx, udp6_csum_rx control flags to ip l2tp add tunnel
@ 2016-04-27 12:21 James Chapman
2016-04-27 15:33 ` Wang Shanker
0 siblings, 1 reply; 7+ messages in thread
From: James Chapman @ 2016-04-27 12:21 UTC (permalink / raw)
To: Wang Shanker; +Cc: netdev
On 26 April 2016 at 15:15, Wang Shanker <shankerwangmiao@gmail.com> wrote:
> Hi, all
>
> It’s my first time to contribute to such an important open source project. Things began when I upgraded my server, called "Server A", form ubuntu 14.04 to 16.04, which is shipped with new kernel version, 4.4. After upgrade, I soon found a l2tp tunnel between this server and another linux server, called "Server B", via ipv6 broke down. Here is the network topology:
>
> +----------+ +----------+
> | Server A | -- IPV6 Network -- | Server B |
> +----------+ +----------+
>
> The l2tp tunnel was encapsulated in udp datagrams. All the configuration was normal and could work after I reverted the kernel on Server A to original version.
>
> Here is what i did to create the tunnel:
>
> ```
> on Server A:
>
> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 remote 2001:db8::aaaa local 2001:db8::bbbb udp_sport 1086 udp_dport 1086
> ip l2tp add session name l2tpeth0 tunnel_id 86 session_id 86 peer_session_id 86
> ip l s l2tpeth0 up
>
> on Server B:
>
> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 local 2001:db8::aaaa remote 2001:db8::bbbb udp_sport 1086 udp_dport 1086
> ip l2tp add session name l2tpeth0 tunnel_id 86 session_id 86 peer_session_id 86
> ip l s l2tpeth0 up
>
> ```
>
> When I used tcpdump to diagnose the problem, I got such result:
>
> ```
> on Server A:
>
> arping -i l2tpeth0 -0 1.2.3.4
>
> on Server B:
>
> tcpdump -i eth0 -n port 1086 -v
>
> 21:35:57.818810 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
> 21:35:58.820572 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
> 21:35:59.822216 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
>
> ```
>
> After looking into kernel source, I found out that in this commit a new feature to set udp6 checksum to zero in commit 6b649fe, which added `L2TP_ATTR_UDP_ZERO_CSUM6_TX` and `L2TP_ATTR_UDP_ZERO_CSUM6_TX`.
>
> As a result, I added `udp_csum`, `udp6_csum_tx`, `udp6_csum_rx` control flags to `ip l2tp add tunnel` to control those attributes about checksum.
>
> Using this to create the tunnel instead on Server A:
>
> ```
> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 remote 2001:db8::aaaa local 2001:db8::bbbb udp_sport 1086 udp_dport 1086 udp6_csum_tx on udp6_csum_rx on
> ```
>
> I finally got:
>
> ```
> on Server A:
>
> arping -i l2tpeth0 -0 1.2.3.4
>
> on Server B:
>
> tcpdump -i eth0 -n port 1086 -v
>
> 22:07:03.844297 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
> 22:07:04.845717 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
> 22:07:05.847965 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
>
> tcpdump -i l2tpeth0 -v
>
> 22:10:35.691326 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
> 22:10:36.693627 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
> 22:10:37.695010 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
> 22:10:38.697121 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>
> ```
>
> It seems to work. However, is it the real point that should be fixed and does my patch fix it well? I need your suggestion.
This seems reasonable to me. It is good to provide user control of
L2TP checksum options.
However, there is a problem with your patch. The netlink attributes
you are accessing to control checksums are flags, not u8 values.
Maybe the default checksum setting for such l2tp tunnels should be
changed in the l2tp kernel code to match the previous behaviour where
IPv6 checksums were disabled?
>
>
> ---
> ip/ipl2tp.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c
> index 3c8ee93..67a6482 100644
> --- a/ip/ipl2tp.c
> +++ b/ip/ipl2tp.c
> @@ -56,6 +56,8 @@ struct l2tp_parm {
>
> uint16_t pw_type;
> uint16_t mtu;
> + int udp6_csum_tx:1;
> + int udp6_csum_rx:1;
> int udp_csum:1;
> int recv_seq:1;
> int send_seq:1;
> @@ -117,6 +119,9 @@ static int create_tunnel(struct l2tp_parm *p)
> if (p->encap == L2TP_ENCAPTYPE_UDP) {
> addattr16(&req.n, 1024, L2TP_ATTR_UDP_SPORT, p->local_udp_port);
> addattr16(&req.n, 1024, L2TP_ATTR_UDP_DPORT, p->peer_udp_port);
> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_CSUM, p->udp_csum);
> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_ZERO_CSUM6_TX, p->udp6_csum_tx);
> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_ZERO_CSUM6_RX, p->udp6_csum_rx);
> }
>
> if (rtnl_talk(&genl_rth, &req.n, NULL, 0) < 0)
> @@ -282,6 +287,14 @@ static int get_response(struct nlmsghdr *n, void *arg)
> p->l2spec_len = rta_getattr_u8(attrs[L2TP_ATTR_L2SPEC_LEN]);
>
> p->udp_csum = !!attrs[L2TP_ATTR_UDP_CSUM];
> + if (attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX])
> + p->udp6_csum_tx = rta_getattr_u8(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
> + else
> + p->udp6_csum_tx = 1;
> + if (attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX])
> + p->udp6_csum_rx = rta_getattr_u8(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
> + else
> + p->udp6_csum_rx = 1;
> if (attrs[L2TP_ATTR_COOKIE])
> memcpy(p->cookie, RTA_DATA(attrs[L2TP_ATTR_COOKIE]),
> p->cookie_len = RTA_PAYLOAD(attrs[L2TP_ATTR_COOKIE]));
> @@ -470,6 +483,9 @@ static void usage(void)
> fprintf(stderr, " tunnel_id ID peer_tunnel_id ID\n");
> fprintf(stderr, " [ encap { ip | udp } ]\n");
> fprintf(stderr, " [ udp_sport PORT ] [ udp_dport PORT ]\n");
> + fprintf(stderr, " [ udp_csum { on | off } ]\n");
> + fprintf(stderr, " [ udp6_csum_tx { on | off } ]\n");
> + fprintf(stderr, " [ udp6_csum_rx { on | off } ]\n");
> fprintf(stderr, "Usage: ip l2tp add session [ name NAME ]\n");
> fprintf(stderr, " tunnel_id ID\n");
> fprintf(stderr, " session_id ID peer_session_id ID\n");
> @@ -500,6 +516,8 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
> /* Defaults */
> p->l2spec_type = L2TP_L2SPECTYPE_DEFAULT;
> p->l2spec_len = 4;
> + p->udp6_csum_rx = 1;
> + p->udp6_csum_tx = 1;
>
> while (argc > 0) {
> if (strcmp(*argv, "encap") == 0) {
> @@ -569,6 +587,33 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
> if (get_u16(&uval, *argv, 0))
> invarg("invalid port\n", *argv);
> p->peer_udp_port = uval;
> + } else if (strcmp(*argv, "udp_csum") == 0) {
> + NEXT_ARG();
> + if (strcmp(*argv, "on") == 0) {
> + p->udp_csum = 1;
> + } else if (strcmp(*argv, "off") == 0) {
> + p->udp_csum = 0;
> + } else {
> + invarg("invalid option for udp_csum\n", *argv);
> + }
> + } else if (strcmp(*argv, "udp6_csum_rx") == 0) {
> + NEXT_ARG();
> + if (strcmp(*argv, "on") == 0) {
> + p->udp6_csum_rx = 1;
> + } else if (strcmp(*argv, "off") == 0) {
> + p->udp6_csum_rx = 0;
> + } else {
> + invarg("invalid option for udp6_csum_rx\n", *argv);
> + }
> + } else if (strcmp(*argv, "udp6_csum_tx") == 0) {
> + NEXT_ARG();
> + if (strcmp(*argv, "on") == 0) {
> + p->udp6_csum_tx = 1;
> + } else if (strcmp(*argv, "off") == 0) {
> + p->udp6_csum_tx = 0;
> + } else {
> + invarg("invalid option for udp6_csum_tx\n", *argv);
> + }
> } else if (strcmp(*argv, "offset") == 0) {
> __u8 uval;
>
> --
> 2.5.2
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ip: add udp_csum, udp6_csum_tx, udp6_csum_rx control flags to ip l2tp add tunnel
2016-04-27 12:21 [PATCH] ip: add udp_csum, udp6_csum_tx, udp6_csum_rx control flags to ip l2tp add tunnel James Chapman
@ 2016-04-27 15:33 ` Wang Shanker
2016-04-27 17:06 ` Wang Shanker
0 siblings, 1 reply; 7+ messages in thread
From: Wang Shanker @ 2016-04-27 15:33 UTC (permalink / raw)
To: James Chapman; +Cc: netdev
> 在 2016年4月27日，20:21，James Chapman <jchapman@katalix.com> 写道：
>
> On 26 April 2016 at 15:15, Wang Shanker <shankerwangmiao@gmail.com> wrote:
>> Hi, all
>>
>> It’s my first time to contribute to such an important open source project. Things began when I upgraded my server, called "Server A", form ubuntu 14.04 to 16.04, which is shipped with new kernel version, 4.4. After upgrade, I soon found a l2tp tunnel between this server and another linux server, called "Server B", via ipv6 broke down. Here is the network topology:
>>
>> +----------+ +----------+
>> | Server A | -- IPV6 Network -- | Server B |
>> +----------+ +----------+
>>
>> The l2tp tunnel was encapsulated in udp datagrams. All the configuration was normal and could work after I reverted the kernel on Server A to original version.
>>
>> Here is what i did to create the tunnel:
>>
>> ```
>> on Server A:
>>
>> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 remote 2001:db8::aaaa local 2001:db8::bbbb udp_sport 1086 udp_dport 1086
>> ip l2tp add session name l2tpeth0 tunnel_id 86 session_id 86 peer_session_id 86
>> ip l s l2tpeth0 up
>>
>> on Server B:
>>
>> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 local 2001:db8::aaaa remote 2001:db8::bbbb udp_sport 1086 udp_dport 1086
>> ip l2tp add session name l2tpeth0 tunnel_id 86 session_id 86 peer_session_id 86
>> ip l s l2tpeth0 up
>>
>> ```
>>
>> When I used tcpdump to diagnose the problem, I got such result:
>>
>> ```
>> on Server A:
>>
>> arping -i l2tpeth0 -0 1.2.3.4
>>
>> on Server B:
>>
>> tcpdump -i eth0 -n port 1086 -v
>>
>> 21:35:57.818810 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
>> 21:35:58.820572 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
>> 21:35:59.822216 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
>>
>> ```
>>
>> After looking into kernel source, I found out that in this commit a new feature to set udp6 checksum to zero in commit 6b649fe, which added `L2TP_ATTR_UDP_ZERO_CSUM6_TX` and `L2TP_ATTR_UDP_ZERO_CSUM6_TX`.
>>
>> As a result, I added `udp_csum`, `udp6_csum_tx`, `udp6_csum_rx` control flags to `ip l2tp add tunnel` to control those attributes about checksum.
>>
>> Using this to create the tunnel instead on Server A:
>>
>> ```
>> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 remote 2001:db8::aaaa local 2001:db8::bbbb udp_sport 1086 udp_dport 1086 udp6_csum_tx on udp6_csum_rx on
>> ```
>>
>> I finally got:
>>
>> ```
>> on Server A:
>>
>> arping -i l2tpeth0 -0 1.2.3.4
>>
>> on Server B:
>>
>> tcpdump -i eth0 -n port 1086 -v
>>
>> 22:07:03.844297 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
>> 22:07:04.845717 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
>> 22:07:05.847965 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
>>
>> tcpdump -i l2tpeth0 -v
>>
>> 22:10:35.691326 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>> 22:10:36.693627 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>> 22:10:37.695010 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>> 22:10:38.697121 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>>
>> ```
>>
>> It seems to work. However, is it the real point that should be fixed and does my patch fix it well? I need your suggestion.
>
> This seems reasonable to me. It is good to provide user control of
> L2TP checksum options.
>
> However, there is a problem with your patch. The netlink attributes
> you are accessing to control checksums are flags, not u8 values.
I’m not so familiar with kernel code. However, in `<linux/l2tp.h>` :
```
/*
* ATTR types defined for L2TP
*/
enum {
L2TP_ATTR_NONE, /* no data */
// ...
L2TP_ATTR_IP6_DADDR, /* struct in6_addr */
L2TP_ATTR_UDP_ZERO_CSUM6_TX, /* u8 */
L2TP_ATTR_UDP_ZERO_CSUM6_RX, /* u8 */
// ...
}
```
isn’t L2TP_ATTR_UDP_ZERO_CSUM6_TX a u8 value? Or should I use `addattr` instead of `addattr8`?
>
> Maybe the default checksum setting for such l2tp tunnels should be
> changed in the l2tp kernel code to match the previous behaviour where
> IPv6 checksums were disabled?
I think so. However, I’m confused with those code.
From the name of the attrs `L2TP_ATTR_UDP_ZERO_CSUM6_TX` and `L2TP_ATTR_UDP_ZERO_CSUM6_RX`, I
can tell that when those flags are set, the checksum will be zero. Also, according to the
comment of commit 6b649fe in kernel source, “Added new L2TP configuration options to allow
TX and RX of zero checksums in IPv6. Default is not to use them.”, checksums shouldn't have
been zero by default. However, in fact, they are. I think there may be some bugs in kernel
source.
>
>>
>>
>> ---
>> ip/ipl2tp.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 45 insertions(+)
>>
>> diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c
>> index 3c8ee93..67a6482 100644
>> --- a/ip/ipl2tp.c
>> +++ b/ip/ipl2tp.c
>> @@ -56,6 +56,8 @@ struct l2tp_parm {
>>
>> uint16_t pw_type;
>> uint16_t mtu;
>> + int udp6_csum_tx:1;
>> + int udp6_csum_rx:1;
>> int udp_csum:1;
>> int recv_seq:1;
>> int send_seq:1;
>> @@ -117,6 +119,9 @@ static int create_tunnel(struct l2tp_parm *p)
>> if (p->encap == L2TP_ENCAPTYPE_UDP) {
>> addattr16(&req.n, 1024, L2TP_ATTR_UDP_SPORT, p->local_udp_port);
>> addattr16(&req.n, 1024, L2TP_ATTR_UDP_DPORT, p->peer_udp_port);
>> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_CSUM, p->udp_csum);
>> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_ZERO_CSUM6_TX, p->udp6_csum_tx);
>> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_ZERO_CSUM6_RX, p->udp6_csum_rx);
>> }
>>
>> if (rtnl_talk(&genl_rth, &req.n, NULL, 0) < 0)
>> @@ -282,6 +287,14 @@ static int get_response(struct nlmsghdr *n, void *arg)
>> p->l2spec_len = rta_getattr_u8(attrs[L2TP_ATTR_L2SPEC_LEN]);
>>
>> p->udp_csum = !!attrs[L2TP_ATTR_UDP_CSUM];
>> + if (attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX])
>> + p->udp6_csum_tx = rta_getattr_u8(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
>> + else
>> + p->udp6_csum_tx = 1;
>> + if (attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX])
>> + p->udp6_csum_rx = rta_getattr_u8(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
>> + else
>> + p->udp6_csum_rx = 1;
>> if (attrs[L2TP_ATTR_COOKIE])
>> memcpy(p->cookie, RTA_DATA(attrs[L2TP_ATTR_COOKIE]),
>> p->cookie_len = RTA_PAYLOAD(attrs[L2TP_ATTR_COOKIE]));
>> @@ -470,6 +483,9 @@ static void usage(void)
>> fprintf(stderr, " tunnel_id ID peer_tunnel_id ID\n");
>> fprintf(stderr, " [ encap { ip | udp } ]\n");
>> fprintf(stderr, " [ udp_sport PORT ] [ udp_dport PORT ]\n");
>> + fprintf(stderr, " [ udp_csum { on | off } ]\n");
>> + fprintf(stderr, " [ udp6_csum_tx { on | off } ]\n");
>> + fprintf(stderr, " [ udp6_csum_rx { on | off } ]\n");
>> fprintf(stderr, "Usage: ip l2tp add session [ name NAME ]\n");
>> fprintf(stderr, " tunnel_id ID\n");
>> fprintf(stderr, " session_id ID peer_session_id ID\n");
>> @@ -500,6 +516,8 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
>> /* Defaults */
>> p->l2spec_type = L2TP_L2SPECTYPE_DEFAULT;
>> p->l2spec_len = 4;
>> + p->udp6_csum_rx = 1;
>> + p->udp6_csum_tx = 1;
>>
>> while (argc > 0) {
>> if (strcmp(*argv, "encap") == 0) {
>> @@ -569,6 +587,33 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
>> if (get_u16(&uval, *argv, 0))
>> invarg("invalid port\n", *argv);
>> p->peer_udp_port = uval;
>> + } else if (strcmp(*argv, "udp_csum") == 0) {
>> + NEXT_ARG();
>> + if (strcmp(*argv, "on") == 0) {
>> + p->udp_csum = 1;
>> + } else if (strcmp(*argv, "off") == 0) {
>> + p->udp_csum = 0;
>> + } else {
>> + invarg("invalid option for udp_csum\n", *argv);
>> + }
>> + } else if (strcmp(*argv, "udp6_csum_rx") == 0) {
>> + NEXT_ARG();
>> + if (strcmp(*argv, "on") == 0) {
>> + p->udp6_csum_rx = 1;
>> + } else if (strcmp(*argv, "off") == 0) {
>> + p->udp6_csum_rx = 0;
>> + } else {
>> + invarg("invalid option for udp6_csum_rx\n", *argv);
>> + }
>> + } else if (strcmp(*argv, "udp6_csum_tx") == 0) {
>> + NEXT_ARG();
>> + if (strcmp(*argv, "on") == 0) {
>> + p->udp6_csum_tx = 1;
>> + } else if (strcmp(*argv, "off") == 0) {
>> + p->udp6_csum_tx = 0;
>> + } else {
>> + invarg("invalid option for udp6_csum_tx\n", *argv);
>> + }
>> } else if (strcmp(*argv, "offset") == 0) {
>> __u8 uval;
>>
>> --
>> 2.5.2
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ip: add udp_csum, udp6_csum_tx, udp6_csum_rx control flags to ip l2tp add tunnel
2016-04-27 15:33 ` Wang Shanker
@ 2016-04-27 17:06 ` Wang Shanker
2016-04-28 14:50 ` James Chapman
0 siblings, 1 reply; 7+ messages in thread
From: Wang Shanker @ 2016-04-27 17:06 UTC (permalink / raw)
To: James Chapman; +Cc: netdev
> 在 2016年4月27日，23:33，Wang Shanker <shankerwangmiao@gmail.com> 写道：
>
>
>
>> 在 2016年4月27日，20:21，James Chapman <jchapman@katalix.com> 写道：
>>
>> On 26 April 2016 at 15:15, Wang Shanker <shankerwangmiao@gmail.com> wrote:
>>> Hi, all
>>>
>>> It’s my first time to contribute to such an important open source project. Things began when I upgraded my server, called "Server A", form ubuntu 14.04 to 16.04, which is shipped with new kernel version, 4.4. After upgrade, I soon found a l2tp tunnel between this server and another linux server, called "Server B", via ipv6 broke down. Here is the network topology:
>>>
>>> +----------+ +----------+
>>> | Server A | -- IPV6 Network -- | Server B |
>>> +----------+ +----------+
>>>
>>> The l2tp tunnel was encapsulated in udp datagrams. All the configuration was normal and could work after I reverted the kernel on Server A to original version.
>>>
>>> Here is what i did to create the tunnel:
>>>
>>> ```
>>> on Server A:
>>>
>>> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 remote 2001:db8::aaaa local 2001:db8::bbbb udp_sport 1086 udp_dport 1086
>>> ip l2tp add session name l2tpeth0 tunnel_id 86 session_id 86 peer_session_id 86
>>> ip l s l2tpeth0 up
>>>
>>> on Server B:
>>>
>>> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 local 2001:db8::aaaa remote 2001:db8::bbbb udp_sport 1086 udp_dport 1086
>>> ip l2tp add session name l2tpeth0 tunnel_id 86 session_id 86 peer_session_id 86
>>> ip l s l2tpeth0 up
>>>
>>> ```
>>>
>>> When I used tcpdump to diagnose the problem, I got such result:
>>>
>>> ```
>>> on Server A:
>>>
>>> arping -i l2tpeth0 -0 1.2.3.4
>>>
>>> on Server B:
>>>
>>> tcpdump -i eth0 -n port 1086 -v
>>>
>>> 21:35:57.818810 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
>>> 21:35:58.820572 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
>>> 21:35:59.822216 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
>>>
>>> ```
>>>
>>> After looking into kernel source, I found out that in this commit a new feature to set udp6 checksum to zero in commit 6b649fe, which added `L2TP_ATTR_UDP_ZERO_CSUM6_TX` and `L2TP_ATTR_UDP_ZERO_CSUM6_TX`.
>>>
>>> As a result, I added `udp_csum`, `udp6_csum_tx`, `udp6_csum_rx` control flags to `ip l2tp add tunnel` to control those attributes about checksum.
>>>
>>> Using this to create the tunnel instead on Server A:
>>>
>>> ```
>>> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 remote 2001:db8::aaaa local 2001:db8::bbbb udp_sport 1086 udp_dport 1086 udp6_csum_tx on udp6_csum_rx on
>>> ```
>>>
>>> I finally got:
>>>
>>> ```
>>> on Server A:
>>>
>>> arping -i l2tpeth0 -0 1.2.3.4
>>>
>>> on Server B:
>>>
>>> tcpdump -i eth0 -n port 1086 -v
>>>
>>> 22:07:03.844297 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
>>> 22:07:04.845717 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
>>> 22:07:05.847965 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
>>>
>>> tcpdump -i l2tpeth0 -v
>>>
>>> 22:10:35.691326 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>>> 22:10:36.693627 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>>> 22:10:37.695010 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>>> 22:10:38.697121 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>>>
>>> ```
>>>
>>> It seems to work. However, is it the real point that should be fixed and does my patch fix it well? I need your suggestion.
>>
>> This seems reasonable to me. It is good to provide user control of
>> L2TP checksum options.
>>
>> However, there is a problem with your patch. The netlink attributes
>> you are accessing to control checksums are flags, not u8 values.
>
> I’m not so familiar with kernel code. However, in `<linux/l2tp.h>` :
>
> ```
>
> /*
> * ATTR types defined for L2TP
> */
> enum {
> L2TP_ATTR_NONE, /* no data */
> // ...
> L2TP_ATTR_IP6_DADDR, /* struct in6_addr */
> L2TP_ATTR_UDP_ZERO_CSUM6_TX, /* u8 */
> L2TP_ATTR_UDP_ZERO_CSUM6_RX, /* u8 */
> // ...
>
> }
> ```
>
> isn’t L2TP_ATTR_UDP_ZERO_CSUM6_TX a u8 value? Or should I use `addattr` instead of `addattr8`?
>
>>
>> Maybe the default checksum setting for such l2tp tunnels should be
>> changed in the l2tp kernel code to match the previous behaviour where
>> IPv6 checksums were disabled?
>
> I think so. However, I’m confused with those code.
>
> From the name of the attrs `L2TP_ATTR_UDP_ZERO_CSUM6_TX` and `L2TP_ATTR_UDP_ZERO_CSUM6_RX`, I
> can tell that when those flags are set, the checksum will be zero. Also, according to the
> comment of commit 6b649fe in kernel source, “Added new L2TP configuration options to allow
> TX and RX of zero checksums in IPv6. Default is not to use them.”, checksums shouldn't have
> been zero by default. However, in fact, they are. I think there may be some bugs in kernel
> source.
I think I’ve got the bug. Here is the patch for kernel
---
net/l2tp/l2tp_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index afca2eb..6edfa99 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1376,9 +1376,9 @@ static int l2tp_tunnel_sock_create(struct net *net,
memcpy(&udp_conf.peer_ip6, cfg->peer_ip6,
sizeof(udp_conf.peer_ip6));
udp_conf.use_udp6_tx_checksums =
- cfg->udp6_zero_tx_checksums;
+ ! cfg->udp6_zero_tx_checksums;
udp_conf.use_udp6_rx_checksums =
- cfg->udp6_zero_rx_checksums;
+ ! cfg->udp6_zero_rx_checksums;
} else
#endif
{
--
>>
>>>
>>>
>>> ---
>>> ip/ipl2tp.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 45 insertions(+)
>>>
>>> diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c
>>> index 3c8ee93..67a6482 100644
>>> --- a/ip/ipl2tp.c
>>> +++ b/ip/ipl2tp.c
>>> @@ -56,6 +56,8 @@ struct l2tp_parm {
>>>
>>> uint16_t pw_type;
>>> uint16_t mtu;
>>> + int udp6_csum_tx:1;
>>> + int udp6_csum_rx:1;
>>> int udp_csum:1;
>>> int recv_seq:1;
>>> int send_seq:1;
>>> @@ -117,6 +119,9 @@ static int create_tunnel(struct l2tp_parm *p)
>>> if (p->encap == L2TP_ENCAPTYPE_UDP) {
>>> addattr16(&req.n, 1024, L2TP_ATTR_UDP_SPORT, p->local_udp_port);
>>> addattr16(&req.n, 1024, L2TP_ATTR_UDP_DPORT, p->peer_udp_port);
>>> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_CSUM, p->udp_csum);
>>> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_ZERO_CSUM6_TX, p->udp6_csum_tx);
>>> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_ZERO_CSUM6_RX, p->udp6_csum_rx);
>>> }
>>>
>>> if (rtnl_talk(&genl_rth, &req.n, NULL, 0) < 0)
>>> @@ -282,6 +287,14 @@ static int get_response(struct nlmsghdr *n, void *arg)
>>> p->l2spec_len = rta_getattr_u8(attrs[L2TP_ATTR_L2SPEC_LEN]);
>>>
>>> p->udp_csum = !!attrs[L2TP_ATTR_UDP_CSUM];
>>> + if (attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX])
>>> + p->udp6_csum_tx = rta_getattr_u8(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
>>> + else
>>> + p->udp6_csum_tx = 1;
>>> + if (attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX])
>>> + p->udp6_csum_rx = rta_getattr_u8(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
>>> + else
>>> + p->udp6_csum_rx = 1;
>>> if (attrs[L2TP_ATTR_COOKIE])
>>> memcpy(p->cookie, RTA_DATA(attrs[L2TP_ATTR_COOKIE]),
>>> p->cookie_len = RTA_PAYLOAD(attrs[L2TP_ATTR_COOKIE]));
>>> @@ -470,6 +483,9 @@ static void usage(void)
>>> fprintf(stderr, " tunnel_id ID peer_tunnel_id ID\n");
>>> fprintf(stderr, " [ encap { ip | udp } ]\n");
>>> fprintf(stderr, " [ udp_sport PORT ] [ udp_dport PORT ]\n");
>>> + fprintf(stderr, " [ udp_csum { on | off } ]\n");
>>> + fprintf(stderr, " [ udp6_csum_tx { on | off } ]\n");
>>> + fprintf(stderr, " [ udp6_csum_rx { on | off } ]\n");
>>> fprintf(stderr, "Usage: ip l2tp add session [ name NAME ]\n");
>>> fprintf(stderr, " tunnel_id ID\n");
>>> fprintf(stderr, " session_id ID peer_session_id ID\n");
>>> @@ -500,6 +516,8 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
>>> /* Defaults */
>>> p->l2spec_type = L2TP_L2SPECTYPE_DEFAULT;
>>> p->l2spec_len = 4;
>>> + p->udp6_csum_rx = 1;
>>> + p->udp6_csum_tx = 1;
>>>
>>> while (argc > 0) {
>>> if (strcmp(*argv, "encap") == 0) {
>>> @@ -569,6 +587,33 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
>>> if (get_u16(&uval, *argv, 0))
>>> invarg("invalid port\n", *argv);
>>> p->peer_udp_port = uval;
>>> + } else if (strcmp(*argv, "udp_csum") == 0) {
>>> + NEXT_ARG();
>>> + if (strcmp(*argv, "on") == 0) {
>>> + p->udp_csum = 1;
>>> + } else if (strcmp(*argv, "off") == 0) {
>>> + p->udp_csum = 0;
>>> + } else {
>>> + invarg("invalid option for udp_csum\n", *argv);
>>> + }
>>> + } else if (strcmp(*argv, "udp6_csum_rx") == 0) {
>>> + NEXT_ARG();
>>> + if (strcmp(*argv, "on") == 0) {
>>> + p->udp6_csum_rx = 1;
>>> + } else if (strcmp(*argv, "off") == 0) {
>>> + p->udp6_csum_rx = 0;
>>> + } else {
>>> + invarg("invalid option for udp6_csum_rx\n", *argv);
>>> + }
>>> + } else if (strcmp(*argv, "udp6_csum_tx") == 0) {
>>> + NEXT_ARG();
>>> + if (strcmp(*argv, "on") == 0) {
>>> + p->udp6_csum_tx = 1;
>>> + } else if (strcmp(*argv, "off") == 0) {
>>> + p->udp6_csum_tx = 0;
>>> + } else {
>>> + invarg("invalid option for udp6_csum_tx\n", *argv);
>>> + }
>>> } else if (strcmp(*argv, "offset") == 0) {
>>> __u8 uval;
>>>
>>> --
>>> 2.5.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ip: add udp_csum, udp6_csum_tx, udp6_csum_rx control flags to ip l2tp add tunnel
2016-04-27 17:06 ` Wang Shanker
@ 2016-04-28 14:50 ` James Chapman
2016-05-02 22:19 ` Stephen Hemminger
0 siblings, 1 reply; 7+ messages in thread
From: James Chapman @ 2016-04-28 14:50 UTC (permalink / raw)
To: Wang Shanker; +Cc: netdev
Yes, that looks like the problem.
The comments in l2tp.h which indicate that the csum attributes are u8
values are wrong. Code in net/l2tp/l2tp_netlink.c accesses these
attributes using nla_get_flag().
Please submit a patch to fix l2tp_tunnel_sock_create(). Include good
change notes and your signed-off-by tag so that it gets reviewed. See
Documentation/SubmittingPatches if you haven't submitted a kernel
patch here before.
On 27 April 2016 at 18:06, Wang Shanker <shankerwangmiao@gmail.com> wrote:
>
>
>> 在 2016年4月27日，23:33，Wang Shanker <shankerwangmiao@gmail.com> 写道：
>>
>>
>>
>>> 在 2016年4月27日，20:21，James Chapman <jchapman@katalix.com> 写道：
>>>
>>> On 26 April 2016 at 15:15, Wang Shanker <shankerwangmiao@gmail.com> wrote:
>>>> Hi, all
>>>>
>>>> It’s my first time to contribute to such an important open source project. Things began when I upgraded my server, called "Server A", form ubuntu 14.04 to 16.04, which is shipped with new kernel version, 4.4. After upgrade, I soon found a l2tp tunnel between this server and another linux server, called "Server B", via ipv6 broke down. Here is the network topology:
>>>>
>>>> +----------+ +----------+
>>>> | Server A | -- IPV6 Network -- | Server B |
>>>> +----------+ +----------+
>>>>
>>>> The l2tp tunnel was encapsulated in udp datagrams. All the configuration was normal and could work after I reverted the kernel on Server A to original version.
>>>>
>>>> Here is what i did to create the tunnel:
>>>>
>>>> ```
>>>> on Server A:
>>>>
>>>> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 remote 2001:db8::aaaa local 2001:db8::bbbb udp_sport 1086 udp_dport 1086
>>>> ip l2tp add session name l2tpeth0 tunnel_id 86 session_id 86 peer_session_id 86
>>>> ip l s l2tpeth0 up
>>>>
>>>> on Server B:
>>>>
>>>> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 local 2001:db8::aaaa remote 2001:db8::bbbb udp_sport 1086 udp_dport 1086
>>>> ip l2tp add session name l2tpeth0 tunnel_id 86 session_id 86 peer_session_id 86
>>>> ip l s l2tpeth0 up
>>>>
>>>> ```
>>>>
>>>> When I used tcpdump to diagnose the problem, I got such result:
>>>>
>>>> ```
>>>> on Server A:
>>>>
>>>> arping -i l2tpeth0 -0 1.2.3.4
>>>>
>>>> on Server B:
>>>>
>>>> tcpdump -i eth0 -n port 1086 -v
>>>>
>>>> 21:35:57.818810 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
>>>> 21:35:58.820572 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
>>>> 21:35:59.822216 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
>>>>
>>>> ```
>>>>
>>>> After looking into kernel source, I found out that in this commit a new feature to set udp6 checksum to zero in commit 6b649fe, which added `L2TP_ATTR_UDP_ZERO_CSUM6_TX` and `L2TP_ATTR_UDP_ZERO_CSUM6_TX`.
>>>>
>>>> As a result, I added `udp_csum`, `udp6_csum_tx`, `udp6_csum_rx` control flags to `ip l2tp add tunnel` to control those attributes about checksum.
>>>>
>>>> Using this to create the tunnel instead on Server A:
>>>>
>>>> ```
>>>> ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 remote 2001:db8::aaaa local 2001:db8::bbbb udp_sport 1086 udp_dport 1086 udp6_csum_tx on udp6_csum_rx on
>>>> ```
>>>>
>>>> I finally got:
>>>>
>>>> ```
>>>> on Server A:
>>>>
>>>> arping -i l2tpeth0 -0 1.2.3.4
>>>>
>>>> on Server B:
>>>>
>>>> tcpdump -i eth0 -n port 1086 -v
>>>>
>>>> 22:07:03.844297 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
>>>> 22:07:04.845717 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
>>>> 22:07:05.847965 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
>>>>
>>>> tcpdump -i l2tpeth0 -v
>>>>
>>>> 22:10:35.691326 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>>>> 22:10:36.693627 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>>>> 22:10:37.695010 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>>>> 22:10:38.697121 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
>>>>
>>>> ```
>>>>
>>>> It seems to work. However, is it the real point that should be fixed and does my patch fix it well? I need your suggestion.
>>>
>>> This seems reasonable to me. It is good to provide user control of
>>> L2TP checksum options.
>>>
>>> However, there is a problem with your patch. The netlink attributes
>>> you are accessing to control checksums are flags, not u8 values.
>>
>> I’m not so familiar with kernel code. However, in `<linux/l2tp.h>` :
>>
>> ```
>>
>> /*
>> * ATTR types defined for L2TP
>> */
>> enum {
>> L2TP_ATTR_NONE, /* no data */
>> // ...
>> L2TP_ATTR_IP6_DADDR, /* struct in6_addr */
>> L2TP_ATTR_UDP_ZERO_CSUM6_TX, /* u8 */
>> L2TP_ATTR_UDP_ZERO_CSUM6_RX, /* u8 */
>> // ...
>>
>> }
>> ```
>>
>> isn’t L2TP_ATTR_UDP_ZERO_CSUM6_TX a u8 value? Or should I use `addattr` instead of `addattr8`?
>>
>>>
>>> Maybe the default checksum setting for such l2tp tunnels should be
>>> changed in the l2tp kernel code to match the previous behaviour where
>>> IPv6 checksums were disabled?
>>
>> I think so. However, I’m confused with those code.
>>
>> From the name of the attrs `L2TP_ATTR_UDP_ZERO_CSUM6_TX` and `L2TP_ATTR_UDP_ZERO_CSUM6_RX`, I
>> can tell that when those flags are set, the checksum will be zero. Also, according to the
>> comment of commit 6b649fe in kernel source, “Added new L2TP configuration options to allow
>> TX and RX of zero checksums in IPv6. Default is not to use them.”, checksums shouldn't have
>> been zero by default. However, in fact, they are. I think there may be some bugs in kernel
>> source.
>
> I think I’ve got the bug. Here is the patch for kernel
>
> ---
> net/l2tp/l2tp_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index afca2eb..6edfa99 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1376,9 +1376,9 @@ static int l2tp_tunnel_sock_create(struct net *net,
> memcpy(&udp_conf.peer_ip6, cfg->peer_ip6,
> sizeof(udp_conf.peer_ip6));
> udp_conf.use_udp6_tx_checksums =
> - cfg->udp6_zero_tx_checksums;
> + ! cfg->udp6_zero_tx_checksums;
> udp_conf.use_udp6_rx_checksums =
> - cfg->udp6_zero_rx_checksums;
> + ! cfg->udp6_zero_rx_checksums;
> } else
> #endif
> {
> --
>
>
>>>
>>>>
>>>>
>>>> ---
>>>> ip/ipl2tp.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 45 insertions(+)
>>>>
>>>> diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c
>>>> index 3c8ee93..67a6482 100644
>>>> --- a/ip/ipl2tp.c
>>>> +++ b/ip/ipl2tp.c
>>>> @@ -56,6 +56,8 @@ struct l2tp_parm {
>>>>
>>>> uint16_t pw_type;
>>>> uint16_t mtu;
>>>> + int udp6_csum_tx:1;
>>>> + int udp6_csum_rx:1;
>>>> int udp_csum:1;
>>>> int recv_seq:1;
>>>> int send_seq:1;
>>>> @@ -117,6 +119,9 @@ static int create_tunnel(struct l2tp_parm *p)
>>>> if (p->encap == L2TP_ENCAPTYPE_UDP) {
>>>> addattr16(&req.n, 1024, L2TP_ATTR_UDP_SPORT, p->local_udp_port);
>>>> addattr16(&req.n, 1024, L2TP_ATTR_UDP_DPORT, p->peer_udp_port);
>>>> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_CSUM, p->udp_csum);
>>>> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_ZERO_CSUM6_TX, p->udp6_csum_tx);
>>>> + addattr8 (&req.n, 1024, L2TP_ATTR_UDP_ZERO_CSUM6_RX, p->udp6_csum_rx);
>>>> }
>>>>
>>>> if (rtnl_talk(&genl_rth, &req.n, NULL, 0) < 0)
>>>> @@ -282,6 +287,14 @@ static int get_response(struct nlmsghdr *n, void *arg)
>>>> p->l2spec_len = rta_getattr_u8(attrs[L2TP_ATTR_L2SPEC_LEN]);
>>>>
>>>> p->udp_csum = !!attrs[L2TP_ATTR_UDP_CSUM];
>>>> + if (attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX])
>>>> + p->udp6_csum_tx = rta_getattr_u8(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
>>>> + else
>>>> + p->udp6_csum_tx = 1;
>>>> + if (attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX])
>>>> + p->udp6_csum_rx = rta_getattr_u8(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
>>>> + else
>>>> + p->udp6_csum_rx = 1;
>>>> if (attrs[L2TP_ATTR_COOKIE])
>>>> memcpy(p->cookie, RTA_DATA(attrs[L2TP_ATTR_COOKIE]),
>>>> p->cookie_len = RTA_PAYLOAD(attrs[L2TP_ATTR_COOKIE]));
>>>> @@ -470,6 +483,9 @@ static void usage(void)
>>>> fprintf(stderr, " tunnel_id ID peer_tunnel_id ID\n");
>>>> fprintf(stderr, " [ encap { ip | udp } ]\n");
>>>> fprintf(stderr, " [ udp_sport PORT ] [ udp_dport PORT ]\n");
>>>> + fprintf(stderr, " [ udp_csum { on | off } ]\n");
>>>> + fprintf(stderr, " [ udp6_csum_tx { on | off } ]\n");
>>>> + fprintf(stderr, " [ udp6_csum_rx { on | off } ]\n");
>>>> fprintf(stderr, "Usage: ip l2tp add session [ name NAME ]\n");
>>>> fprintf(stderr, " tunnel_id ID\n");
>>>> fprintf(stderr, " session_id ID peer_session_id ID\n");
>>>> @@ -500,6 +516,8 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
>>>> /* Defaults */
>>>> p->l2spec_type = L2TP_L2SPECTYPE_DEFAULT;
>>>> p->l2spec_len = 4;
>>>> + p->udp6_csum_rx = 1;
>>>> + p->udp6_csum_tx = 1;
>>>>
>>>> while (argc > 0) {
>>>> if (strcmp(*argv, "encap") == 0) {
>>>> @@ -569,6 +587,33 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
>>>> if (get_u16(&uval, *argv, 0))
>>>> invarg("invalid port\n", *argv);
>>>> p->peer_udp_port = uval;
>>>> + } else if (strcmp(*argv, "udp_csum") == 0) {
>>>> + NEXT_ARG();
>>>> + if (strcmp(*argv, "on") == 0) {
>>>> + p->udp_csum = 1;
>>>> + } else if (strcmp(*argv, "off") == 0) {
>>>> + p->udp_csum = 0;
>>>> + } else {
>>>> + invarg("invalid option for udp_csum\n", *argv);
>>>> + }
>>>> + } else if (strcmp(*argv, "udp6_csum_rx") == 0) {
>>>> + NEXT_ARG();
>>>> + if (strcmp(*argv, "on") == 0) {
>>>> + p->udp6_csum_rx = 1;
>>>> + } else if (strcmp(*argv, "off") == 0) {
>>>> + p->udp6_csum_rx = 0;
>>>> + } else {
>>>> + invarg("invalid option for udp6_csum_rx\n", *argv);
>>>> + }
>>>> + } else if (strcmp(*argv, "udp6_csum_tx") == 0) {
>>>> + NEXT_ARG();
>>>> + if (strcmp(*argv, "on") == 0) {
>>>> + p->udp6_csum_tx = 1;
>>>> + } else if (strcmp(*argv, "off") == 0) {
>>>> + p->udp6_csum_tx = 0;
>>>> + } else {
>>>> + invarg("invalid option for udp6_csum_tx\n", *argv);
>>>> + }
>>>> } else if (strcmp(*argv, "offset") == 0) {
>>>> __u8 uval;
>>>>
>>>> --
>>>> 2.5.2
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ip: add udp_csum, udp6_csum_tx, udp6_csum_rx control flags to ip l2tp add tunnel
2016-04-28 14:50 ` James Chapman
@ 2016-05-02 22:19 ` Stephen Hemminger
2016-05-03 2:40 ` Wang Shanker
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2016-05-02 22:19 UTC (permalink / raw)
To: James Chapman; +Cc: Wang Shanker, netdev
On Thu, 28 Apr 2016 15:50:47 +0100
James Chapman <jchapman@katalix.com> wrote:
> Yes, that looks like the problem.
>
> The comments in l2tp.h which indicate that the csum attributes are u8
> values are wrong. Code in net/l2tp/l2tp_netlink.c accesses these
> attributes using nla_get_flag().
>
> Please submit a patch to fix l2tp_tunnel_sock_create(). Include good
> change notes and your signed-off-by tag so that it gets reviewed. See
> Documentation/SubmittingPatches if you haven't submitted a kernel
> patch here before.
Thank you for fixing this. James is is correct.
Please format the patch according to the submission guidelines.
For example, checkpatch complains about current patch.
ERROR: code indent should use tabs where possible
#156: FILE: net/l2tp/l2tp_core.c:1379:
+ ! cfg->udp6_zero_tx_checksums;$
WARNING: please, no spaces at the start of a line
#156: FILE: net/l2tp/l2tp_core.c:1379:
+ ! cfg->udp6_zero_tx_checksums;$
ERROR: space prohibited after that '!' (ctx:ExW)
#156: FILE: net/l2tp/l2tp_core.c:1379:
+ ! cfg->udp6_zero_tx_checksums;
^
ERROR: code indent should use tabs where possible
#159: FILE: net/l2tp/l2tp_core.c:1381:
+ ! cfg->udp6_zero_rx_checksums;$
WARNING: please, no spaces at the start of a line
#159: FILE: net/l2tp/l2tp_core.c:1381:
+ ! cfg->udp6_zero_rx_checksums;$
ERROR: space prohibited after that '!' (ctx:ExW)
#159: FILE: net/l2tp/l2tp_core.c:1381:
+ ! cfg->udp6_zero_rx_checksums;
^
ERROR: Missing Signed-off-by: line(s)
I am sorry that maintainers may seem like picky teachers in school
always putting redline around spelling errors, but this is how we work to teach
others how to follow the process.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ip: add udp_csum, udp6_csum_tx, udp6_csum_rx control flags to ip l2tp add tunnel
2016-05-02 22:19 ` Stephen Hemminger
@ 2016-05-03 2:40 ` Wang Shanker
0 siblings, 0 replies; 7+ messages in thread
From: Wang Shanker @ 2016-05-03 2:40 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: James Chapman, netdev
[-- Attachment #1: Type: text/plain, Size: 2198 bytes --]
Thanks a lot.
I feel it excited to contribute to the development of kernel. I’ll do
better next time.
> 在 2016年5月3日，06:19，Stephen Hemminger <stephen@networkplumber.org> 写道：
>
> On Thu, 28 Apr 2016 15:50:47 +0100
> James Chapman <jchapman@katalix.com> wrote:
>
>> Yes, that looks like the problem.
>>
>> The comments in l2tp.h which indicate that the csum attributes are u8
>> values are wrong. Code in net/l2tp/l2tp_netlink.c accesses these
>> attributes using nla_get_flag().
>>
>> Please submit a patch to fix l2tp_tunnel_sock_create(). Include good
>> change notes and your signed-off-by tag so that it gets reviewed. See
>> Documentation/SubmittingPatches if you haven't submitted a kernel
>> patch here before.
>
> Thank you for fixing this. James is is correct.
> Please format the patch according to the submission guidelines.
> For example, checkpatch complains about current patch.
>
> ERROR: code indent should use tabs where possible
> #156: FILE: net/l2tp/l2tp_core.c:1379:
> + ! cfg->udp6_zero_tx_checksums;$
>
> WARNING: please, no spaces at the start of a line
> #156: FILE: net/l2tp/l2tp_core.c:1379:
> + ! cfg->udp6_zero_tx_checksums;$
>
> ERROR: space prohibited after that '!' (ctx:ExW)
> #156: FILE: net/l2tp/l2tp_core.c:1379:
> + ! cfg->udp6_zero_tx_checksums;
> ^
>
> ERROR: code indent should use tabs where possible
> #159: FILE: net/l2tp/l2tp_core.c:1381:
> + ! cfg->udp6_zero_rx_checksums;$
>
> WARNING: please, no spaces at the start of a line
> #159: FILE: net/l2tp/l2tp_core.c:1381:
> + ! cfg->udp6_zero_rx_checksums;$
>
> ERROR: space prohibited after that '!' (ctx:ExW)
> #159: FILE: net/l2tp/l2tp_core.c:1381:
> + ! cfg->udp6_zero_rx_checksums;
> ^
>
> ERROR: Missing Signed-off-by: line(s)
>
>
> I am sorry that maintainers may seem like picky teachers in school
> always putting redline around spelling errors, but this is how we work to teach
> others how to follow the process.
>
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4130 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ip: add udp_csum, udp6_csum_tx, udp6_csum_rx control flags to ip l2tp add tunnel
@ 2016-04-26 14:15 Wang Shanker
0 siblings, 0 replies; 7+ messages in thread
From: Wang Shanker @ 2016-04-26 14:15 UTC (permalink / raw)
To: netdev
Hi, all
It’s my first time to contribute to such an important open source project. Things began when I upgraded my server, called "Server A", form ubuntu 14.04 to 16.04, which is shipped with new kernel version, 4.4. After upgrade, I soon found a l2tp tunnel between this server and another linux server, called "Server B", via ipv6 broke down. Here is the network topology:
+----------+ +----------+
| Server A | -- IPV6 Network -- | Server B |
+----------+ +----------+
The l2tp tunnel was encapsulated in udp datagrams. All the configuration was normal and could work after I reverted the kernel on Server A to original version.
Here is what i did to create the tunnel:
```
on Server A:
ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 remote 2001:db8::aaaa local 2001:db8::bbbb udp_sport 1086 udp_dport 1086
ip l2tp add session name l2tpeth0 tunnel_id 86 session_id 86 peer_session_id 86
ip l s l2tpeth0 up
on Server B:
ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 local 2001:db8::aaaa remote 2001:db8::bbbb udp_sport 1086 udp_dport 1086
ip l2tp add session name l2tpeth0 tunnel_id 86 session_id 86 peer_session_id 86
ip l s l2tpeth0 up
```
When I used tcpdump to diagnose the problem, I got such result:
```
on Server A:
arping -i l2tpeth0 -0 1.2.3.4
on Server B:
tcpdump -i eth0 -n port 1086 -v
21:35:57.818810 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
21:35:58.820572 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
21:35:59.822216 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [bad udp cksum 0x0000 -> 0x1140!] UDP, length 54
```
After looking into kernel source, I found out that in this commit a new feature to set udp6 checksum to zero in commit 6b649fe, which added `L2TP_ATTR_UDP_ZERO_CSUM6_TX` and `L2TP_ATTR_UDP_ZERO_CSUM6_TX`.
As a result, I added `udp_csum`, `udp6_csum_tx`, `udp6_csum_rx` control flags to `ip l2tp add tunnel` to control those attributes about checksum.
Using this to create the tunnel instead on Server A:
```
ip l2tp add tunnel tunnel_id 86 peer_tunnel_id 86 remote 2001:db8::aaaa local 2001:db8::bbbb udp_sport 1086 udp_dport 1086 udp6_csum_tx on udp6_csum_rx on
```
I finally got:
```
on Server A:
arping -i l2tpeth0 -0 1.2.3.4
on Server B:
tcpdump -i eth0 -n port 1086 -v
22:07:03.844297 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
22:07:04.845717 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
22:07:05.847965 IP6 (flowlabel 0x8f028, hlim 64, next-header UDP (17) payload length: 62) 2001:db8::aaaa.1086 > 2001:db8::bbbb.1086: [udp sum ok] UDP, length 54
tcpdump -i l2tpeth0 -v
22:10:35.691326 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
22:10:36.693627 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
22:10:37.695010 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
22:10:38.697121 ARP, Ethernet (len 6), IPv4 (len 4), Request who-has 1.2.3.4 tell 0.0.0.0, length 28
```
It seems to work. However, is it the real point that should be fixed and does my patch fix it well? I need your suggestion.
---
ip/ipl2tp.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c
index 3c8ee93..67a6482 100644
--- a/ip/ipl2tp.c
+++ b/ip/ipl2tp.c
@@ -56,6 +56,8 @@ struct l2tp_parm {
uint16_t pw_type;
uint16_t mtu;
+ int udp6_csum_tx:1;
+ int udp6_csum_rx:1;
int udp_csum:1;
int recv_seq:1;
int send_seq:1;
@@ -117,6 +119,9 @@ static int create_tunnel(struct l2tp_parm *p)
if (p->encap == L2TP_ENCAPTYPE_UDP) {
addattr16(&req.n, 1024, L2TP_ATTR_UDP_SPORT, p->local_udp_port);
addattr16(&req.n, 1024, L2TP_ATTR_UDP_DPORT, p->peer_udp_port);
+ addattr8 (&req.n, 1024, L2TP_ATTR_UDP_CSUM, p->udp_csum);
+ addattr8 (&req.n, 1024, L2TP_ATTR_UDP_ZERO_CSUM6_TX, p->udp6_csum_tx);
+ addattr8 (&req.n, 1024, L2TP_ATTR_UDP_ZERO_CSUM6_RX, p->udp6_csum_rx);
}
if (rtnl_talk(&genl_rth, &req.n, NULL, 0) < 0)
@@ -282,6 +287,14 @@ static int get_response(struct nlmsghdr *n, void *arg)
p->l2spec_len = rta_getattr_u8(attrs[L2TP_ATTR_L2SPEC_LEN]);
p->udp_csum = !!attrs[L2TP_ATTR_UDP_CSUM];
+ if (attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX])
+ p->udp6_csum_tx = rta_getattr_u8(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_TX]);
+ else
+ p->udp6_csum_tx = 1;
+ if (attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX])
+ p->udp6_csum_rx = rta_getattr_u8(attrs[L2TP_ATTR_UDP_ZERO_CSUM6_RX]);
+ else
+ p->udp6_csum_rx = 1;
if (attrs[L2TP_ATTR_COOKIE])
memcpy(p->cookie, RTA_DATA(attrs[L2TP_ATTR_COOKIE]),
p->cookie_len = RTA_PAYLOAD(attrs[L2TP_ATTR_COOKIE]));
@@ -470,6 +483,9 @@ static void usage(void)
fprintf(stderr, " tunnel_id ID peer_tunnel_id ID\n");
fprintf(stderr, " [ encap { ip | udp } ]\n");
fprintf(stderr, " [ udp_sport PORT ] [ udp_dport PORT ]\n");
+ fprintf(stderr, " [ udp_csum { on | off } ]\n");
+ fprintf(stderr, " [ udp6_csum_tx { on | off } ]\n");
+ fprintf(stderr, " [ udp6_csum_rx { on | off } ]\n");
fprintf(stderr, "Usage: ip l2tp add session [ name NAME ]\n");
fprintf(stderr, " tunnel_id ID\n");
fprintf(stderr, " session_id ID peer_session_id ID\n");
@@ -500,6 +516,8 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
/* Defaults */
p->l2spec_type = L2TP_L2SPECTYPE_DEFAULT;
p->l2spec_len = 4;
+ p->udp6_csum_rx = 1;
+ p->udp6_csum_tx = 1;
while (argc > 0) {
if (strcmp(*argv, "encap") == 0) {
@@ -569,6 +587,33 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
if (get_u16(&uval, *argv, 0))
invarg("invalid port\n", *argv);
p->peer_udp_port = uval;
+ } else if (strcmp(*argv, "udp_csum") == 0) {
+ NEXT_ARG();
+ if (strcmp(*argv, "on") == 0) {
+ p->udp_csum = 1;
+ } else if (strcmp(*argv, "off") == 0) {
+ p->udp_csum = 0;
+ } else {
+ invarg("invalid option for udp_csum\n", *argv);
+ }
+ } else if (strcmp(*argv, "udp6_csum_rx") == 0) {
+ NEXT_ARG();
+ if (strcmp(*argv, "on") == 0) {
+ p->udp6_csum_rx = 1;
+ } else if (strcmp(*argv, "off") == 0) {
+ p->udp6_csum_rx = 0;
+ } else {
+ invarg("invalid option for udp6_csum_rx\n", *argv);
+ }
+ } else if (strcmp(*argv, "udp6_csum_tx") == 0) {
+ NEXT_ARG();
+ if (strcmp(*argv, "on") == 0) {
+ p->udp6_csum_tx = 1;
+ } else if (strcmp(*argv, "off") == 0) {
+ p->udp6_csum_tx = 0;
+ } else {
+ invarg("invalid option for udp6_csum_tx\n", *argv);
+ }
} else if (strcmp(*argv, "offset") == 0) {
__u8 uval;
--
2.5.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-05-03 2:40 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-27 12:21 [PATCH] ip: add udp_csum, udp6_csum_tx, udp6_csum_rx control flags to ip l2tp add tunnel James Chapman
2016-04-27 15:33 ` Wang Shanker
2016-04-27 17:06 ` Wang Shanker
2016-04-28 14:50 ` James Chapman
2016-05-02 22:19 ` Stephen Hemminger
2016-05-03 2:40 ` Wang Shanker
-- strict thread matches above, loose matches on Subject: below --
2016-04-26 14:15 Wang Shanker
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.