mptcp.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Geliang Tang <geliangtang@gmail.com>
To: Mat Martineau <mathew.j.martineau@linux.intel.com>
Cc: mptcp@lists.linux.dev
Subject: Re: [MPTCP][PATCH v3 mptcp-next 3/8] mptcp: send out MP_FAIL when data checksum fail
Date: Tue, 13 Jul 2021 20:44:26 +0800	[thread overview]
Message-ID: <CA+WQbwv_dpYt6aKAA4g-pV2HYDKM5Y4G_K-nY31ZLLLwkpLCHA@mail.gmail.com> (raw)
In-Reply-To: <ce51fdd8-f261-a17-ce9e-362b8712cc55@linux.intel.com>

Hi Mat,

Sorry for late response on this.

Mat Martineau <mathew.j.martineau@linux.intel.com> 于2021年7月8日周四 上午7:20写道:
>
> On Mon, 28 Jun 2021, Geliang Tang wrote:
>
> > When a bad checksum is detected, send out the MP_FAIL option.
> >
> > When multiple subflows are in use, close the affected subflow with a RST
> > that includes an MP_FAIL option.
> >
> > When a single subfow is in use, send back an MP_FAIL option on the
> > subflow-level ACK. And the receiver of this MP_FAIL respond with an
> > MP_FAIL in the reverse direction.
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> > net/mptcp/pm.c       | 10 ++++++++++
> > net/mptcp/protocol.h | 14 ++++++++++++++
> > net/mptcp/subflow.c  | 18 ++++++++++++++++++
> > 3 files changed, 42 insertions(+)
> >
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index d4c19420194a..c34c9c0b2fa5 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -250,8 +250,18 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> > void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
> > {
> >       struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > +     struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> >
> >       pr_debug("map_seq=%llu fail_seq=%llu", subflow->map_seq, fail_seq);
> > +
> > +     if (!msk->pm.subflows) {
>
> The pm.lock isn't held so it's not safe to access pm.subflows
>
> I don't think it's sufficient to read pm.subflows with the lock or add
> READ_ONCE/WRITE_ONCE, since that would still allow race conditions with
> the msk. To handle fallback when receiving MP_FAIL I think the
> sock_owned_by_user() checks and delegated callback (similar to
> mptcp_subflow_process_delegated()) may be needed.
>

I think it's better to use the below mptcp_has_another_subflow() function
here instead of using msk->pm.subflows to check the single subflow case.

> > +             if (!subflow->mp_fail_need_echo) {
> > +                     subflow->send_mp_fail = 1;
> > +                     subflow->fail_seq = fail_seq;
>
> Echoing the fail_seq back doesn't seem correct, from the RFC it seems like
> this side should send a sequence number for the opposite data direction?
> Do you agree?

So we should use 'subflow->fail_seq = subflow->map_seq;' here?

>
> > +             } else {
> > +                     subflow->mp_fail_need_echo = 0;
> > +             }
> > +     }
> > }
> >
> > /* path manager helpers */
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 8e050575a2d9..7a49454c77a6 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -422,6 +422,7 @@ struct mptcp_subflow_context {
> >               backup : 1,
> >               send_mp_prio : 1,
> >               send_mp_fail : 1,
> > +             mp_fail_need_echo : 1,
>
> I think mp_fail_expect_echo would be a more accurate name.

Updated in v4.

>
> >               rx_eof : 1,
> >               can_ack : 1,        /* only after processing the remote a key */
> >               disposable : 1;     /* ctx can be free at ulp release time */
> > @@ -594,6 +595,19 @@ static inline void mptcp_subflow_tcp_fallback(struct sock *sk,
> >       inet_csk(sk)->icsk_af_ops = ctx->icsk_af_ops;
> > }
> >
> > +static inline bool mptcp_has_another_subflow_established(struct sock *ssk)
> > +{
> > +     struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk), *tmp;
> > +     struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> > +
> > +     mptcp_for_each_subflow(msk, tmp) {
> > +             if (tmp->fully_established && tmp != subflow)
>
> Why check tmp->fully_established here?

I'll drop tmp->fully_established, and rename this function
mptcp_has_another_subflow to check whether there's a single subflow.


In my test, this MP_FAIL patchset only works in the multiple subflows case.
The receiver detected the bad checksum, sent RST + MP_FAIL to close this
subflow and discard the data with the bad checksum. Then the sender will
retransmit a new data with good checksum from other subflow. And the
receiver can resemble the whole data successfully.

But the single subflow case dosen't work. I don't know how the new data
can be retransmitted from the sender side when it fallback to regular TCP.
So I'm not sure how to implement the single subflow case.

Could you please give me some suggestions?

Thanks,
-Geliang


>
>
> - Mat
>
>
> > +                     return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > void __init mptcp_proto_init(void);
> > #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> > int __init mptcp_proto_v6_init(void);
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 0b5d4a3eadcd..46302208c474 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -913,6 +913,8 @@ static enum mapping_status validate_data_csum(struct sock *ssk, struct sk_buff *
> >       csum = csum_partial(&header, sizeof(header), subflow->map_data_csum);
> >       if (unlikely(csum_fold(csum))) {
> >               MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DATACSUMERR);
> > +             subflow->send_mp_fail = 1;
> > +             subflow->fail_seq = subflow->map_seq;
> >               return subflow->mp_join ? MAPPING_INVALID : MAPPING_DUMMY;
> >       }
> >
> > @@ -1160,6 +1162,22 @@ static bool subflow_check_data_avail(struct sock *ssk)
> >
> > fallback:
> >       /* RFC 8684 section 3.7. */
> > +     if (subflow->send_mp_fail) {
> > +             if (mptcp_has_another_subflow_established(ssk)) {
> > +                     mptcp_subflow_reset(ssk);
> > +                     while ((skb = skb_peek(&ssk->sk_receive_queue)))
> > +                             sk_eat_skb(ssk, skb);
> > +                     WRITE_ONCE(subflow->data_avail, 0);
> > +                     return true;
> > +             }
> > +
> > +             if (!msk->pm.subflows) {
> > +                     subflow->mp_fail_need_echo = 1;
> > +                     WRITE_ONCE(subflow->data_avail, 0);
> > +                     return true;
> > +             }
> > +     }
> > +
> >       if (subflow->mp_join || subflow->fully_established) {
> >               /* fatal protocol error, close the socket.
> >                * subflow_error_report() will introduce the appropriate barriers
> > --
> > 2.31.1
> >
> >
> >
>
> --
> Mat Martineau
> Intel

  reply	other threads:[~2021-07-13 12:44 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-28  4:28 [MPTCP][PATCH v3 mptcp-next 0/8] MP_FAIL support Geliang Tang
2021-06-28  4:28 ` [MPTCP][PATCH v3 mptcp-next 1/8] mptcp: MP_FAIL suboption sending Geliang Tang
2021-06-28  4:28   ` [MPTCP][PATCH v3 mptcp-next 2/8] mptcp: MP_FAIL suboption receiving Geliang Tang
2021-06-28  4:28     ` [MPTCP][PATCH v3 mptcp-next 3/8] mptcp: send out MP_FAIL when data checksum fail Geliang Tang
2021-06-28  4:29       ` [MPTCP][PATCH v3 mptcp-next 4/8] mptcp: add the mibs for MP_FAIL Geliang Tang
2021-06-28  4:29         ` [MPTCP][PATCH v3 mptcp-next 5/8] selftests: mptcp: add MP_FAIL mibs check Geliang Tang
2021-06-28  4:29           ` [MPTCP][PATCH v3 mptcp-next 6/8] mptcp: infinite mapping sending Geliang Tang
2021-06-28  4:29             ` [MPTCP][PATCH v3 mptcp-next 7/8] mptcp: infinite mapping receiving Geliang Tang
2021-06-28  4:29               ` [MPTCP][PATCH v3 mptcp-next 8/8] mptcp: add a mib for the infinite mapping sending Geliang Tang
2021-07-07 23:49               ` [MPTCP][PATCH v3 mptcp-next 7/8] mptcp: infinite mapping receiving Mat Martineau
2021-07-07 23:44             ` [MPTCP][PATCH v3 mptcp-next 6/8] mptcp: infinite mapping sending Mat Martineau
2021-07-08  0:44               ` Mat Martineau
2021-07-07 23:20       ` [MPTCP][PATCH v3 mptcp-next 3/8] mptcp: send out MP_FAIL when data checksum fail Mat Martineau
2021-07-13 12:44         ` Geliang Tang [this message]
2021-07-13 20:35           ` Mat Martineau
2021-07-14  3:56             ` Geliang Tang
2021-07-14 17:48               ` Mat Martineau
2021-07-07 23:07   ` [MPTCP][PATCH v3 mptcp-next 1/8] mptcp: MP_FAIL suboption sending Mat Martineau
2021-07-14  8:45   ` Paolo Abeni
2021-07-14  9:16     ` Geliang Tang
2021-07-14 15:49       ` Paolo Abeni

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CA+WQbwv_dpYt6aKAA4g-pV2HYDKM5Y4G_K-nY31ZLLLwkpLCHA@mail.gmail.com \
    --to=geliangtang@gmail.com \
    --cc=mathew.j.martineau@linux.intel.com \
    --cc=mptcp@lists.linux.dev \
    --subject='Re: [MPTCP][PATCH v3 mptcp-next 3/8] mptcp: send out MP_FAIL when data checksum fail' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox