* [PATCH net-next 1/3] l2tp: define l2tp_tunnel_dst_mtu()
2018-08-03 10:38 [PATCH net-next 0/3] l2tp: sanitise MTU handling on sessions Guillaume Nault
@ 2018-08-03 10:38 ` Guillaume Nault
2018-08-03 10:38 ` [PATCH net-next 2/3] l2tp: simplify MTU handling in l2tp_ppp Guillaume Nault
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Guillaume Nault @ 2018-08-03 10:38 UTC (permalink / raw)
To: netdev; +Cc: James Chapman
Consolidate retrieval of tunnel's socket mtu in order to simplify
l2tp_eth and l2tp_ppp a bit.
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
net/l2tp/l2tp_core.h | 18 ++++++++++++++++++
net/l2tp/l2tp_eth.c | 14 ++++----------
net/l2tp/l2tp_ppp.c | 15 ++++-----------
3 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index fa5ae9432d38..1ca39629031b 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -12,6 +12,9 @@
#ifndef _L2TP_CORE_H_
#define _L2TP_CORE_H_
+#include <net/dst.h>
+#include <net/sock.h>
+
/* Just some random numbers */
#define L2TP_TUNNEL_MAGIC 0x42114DDA
#define L2TP_SESSION_MAGIC 0x0C04EB7D
@@ -268,6 +271,21 @@ static inline int l2tp_get_l2specific_len(struct l2tp_session *session)
}
}
+static inline u32 l2tp_tunnel_dst_mtu(const struct l2tp_tunnel *tunnel)
+{
+ struct dst_entry *dst;
+ u32 mtu;
+
+ dst = sk_dst_get(tunnel->sock);
+ if (!dst)
+ return 0;
+
+ mtu = dst_mtu(dst);
+ dst_release(dst);
+
+ return mtu;
+}
+
#define l2tp_printk(ptr, type, func, fmt, ...) \
do { \
if (((ptr)->debug) & (type)) \
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 5c366ecfa1cb..cfca5e63ae31 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -226,8 +226,8 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
struct net_device *dev)
{
unsigned int overhead = 0;
- struct dst_entry *dst;
u32 l3_overhead = 0;
+ u32 mtu;
/* if the encap is UDP, account for UDP header size */
if (tunnel->encap == L2TP_ENCAPTYPE_UDP) {
@@ -256,15 +256,9 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
overhead += session->hdr_len + ETH_HLEN + l3_overhead;
/* If PMTU discovery was enabled, use discovered MTU on L2TP device */
- dst = sk_dst_get(tunnel->sock);
- if (dst) {
- /* dst_mtu will use PMTU if found, else fallback to intf MTU */
- u32 pmtu = dst_mtu(dst);
-
- if (pmtu != 0)
- dev->mtu = pmtu;
- dst_release(dst);
- }
+ mtu = l2tp_tunnel_dst_mtu(tunnel);
+ if (mtu)
+ dev->mtu = mtu;
session->mtu = dev->mtu - overhead;
dev->mtu = session->mtu;
dev->needed_headroom += session->hdr_len;
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 44cac66284a5..1c6da02f976a 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -93,7 +93,6 @@
#include <linux/nsproxy.h>
#include <net/net_namespace.h>
#include <net/netns/generic.h>
-#include <net/dst.h>
#include <net/ip.h>
#include <net/udp.h>
#include <net/xfrm.h>
@@ -554,7 +553,7 @@ static void pppol2tp_show(struct seq_file *m, void *arg)
static void pppol2tp_session_init(struct l2tp_session *session)
{
struct pppol2tp_session *ps;
- struct dst_entry *dst;
+ u32 mtu;
session->recv_skb = pppol2tp_recv;
#if IS_ENABLED(CONFIG_L2TP_DEBUGFS)
@@ -566,15 +565,9 @@ static void pppol2tp_session_init(struct l2tp_session *session)
ps->owner = current->pid;
/* If PMTU discovery was enabled, use the MTU that was discovered */
- dst = sk_dst_get(session->tunnel->sock);
- if (dst) {
- u32 pmtu = dst_mtu(dst);
-
- if (pmtu)
- session->mtu = pmtu - PPPOL2TP_HEADER_OVERHEAD;
-
- dst_release(dst);
- }
+ mtu = l2tp_tunnel_dst_mtu(session->tunnel);
+ if (mtu)
+ session->mtu = mtu - PPPOL2TP_HEADER_OVERHEAD;
}
struct l2tp_connect_info {
--
2.18.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net-next 2/3] l2tp: simplify MTU handling in l2tp_ppp
2018-08-03 10:38 [PATCH net-next 0/3] l2tp: sanitise MTU handling on sessions Guillaume Nault
2018-08-03 10:38 ` [PATCH net-next 1/3] l2tp: define l2tp_tunnel_dst_mtu() Guillaume Nault
@ 2018-08-03 10:38 ` Guillaume Nault
2018-08-03 10:38 ` [PATCH net-next 3/3] l2tp: ignore L2TP_ATTR_MTU Guillaume Nault
2018-08-03 17:04 ` [PATCH net-next 0/3] l2tp: sanitise MTU handling on sessions David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Guillaume Nault @ 2018-08-03 10:38 UTC (permalink / raw)
To: netdev; +Cc: James Chapman
The value of the session's .mtu field, as defined by
pppol2tp_connect() or pppol2tp_session_create(), is later overwritten
by pppol2tp_session_init() (unless getting the tunnel's socket PMTU
fails). This field is then only used when setting the PPP channel's MTU
in pppol2tp_connect().
Furthermore, the SIOC[GS]IFMTU ioctls only act on the session's .mtu
without propagating this value to the PPP channel, making them useless.
This patch initialises the PPP channel's MTU directly and ignores the
session's .mtu entirely. MTU is still computed by subtracting the
PPPOL2TP_HEADER_OVERHEAD constant. It is not optimal, but that doesn't
really matter: po->chan.mtu is only used when the channel is part of a
multilink PPP bundle. Running multilink PPP over packet switched
networks is certainly not going to be efficient, so not picking the
best MTU does not harm (in the worst case, packets will just be
fragmented by the underlay).
The SIOC[GS]IFMTU ioctls are removed entirely (as opposed to simply
ignored), because these ioctls commands are part of the requests that
should be handled generically by the socket layer. PX_PROTO_OL2TP was
the only socket type abusing these ioctls.
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
net/l2tp/l2tp_ppp.c | 67 ++++++++++++---------------------------------
1 file changed, 18 insertions(+), 49 deletions(-)
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 1c6da02f976a..b403728e2757 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -553,7 +553,6 @@ static void pppol2tp_show(struct seq_file *m, void *arg)
static void pppol2tp_session_init(struct l2tp_session *session)
{
struct pppol2tp_session *ps;
- u32 mtu;
session->recv_skb = pppol2tp_recv;
#if IS_ENABLED(CONFIG_L2TP_DEBUGFS)
@@ -563,11 +562,6 @@ static void pppol2tp_session_init(struct l2tp_session *session)
ps = l2tp_session_priv(session);
mutex_init(&ps->sk_lock);
ps->owner = current->pid;
-
- /* If PMTU discovery was enabled, use the MTU that was discovered */
- mtu = l2tp_tunnel_dst_mtu(session->tunnel);
- if (mtu)
- session->mtu = mtu - PPPOL2TP_HEADER_OVERHEAD;
}
struct l2tp_connect_info {
@@ -654,6 +648,22 @@ static int pppol2tp_sockaddr_get_info(const void *sa, int sa_len,
return 0;
}
+/* Rough estimation of the maximum payload size a tunnel can transmit without
+ * fragmenting at the lower IP layer. Assumes L2TPv2 with sequence
+ * numbers and no IP option. Not quite accurate, but the result is mostly
+ * unused anyway.
+ */
+static int pppol2tp_tunnel_mtu(const struct l2tp_tunnel *tunnel)
+{
+ int mtu;
+
+ mtu = l2tp_tunnel_dst_mtu(tunnel);
+ if (mtu <= PPPOL2TP_HEADER_OVERHEAD)
+ return 1500 - PPPOL2TP_HEADER_OVERHEAD;
+
+ return mtu - PPPOL2TP_HEADER_OVERHEAD;
+}
+
/* connect() handler. Attach a PPPoX socket to a tunnel UDP socket
*/
static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
@@ -771,8 +781,6 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
goto end;
}
} else {
- /* Default MTU must allow space for UDP/L2TP/PPP headers */
- cfg.mtu = 1500 - PPPOL2TP_HEADER_OVERHEAD;
cfg.pw_type = L2TP_PWTYPE_PPP;
session = l2tp_session_create(sizeof(struct pppol2tp_session),
@@ -817,7 +825,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
po->chan.private = sk;
po->chan.ops = &pppol2tp_chan_ops;
- po->chan.mtu = session->mtu;
+ po->chan.mtu = pppol2tp_tunnel_mtu(tunnel);
error = ppp_register_net_channel(sock_net(sk), &po->chan);
if (error) {
@@ -873,10 +881,6 @@ static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
goto err;
}
- /* Default MTU values. */
- if (cfg->mtu == 0)
- cfg->mtu = 1500 - PPPOL2TP_HEADER_OVERHEAD;
-
/* Allocate and initialize a new session context. */
session = l2tp_session_create(sizeof(struct pppol2tp_session),
tunnel, session_id,
@@ -1040,7 +1044,6 @@ static void pppol2tp_copy_stats(struct pppol2tp_ioc_stats *dest,
static int pppol2tp_session_ioctl(struct l2tp_session *session,
unsigned int cmd, unsigned long arg)
{
- struct ifreq ifr;
int err = 0;
struct sock *sk;
int val = (int) arg;
@@ -1056,39 +1059,6 @@ static int pppol2tp_session_ioctl(struct l2tp_session *session,
return -EBADR;
switch (cmd) {
- case SIOCGIFMTU:
- err = -ENXIO;
- if (!(sk->sk_state & PPPOX_CONNECTED))
- break;
-
- err = -EFAULT;
- if (copy_from_user(&ifr, (void __user *) arg, sizeof(struct ifreq)))
- break;
- ifr.ifr_mtu = session->mtu;
- if (copy_to_user((void __user *) arg, &ifr, sizeof(struct ifreq)))
- break;
-
- l2tp_info(session, L2TP_MSG_CONTROL, "%s: get mtu=%d\n",
- session->name, session->mtu);
- err = 0;
- break;
-
- case SIOCSIFMTU:
- err = -ENXIO;
- if (!(sk->sk_state & PPPOX_CONNECTED))
- break;
-
- err = -EFAULT;
- if (copy_from_user(&ifr, (void __user *) arg, sizeof(struct ifreq)))
- break;
-
- session->mtu = ifr.ifr_mtu;
-
- l2tp_info(session, L2TP_MSG_CONTROL, "%s: set mtu=%d\n",
- session->name, session->mtu);
- err = 0;
- break;
-
case PPPIOCGMRU:
case PPPIOCGFLAGS:
err = -EFAULT;
@@ -1685,8 +1655,7 @@ static void pppol2tp_seq_session_show(struct seq_file *m, void *v)
tunnel->peer_tunnel_id,
session->peer_session_id,
state, user_data_ok);
- seq_printf(m, " %d/0/%c/%c/%s %08x %u\n",
- session->mtu,
+ seq_printf(m, " 0/0/%c/%c/%s %08x %u\n",
session->recv_seq ? 'R' : '-',
session->send_seq ? 'S' : '-',
session->lns_mode ? "LNS" : "LAC",
--
2.18.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net-next 3/3] l2tp: ignore L2TP_ATTR_MTU
2018-08-03 10:38 [PATCH net-next 0/3] l2tp: sanitise MTU handling on sessions Guillaume Nault
2018-08-03 10:38 ` [PATCH net-next 1/3] l2tp: define l2tp_tunnel_dst_mtu() Guillaume Nault
2018-08-03 10:38 ` [PATCH net-next 2/3] l2tp: simplify MTU handling in l2tp_ppp Guillaume Nault
@ 2018-08-03 10:38 ` Guillaume Nault
2018-08-03 17:04 ` [PATCH net-next 0/3] l2tp: sanitise MTU handling on sessions David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Guillaume Nault @ 2018-08-03 10:38 UTC (permalink / raw)
To: netdev; +Cc: James Chapman, R. Parameswaran
This attribute's handling is broken. It can only be used when creating
Ethernet pseudo-wires, in which case its value can be used as the
initial MTU for the l2tpeth device.
However, when handling update requests, L2TP_ATTR_MTU only modifies
session->mtu. This value is never propagated to the l2tpeth device.
Dump requests also return the value of session->mtu, which is not
synchronised anymore with the device MTU.
The same problem occurs if the device MTU is properly updated using the
generic IFLA_MTU attribute. In this case, session->mtu is not updated,
and L2TP_ATTR_MTU will report an invalid value again when dumping the
session.
It does not seem worthwhile to complexify l2tp_eth.c to synchronise
session->mtu with the device MTU. Even the ip-l2tp manpage advises to
use 'ip link' to initialise the MTU of l2tpeth devices (iproute2 does
not handle L2TP_ATTR_MTU at all anyway). So let's just ignore it
entirely.
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
include/uapi/linux/l2tp.h | 2 +-
net/l2tp/l2tp_core.c | 1 -
net/l2tp/l2tp_core.h | 2 --
net/l2tp/l2tp_debugfs.c | 3 +--
net/l2tp/l2tp_eth.c | 17 +++++++----------
net/l2tp/l2tp_netlink.c | 9 +--------
6 files changed, 10 insertions(+), 24 deletions(-)
diff --git a/include/uapi/linux/l2tp.h b/include/uapi/linux/l2tp.h
index 8bb8c7cfabe5..61158f5a1a5b 100644
--- a/include/uapi/linux/l2tp.h
+++ b/include/uapi/linux/l2tp.h
@@ -119,7 +119,7 @@ enum {
L2TP_ATTR_IP_DADDR, /* u32 */
L2TP_ATTR_UDP_SPORT, /* u16 */
L2TP_ATTR_UDP_DPORT, /* u16 */
- L2TP_ATTR_MTU, /* u16 */
+ L2TP_ATTR_MTU, /* u16 (not used) */
L2TP_ATTR_MRU, /* u16 (not used) */
L2TP_ATTR_STATS, /* nested */
L2TP_ATTR_IP6_SADDR, /* struct in6_addr */
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index c61a467fd9b8..ac6a00bcec71 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1674,7 +1674,6 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
if (cfg) {
session->pwtype = cfg->pw_type;
session->debug = cfg->debug;
- session->mtu = cfg->mtu;
session->send_seq = cfg->send_seq;
session->recv_seq = cfg->recv_seq;
session->lns_mode = cfg->lns_mode;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 1ca39629031b..5804065dfbfb 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -64,7 +64,6 @@ struct l2tp_session_cfg {
int peer_cookie_len; /* 0, 4 or 8 bytes */
int reorder_timeout; /* configured reorder timeout
* (in jiffies) */
- int mtu;
char *ifname;
};
@@ -108,7 +107,6 @@ struct l2tp_session {
int reorder_timeout; /* configured reorder timeout
* (in jiffies) */
int reorder_skip; /* set if skip to next nr */
- int mtu;
enum l2tp_pwtype pwtype;
struct l2tp_stats stats;
struct hlist_node global_hlist; /* Global hash list node */
diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c
index aee271741f5b..9821a1458555 100644
--- a/net/l2tp/l2tp_debugfs.c
+++ b/net/l2tp/l2tp_debugfs.c
@@ -191,8 +191,7 @@ static void l2tp_dfs_seq_session_show(struct seq_file *m, void *v)
if (session->send_seq || session->recv_seq)
seq_printf(m, " nr %hu, ns %hu\n", session->nr, session->ns);
seq_printf(m, " refcnt %d\n", refcount_read(&session->ref_count));
- seq_printf(m, " config %d/0/%c/%c/-/%s %08x %u\n",
- session->mtu,
+ seq_printf(m, " config 0/0/%c/%c/-/%s %08x %u\n",
session->recv_seq ? 'R' : '-',
session->send_seq ? 'S' : '-',
session->lns_mode ? "LNS" : "LAC",
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index cfca5e63ae31..3728986ec885 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -234,14 +234,11 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
overhead += sizeof(struct udphdr);
dev->needed_headroom += sizeof(struct udphdr);
}
- if (session->mtu != 0) {
- dev->mtu = session->mtu;
- dev->needed_headroom += session->hdr_len;
- return;
- }
+
lock_sock(tunnel->sock);
l3_overhead = kernel_sock_ip_overhead(tunnel->sock);
release_sock(tunnel->sock);
+
if (l3_overhead == 0) {
/* L3 Overhead couldn't be identified, this could be
* because tunnel->sock was NULL or the socket's
@@ -255,12 +252,12 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
*/
overhead += session->hdr_len + ETH_HLEN + l3_overhead;
- /* If PMTU discovery was enabled, use discovered MTU on L2TP device */
- mtu = l2tp_tunnel_dst_mtu(tunnel);
- if (mtu)
+ mtu = l2tp_tunnel_dst_mtu(tunnel) - overhead;
+ if (mtu < dev->min_mtu || mtu > dev->max_mtu)
+ dev->mtu = ETH_DATA_LEN - overhead;
+ else
dev->mtu = mtu;
- session->mtu = dev->mtu - overhead;
- dev->mtu = session->mtu;
+
dev->needed_headroom += session->hdr_len;
}
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index a7c409215336..2e1e92651545 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -608,9 +608,6 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
if (info->attrs[L2TP_ATTR_RECV_TIMEOUT])
cfg.reorder_timeout = nla_get_msecs(info->attrs[L2TP_ATTR_RECV_TIMEOUT]);
- if (info->attrs[L2TP_ATTR_MTU])
- cfg.mtu = nla_get_u16(info->attrs[L2TP_ATTR_MTU]);
-
#ifdef CONFIG_MODULES
if (l2tp_nl_cmd_ops[cfg.pw_type] == NULL) {
genl_unlock();
@@ -698,9 +695,6 @@ static int l2tp_nl_cmd_session_modify(struct sk_buff *skb, struct genl_info *inf
if (info->attrs[L2TP_ATTR_RECV_TIMEOUT])
session->reorder_timeout = nla_get_msecs(info->attrs[L2TP_ATTR_RECV_TIMEOUT]);
- if (info->attrs[L2TP_ATTR_MTU])
- session->mtu = nla_get_u16(info->attrs[L2TP_ATTR_MTU]);
-
ret = l2tp_session_notify(&l2tp_nl_family, info,
session, L2TP_CMD_SESSION_MODIFY);
@@ -730,8 +724,7 @@ static int l2tp_nl_session_send(struct sk_buff *skb, u32 portid, u32 seq, int fl
nla_put_u32(skb, L2TP_ATTR_PEER_SESSION_ID,
session->peer_session_id) ||
nla_put_u32(skb, L2TP_ATTR_DEBUG, session->debug) ||
- nla_put_u16(skb, L2TP_ATTR_PW_TYPE, session->pwtype) ||
- nla_put_u16(skb, L2TP_ATTR_MTU, session->mtu))
+ nla_put_u16(skb, L2TP_ATTR_PW_TYPE, session->pwtype))
goto nla_put_failure;
if ((session->ifname[0] &&
--
2.18.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net-next 0/3] l2tp: sanitise MTU handling on sessions
2018-08-03 10:38 [PATCH net-next 0/3] l2tp: sanitise MTU handling on sessions Guillaume Nault
` (2 preceding siblings ...)
2018-08-03 10:38 ` [PATCH net-next 3/3] l2tp: ignore L2TP_ATTR_MTU Guillaume Nault
@ 2018-08-03 17:04 ` David Miller
3 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2018-08-03 17:04 UTC (permalink / raw)
To: g.nault; +Cc: netdev, jchapman
From: Guillaume Nault <g.nault@alphalink.fr>
Date: Fri, 3 Aug 2018 12:38:32 +0200
> Most of the code handling sessions' MTU has no effect. The ->mtu field
> in struct l2tp_session might be used at session creation time, but
> neither PPP nor Ethernet pseudo-wires take updates into account.
>
> L2TP sessions don't have a concept of MTU, which is the reason why
> ->mtu is mostly ignored. MTU should remain a network device thing.
> Therefore this patch set does not try to propagate/update ->mtu to/from
> the device. That would complicate the code unnecessarily. Instead this
> field and the associated ioctl commands and netlink attributes are
> removed.
>
> Patch #1 defines l2tp_tunnel_dst_mtu() in order to simplify the
> following patches. Then patches #2 and #3 remove MTU handling from PPP
> and Ethernet pseudo-wires respectively.
Series applied, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread