Re: [PATCH net] sctp: return a one-to-one type socket when doing peeloff

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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@nokia.com> 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