[PATCH] route:ip_rt_frag_needed always return unzero

[PATCH] route:ip_rt_frag_needed always return unzero

[Gao feng]

int function ip_rt_frag_need,if peer is null,
there is no need to do ipprot->err_handler.
I am right?

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 net/ipv4/route.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 075212e..6cde0fa 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1574,7 +1574,7 @@ unsigned short ip_rt_frag_needed(struct net *net, const struct iphdr *iph,
 
 		atomic_inc(&__rt_peer_genid);
 	}
-	return est_mtu ? : new_mtu;
+	return est_mtu;
 }
 
 static void check_peer_pmtu(struct dst_entry *dst, struct inet_peer *peer)
-- 
1.7.1

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Eric Dumazet]

Le mardi 18 octobre 2011 à 15:04 +0800, Gao feng a écrit :
> int function ip_rt_frag_need,if peer is null,
> there is no need to do ipprot->err_handler.
> I am right?
> 
> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> ---
>  net/ipv4/route.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 075212e..6cde0fa 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1574,7 +1574,7 @@ unsigned short ip_rt_frag_needed(struct net *net, const struct iphdr *iph,
>  
>  		atomic_inc(&__rt_peer_genid);
>  	}
> -	return est_mtu ? : new_mtu;
> +	return est_mtu;
>  }
>  
>  static void check_peer_pmtu(struct dst_entry *dst, struct inet_peer *peer)

No idea why you want this, your changelog is a bit cryptic :)

Wont this bypass the raw_icmp_error(skb, protocol, info);
call in icmp_unreach() as well ?

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Gao feng]

2011.10.18 17:23, Eric Dumazet wrote:
> Le mardi 18 octobre 2011 à 15:04 +0800, Gao feng a écrit :
>> int function ip_rt_frag_need,if peer is null,
>> there is no need to do ipprot->err_handler.
>> I am right?
>>
>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>> ---
>>  net/ipv4/route.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>> index 075212e..6cde0fa 100644
>> --- a/net/ipv4/route.c
>> +++ b/net/ipv4/route.c
>> @@ -1574,7 +1574,7 @@ unsigned short ip_rt_frag_needed(struct net *net, const struct iphdr *iph,
>>  
>>  		atomic_inc(&__rt_peer_genid);
>>  	}
>> -	return est_mtu ? : new_mtu;
>> +	return est_mtu;
>>  }
>>  
>>  static void check_peer_pmtu(struct dst_entry *dst, struct inet_peer *peer)
> 
> No idea why you want this, your changelog is a bit cryptic :)
> 
> Wont this bypass the raw_icmp_error(skb, protocol, info);
> call in icmp_unreach() as well ?
> 
> 

thanks Eric!

I mean that the pmtu is update by inet_peer->pmtu_learned as I know.
so in function ip_rt_frag_needed,
if inet_peer is null or someting else make the setting of inet_peer->pmtu_learned failed.
there is no need to call function tcp_v4_err.

the call stack is
icmp_unreach
  |
  |--->ip_rt_frag_needed(fill inet_peer)
  |
  |--->raw_icmp_error()
  |
  |--->ipprot->err_handler(tcp_v4_err or something else)
	|
	|--->tcp_v4_err(frag need icmp is triggered by tcp packet)
		|
		|--->do_pmtu_discovery
		(in this function both __sk_dst_check or dst->ops->update_pmtu
		need struct inet_peer to update pmtu)

so,I think when set inet_peer->pmtu_learned failed,
in func icmp_unreach we should goto out immediately.

And it's confuse me that why func ping_err and udp_err not update the pmtu?
What I miss?

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Gao feng]

