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@xxxxxxxxxxxxxxxxxx> >>> >>> 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@xxxxxxxxxxxxxxxxxx> >>> Signed-off-by: Ursula Braun <ubraun@xxxxxxxxxxxxxxxxxx> >>> --- >>> 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? -- To unsubscribe from this list: send the line "unsubscribe linux-s390" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
