[PATCH net-next 0/1] net/ipv4: SMC and SYN Cookies

[PATCH net-next 0/1] net/ipv4: SMC and SYN Cookies

[Ursula Braun]

Dave,

net/smc code is based on the SMC TCP experimental option.
Please apply Hans' patch to fix an inconsistency when SYN cookies
are active.

Thanks, Ursula

Hans Wippel (1):
  net/ipv4: disable SMC TCP option with SYN Cookies

 net/ipv4/tcp_output.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.13.5

[PATCH net-next 1/1] net/ipv4: disable SMC TCP option with SYN Cookies

[Ursula Braun]

From: Hans Wippel <hwippel@linux.vnet.ibm.com>

Currently, the SMC experimental TCP option in a SYN packet is lost on
the server side when SYN Cookies are active. However, the corresponding
SYNACK sent back to the client contains the SMC option. This causes an
inconsistent view of the SMC capabilities on the client and server.

This patch disables the SMC option in the SYNACK when SYN Cookies are
active to avoid this issue.

Signed-off-by: Hans Wippel <hwippel@linux.vnet.ibm.com>
Signed-off-by: Ursula Braun <ubraun@linux.vnet.ibm.com>
---
 net/ipv4/tcp_output.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 383cac0ff0ec..22894514feae 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3199,6 +3199,8 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
 		/* Under synflood, we do not attach skb to a socket,
 		 * to avoid false sharing.
 		 */