于 2011年10月19日 09:34, Gao feng 写道:
> 2011.10.18 17:23, Eric Dumazet wrote:
>> Le mardi 18 octobre 2011 à 15:04 +0800, Gao feng a écrit :
>>> int function ip_rt_frag_need,if peer is null,
>>> there is no need to do ipprot->err_handler.
>>> I am right?
>>>
>>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>>> ---
>>>  net/ipv4/route.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>>> index 075212e..6cde0fa 100644
>>> --- a/net/ipv4/route.c
>>> +++ b/net/ipv4/route.c
>>> @@ -1574,7 +1574,7 @@ unsigned short ip_rt_frag_needed(struct net *net, const struct iphdr *iph,
>>>  
>>>  		atomic_inc(&__rt_peer_genid);
>>>  	}
>>> -	return est_mtu ? : new_mtu;
>>> +	return est_mtu;
>>>  }
>>>  
>>>  static void check_peer_pmtu(struct dst_entry *dst, struct inet_peer *peer)
>>
>> No idea why you want this, your changelog is a bit cryptic :)
>>
>> Wont this bypass the raw_icmp_error(skb, protocol, info);
>> call in icmp_unreach() as well ?
>>
>>
> 
> thanks Eric!
> 
> I mean that the pmtu is update by inet_peer->pmtu_learned as I know.
> so in function ip_rt_frag_needed,
> if inet_peer is null or someting else make the setting of inet_peer->pmtu_learned failed.
> there is no need to call function tcp_v4_err.
> 
> the call stack is
> icmp_unreach
>   |
>   |--->ip_rt_frag_needed(fill inet_peer)
>   |
>   |--->raw_icmp_error()
>   |
>   |--->ipprot->err_handler(tcp_v4_err or something else)
> 	|
> 	|--->tcp_v4_err(frag need icmp is triggered by tcp packet)
> 		|
> 		|--->do_pmtu_discovery
> 		(in this function both __sk_dst_check or dst->ops->update_pmtu
> 		need struct inet_peer to update pmtu)
> 
> so,I think when set inet_peer->pmtu_learned failed,
> in func icmp_unreach we should goto out immediately.
> 
> And it's confuse me that why func ping_err and udp_err not update the pmtu?
> What I miss?
> --

And move atomic_inc(&__rt_peer_genid) just like func ip_rt_update_pmtu?

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 6cde0fa..3e1aa5c 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1568,11 +1568,12 @@ unsigned short ip_rt_frag_needed(struct net *net, const
                        est_mtu = mtu;
                        peer->pmtu_learned = mtu;
                        peer->pmtu_expires = pmtu_expires;
+
+                       atomic_inc(&__rt_peer_genid);
                }

                inet_putpeer(peer);

-               atomic_inc(&__rt_peer_genid);
        }
        return est_mtu;
 }

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Eric Dumazet]

Le mercredi 19 octobre 2011 à 09:34 +0800, Gao feng a écrit :
> 2011.10.18 17:23, Eric Dumazet wrote:
> > Le mardi 18 octobre 2011 à 15:04 +0800, Gao feng a écrit :
> >> int function ip_rt_frag_need,if peer is null,
> >> there is no need to do ipprot->err_handler.
> >> I am right?
> >>
> >> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> >> ---
> >>  net/ipv4/route.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> >> index 075212e..6cde0fa 100644
> >> --- a/net/ipv4/route.c
> >> +++ b/net/ipv4/route.c
> >> @@ -1574,7 +1574,7 @@ unsigned short ip_rt_frag_needed(struct net *net, const struct iphdr *iph,
> >>  
> >>  		atomic_inc(&__rt_peer_genid);
> >>  	}
> >> -	return est_mtu ? : new_mtu;
> >> +	return est_mtu;
> >>  }
> >>  
> >>  static void check_peer_pmtu(struct dst_entry *dst, struct inet_peer *peer)
> > 
> > No idea why you want this, your changelog is a bit cryptic :)
> > 
> > Wont this bypass the raw_icmp_error(skb, protocol, info);
> > call in icmp_unreach() as well ?
> > 
> > 
> 
> thanks Eric!
> 
> I mean that the pmtu is update by inet_peer->pmtu_learned as I know.
> so in function ip_rt_frag_needed,
> if inet_peer is null or someting else make the setting of inet_peer->pmtu_learned failed.
> there is no need to call function tcp_v4_err.
> 
> the call stack is
> icmp_unreach
>   |
>   |--->ip_rt_frag_needed(fill inet_peer)
>   |
>   |--->raw_icmp_error()
>   |
>   |--->ipprot->err_handler(tcp_v4_err or something else)
> 	|
> 	|--->tcp_v4_err(frag need icmp is triggered by tcp packet)
> 		|
> 		|--->do_pmtu_discovery
> 		(in this function both __sk_dst_check or dst->ops->update_pmtu
> 		need struct inet_peer to update pmtu)
> 
> so,I think when set inet_peer->pmtu_learned failed,
> in func icmp_unreach we should goto out immediately.
> 
> And it's confuse me that why func ping_err and udp_err not update the pmtu?
> What I miss?

You dont answer my question : After your patch, we now dont call
raw_icmp_error() anymore. Why is is valid ?

Not finding/create inet_peer is very unlikely : This occurs only under
high stress and out of memory condition. Is it really happening on your
machines ?

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Eric Dumazet]

Le mercredi 19 octobre 2011 à 10:33 +0800, Gao feng a écrit :

> And move atomic_inc(&__rt_peer_genid) just like func ip_rt_update_pmtu?
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 6cde0fa..3e1aa5c 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1568,11 +1568,12 @@ unsigned short ip_rt_frag_needed(struct net *net, const
>                         est_mtu = mtu;
>                         peer->pmtu_learned = mtu;
>                         peer->pmtu_expires = pmtu_expires;
> +
> +                       atomic_inc(&__rt_peer_genid);
>                 }
> 
>                 inet_putpeer(peer);
> 
> -               atomic_inc(&__rt_peer_genid);
>         }
>         return est_mtu;
>  }
> 

