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

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

[Maciej Żenczykowski]

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 <maze@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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/net/ip.h b/include/net/ip.h
index b09c48d862cc..1262011d00b8 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -442,6 +442,10 @@ static inline unsigned int ip_dst_mtu_maybe_forward(const struct dst_entry *dst,
 	    !forwarding)
 		return dst_mtu(dst);
 
+	/* 'forwarding = true' case should always honour route mtu */
+	unsigned int mtu = dst_metric_raw(dst, RTAX_MTU);
+	if (mtu) return mtu;
+
 	return min(READ_ONCE(dst->dev->mtu), IP_MAX_MTU);
 }
 
-- 
2.28.0.681.g6f77f65b4e-goog

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

[Maciej Żenczykowski]

On Tue, Sep 22, 2020 at 9:41 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
>
> 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 <maze@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 | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/net/ip.h b/include/net/ip.h
> index b09c48d862cc..1262011d00b8 100644
> --- a/include/net/ip.h
> +++ b/include/net/ip.h
> @@ -442,6 +442,10 @@ static inline unsigned int ip_dst_mtu_maybe_forward(const struct dst_entry *dst,
>             !forwarding)
>                 return dst_mtu(dst);
>
> +       /* 'forwarding = true' case should always honour route mtu */
> +       unsigned int mtu = dst_metric_raw(dst, RTAX_MTU);
> +       if (mtu) return mtu;
> +
>         return min(READ_ONCE(dst->dev->mtu), IP_MAX_MTU);
>  }
>
> --
> 2.28.0.681.g6f77f65b4e-goog

Eh, what I get for last minute removal of 'if (forwarding) {}' wrapper.

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

[Maciej Żenczykowski]

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 <maze@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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/net/ip.h b/include/net/ip.h
index b09c48d862cc..c2188bebbc54 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -436,12 +436,17 @@ 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);
 }
 
-- 
2.28.0.681.g6f77f65b4e-goog

Re: [PATCH v2] net/ipv4: always honour route mtu during forwarding

[Eric Dumazet]

On 9/23/20 6:51 AM, Maciej Żenczykowski 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
> 
> 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 <maze@google.com>

Note that my email address is more like : <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 | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/net/ip.h b/include/net/ip.h
> index b09c48d862cc..c2188bebbc54 100644
> --- a/include/net/ip.h
> +++ b/include/net/ip.h
> @@ -436,12 +436,17 @@ 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;


        if (mtu)
                return mtu;

Apparently route mtu are capped to 65520, not sure where it is done exactly IP_MAX_MTU being 65535)

# ip ro add 1.1.1.4 dev wlp2s0 mtu 100000
# ip ro get 1.1.1.4
1.1.1.4 dev wlp2s0 src 192.168.8.147 uid 0 
    cache mtu 65520 




> +
>  	return min(READ_ONCE(dst->dev->mtu), IP_MAX_MTU);
>  }
>  
> 

Re: [PATCH v2] net/ipv4: always honour route mtu during forwarding

[David Ahern]

On 9/23/20 2:46 AM, Eric Dumazet wrote:
>> diff --git a/include/net/ip.h b/include/net/ip.h
>> index b09c48d862cc..c2188bebbc54 100644
>> --- a/include/net/ip.h
>> +++ b/include/net/ip.h
>> @@ -436,12 +436,17 @@ 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;
> 
> 
>         if (mtu)
>                 return mtu;
> 
> Apparently route mtu are capped to 65520, not sure where it is done exactly IP_MAX_MTU being 65535)

ip_metrics_convert seems to be the place"
                if (type == RTAX_MTU && val > 65535 - 15)
                        val = 65535 - 15;

going back through the code moves, and it was added by Dave with
710ab6c03122c

> 
> # ip ro add 1.1.1.4 dev wlp2s0 mtu 100000
> # ip ro get 1.1.1.4
> 1.1.1.4 dev wlp2s0 src 192.168.8.147 uid 0 
>     cache mtu 65520 
> 
> 
> 
> 
>> +
>>  	return min(READ_ONCE(dst->dev->mtu), IP_MAX_MTU);
>>  }
>>  
>>

Re: [PATCH v2] net/ipv4: always honour route mtu during forwarding

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Wed, 23 Sep 2020 08:36:50 -0600

> On 9/23/20 2:46 AM, Eric Dumazet wrote:
>> Apparently route mtu are capped to 65520, not sure where it is done exactly IP_MAX_MTU being 65535)
> 
> ip_metrics_convert seems to be the place"
>                 if (type == RTAX_MTU && val > 65535 - 15)
>                         val = 65535 - 15;
> 
> going back through the code moves, and it was added by Dave with
> 710ab6c03122c

At the time of that commit IP_MAX_MTU only existed in net/ipv4/route.c
and it's value was 0xFFF0.

So I didn't add anything :-)  Just moving stuff around.

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

[Maciej Żenczykowski]

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(+)

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);
 }
 
-- 
2.28.0.681.g6f77f65b4e-goog

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

[Maciej Żenczykowski]

On Wed, Sep 23, 2020 at 2:34 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi "Maciej,
>
> Thank you for the patch! Perhaps something to improve:

This patchset was already superseded precisely because of this issue.

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2589 bytes --]

Hi "Maciej,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on net/master linus/master sparc-next/master next-20200923]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Maciej-enczykowski/net-ipv4-always-honour-route-mtu-during-forwarding/20200923-124249
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 748d1c8a425ec529d541f082ee7a81f6a51fa120
config: parisc-randconfig-c003-20200923 (attached as .config)
compiler: hppa64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/net/busy_poll.h:18,
                    from fs/select.c:32:
   include/net/ip.h: In function 'ip_dst_mtu_maybe_forward':
>> include/net/ip.h:446:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
     446 |  unsigned int mtu = dst_metric_raw(dst, RTAX_MTU);
         |  ^~~~~~~~

# https://github.com/0day-ci/linux/commit/d9552d77468fe90224e07225cfc8c3f838b28b1b
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Maciej-enczykowski/net-ipv4-always-honour-route-mtu-during-forwarding/20200923-124249
git checkout d9552d77468fe90224e07225cfc8c3f838b28b1b
vim +446 include/net/ip.h

   434	
   435	static inline unsigned int ip_dst_mtu_maybe_forward(const struct dst_entry *dst,
   436							    bool forwarding)
   437	{
   438		struct net *net = dev_net(dst->dev);
   439	
   440		if (net->ipv4.sysctl_ip_fwd_use_pmtu ||
   441		    ip_mtu_locked(dst) ||
   442		    !forwarding)
   443			return dst_mtu(dst);
   444	
   445		/* 'forwarding = true' case should always honour route mtu */
 > 446		unsigned int mtu = dst_metric_raw(dst, RTAX_MTU);
   447		if (mtu) return mtu;
   448	
   449		return min(READ_ONCE(dst->dev->mtu), IP_MAX_MTU);
   450	}
   451	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24597 bytes --]

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

[Eric Dumazet]

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>

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

[David Miller]

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.