+		if (IS_ENABLED(CONFIG_SMC))
+			ireq->smc_ok = 0;
 		break;
 	case TCP_SYNACK_FASTOPEN:
 		/* sk is a const pointer, because we want to express multiple
-- 
2.13.5

Re: [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with SYN Cookies

[Eric Dumazet]

On 03/20/2018 08:53 AM, Ursula Braun wrote:
> From: Hans Wippel <hwippel@linux.vnet.ibm.com>
> 
> Currently, the SMC experimental TCP option in a SYN packet is lost on
> the server side when SYN Cookies are active. However, the corresponding
> SYNACK sent back to the client contains the SMC option. This causes an
> inconsistent view of the SMC capabilities on the client and server.
> 
> This patch disables the SMC option in the SYNACK when SYN Cookies are
> active to avoid this issue.
> 
> Signed-off-by: Hans Wippel <hwippel@linux.vnet.ibm.com>
> Signed-off-by: Ursula Braun <ubraun@linux.vnet.ibm.com>
> ---
>  net/ipv4/tcp_output.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 383cac0ff0ec..22894514feae 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -3199,6 +3199,8 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
>  		/* Under synflood, we do not attach skb to a socket,
>  		 * to avoid false sharing.
>  		 */
> +		if (IS_ENABLED(CONFIG_SMC))
> +			ireq->smc_ok = 0;
>  		break;
>  	case TCP_SYNACK_FASTOPEN:
>  		/* sk is a const pointer, because we want to express multiple
> 

I disagree with net-next qualification.

This fixes a bug, so please send it for net tree, and including an appropriate Fixes: tag.

Thanks !

Re: [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with SYN Cookies

[Eric Dumazet]

On 03/20/2018 09:21 AM, Eric Dumazet wrote:
> 
> 
> On 03/20/2018 08:53 AM, Ursula Braun wrote:
>> From: Hans Wippel <hwippel@linux.vnet.ibm.com>
>>
>> Currently, the SMC experimental TCP option in a SYN packet is lost on
>> the server side when SYN Cookies are active. However, the corresponding
>> SYNACK sent back to the client contains the SMC option. This causes an
>> inconsistent view of the SMC capabilities on the client and server.
>>
>> This patch disables the SMC option in the SYNACK when SYN Cookies are
>> active to avoid this issue.
>>
>> Signed-off-by: Hans Wippel <hwippel@linux.vnet.ibm.com>
>> Signed-off-by: Ursula Braun <ubraun@linux.vnet.ibm.com>
>> ---
>>  net/ipv4/tcp_output.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>> index 383cac0ff0ec..22894514feae 100644
>> --- a/net/ipv4/tcp_output.c
>> +++ b/net/ipv4/tcp_output.c
>> @@ -3199,6 +3199,8 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
>>  		/* Under synflood, we do not attach skb to a socket,
>>  		 * to avoid false sharing.
>>  		 */
>> +		if (IS_ENABLED(CONFIG_SMC))
>> +			ireq->smc_ok = 0;
>>  		break;
>>  	case TCP_SYNACK_FASTOPEN:
>>  		/* sk is a const pointer, because we want to express multiple
>>
> 
> I disagree with net-next qualification.
>
> This fixes a bug, so please send it for net tree, and including an appropriate Fixes: tag.
>

Also, please do not add the fix in tcp_make_synack()

tcp_make_synack() builds an skb, and really should not modify ireq, ideally.
The only reason ireq is not const is because of the skb_set_owner_w().

I would clear it in cookie_v4_check()/cookie_v6_check()

(We could have a common helper to allocate a TCP ireq btw, but this will wait a future patch for net-next)

Re: [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with SYN Cookies

[Ursula Braun]

On 03/20/2018 05:43 PM, Eric Dumazet wrote:
> 
> 
> On 03/20/2018 09:21 AM, Eric Dumazet wrote:
>>
>>
>> On 03/20/2018 08:53 AM, Ursula Braun wrote:
>>> From: Hans Wippel <hwippel@linux.vnet.ibm.com>
>>>
>>> Currently, the SMC experimental TCP option in a SYN packet is lost on
>>> the server side when SYN Cookies are active. However, the corresponding
>>> SYNACK sent back to the client contains the SMC option. This causes an
>>> inconsistent view of the SMC capabilities on the client and server.
>>>
>>> This patch disables the SMC option in the SYNACK when SYN Cookies are
>>> active to avoid this issue.
>>>
>>> Signed-off-by: Hans Wippel <hwippel@linux.vnet.ibm.com>
>>> Signed-off-by: Ursula Braun <ubraun@linux.vnet.ibm.com>
>>> ---
>>>  net/ipv4/tcp_output.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>>> index 383cac0ff0ec..22894514feae 100644
>>> --- a/net/ipv4/tcp_output.c
>>> +++ b/net/ipv4/tcp_output.c
>>> @@ -3199,6 +3199,8 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
>>>  		/* Under synflood, we do not attach skb to a socket,
>>>  		 * to avoid false sharing.
>>>  		 */
>>> +		if (IS_ENABLED(CONFIG_SMC))
>>> +			ireq->smc_ok = 0;
>>>  		break;
>>>  	case TCP_SYNACK_FASTOPEN:
>>>  		/* sk is a const pointer, because we want to express multiple
>>>
>>
>> I disagree with net-next qualification.
>>
>> This fixes a bug, so please send it for net tree, and including an appropriate Fixes: tag.
>>

Okay, I will send it for the net tree.

> 
> Also, please do not add the fix in tcp_make_synack()
> 
> tcp_make_synack() builds an skb, and really should not modify ireq, ideally.
> The only reason ireq is not const is because of the skb_set_owner_w().
> 
> I would clear it in cookie_v4_check()/cookie_v6_check()
> 
> (We could have a common helper to allocate a TCP ireq btw, but this will wait a future patch for net-next)
>

We moved the clear to cookie_v4_check()/cookie_v6_check. However, this does not seem to
be sufficient to prevent the SYNACK from containing the SMC experimental option.
We found that an additional check in tcp_conn_request() helps:

--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6248,6 +6248,9 @@ int tcp_conn_request(struct request_sock
 	if (want_cookie && !tmp_opt.saw_tstamp)
 		tcp_clear_options(&tmp_opt);
 
+	if (IS_ENABLED(CONFIG_SMC) && want_cookie && tmp_opt.smc_ok)
+		tmp_opt.smc_ok = 0;
+
 	tmp_opt.tstamp_ok = tmp_opt.saw_tstamp;
 	tcp_openreq_init(req, &tmp_opt, skb, sk);
 	inet_rsk(req)->no_srccheck = inet_sk(sk)->transparent;

Do you think this could be the right place for clearing the smc_ok bit?

  

Re: [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with SYN Cookies

[Eric Dumazet]

On 03/22/2018 06:23 AM, Ursula Braun wrote:

> We moved the clear to cookie_v4_check()/cookie_v6_check. However, this does not seem to
> be sufficient to prevent the SYNACK from containing the SMC experimental option.
> We found that an additional check in tcp_conn_request() helps:
> 
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6248,6 +6248,9 @@ int tcp_conn_request(struct request_sock
>  	if (want_cookie && !tmp_opt.saw_tstamp)
>  		tcp_clear_options(&tmp_opt);
>  
> +	if (IS_ENABLED(CONFIG_SMC) && want_cookie && tmp_opt.smc_ok)
> +		tmp_opt.smc_ok = 0;
> +
>  	tmp_opt.tstamp_ok = tmp_opt.saw_tstamp;
>  	tcp_openreq_init(req, &tmp_opt, skb, sk);
>  	inet_rsk(req)->no_srccheck = inet_sk(sk)->transparent;
> 
> Do you think this could be the right place for clearing the smc_ok bit?


Yes, but since tmp_opt is private to this thread/cpu, no false sharing to be afraid of

if (IS_ENABLED(CONFIG_SMC) && want_cookie)
    tmp_opt.smc_ok = 0;