Re: [PATCH net v2 RESEND] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[David Miller]

From: Stefano Brivio 
Date: Tue,  6 Mar 2018 11:10:19 +0100

> Currently, administrative MTU changes on a given netdevice are
> not reflected on route exceptions for MTU-less routes, with a
> set PMTU value, for that device:
> 
>  # ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a proto kernel src 2001:db8::a metric 256 pref 
> medium
>  # ping6 -c 1 -q -s1 2001:db8::b > /dev/null
>  # ip netns exec a ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
>  cache expires 571sec mtu 4926 pref medium
>  # ip link set dev vti_a mtu 3000
>  # ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
>  cache expires 571sec mtu 4926 pref medium
>  # ip link set dev vti_a mtu 9000
>  # ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
>  cache expires 571sec mtu 4926 pref medium
> 
> The first issue is that since commit fb56be83e43d ("net-ipv6: on
> device mtu change do not add mtu to mtu-less routes") we don't
> call rt6_exceptions_update_pmtu() from rt6_mtu_change_route(),
> which handles administrative MTU changes, if the regular route
> is MTU-less.
> 
> However, PMTU exceptions should be always updated, as long as
> RTAX_MTU is not locked. Keep the check for MTU-less main route,
> as introduced by that commit, but, for exceptions,
> call rt6_exceptions_update_pmtu() regardless of that check.
> 
> Once that is fixed, one problem remains: MTU changes are not
> reflected if the new MTU is higher than the previous one,
> because rt6_exceptions_update_pmtu() doesn't allow that. We
> should instead allow PMTU increase if the old PMTU matches the
> local MTU, as that implies that the old MTU was the lowest in the
> path, and PMTU discovery might lead to different results.
> 
> The existing check in rt6_mtu_change_route() correctly took that
> case into account (for regular routes only), so factor it out
> and re-use it also in rt6_exceptions_update_pmtu().
> 
> While at it, fix comments style and grammar, and try to be a bit
> more descriptive.
> 
> Reported-by: Xiumei Mu 
> Fixes: fb56be83e43d ("net-ipv6: on device mtu change do not add mtu to 
> mtu-less routes")
> Fixes: f5bbe7ee79c2 ("ipv6: prepare rt6_mtu_change() for exception table")
> Signed-off-by: Stefano Brivio 

Applied and queued up for -stable, thanks.

Re: [PATCH net v2 RESEND] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[David Ahern]

On 3/6/18 3:10 AM, Stefano Brivio wrote:
> Currently, administrative MTU changes on a given netdevice are
> not reflected on route exceptions for MTU-less routes, with a
> set PMTU value, for that device:
> 
>  # ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a proto kernel src 2001:db8::a metric 256 pref 
> medium
>  # ping6 -c 1 -q -s1 2001:db8::b > /dev/null
>  # ip netns exec a ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
>  cache expires 571sec mtu 4926 pref medium
>  # ip link set dev vti_a mtu 3000
>  # ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
>  cache expires 571sec mtu 4926 pref medium
>  # ip link set dev vti_a mtu 9000
>  # ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
>  cache expires 571sec mtu 4926 pref medium
> 
> The first issue is that since commit fb56be83e43d ("net-ipv6: on
> device mtu change do not add mtu to mtu-less routes") we don't
> call rt6_exceptions_update_pmtu() from rt6_mtu_change_route(),
> which handles administrative MTU changes, if the regular route
> is MTU-less.
> 
> However, PMTU exceptions should be always updated, as long as
> RTAX_MTU is not locked. Keep the check for MTU-less main route,
> as introduced by that commit, but, for exceptions,
> call rt6_exceptions_update_pmtu() regardless of that check.
> 
> Once that is fixed, one problem remains: MTU changes are not
> reflected if the new MTU is higher than the previous one,
> because rt6_exceptions_update_pmtu() doesn't allow that. We
> should instead allow PMTU increase if the old PMTU matches the
> local MTU, as that implies that the old MTU was the lowest in the
> path, and PMTU discovery might lead to different results.
> 
> The existing check in rt6_mtu_change_route() correctly took that
> case into account (for regular routes only), so factor it out
> and re-use it also in rt6_exceptions_update_pmtu().
> 
> While at it, fix comments style and grammar, and try to be a bit
> more descriptive.
> 
> Reported-by: Xiumei Mu 
> Fixes: fb56be83e43d ("net-ipv6: on device mtu change do not add mtu to 
> mtu-less routes")
> Fixes: f5bbe7ee79c2 ("ipv6: prepare rt6_mtu_change() for exception table")
> Signed-off-by: Stefano Brivio 
> ---

Acked-by: David Ahern 

Re: [PATCH net v2 RESEND] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[Stefano Brivio]

On Tue, 6 Mar 2018 13:30:05 -0700
David Ahern  wrote:

> On 3/6/18 3:10 AM, Stefano Brivio wrote:
> > Currently, administrative MTU changes on a given netdevice are
> > not reflected on route exceptions for MTU-less routes, with a
> > set PMTU value, for that device:
> > 
> >  # ip -6 route get 2001:db8::b
> >  2001:db8::b from :: dev vti_a proto kernel src 2001:db8::a metric 256 pref 
> > medium
> >  # ping6 -c 1 -q -s1 2001:db8::b > /dev/null
> >  # ip netns exec a ip -6 route get 2001:db8::b
> >  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
> >  cache expires 571sec mtu 4926 pref medium
> >  # ip link set dev vti_a mtu 3000
> >  # ip -6 route get 2001:db8::b
> >  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
> >  cache expires 571sec mtu 4926 pref medium
> >  # ip link set dev vti_a mtu 9000
> >  # ip -6 route get 2001:db8::b
> >  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
> >  cache expires 571sec mtu 4926 pref medium  
> 
> Using your test script, I never see the route get an updated MTU -- it
> is always 1426.
> 
> ++ exception='fd00:2::b from :: dev vti_a src fd00:2::a metric 0 expires
> 598sec mtu 1426 pref medium'

Thanks for reporting this.

There's another issue in the test script: the initial PMTU of the
exception depends on the veth MTU value, which I'm not explicitly
setting. It happened to be 5000 on my host, it's a more reasonable 1500
on yours.

If you have 1426 as initial PMTU, by setting the MTU to 3000 as second
step in the script, I'm clearly not decreasing it.

I'll send a v3 of the test script.

-- 
Stefano

Re: [PATCH net v2 RESEND] ipv6: Reflect MTU changes on PMTU of exceptions for MTU-less routes

[David Ahern]

On 3/6/18 3:10 AM, Stefano Brivio wrote:
> Currently, administrative MTU changes on a given netdevice are
> not reflected on route exceptions for MTU-less routes, with a
> set PMTU value, for that device:
> 
>  # ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a proto kernel src 2001:db8::a metric 256 pref 
> medium
>  # ping6 -c 1 -q -s1 2001:db8::b > /dev/null
>  # ip netns exec a ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
>  cache expires 571sec mtu 4926 pref medium
>  # ip link set dev vti_a mtu 3000
>  # ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
>  cache expires 571sec mtu 4926 pref medium
>  # ip link set dev vti_a mtu 9000
>  # ip -6 route get 2001:db8::b
>  2001:db8::b from :: dev vti_a src 2001:db8::a metric 0
>  cache expires 571sec mtu 4926 pref medium

Using your test script, I never see the route get an updated MTU -- it
is always 1426.

++ exception='fd00:2::b from :: dev vti_a src fd00:2::a metric 0 expires
598sec mtu 1426 pref medium'