[PATCH] ipv4: Remove output route check in ipv4_mtu

[PATCH] ipv4: Remove output route check in ipv4_mtu

[Steffen Klassert]

The output route check was introduced with git commit 261663b0
(ipv4: Don't use the cached pmtu informations for input routes)
during times when we cached the pmtu informations on the
inetpeer. Now the pmtu informations are back in the routes,
so this check is obsolete. It also had some unwanted side effects,
as reported by Timo Teras and Lukas Tribus.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv4/route.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 844a9ef..6e4a89c 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1120,7 +1120,7 @@ static unsigned int ipv4_mtu(const struct dst_entry *dst)
 	if (!mtu || time_after_eq(jiffies, rt->dst.expires))
 		mtu = dst_metric_raw(dst, RTAX_MTU);
 
-	if (mtu && rt_is_output_route(rt))
+	if (mtu)
 		return mtu;
 
 	mtu = dst->dev->mtu;
-- 
1.7.9.5

Re: [PATCH] ipv4: Remove output route check in ipv4_mtu

[Julian Anastasov]

	Hello,

On Wed, 16 Jan 2013, Steffen Klassert wrote:

> The output route check was introduced with git commit 261663b0
> (ipv4: Don't use the cached pmtu informations for input routes)
> during times when we cached the pmtu informations on the
> inetpeer. Now the pmtu informations are back in the routes,
> so this check is obsolete. It also had some unwanted side effects,
> as reported by Timo Teras and Lukas Tribus.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  net/ipv4/route.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 844a9ef..6e4a89c 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1120,7 +1120,7 @@ static unsigned int ipv4_mtu(const struct dst_entry *dst)
>  	if (!mtu || time_after_eq(jiffies, rt->dst.expires))
>  		mtu = dst_metric_raw(dst, RTAX_MTU);
>  
> -	if (mtu && rt_is_output_route(rt))
> +	if (mtu)
>  		return mtu;

	This fix looks good to me. But I see that we
have another problem here. doc/ip-cref.tex in iproute2
claims that a locked MTU value has priority and
PMTU should not be considered.

	IIRC, rt_pmtu is valid only for output routes but
we do not check the lock flag here. What about such
variant:

static unsigned int ipv4_mtu(const struct dst_entry *dst)
{
	const struct rtable *rt = (const struct rtable *) dst;
	unsigned int mtu = rt->rt_pmtu;

	if (unlikely(dst_metric_locked(dst, RTAX_MTU))) {
		mtu = dst_metric_raw(dst, RTAX_MTU);
		if (!mtu) {
			mtu = dst->dev->mtu;
			if (rt->rt_uses_gateway && mtu > 576)
				mtu = 576;
		}
	} else if (!mtu || time_after_eq(jiffies, rt->dst.expires)) {
		mtu = dst_metric_raw(dst, RTAX_MTU);
		if (!mtu)
			mtu = dst->dev->mtu;
	}
	if (mtu > IP_MAX_MTU)
		mtu = IP_MAX_MTU;

	return mtu;
}

	I.e. order becomes:

mtu lock non-zero => fixed fib_mtu
mtu lock 0 => device MTU, up to 576 if via GW, ignore PMTU
PMTU (output routes)
mtu non-zero => fib_mtu
device MTU

	Also, it seems __ip_rt_update_pmtu should start
with such dst_metric_locked(dst, RTAX_MTU) check?

>  	mtu = dst->dev->mtu;
> -- 
> 1.7.9.5

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipv4: Remove output route check in ipv4_mtu

[Steffen Klassert]

On Wed, Jan 16, 2013 at 10:58:13AM +0200, Julian Anastasov wrote:
> 
> 	This fix looks good to me. But I see that we
> have another problem here. doc/ip-cref.tex in iproute2
> claims that a locked MTU value has priority and
> PMTU should not be considered.
> 
> 	IIRC, rt_pmtu is valid only for output routes but
> we do not check the lock flag here. What about such
> variant:
> 
> static unsigned int ipv4_mtu(const struct dst_entry *dst)
> {
> 	const struct rtable *rt = (const struct rtable *) dst;
> 	unsigned int mtu = rt->rt_pmtu;
> 
> 	if (unlikely(dst_metric_locked(dst, RTAX_MTU))) {
> 		mtu = dst_metric_raw(dst, RTAX_MTU);
> 		if (!mtu) {
> 			mtu = dst->dev->mtu;
> 			if (rt->rt_uses_gateway && mtu > 576)
> 				mtu = 576;
> 		}
> 	} else if (!mtu || time_after_eq(jiffies, rt->dst.expires)) {
> 		mtu = dst_metric_raw(dst, RTAX_MTU);
> 		if (!mtu)
> 			mtu = dst->dev->mtu;
> 	}
> 	if (mtu > IP_MAX_MTU)
> 		mtu = IP_MAX_MTU;
> 
> 	return mtu;
> }
> 
> 	I.e. order becomes:
> 
> mtu lock non-zero => fixed fib_mtu
> mtu lock 0 => device MTU, up to 576 if via GW, ignore PMTU
> PMTU (output routes)
> mtu non-zero => fib_mtu
> device MTU

If this is the desired order, I'm fine with the above variant.

> 
> 	Also, it seems __ip_rt_update_pmtu should start
> with such dst_metric_locked(dst, RTAX_MTU) check?
> 

Not absolutely sure what you want to do if the mtu is locked.
But if you don't want to genetate a nh exception and don't update
rt->rt_pmtu in this case, rt->rt_pmtu is never set on routes with
locked mtu. We probably would not need to change ipv4_mtu() to the
above variant.

Re: [PATCH] ipv4: Remove output route check in ipv4_mtu

[Julian Anastasov]

	Hello,

On Wed, 16 Jan 2013, Steffen Klassert wrote:

> On Wed, Jan 16, 2013 at 10:58:13AM +0200, Julian Anastasov wrote:
> > 
> > 	This fix looks good to me. But I see that we
> > have another problem here. doc/ip-cref.tex in iproute2
> > claims that a locked MTU value has priority and
> > PMTU should not be considered.
> > 
> > 	IIRC, rt_pmtu is valid only for output routes but
> > we do not check the lock flag here. What about such
> > variant:
> > 
> > static unsigned int ipv4_mtu(const struct dst_entry *dst)
> > {
> > 	const struct rtable *rt = (const struct rtable *) dst;
> > 	unsigned int mtu = rt->rt_pmtu;
> > 
> > 	if (unlikely(dst_metric_locked(dst, RTAX_MTU))) {
> > 		mtu = dst_metric_raw(dst, RTAX_MTU);
> > 		if (!mtu) {
> > 			mtu = dst->dev->mtu;
> > 			if (rt->rt_uses_gateway && mtu > 576)
> > 				mtu = 576;
> > 		}
> > 	} else if (!mtu || time_after_eq(jiffies, rt->dst.expires)) {
> > 		mtu = dst_metric_raw(dst, RTAX_MTU);
> > 		if (!mtu)
> > 			mtu = dst->dev->mtu;
> > 	}
> > 	if (mtu > IP_MAX_MTU)
> > 		mtu = IP_MAX_MTU;
> > 
> > 	return mtu;
> > }
> > 
> > 	I.e. order becomes:
> > 
> > mtu lock non-zero => fixed fib_mtu
> > mtu lock 0 => device MTU, up to 576 if via GW, ignore PMTU
> > PMTU (output routes)
> > mtu non-zero => fib_mtu
> > device MTU
> 
> If this is the desired order, I'm fine with the above variant.
> 
> > 
> > 	Also, it seems __ip_rt_update_pmtu should start
> > with such dst_metric_locked(dst, RTAX_MTU) check?
> > 
> 
> Not absolutely sure what you want to do if the mtu is locked.
> But if you don't want to genetate a nh exception and don't update
> rt->rt_pmtu in this case, rt->rt_pmtu is never set on routes with
> locked mtu. We probably would not need to change ipv4_mtu() to the
> above variant.

	Yes, we have to exit __ip_rt_update_pmtu if
fib_mtu is locked. Then rt_pmtu should stay 0 and
your variant of ipv4_mtu should be ok. I hope you
can simply extend your patch with such additional
check in __ip_rt_update_pmtu:

	if (dst_metric_locked(dst, RTAX_MTU))
		return;

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipv4: Remove output route check in ipv4_mtu

[Steffen Klassert]

On Wed, Jan 16, 2013 at 11:01:39PM +0200, Julian Anastasov wrote:
> 
> On Wed, 16 Jan 2013, Steffen Klassert wrote:
> > 
> > Not absolutely sure what you want to do if the mtu is locked.
> > But if you don't want to genetate a nh exception and don't update
> > rt->rt_pmtu in this case, rt->rt_pmtu is never set on routes with
> > locked mtu. We probably would not need to change ipv4_mtu() to the
> > above variant.
> 
> 	Yes, we have to exit __ip_rt_update_pmtu if
> fib_mtu is locked. Then rt_pmtu should stay 0 and
> your variant of ipv4_mtu should be ok. I hope you
> can simply extend your patch with such additional
> check in __ip_rt_update_pmtu:
> 
> 	if (dst_metric_locked(dst, RTAX_MTU))
> 		return;
> 

That's what I had in mind. I'll do that with an additional
patch, it's a another issue.

Thanks for the input!