Date:   Wed, 11 Mar 2020 20:41:09 +0200 (EET)
From:   Jere Leppanen <jere.leppanen@...ia.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
cc:     Xin Long <lucien.xin@...il.com>,
        network dev <netdev@...r.kernel.org>,
        "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Neil Horman <nhorman@...driver.com>,
        "michael.tuexen@...chi.franken.de" <michael.tuexen@...chi.franken.de>
Subject: Re: [PATCH net] sctp: return a one-to-one type socket when doing
 peeloff

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@...ia.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
> 

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.

Powered by blists - more mailing lists