diff mbox series

[v3] net/ipv4: always honour route mtu during forwarding

Message ID 20200923201815.388347-1-zenczykowski@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series [v3] net/ipv4: always honour route mtu during forwarding | expand

Commit Message

Maciej Żenczykowski Sept. 23, 2020, 8:18 p.m. UTC 
From: Maciej Żenczykowski <maze@google.com>

Documentation/networking/ip-sysctl.txt:46 says:
  ip_forward_use_pmtu - BOOLEAN
    By default we don't trust protocol path MTUs while forwarding
    because they could be easily forged and can lead to unwanted
    fragmentation by the router.
    You only need to enable this if you have user-space software
    which tries to discover path mtus by itself and depends on the
    kernel honoring this information. This is normally not the case.
    Default: 0 (disabled)
    Possible values:
    0 - disabled
    1 - enabled

Which makes it pretty clear that setting it to 1 is a potential
security/safety/DoS issue, and yet it is entirely reasonable to want
forwarded traffic to honour explicitly administrator configured
route mtus (instead of defaulting to device mtu).

Indeed, I can't think of a single reason why you wouldn't want to.
Since you configured a route mtu you probably know better...

It is pretty common to have a higher device mtu to allow receiving
large (jumbo) frames, while having some routes via that interface
(potentially including the default route to the internet) specify
a lower mtu.

Note that ipv6 forwarding uses device mtu unless the route is locked
(in which case it will use the route mtu).

This approach is not usable for IPv4 where an 'mtu lock' on a route
also has the side effect of disabling TCP path mtu discovery via
disabling the IPv4 DF (don't frag) bit on all outgoing frames.

I'm not aware of a way to lock a route from an IPv6 RA, so that also
potentially seems wrong.

Signed-off-by: Maciej Żenczykowski <maze@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Lorenzo Colitti <lorenzo@google.com>
Cc: Sunmeet Gill (Sunny) <sgill@quicinc.com>
Cc: Vinay Paradkar <vparadka@qti.qualcomm.com>
Cc: Tyler Wear <twear@quicinc.com>
Cc: David Ahern <dsahern@kernel.org>
---
 include/net/ip.h | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Eric Dumazet Sept. 24, 2020, 9:14 a.m. UTC | #1 
On Wed, Sep 23, 2020 at 10:18 PM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
>
> From: Maciej Żenczykowski <maze@google.com>
>
> Documentation/networking/ip-sysctl.txt:46 says:
>   ip_forward_use_pmtu - BOOLEAN
>     By default we don't trust protocol path MTUs while forwarding
>     because they could be easily forged and can lead to unwanted
>     fragmentation by the router.
>     You only need to enable this if you have user-space software
>     which tries to discover path mtus by itself and depends on the
>     kernel honoring this information. This is normally not the case.
>     Default: 0 (disabled)
>     Possible values:
>     0 - disabled
>     1 - enabled
>
>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>
David Miller Sept. 25, 2020, 2:54 a.m. UTC | #2 
From: Maciej Żenczykowski <zenczykowski@gmail.com>
Date: Wed, 23 Sep 2020 13:18:15 -0700

> From: Maciej Żenczykowski <maze@google.com>
> 
> Documentation/networking/ip-sysctl.txt:46 says:
>   ip_forward_use_pmtu - BOOLEAN
>     By default we don't trust protocol path MTUs while forwarding
>     because they could be easily forged and can lead to unwanted
>     fragmentation by the router.
>     You only need to enable this if you have user-space software
>     which tries to discover path mtus by itself and depends on the
>     kernel honoring this information. This is normally not the case.
>     Default: 0 (disabled)
>     Possible values:
>     0 - disabled
>     1 - enabled
> 
> Which makes it pretty clear that setting it to 1 is a potential
> security/safety/DoS issue, and yet it is entirely reasonable to want
> forwarded traffic to honour explicitly administrator configured
> route mtus (instead of defaulting to device mtu).
> 
> Indeed, I can't think of a single reason why you wouldn't want to.
> Since you configured a route mtu you probably know better...
> 
> It is pretty common to have a higher device mtu to allow receiving
> large (jumbo) frames, while having some routes via that interface
> (potentially including the default route to the internet) specify
> a lower mtu.
> 
> Note that ipv6 forwarding uses device mtu unless the route is locked
> (in which case it will use the route mtu).
> 
> This approach is not usable for IPv4 where an 'mtu lock' on a route
> also has the side effect of disabling TCP path mtu discovery via
> disabling the IPv4 DF (don't frag) bit on all outgoing frames.
> 
> I'm not aware of a way to lock a route from an IPv6 RA, so that also
> potentially seems wrong.
> 
> Signed-off-by: Maciej Żenczykowski <maze@google.com>

Applied and queued up for -stable, thank you.
diff mbox series

Patch

diff --git a/include/net/ip.h b/include/net/ip.h
index b09c48d862cc..2a52787db64a 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -436,12 +436,18 @@  static inline unsigned int ip_dst_mtu_maybe_forward(const struct dst_entry *dst,
 						    bool forwarding)
 {
 	struct net *net = dev_net(dst->dev);
+	unsigned int mtu;
 
 	if (net->ipv4.sysctl_ip_fwd_use_pmtu ||
 	    ip_mtu_locked(dst) ||
 	    !forwarding)
 		return dst_mtu(dst);
 
+	/* 'forwarding = true' case should always honour route mtu */
+	mtu = dst_metric_raw(dst, RTAX_MTU);
+	if (mtu)
+		return mtu;
+
 	return min(READ_ONCE(dst->dev->mtu), IP_MAX_MTU);
 }