| Message ID | 20200923044059.3442423-1-zenczykowski@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net/ipv4: always honour route mtu during forwarding | expand |
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.
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.
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
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); }