On Wed, 11 Mar 2020, Marcelo Ricardo Leitner wrote: > On Wed, Mar 04, 2020 at 07:13:14PM +0200, Jere Leppanen wrote: > > 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. > > Me too. > > > > > 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. > > I agree, but.. (in the end of the email) > > > > > 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. > > Which means the currect check in 1886 is wrong and should be fixed regardless. > > > > > > > > > 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 suppose, and with no grounds, just random thoughts, that > UDP_HIGH_BANDWIDTH is a left-over from an early draft/implementation. > > > > > 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. > > Xin's initial patch here or this without backward compatibility, will > create some user-noticeable differences, yes. For example, in > sctp_recvmsg(): > if (sctp_style(sk, TCP) && !sctp_sstate(sk, ESTABLISHED) && > !sctp_sstate(sk, CLOSING) && !sctp_sstate(sk, CLOSED)) { > err = -ENOTCONN; > goto out; > > And in sctp_setsockopt_autoclose(): > " * This socket option is applicable to the UDP-style socket only. When" > /* Applicable to UDP-style socket only */ > if (sctp_style(sk, TCP)) > return -EOPNOTSUPP; > > Although on RFC it was updated to: > 8.1.8. Automatic Close of Associations (SCTP_AUTOCLOSE) > This socket option is applicable to the one-to-many style socket > only. > > These would start to be checked with such change. The first is a > minor, because that return code is already possible from within > sctp_wait_for_packet(), it's mostly just enforced later. But the > second.. Yes, we're violating the RFC in there, but OTOH, I'm afraid > it may be too late to fix it. > > Removing UDP_HIGH_BANDWIDTH would thus require some weird checks, like > in the autoclose example above, something like: > /* Applicable to one-to-many sockets only */ > if (sctp_style(sk, TCP) && !sctp_peeledoff(sk)) > return -EOPNOTSUPP; > > Which doesn't help much by now. Yet, maybe there is only a few cases > like this around? > > Marcelo > Right, I agree on every point, Marcelo. Weird checks are required regardless of whether UDP_HIGH_BANDWIDTH is removed or not. Either way, it's probably wise to explicitly point out bug compatibility in the code. Removing UDP_HIGH_BANDWIDTH is in some sense cleaner, but on the other hand, not removing it allows for smaller incremental changes. Maybe keeping UDP_HIGH_BANDWIDTH is fine, after all. Less risk. So due to this issue, there are probably multiple unfixable RFC violations in place. I suppose the known problems should at least be documented somewhere.