This one is fine, please provide a changelog and official submission.

Thanks

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Gao feng]

于 2011年10月19日 11:49, Eric Dumazet 写道:
> Le mercredi 19 octobre 2011 à 09:34 +0800, Gao feng a écrit :
>> 2011.10.18 17:23, Eric Dumazet wrote:
>>> Le mardi 18 octobre 2011 à 15:04 +0800, Gao feng a écrit :
>>>> int function ip_rt_frag_need,if peer is null,
>>>> there is no need to do ipprot->err_handler.
>>>> I am right?
>>>>
>>>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>>>> ---
>>>>  net/ipv4/route.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>>>> index 075212e..6cde0fa 100644
>>>> --- a/net/ipv4/route.c
>>>> +++ b/net/ipv4/route.c
>>>> @@ -1574,7 +1574,7 @@ unsigned short ip_rt_frag_needed(struct net *net, const struct iphdr *iph,
>>>>  
>>>>  		atomic_inc(&__rt_peer_genid);
>>>>  	}
>>>> -	return est_mtu ? : new_mtu;
>>>> +	return est_mtu;
>>>>  }
>>>>  
>>>>  static void check_peer_pmtu(struct dst_entry *dst, struct inet_peer *peer)
>>>
>>> No idea why you want this, your changelog is a bit cryptic :)
>>>
>>> Wont this bypass the raw_icmp_error(skb, protocol, info);
>>> call in icmp_unreach() as well ?
>>>
>>>
>>
>> thanks Eric!
>>
>> I mean that the pmtu is update by inet_peer->pmtu_learned as I know.
>> so in function ip_rt_frag_needed,
>> if inet_peer is null or someting else make the setting of inet_peer->pmtu_learned failed.
>> there is no need to call function tcp_v4_err.
>>
>> the call stack is
>> icmp_unreach
>>   |
>>   |--->ip_rt_frag_needed(fill inet_peer)
>>   |
>>   |--->raw_icmp_error()
>>   |
>>   |--->ipprot->err_handler(tcp_v4_err or something else)
>> 	|
>> 	|--->tcp_v4_err(frag need icmp is triggered by tcp packet)
>> 		|
>> 		|--->do_pmtu_discovery
>> 		(in this function both __sk_dst_check or dst->ops->update_pmtu
>> 		need struct inet_peer to update pmtu)
>>
>> so,I think when set inet_peer->pmtu_learned failed,
>> in func icmp_unreach we should goto out immediately.
>>
>> And it's confuse me that why func ping_err and udp_err not update the pmtu?
>> What I miss?
> 
> You dont answer my question : After your patch, we now dont call
> raw_icmp_error() anymore. Why is is valid ?

After my patch
raw_icmp_error don't call only when setting inet_peer failed(ip_rt_frag_needed return zero).
And I think it's unexpected,should goto out immediately.

In orig ip_rt_frag_need,
zero can be return only when pmtu(get from icmp packet) is zero and peer is NULL.
in this case,raw_icmp_error will not be call too.this is valid??

The return value of ip_rt_frag_needed is not very clear.
Eric,can you tell me something?

> 
> Not finding/create inet_peer is very unlikely : This occurs only under
> high stress and out of memory condition. Is it really happening on your
> machines ?
> 
> 

It's not happening on my machines.

thanks.

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Eric Dumazet]

Le mercredi 19 octobre 2011 à 13:20 +0800, Gao feng a écrit :

> In orig ip_rt_frag_need,
> zero can be return only when pmtu(get from icmp packet) is zero and peer is NULL.
> in this case,raw_icmp_error will not be call too.this is valid??

I dont know, tell me.

I am afraid you're adding a possible regression here.

pmtu from icmp_packet is not zero in the common case.

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Gao feng]

2011.10.19 13:47, Eric Dumazet wrote:
> Le mercredi 19 octobre 2011 à 13:20 +0800, Gao feng a écrit :
> 
>> In orig ip_rt_frag_need,
>> zero can be return only when pmtu(get from icmp packet) is zero and peer is NULL.
>> in this case,raw_icmp_error will not be call too.this is valid??
> 
> I dont know, tell me.
> 
> I am afraid you're adding a possible regression here.
> 
> pmtu from icmp_packet is not zero in the common case.
> 
> 

AS raw socket is special,I think it's should call raw_icmp_error even the icmp packet is invalid.
if I am right,I think it's another BUG.

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Steffen Klassert]

