Re: [PATCH] ip: add udp_csum, udp6_csum_tx, udp6_csum_rx control flags to ip l2tp add tunnel

James Chapman
2016-04-28T14:50:52Z

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
>