On Jun 16, 2017, at 4:04 PM, Christoph Paasch <cpaasch(a)apple.com&gt; wrote: Hello Mat, On 15/06/17 - 16:35:17, Mat Martineau wrote: > > Hello, > > A new piece of TCP extension infrastructure was just merged to net-next: > generic upper layer protocol (ULP) support for TCP. > > The relevant commit is https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit... <https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit... thanks for the pointer. That's really interesting. One thing I am wondering is, how this covers against races? Because, one might do the setsockopt() at the same time as calling sendmsg(). The following scenario could happen in my opinion: setsockopt is first, goes all the way until lock_sock() in do_tcp_setsockopt(). Then, sendmsg() gets some CPU-cycles and goes until tcp_sendmsg() (thanks to sk->sk_prot->sendmsg) and blocks on the lock_sock(). Now, back to the setsockopt, which will end up changing sk->sk_prot. setsockopt now finishes and releases the lock. Now we end up with the sendmsg inside tcp_sendmsg(), while it should be in the ULP's sendmsg. This looks extremely unsafe to me. I will check on netdev if there has been discussion about this. Christoph > > It was added as part of in-kernel TLS support. The TLS code didn't initially > have this framework, but Tom Herbert asked them to generalize this feature > when reviewing an early patch set. > > If we use ULP for MPTCP, creating a multipatch socket would look like: > > sock = socket(AF_INET, SOCK_STREAM, 0); > setsockopt(sock, SOL_TCP, TCP_ULP, "mptcp", sizeof("mptcp")); > > instead of: > > sock = socket(AF_INET, SOCK_STREAM, IPPROTO_MPTCP); > > > A case can be made for either interface. MPTCP shares an IP protocol number > with TCP, so IPPROTO_MPTCP is a awkward to define in a way that's consistent > with other AF_INET{,6} protocols. On the other hand, MPTCP is not a layer > above TCP like TLS - however, we have the same need to override some TCP > socket behavior.

> In the end, there's not much difference in functionality, but the > maintainers may favor the ULP approach. Any preferences here on this list? > > -- > Mat Martineau > Intel OTC > _______________________________________________ > mptcp mailing list > mptcp(a)lists.01.org > https://lists.01.org/mailman/listinfo/mptcp _______________________________________________ mptcp mailing list mptcp(a)lists.01.org <mailto:mptcp@lists.01.org> https://lists.01.org/mailman/listinfo/mptcp <https://lists.01.org/mailman/listinfo/mptcp>

On Fri, 16 Jun 2017, Christoph Paasch wrote: > Hello Mat, > > On 15/06/17 - 16:35:17, Mat Martineau wrote: > > > > Hello, > > > > A new piece of TCP extension infrastructure was just merged to net-next: > > generic upper layer protocol (ULP) support for TCP. > > > > The relevant commit is https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit... > > thanks for the pointer. That's really interesting. > > > One thing I am wondering is, how this covers against races? > > Because, one might do the setsockopt() at the same time as calling sendmsg(). > > The following scenario could happen in my opinion: > > setsockopt is first, goes all the way until lock_sock() in do_tcp_setsockopt(). > Then, sendmsg() gets some CPU-cycles and goes until tcp_sendmsg() (thanks to > sk->sk_prot->sendmsg) and blocks on the lock_sock(). Now, back to the setsockopt, > which will end up changing sk->sk_prot. setsockopt now finishes and releases > the lock. > > Now we end up with the sendmsg inside tcp_sendmsg(), while it should be in > the ULP's sendmsg. This looks extremely unsafe to me. Yes, indeed. Glad you spotted that. > > I will check on netdev if there has been discussion about this. I spent some time yesterday looking for any discussion of the ULP patch and the only thing I found in the archives was the original suggestion to create the generalized ULP feature: http://www.mail-archive.com/netdev@vger.kernel.org/msg160375.html