Re: [PATCH v2 2/8] Allow tcp_parse_options to consult dst entry

[Gilad Ben-Yossef <gilad@codefidence.com>]

Hi Ilpo,


Thanks for the feedback :-)


Ilpo Järvinen wrote:

> On Wed, 21 Oct 2009, Gilad Ben-Yossef wrote:
>
>   
>> We need tcp_parse_options to be aware of dst_entry to 
>> take into account per dst_entry TCP options settings
>>
>> Signed-off-by: Gilad Ben-Yossef <gilad@codefidence.com>
>> Sigend-off-by: Ori Finkelman <ori@comsleep.com>
>> Sigend-off-by: Yony Amit <yony@comsleep.com>
>>
>> ---
>>  include/net/tcp.h        |    3 ++-
>>  net/ipv4/syncookies.c    |   27 ++++++++++++++-------------
>>  net/ipv4/tcp_input.c     |    9 ++++++---
>>  net/ipv4/tcp_ipv4.c      |   19 ++++++++++---------
>>  net/ipv4/tcp_minisocks.c |    7 +++++--
>>  net/ipv6/syncookies.c    |   28 +++++++++++++++-------------
>>  net/ipv6/tcp_ipv6.c      |    3 ++-
>>  7 files changed, 54 insertions(+), 42 deletions(-)
>>
>>
>>     
<snip>
>> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
>> index 7cda24b..1cb0ec4 100644
>> --- a/net/ipv4/tcp_ipv4.c
>> +++ b/net/ipv4/tcp_ipv4.c
>> @@ -1256,11 +1256,18 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
>>  	tcp_rsk(req)->af_specific = &tcp_request_sock_ipv4_ops;
>>  #endif
>>  
>> +	ireq = inet_rsk(req);
>> +	ireq->loc_addr = daddr;
>> +	ireq->rmt_addr = saddr;
>> +	ireq->no_srccheck = inet_sk(sk)->transparent;
>> +	ireq->opt = tcp_v4_save_options(sk, skb);
>> +
>> +	dst = inet_csk_route_req(sk, req);
>>  	tcp_clear_options(&tmp_opt);
>>  	tmp_opt.mss_clamp = 536;
>>  	tmp_opt.user_mss  = tcp_sk(sk)->rx_opt.user_mss;
>>  
>> -	tcp_parse_options(skb, &tmp_opt, 0);
>> +	tcp_parse_options(skb, &tmp_opt, 0, dst);
>>  
>>  	if (want_cookie && !tmp_opt.saw_tstamp)
>>  		tcp_clear_options(&tmp_opt);
>> @@ -1269,14 +1276,8 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
>>  
>>  	tcp_openreq_init(req, &tmp_opt, skb);
>>  
>> -	ireq = inet_rsk(req);
>> -	ireq->loc_addr = daddr;
>> -	ireq->rmt_addr = saddr;
>> -	ireq->no_srccheck = inet_sk(sk)->transparent;
>> -	ireq->opt = tcp_v4_save_options(sk, skb);
>> -
>>  	if (security_inet_conn_request(sk, skb, req))
>> -		goto drop_and_free;
>> +		goto drop_and_release;
>>  
>>  	if (!want_cookie)
>>  		TCP_ECN_create_request(req, tcp_hdr(skb));
>> @@ -1301,7 +1302,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
>>  		 */
>>  		if (tmp_opt.saw_tstamp &&
>>  		    tcp_death_row.sysctl_tw_recycle &&
>> -		    (dst = inet_csk_route_req(sk, req)) != NULL &&
>> +		    dst != NULL &&
>>     
>
> Why you need this NULL check this here while you trap it with BUG_ON 
> elsewhere? Does your patch perhaps create a remote DoS opportunity?
>
>
>   
Indeed, I believe you are right. Good catch.

What about this (I know the patch gets eaten by Thunderbird, sorry about 
that. This is just for explaining what I want to do):

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c

index 1cb0ec4..1d611e3 100644

--- a/net/ipv4/tcp_ipv4.c

+++ b/net/ipv4/tcp_ipv4.c

@@ -1263,6 +1263,9 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)

        ireq->opt = tcp_v4_save_options(sk, skb);

 

        dst = inet_csk_route_req(sk, req);

+       if(!dst)

+               goto drop_and_free;

+

        tcp_clear_options(&tmp_opt);

        tmp_opt.mss_clamp = 536;

        tmp_opt.user_mss  = tcp_sk(sk)->rx_opt.user_mss;

@@ -1302,7 +1305,6 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)

                 */

                if (tmp_opt.saw_tstamp &&

                    tcp_death_row.sysctl_tw_recycle &&

-                   dst != NULL &&

                    (peer = rt_get_peer((struct rtable *)dst)) != NULL &&

                    peer->v4daddr == saddr) {

                        if (get_seconds() < peer->tcp_ts_stamp + TCP_PAWS_MSL &&



My rational is that since if the connection is formed we will need to 
send a syn/ack ( call to __tcp_v4_send_synack a couple of lines below) 
and since we can't do that  if we don't have a route, this makes sense.

If this sounds sane, I'll re-spin the patch with this as a fix.

Thanks a bunch!
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker & CTO
Codefidence Ltd.

Web:   http://codefidence.com
Cell:  +972-52-8260388
Skype: gilad_codefidence
Tel:   +972-8-9316883 ext. 201
Fax:   +972-8-9316884
Email: gilad@codefidence.com

Check out our Open Source technology and training blog - http://tuxology.net

	"Sorry cannot parse this, its too long to be true  :)"
	  -- Eric Dumazet on netdev mailing list