* [PATCH net-next 0/1] net/ipv4: SMC and SYN Cookies @ 2018-03-20 15:53 Ursula Braun 2018-03-20 15:53 ` [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with " Ursula Braun 0 siblings, 1 reply; 6+ messages in thread From: Ursula Braun @ 2018-03-20 15:53 UTC (permalink / raw) To: davem; +Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, ubraun 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with SYN Cookies 2018-03-20 15:53 [PATCH net-next 0/1] net/ipv4: SMC and SYN Cookies Ursula Braun @ 2018-03-20 15:53 ` Ursula Braun 2018-03-20 16:21 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Ursula Braun @ 2018-03-20 15:53 UTC (permalink / raw) To: davem; +Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl, ubraun 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with SYN Cookies 2018-03-20 15:53 ` [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with " Ursula Braun @ 2018-03-20 16:21 ` Eric Dumazet 2018-03-20 16:43 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2018-03-20 16:21 UTC (permalink / raw) To: Ursula Braun, davem Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl 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 ! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with SYN Cookies 2018-03-20 16:21 ` Eric Dumazet @ 2018-03-20 16:43 ` Eric Dumazet 2018-03-22 13:23 ` Ursula Braun 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2018-03-20 16:43 UTC (permalink / raw) To: Eric Dumazet, Ursula Braun, davem Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl 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) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with SYN Cookies 2018-03-20 16:43 ` Eric Dumazet @ 2018-03-22 13:23 ` Ursula Braun 2018-03-22 14:30 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Ursula Braun @ 2018-03-22 13:23 UTC (permalink / raw) To: Eric Dumazet, davem Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with SYN Cookies 2018-03-22 13:23 ` Ursula Braun @ 2018-03-22 14:30 ` Eric Dumazet 0 siblings, 0 replies; 6+ messages in thread From: Eric Dumazet @ 2018-03-22 14:30 UTC (permalink / raw) To: Ursula Braun, davem Cc: netdev, linux-s390, schwidefsky, heiko.carstens, raspl 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; ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-22 14:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-20 15:53 [PATCH net-next 0/1] net/ipv4: SMC and SYN Cookies Ursula Braun 2018-03-20 15:53 ` [PATCH net-next 1/1] net/ipv4: disable SMC TCP option with " Ursula Braun 2018-03-20 16:21 ` Eric Dumazet 2018-03-20 16:43 ` Eric Dumazet 2018-03-22 13:23 ` Ursula Braun 2018-03-22 14:30 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).