On Wed, Oct 19, 2011 at 01:20:28PM +0800, Gao feng wrote:
> 于 2011年10月19日 11:49, Eric Dumazet 写道:
> > Le mercredi 19 octobre 2011 à 09:34 +0800, Gao feng a écrit :
> >>
> >> I mean that the pmtu is update by inet_peer->pmtu_learned as I know.
> >> so in function ip_rt_frag_needed,
> >> if inet_peer is null or someting else make the setting of inet_peer->pmtu_learned failed.
> >> there is no need to call function tcp_v4_err.
> >>
> >> the call stack is
> >> icmp_unreach
> >>   |
> >>   |--->ip_rt_frag_needed(fill inet_peer)
> >>   |
> >>   |--->raw_icmp_error()
> >>   |
> >>   |--->ipprot->err_handler(tcp_v4_err or something else)
> >> 	|
> >> 	|--->tcp_v4_err(frag need icmp is triggered by tcp packet)
> >> 		|
> >> 		|--->do_pmtu_discovery
> >> 		(in this function both __sk_dst_check or dst->ops->update_pmtu
> >> 		need struct inet_peer to update pmtu)
> >>
> >> so,I think when set inet_peer->pmtu_learned failed,
> >> in func icmp_unreach we should goto out immediately.
> >>
> >> And it's confuse me that why func ping_err and udp_err not update the pmtu?
> >> What I miss?

On udp and raw sockets, the user is responsible to adjust the packet
size according to the mtu value he may find in the socket's error queue.
So we shoud provide the user with this information, even in the unlikely
case where we could not create an inet_peer.

> > 
> > You dont answer my question : After your patch, we now dont call
> > raw_icmp_error() anymore. Why is is valid ?
> 
> After my patch
> raw_icmp_error don't call only when setting inet_peer failed(ip_rt_frag_needed return zero).
> And I think it's unexpected,should goto out immediately.
> 
> In orig ip_rt_frag_need,
> zero can be return only when pmtu(get from icmp packet) is zero and peer is NULL.
> in this case,raw_icmp_error will not be call too.this is valid??
> 

It is valid in the sense that we should not provide the user
with a mtu information if we know that the value we got from
the icmp packet ist bogus. But perhaps we can think about
making the check for a valid mtu unconditionally and let
ip_rt_frag_needed return a valid mtu in any case.

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Gao feng]

2011.10.19 15:26, Steffen Klassert wrote:

> 
> On udp and raw sockets, the user is responsible to adjust the packet
> size according to the mtu value he may find in the socket's error queue.
> So we shoud provide the user with this information, even in the unlikely
> case where we could not create an inet_peer.
> 

Agree.Maybe we should modify mtu immediately when create inet_peer failed

> 
> It is valid in the sense that we should not provide the user
> with a mtu information if we know that the value we got from
> the icmp packet ist bogus. But perhaps we can think about
> making the check for a valid mtu unconditionally and let
> ip_rt_frag_needed return a valid mtu in any case.
> 
> 

I think we should return the pmtu in icmp packet to the raw socket,
and the valid mtu to tcp_v4_err or something else.

thanks Steffen.

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Steffen Klassert]

On Wed, Oct 19, 2011 at 04:07:46PM +0800, Gao feng wrote:
> 2011.10.19 15:26, Steffen Klassert wrote:
> 
> > 
> > It is valid in the sense that we should not provide the user
> > with a mtu information if we know that the value we got from
> > the icmp packet ist bogus. But perhaps we can think about
> > making the check for a valid mtu unconditionally and let
> > ip_rt_frag_needed return a valid mtu in any case.
> > 
> > 
> 
> I think we should return the pmtu in icmp packet to the raw socket,
> and the valid mtu to tcp_v4_err or something else.
> 

Why you want to handle raw sockets different here?

Re: [PATCH] route:ip_rt_frag_needed always return unzero

[Gao feng]

2011.10.19 16:38, Steffen Klassert wrote:
> On Wed, Oct 19, 2011 at 04:07:46PM +0800, Gao feng wrote:
>> 2011.10.19 15:26, Steffen Klassert wrote:
>>
>>>
>>> It is valid in the sense that we should not provide the user
>>> with a mtu information if we know that the value we got from
>>> the icmp packet ist bogus. But perhaps we can think about
>>> making the check for a valid mtu unconditionally and let
>>> ip_rt_frag_needed return a valid mtu in any case.
>>>
>>>
>>
>> I think we should return the pmtu in icmp packet to the raw socket,
>> and the valid mtu to tcp_v4_err or something else.
>>
> 
> Why you want to handle raw sockets different here?
> 

Return the pmtu in icmp packet to the raw socket,
let user space program to decide what to do.
BUT this is not important.