| Message ID | 20201125174956.9222-1-fw@strlen.de |
|---|---|
| Headers | show |
| Series | mptcp: add fastclose option | expand |
Hi Florian, On 25/11/2020 18:49, Florian Westphal wrote: > This series adds receive side support for fastclose processing. > First patch is needed to avoid UAF after enabling option processing > for tcp resets. Second patch calls mptcp option parsing also for > reset patckets so fastclose-on-tcp-reset is handled. > > Last patch adds fastclose handling. > > I've submitted a pull request vs. packetdrill to add test cases for > this: https://github.com/multipath-tcp/packetdrill/pull/25 > > Those are the three test cases i posted earlier to this list plus > a 4th test to also cover the 'two subflows closed via fastclose+reset' case. > > I would send the fastclose tx side next. Thank you for these patches, they are very good! Just added at the end of our tree with my Reviewed-by tag (there were other reviewers on the previous patches, I can of course add more tags if the new version is OK for others): - 6766eaff5c84: mptcp: hold mptcp socket before calling tcp_done - 37a53605cd6b: tcp: parse mptcp options contained in reset packets - 0b9d6f3f5a2b: mptcp: parse and act on incoming FASTCLOSE option - Results: 9e11c1d2b8db..ba6c5ecb4ea5 Tests + export are in progress! > I plan to hold the MPTCP reset patches back for the time being until > netlink + mptcpd are aware of its existence. I certainly missed something here but are you here talking about MP_TCPRST support? Or still about the send of fast-close? If it is about fast-close, why netlink + mptcpd needs to be aware first? Could we eventually send fast-close when the userspace app (here using packetdrill format) does this: setsockopt(<fd>, SOL_SOCKET, SO_LINGER, {onoff=1, linger=0}, 8); close(<fd>); (if we accept this socket option) No rush of course, just in case these patches were already written :) Cheers, Matt
Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > On 25/11/2020 18:49, Florian Westphal wrote: > > I plan to hold the MPTCP reset patches back for the time being until > > netlink + mptcpd are aware of its existence. > > I certainly missed something here but are you here talking about MP_TCPRST > support? Or still about the send of fast-close? Oh, no, I jumped off the fastclose train here. This is only wrt. MP_TCPRST. Another approach would be to add the *send side* for tcprst first, perhaps it helps with tcpdump deciphering. But for RX there is really no point when the kernel can't pass that information along to a consumer such as mptcpd. > If it is about fast-close, why netlink + mptcpd needs to be aware first? > Could we eventually send fast-close when the userspace app (here using > packetdrill format) does this: > > setsockopt(<fd>, SOL_SOCKET, SO_LINGER, {onoff=1, linger=0}, 8); > close(<fd>); > > (if we accept this socket option) > No rush of course, just in case these patches were already written :) Yes, I think it might make sense to allow userspace to generate a fastclose at will. The last iteration I sent autogenerated a fastclose for the 'unread data in socket queue when userspace called close' case, which is analogous to what the linux TCP stack does when you close with unread data in the local socket. But I think extending it to linger makes sense as well.
Hi Florian, On 26/11/2020 15:44, Florian Westphal wrote: > Matthieu Baerts <matthieu.baerts@tessares.net> wrote: >> On 25/11/2020 18:49, Florian Westphal wrote: >>> I plan to hold the MPTCP reset patches back for the time being until >>> netlink + mptcpd are aware of its existence. >> >> I certainly missed something here but are you here talking about MP_TCPRST >> support? Or still about the send of fast-close? > > Oh, no, I jumped off the fastclose train here. This is only wrt. MP_TCPRST. > Another approach would be to add the *send side* for tcprst first, > perhaps it helps with tcpdump deciphering. Good idea yes. MP_TCPRST are not mandatory but can be useful to understand what is going on. > But for RX there is really no point when the kernel can't pass that > information along to a consumer such as mptcpd. Indeed. But if it is easier for the development, we can also just print in debug the received MP_TCPRST. This can be parked in the export branch even if I think it is OK to send them to netdev. >> If it is about fast-close, why netlink + mptcpd needs to be aware first? >> Could we eventually send fast-close when the userspace app (here using >> packetdrill format) does this: >> >> setsockopt(<fd>, SOL_SOCKET, SO_LINGER, {onoff=1, linger=0}, 8); >> close(<fd>); >> >> (if we accept this socket option) >> No rush of course, just in case these patches were already written :) > > Yes, I think it might make sense to allow userspace to generate a > fastclose at will. > > The last iteration I sent autogenerated a fastclose for the 'unread data > in socket queue when userspace called close' case, which is analogous to > what the linux TCP stack does when you close with unread data in the > local socket. Yes, that's not the first case I was thinking when I was thinking about sending FAST_CLOSE but it is a good idea to support that! Cheers, Matt