* [MPTCP] Re: [PATCH RFC 08/11] tcp: syncookies: keep mptcp option enabled
@ 2020-07-27 8:57 Florian Westphal
0 siblings, 0 replies; 4+ messages in thread
From: Florian Westphal @ 2020-07-27 8:57 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 728 bytes --]
Paolo Abeni <pabeni(a)redhat.com> wrote:
> > treq = tcp_rsk(req);
> > treq->tfo_listener = false;
> >
> > - if (IS_ENABLED(CONFIG_MPTCP))
> > - treq->is_mptcp = 0;
> > + if (treq->is_mptcp) {
> > + int err = mptcp_subflow_init_cookie_req(req, sk, skb);
> > +
> > + if (err)
> > + goto out_free;
> > + }
>
> It looks like 'treq->is_mptcp' is not initialized here ?
Yes, this is resolved now.
> What about moving the whole oops selection, init_reqsk_alloc() call,
> is_mptcp init and mptcp_subflow_init_cookie_req() call in an header
> only helper? (say tcp_reqsk_alloc() or the like)
ipv6 uses tcp6_request_sock_ops, so they are not 100% the same,
unfortunately.
What did you have in mind?
^ permalink raw reply [flat|nested] 4+ messages in thread* [MPTCP] Re: [PATCH RFC 08/11] tcp: syncookies: keep mptcp option enabled
@ 2020-07-27 9:13 Paolo Abeni
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Abeni @ 2020-07-27 9:13 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]
On Mon, 2020-07-27 at 10:57 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > treq = tcp_rsk(req);
> > > treq->tfo_listener = false;
> > >
> > > - if (IS_ENABLED(CONFIG_MPTCP))
> > > - treq->is_mptcp = 0;
> > > + if (treq->is_mptcp) {
> > > + int err = mptcp_subflow_init_cookie_req(req, sk, skb);
> > > +
> > > + if (err)
> > > + goto out_free;
> > > + }
> >
> > It looks like 'treq->is_mptcp' is not initialized here ?
>
> Yes, this is resolved now.
>
> > What about moving the whole oops selection, init_reqsk_alloc() call,
> > is_mptcp init and mptcp_subflow_init_cookie_req() call in an header
> > only helper? (say tcp_reqsk_alloc() or the like)
>
> ipv6 uses tcp6_request_sock_ops, so they are not 100% the same,
> unfortunately.
>
> What did you have in mind?
Something alike the following (untested):
struct request_sock *tcp_reqsk_alloc(const struct request_sock_ops *ops,
struct sock *sk)
{
struct tcp_request_sock *treq;
struct request_sock * req;
#ifdef CONFIG_MPTCP
if (sk_is_mptcp(sk))
ops = &mptcp_subflow_request_sock_ops;
#endif
req = inet_reqsk_alloc(ops, sk, false);
if (!req)
return req;
treq = tcp_rsk(req);
treq->is_mptcp = sk_is_mptcp(sk);
if (treq->is_mptcp &&
mptcp_subflow_init_cookie_req(req, sk, skb)) {
reqsk_free(req);
return NULL;
}
return req;
}
than ipv4 may call tcp_reqsk_alloc(tcp_request_sock_ops, sk) and
ipv6 tcp_reqsk_alloc(tcp6_request_sock_ops, sk). That could possibly
remove the need for patch "tcp: syncookies: use single reqsk_free
location"
Not sure if I missed something more...
Cheers,
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread* [MPTCP] Re: [PATCH RFC 08/11] tcp: syncookies: keep mptcp option enabled
@ 2020-07-15 14:50 Christoph Paasch
0 siblings, 0 replies; 4+ messages in thread
From: Christoph Paasch @ 2020-07-15 14:50 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 5028 bytes --]
On 07/15/20 - 15:22, Paolo Abeni wrote:
> On Tue, 2020-07-14 at 20:25 +0200, Florian Westphal wrote:
> > If SYN packet contains MP_CAPABLE option, keep it enabled.
> > The mptcp relevant code is changed to detect the new 'sycookie' flag
> > in the request socket. If it is set, the socket isn't inserted into
> > the token tree. This will only be done when the cookie ACK comes back.
> >
> > Signed-off-by: Florian Westphal <fw(a)strlen.de>
> > ---
> > net/ipv4/syncookies.c | 21 +++++++++++++++++----
> > net/ipv4/tcp_input.c | 4 +---
> > net/ipv6/syncookies.c | 17 ++++++++++++++---
> > 3 files changed, 32 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> > index ee17f55401ef..5da49dc126f9 100644
> > --- a/net/ipv4/syncookies.c
> > +++ b/net/ipv4/syncookies.c
> > @@ -289,6 +289,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> > struct tcp_sock *tp = tcp_sk(sk);
> > const struct tcphdr *th = tcp_hdr(skb);
> > __u32 cookie = ntohl(th->ack_seq) - 1;
> > + const struct request_sock_ops *ops;
> > struct sock *ret = sk;
> > struct request_sock *req;
> > int mss;
> > @@ -326,12 +327,27 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> > goto out;
> >
> > ret = NULL;
> > - req = inet_reqsk_alloc(&tcp_request_sock_ops, sk, false); /* for safety */
> > + ops = &tcp_request_sock_ops;
> > +#ifdef CONFIG_MPTCP
> > + if (sk_is_mptcp(sk))
> > + ops = &mptcp_subflow_request_sock_ops;
> > +#endif
> > +
> > + req = inet_reqsk_alloc(ops, sk, false); /* for safety */
> > if (!req)
> > goto out;
> >
> > ireq = inet_rsk(req);
> > treq = tcp_rsk(req);
> > + treq->is_mptcp = sk_is_mptcp(sk);
> > +
> > + if (treq->is_mptcp) {
> > + int err = mptcp_subflow_init_cookie_req(req, sk, skb);
> > +
> > + if (err)
> > + goto out_free;
> > + }
> > +
> > treq->rcv_isn = ntohl(th->seq) - 1;
> > treq->snt_isn = cookie;
> > treq->ts_off = 0;
> > @@ -350,9 +366,6 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> > treq->snt_synack = 0;
> > treq->tfo_listener = false;
> >
> > - if (IS_ENABLED(CONFIG_MPTCP))
> > - treq->is_mptcp = 0;
> > -
> > if (IS_ENABLED(CONFIG_SMC))
> > ireq->smc_ok = 0;
> >
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index d8b9d09eb472..5ae612902806 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -6669,6 +6669,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> > #if IS_ENABLED(CONFIG_MPTCP)
> > tcp_rsk(req)->is_mptcp = 0;
> > #endif
> > + tcp_rsk(req)->syncookie = want_cookie;
> >
> > tcp_clear_options(&tmp_opt);
> > tmp_opt.mss_clamp = af_ops->mss_clamp;
> > @@ -6691,9 +6692,6 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> >
> > af_ops->init_req(req, sk, skb);
> >
> > - if (IS_ENABLED(CONFIG_MPTCP) && want_cookie)
> > - tcp_rsk(req)->is_mptcp = 0;
> > -
> > if (security_inet_conn_request(sk, skb, req))
> > goto drop_and_free;
> >
> > diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
> > index 13235a012388..0a05c3c1e776 100644
> > --- a/net/ipv6/syncookies.c
> > +++ b/net/ipv6/syncookies.c
> > @@ -134,6 +134,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
> > struct tcp_sock *tp = tcp_sk(sk);
> > const struct tcphdr *th = tcp_hdr(skb);
> > __u32 cookie = ntohl(th->ack_seq) - 1;
> > + const struct request_sock_ops *ops;
> > struct sock *ret = sk;
> > struct request_sock *req;
> > int mss;
> > @@ -170,7 +171,13 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
> > goto out;
> >
> > ret = NULL;
> > - req = inet_reqsk_alloc(&tcp6_request_sock_ops, sk, false);
> > + ops = &tcp6_request_sock_ops;
> > +#ifdef CONFIG_MPTCP
> > + if (sk_is_mptcp(sk))
> > + ops = &mptcp_subflow_request_sock_ops;
> > +#endif
> > +
> > + req = inet_reqsk_alloc(ops, sk, false);
> > if (!req)
> > goto out;
> >
> > @@ -178,8 +185,12 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
> > treq = tcp_rsk(req);
> > treq->tfo_listener = false;
> >
> > - if (IS_ENABLED(CONFIG_MPTCP))
> > - treq->is_mptcp = 0;
> > + if (treq->is_mptcp) {
> > + int err = mptcp_subflow_init_cookie_req(req, sk, skb);
> > +
> > + if (err)
> > + goto out_free;
> > + }
>
> It looks like 'treq->is_mptcp' is not initialized here ?
Yep, I just hit a "KASAN: slab-out-of-bounds in __subflow_init_req" due to that.
Christoph
>
> What about moving the whole oops selection, init_reqsk_alloc() call,
> is_mptcp init and mptcp_subflow_init_cookie_req() call in an header
> only helper? (say tcp_reqsk_alloc() or the like)
>
> Thanks,
>
> Paolo
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org
^ permalink raw reply [flat|nested] 4+ messages in thread* [MPTCP] Re: [PATCH RFC 08/11] tcp: syncookies: keep mptcp option enabled
@ 2020-07-15 13:22 Paolo Abeni
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Abeni @ 2020-07-15 13:22 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 4459 bytes --]
On Tue, 2020-07-14 at 20:25 +0200, Florian Westphal wrote:
> If SYN packet contains MP_CAPABLE option, keep it enabled.
> The mptcp relevant code is changed to detect the new 'sycookie' flag
> in the request socket. If it is set, the socket isn't inserted into
> the token tree. This will only be done when the cookie ACK comes back.
>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
> net/ipv4/syncookies.c | 21 +++++++++++++++++----
> net/ipv4/tcp_input.c | 4 +---
> net/ipv6/syncookies.c | 17 ++++++++++++++---
> 3 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index ee17f55401ef..5da49dc126f9 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -289,6 +289,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> struct tcp_sock *tp = tcp_sk(sk);
> const struct tcphdr *th = tcp_hdr(skb);
> __u32 cookie = ntohl(th->ack_seq) - 1;
> + const struct request_sock_ops *ops;
> struct sock *ret = sk;
> struct request_sock *req;
> int mss;
> @@ -326,12 +327,27 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> goto out;
>
> ret = NULL;
> - req = inet_reqsk_alloc(&tcp_request_sock_ops, sk, false); /* for safety */
> + ops = &tcp_request_sock_ops;
> +#ifdef CONFIG_MPTCP
> + if (sk_is_mptcp(sk))
> + ops = &mptcp_subflow_request_sock_ops;
> +#endif
> +
> + req = inet_reqsk_alloc(ops, sk, false); /* for safety */
> if (!req)
> goto out;
>
> ireq = inet_rsk(req);
> treq = tcp_rsk(req);
> + treq->is_mptcp = sk_is_mptcp(sk);
> +
> + if (treq->is_mptcp) {
> + int err = mptcp_subflow_init_cookie_req(req, sk, skb);
> +
> + if (err)
> + goto out_free;
> + }
> +
> treq->rcv_isn = ntohl(th->seq) - 1;
> treq->snt_isn = cookie;
> treq->ts_off = 0;
> @@ -350,9 +366,6 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb)
> treq->snt_synack = 0;
> treq->tfo_listener = false;
>
> - if (IS_ENABLED(CONFIG_MPTCP))
> - treq->is_mptcp = 0;
> -
> if (IS_ENABLED(CONFIG_SMC))
> ireq->smc_ok = 0;
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index d8b9d09eb472..5ae612902806 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6669,6 +6669,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> #if IS_ENABLED(CONFIG_MPTCP)
> tcp_rsk(req)->is_mptcp = 0;
> #endif
> + tcp_rsk(req)->syncookie = want_cookie;
>
> tcp_clear_options(&tmp_opt);
> tmp_opt.mss_clamp = af_ops->mss_clamp;
> @@ -6691,9 +6692,6 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>
> af_ops->init_req(req, sk, skb);
>
> - if (IS_ENABLED(CONFIG_MPTCP) && want_cookie)
> - tcp_rsk(req)->is_mptcp = 0;
> -
> if (security_inet_conn_request(sk, skb, req))
> goto drop_and_free;
>
> diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
> index 13235a012388..0a05c3c1e776 100644
> --- a/net/ipv6/syncookies.c
> +++ b/net/ipv6/syncookies.c
> @@ -134,6 +134,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
> struct tcp_sock *tp = tcp_sk(sk);
> const struct tcphdr *th = tcp_hdr(skb);
> __u32 cookie = ntohl(th->ack_seq) - 1;
> + const struct request_sock_ops *ops;
> struct sock *ret = sk;
> struct request_sock *req;
> int mss;
> @@ -170,7 +171,13 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
> goto out;
>
> ret = NULL;
> - req = inet_reqsk_alloc(&tcp6_request_sock_ops, sk, false);
> + ops = &tcp6_request_sock_ops;
> +#ifdef CONFIG_MPTCP
> + if (sk_is_mptcp(sk))
> + ops = &mptcp_subflow_request_sock_ops;
> +#endif
> +
> + req = inet_reqsk_alloc(ops, sk, false);
> if (!req)
> goto out;
>
> @@ -178,8 +185,12 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
> treq = tcp_rsk(req);
> treq->tfo_listener = false;
>
> - if (IS_ENABLED(CONFIG_MPTCP))
> - treq->is_mptcp = 0;
> + if (treq->is_mptcp) {
> + int err = mptcp_subflow_init_cookie_req(req, sk, skb);
> +
> + if (err)
> + goto out_free;
> + }
It looks like 'treq->is_mptcp' is not initialized here ?
What about moving the whole oops selection, init_reqsk_alloc() call,
is_mptcp init and mptcp_subflow_init_cookie_req() call in an header
only helper? (say tcp_reqsk_alloc() or the like)
Thanks,
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-07-27 9:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-27 8:57 [MPTCP] Re: [PATCH RFC 08/11] tcp: syncookies: keep mptcp option enabled Florian Westphal
-- strict thread matches above, loose matches on Subject: below --
2020-07-27 9:13 Paolo Abeni
2020-07-15 14:50 Christoph Paasch
2020-07-15 13:22 Paolo Abeni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.