On Wed, 4 Mar 2020, Xin Long wrote:
On Wed, Mar 4, 2020 at 2:38 AM Leppanen, Jere (Nokia - FI/Espoo)
<jere.leppanen@xxxxxxxxx> wrote:
On Mon, 2 Mar 2020, Xin Long wrote:
As it says in rfc6458#section-9.2:
The application uses the sctp_peeloff() call to branch off an
association into a separate socket. (Note that the semantics are
somewhat changed from the traditional one-to-one style accept()
call.) Note also that the new socket is a one-to-one style socket.
Thus, it will be confined to operations allowed for a one-to-one
style socket.
Prior to this patch, sctp_peeloff() returned a one-to-many type socket,
on which some operations are not allowed, like shutdown, as Jere
reported.
This patch is to change it to return a one-to-one type socket instead.
Thanks for looking into this. I like the patch, and it fixes my simple
test case.
But with this patch, peeled-off sockets are created by copying from a
one-to-many socket to a one-to-one socket. Are you sure that that's
not going to cause any problems? Is it possible that there was a
reason why peeloff wasn't implemented this way in the first place?
I'm not sure, it's been there since very beginning, and I couldn't find
any changelog about it.
I guess it was trying to differentiate peeled-off socket from TCP style
sockets.
Well, that's probably the reason for UDP_HIGH_BANDWIDTH style. And maybe
there is legitimate need for that differentiation in some cases, but I
think inventing a special socket style is not the best way to handle it.
But actually I meant why is a peeled-off socket created as SOCK_SEQPACKET
instead of SOCK_STREAM. It could be to avoid copying from SOCK_SEQPACKET
to SOCK_STREAM, but why would we need to avoid that?
Mark Butler commented in 2006
(https://sourceforge.net/p/lksctp/mailman/message/10122693/):
In short, SOCK_SEQPACKET could/should be replaced with SOCK_STREAM
right there, but there might be a minor dependency or two that would
need to be fixed.
With this patch there's no way to create UDP_HIGH_BANDWIDTH style
sockets anymore, so the remaining references should probably be
cleaned up:
./net/sctp/socket.c:1886: if (!sctp_style(sk, UDP_HIGH_BANDWIDTH) && msg->msg_name) {
./net/sctp/socket.c:8522: if (sctp_style(sk, UDP_HIGH_BANDWIDTH))
./include/net/sctp/structs.h:144: SCTP_SOCKET_UDP_HIGH_BANDWIDTH,
This patch disables those checks. The first one ignores a destination
address given to sendmsg() with a peeled-off socket - I don't know
why. The second one prevents listen() on a peeled-off socket.
My understanding is:
UDP_HIGH_BANDWIDTH is another kind of one-to-one socket, like TCP style.
it can get asoc by its socket when sending msg, doesn't need daddr.
But on that association, the peer may have multiple addresses. The RFC
says (https://tools.ietf.org/html/rfc6458#section-4.1.8):
When sending, the msg_name field [...] is used to indicate a preferred
peer address if the sender wishes to discourage the stack from sending
the message to the primary address of the receiver.
Now I thinking to fix your issue in sctp_shutdown():
@@ -5163,7 +5163,7 @@ static void sctp_shutdown(struct sock *sk, int how)
struct net *net = sock_net(sk);
struct sctp_endpoint *ep;
- if (!sctp_style(sk, TCP))
+ if (sctp_style(sk, UDP))
return;
in this way, we actually think:
one-to-many socket: UDP style socket
one-to-one socket includes: UDP_HIGH_BANDWIDTH and TCP style sockets.
That would probably fix shutdown(), but there are other problems as well.
sctp_style() is called in nearly a hundred different places, I wonder if
anyone systematically went through all of them back when
UDP_HIGH_BANDWIDTH was added.
I think getting rid of UDP_HIGH_BANDWIDTH altogether is a much cleaner
solution. That's what your patch does, which is why I like it. But such a
change could easily break something.
