From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from m12-15.163.com (m12-15.163.com [220.181.12.15]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 7079970 for ; Thu, 24 Jun 2021 02:14:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=163.com; s=s110527; h=Subject:From:Message-ID:Date:MIME-Version; bh=+ckpR vGgsN5TwX3STJJRXvsMlkRCA7q9x3O8vMeILbk=; b=YgoPY49CiBv7D76nvMK7o KuWQwQJu+gLok9YZFWeEXLykGqZ7x+8d86w2soIm7afsaLdcOYmbDRf7mCuTWloQ jiv2K8oeHgjQvcWzx7ntsvWFrMZwkBQEFe6rPv3Jdof6AoweqbpH17ox3kP11lLS TaNGx6LX4wwVVvSiQ0VSNA= Received: from [10.8.2.10] (unknown [36.111.140.26]) by smtp11 (Coremail) with SMTP id D8CowACnLRGQ5tNgOb8sAQ--.20S2; Thu, 24 Jun 2021 09:58:43 +0800 (CST) Subject: Re: [PATCH v5 4/4] mptcp: avoid processing packet if a subflow reset To: Paolo Abeni , Mat Martineau Cc: mptcp@lists.linux.dev, fw@strlen.de References: <1623840570-42004-1-git-send-email-wujianguo106@163.com> <1623840570-42004-5-git-send-email-wujianguo106@163.com> <45dcfe4c-1918-2d78-accf-141bb4af2c5b@linux.intel.com> <2024b917-84de-4dea-2244-5dce7a7f2495@linux.intel.com> <1ac0561c290bf87aa54277bc6b458a859b8ff080.camel@redhat.com> From: Jianguo Wu Message-ID: Date: Thu, 24 Jun 2021 09:57:35 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 Precedence: bulk X-Mailing-List: mptcp@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 In-Reply-To: <1ac0561c290bf87aa54277bc6b458a859b8ff080.camel@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-CM-TRANSID:D8CowACnLRGQ5tNgOb8sAQ--.20S2 X-Coremail-Antispam: 1Uf129KBjvJXoWxZFy8GFyrury5Zr13uFyDZFb_yoW5tFyrpr yDGa17JrWvqryxJrWIvF1rXr1Svryrtr4UX34Dta4Yyrn8Kr1xKryUtr1a9F4xGFsaka1U Zw42qa9xZw1avaUanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x07jGtC7UUUUU= X-Originating-IP: [36.111.140.26] X-CM-SenderInfo: 5zxmxt5qjx0iiqw6il2tof0z/1tbiJxO7kF5u+1GIGAAAsV On 2021/6/23 17:48, Paolo Abeni wrote: > On Tue, 2021-06-22 at 17:00 -0700, Mat Martineau wrote: >> On Mon, 21 Jun 2021, Jianguo Wu wrote: >> >>> Hi Mat, >>> >>> On 2021/6/19 8:19, Mat Martineau wrote: >>>> On Wed, 16 Jun 2021, wujianguo106@163.com wrote: >>>> >>>>> From: Jianguo Wu >>>>> >>>>> If check_fully_established() causes a subflow reset, it should not >>>>> continue to process the packet in tcp_data_queue(). >>>>> >>>>> setting: >>>>> TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >>>>> >>>>> so that the following check will drop the pkt in >>>>> tcp_data_queue(): >>>>> if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq) { >>>>> __kfree_skb(skb); >>>>> return; >>>>> } >>>>> >>>>> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows") >>>>> Signed-off-by: Jianguo Wu >>>>> --- >>>>> net/mptcp/options.c | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>>>> index 1aec01686c1a..be435c5421cd 100644 >>>>> --- a/net/mptcp/options.c >>>>> +++ b/net/mptcp/options.c >>>>> @@ -926,6 +926,12 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, >>>>> return true; >>>>> >>>>> reset: >>>>> + /* If a subflow is reset, the packet should not continue to be >>>>> + * processed in tcp_data_queue(), so setting: end_seq = seq, >>>>> + * then tcp_data_queue() will drop the packet. >>>>> + */ >>>>> + TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(skb)->seq; >>>>> + >>>> >>>> This does have the desired effect when mptcp_incoming_options() is >>>> called from tcp_data_queue(), but mptcp_incoming_options() is also >>>> called from tcp_reset() and tcp_rcv_state_process(). The other callers >>>> appear to tolerate the sequence number modification. >>>> >>>> I think it would be clearer to either add a return value or output >>>> parameter to mptcp_incoming_options() to explicitly tell the caller >>>> that a reset has been sent and tcp_done() called. Then it would be >>>> clearer in tcp_data_queue() that the packet is being discarded due to >>>> mptcp header content. >>>> >>> >>> If a reset has been sent and tcp_done() called in >>> check_fully_established(), the sk_state will be TCP_CLOSE, how about >>> just do (sk_state == TCP_CLOSE) check in tcp_data_queue() as it did in >>> the V1 of this patch? >> >> Oh, I see now that Paolo suggested the the end_seq assignment in order to >> only modify MPTCP code. >> >> I still think it's better to make it clear that we're discarding a packet >> due to the mptcp headers - using the existing sequence check (intended to >> detect acks) in tcp_data_queue() seems sneaky to me. >> >> Something like >> >> if (sk_is_mptcp(sk) && !mptcp_incoming_options(sk, skb)) { >> __kfree_skb(skb); >> return; >> } >> >> seems both compact and clear. Does that seem ok Paolo? > > Uhmmm... we need to touch every mptcp_incoming_options() call site, and > in tcp_reset() the above chunk looks a bit strange to me. Probably we > could just ignore the mptcp_incoming_options() return value there. > > Otherwise no big objections - not sure about upstream ;) > Hi Mat and Paolo, If you both agree, I will send a new version that mptcp_incoming_options() add a return value, and only check return value in tcp_data_queue(), ignore the return value in other call site. > Cheers, > > Paolo >