| Message ID | 1623840570-42004-5-git-send-email-wujianguo106@163.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mat Martineau |
| Headers | show |
| Series | Fix some mptcp syncookie process bugs | expand |
On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > From: Jianguo Wu <wujianguo@chinatelecom.cn> > > 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 <wujianguo@chinatelecom.cn> > --- > 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. (It also looks like it unexpected behavior may be possible if we get a strange TCP_RST + MP_JOIN packet when not fully established, but that's unrelated to this patch. Maybe I'll try to create a packetdrill test to see what happens) > mptcp_subflow_reset(ssk); > return false; > } > -- > 1.8.3.1 -- Mat Martineau Intel
Hi Mat, On 2021/6/19 8:19, Mat Martineau wrote: > On Wed, 16 Jun 2021, wujianguo106@163.com wrote: > >> From: Jianguo Wu <wujianguo@chinatelecom.cn> >> >> 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 <wujianguo@chinatelecom.cn> >> --- >> 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? > (It also looks like it unexpected behavior may be possible if we get a strange TCP_RST + MP_JOIN packet when not fully established, but that's unrelated to this patch. Maybe I'll try to create a packetdrill test to see what happens) > >> mptcp_subflow_reset(ssk); >> return false; >> } >> -- >> 1.8.3.1 > > -- > Mat Martineau > Intel
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 <wujianguo@chinatelecom.cn> >>> >>> 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 <wujianguo@chinatelecom.cn> >>> --- >>> 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? The optimizer would probably share instructions with the following "end_seq == seq" condition for the kfree+return path anyway. > >> (It also looks like it unexpected behavior may be possible if we get a >> strange TCP_RST + MP_JOIN packet when not fully established, but that's >> unrelated to this patch. Maybe I'll try to create a packetdrill test to >> see what happens) >> >>> mptcp_subflow_reset(ssk); >>> return false; >>> } -- Mat Martineau Intel
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 <wujianguo@chinatelecom.cn> > > > > > > > > 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 <wujianguo@chinatelecom.cn> > > > > --- > > > > 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 ;) Cheers, Paolo
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 <wujianguo@chinatelecom.cn> >>>>> >>>>> 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 <wujianguo@chinatelecom.cn> >>>>> --- >>>>> 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 >
On Thu, 2021-06-24 at 09:57 +0800, Jianguo Wu wrote: > > 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 <wujianguo@chinatelecom.cn> > > > > > > > > > > > > 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 <wujianguo@chinatelecom.cn> > > > > > > --- > > > > > > 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. Even the hook in tcp_rcv_state_process() can be followed by tcp_data_queue(). I *think* it's better ignoring the return value of mptcp_incoming_options() only in tcp_reset(), adding there a comment - something alike "mptcp can't tell us to ignore reset pkts". Cheers, Paolo p.s. I'm sorry for the long, difficult and somewhat on/off review process. This change is indeed tricky. Don't despair, it looks like it's near to an happy ending!
On Thu, 24 Jun 2021, Paolo Abeni wrote: > On Thu, 2021-06-24 at 09:57 +0800, Jianguo Wu wrote: >> >> 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 <wujianguo@chinatelecom.cn> >>>>>>> >>>>>>> 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 <wujianguo@chinatelecom.cn> >>>>>>> --- >>>>>>> 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. > > Even the hook in tcp_rcv_state_process() can be followed by > tcp_data_queue(). > > I *think* it's better ignoring the return value of > mptcp_incoming_options() only in tcp_reset(), adding there a comment - > something alike "mptcp can't tell us to ignore reset pkts". > This sounds like a good approach, thanks. -- Mat Martineau Intel
On 2021/6/24 18:02, Paolo Abeni wrote: > On Thu, 2021-06-24 at 09:57 +0800, Jianguo Wu wrote: >> >> 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 <wujianguo@chinatelecom.cn> >>>>>>> >>>>>>> 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 <wujianguo@chinatelecom.cn> >>>>>>> --- >>>>>>> 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. > > Even the hook in tcp_rcv_state_process() can be followed by > tcp_data_queue(). > > I *think* it's better ignoring the return value of > mptcp_incoming_options() only in tcp_reset(), adding there a comment - > something alike "mptcp can't tell us to ignore reset pkts". > > Cheers, > > Paolo > > p.s. I'm sorry for the long, difficult and somewhat on/off review > process. This change is indeed tricky. Don't despair, it looks like > it's near to an happy ending! > Thanks for your review and suggestions! I will send a new version. >
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; + mptcp_subflow_reset(ssk); return false; }