On Wed, 11 Mar 2020, Marcelo Ricardo Leitner wrote:

On Wed, Mar 04, 2020 at 07:13:14PM +0200, Jere Leppanen wrote:

quoted

On Wed, 4 Mar 2020, Xin Long wrote:

quoted

On Wed, Mar 4, 2020 at 2:38 AM Leppanen, Jere (Nokia - FI/Espoo)
[off-list ref] wrote:

quoted

On Mon, 2 Mar 2020, Xin Long wrote:

quoted

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.

quoted

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)

quoted

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.

quoted

quoted

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.

quoted

quoted

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.

quoted

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.