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

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

[Xin Long]

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.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Leppanen, Jere (Nokia - FI/Espoo) <jere.leppanen@nokia.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 1b56fc4..2b55beb 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -88,8 +88,7 @@ static int sctp_send_asconf(struct sctp_association *asoc,
 static int sctp_do_bind(struct sock *, union sctp_addr *, int);
 static int sctp_autobind(struct sock *sk);
 static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
-			     struct sctp_association *assoc,
-			     enum sctp_socket_type type);
+			     struct sctp_association *assoc);
 
 static unsigned long sctp_memory_pressure;
 static atomic_long_t sctp_memory_allocated;
@@ -4965,7 +4964,7 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
 	/* Populate the fields of the newsk from the oldsk and migrate the
 	 * asoc to the newsk.
 	 */
-	error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
+	error = sctp_sock_migrate(sk, newsk, asoc);
 	if (error) {
 		sk_common_release(newsk);
 		newsk = NULL;
@@ -5711,7 +5710,7 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
 		return -EINVAL;
 
 	/* Create a new socket.  */
-	err = sock_create(sk->sk_family, SOCK_SEQPACKET, IPPROTO_SCTP, &sock);
+	err = sock_create(sk->sk_family, SOCK_STREAM, IPPROTO_SCTP, &sock);
 	if (err < 0)
 		return err;
 
@@ -5727,8 +5726,7 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
 	/* Populate the fields of the newsk from the oldsk and migrate the
 	 * asoc to the newsk.
 	 */
-	err = sctp_sock_migrate(sk, sock->sk, asoc,
-				SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
+	err = sctp_sock_migrate(sk, sock->sk, asoc);
 	if (err) {
 		sock_release(sock);
 		sock = NULL;
@@ -9453,8 +9451,7 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
  * and its messages to the newsk.
  */
 static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
-			     struct sctp_association *assoc,
-			     enum sctp_socket_type type)
+			     struct sctp_association *assoc)
 {
 	struct sctp_sock *oldsp = sctp_sk(oldsk);
 	struct sctp_sock *newsp = sctp_sk(newsk);
@@ -9562,7 +9559,7 @@ static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 	 * original UDP-style socket or created with the accept() call on a
 	 * TCP-style socket..
 	 */
-	newsp->type = type;
+	newsp->type = SCTP_SOCKET_TCP;
 
 	/* Mark the new socket "in-use" by the user so that any packets
 	 * that may arrive on the association after we've moved it are
-- 
2.1.0

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

[Leppanen, Jere (Nokia - FI/Espoo)]

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?

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.

> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Leppanen, Jere (Nokia - FI/Espoo) <jere.leppanen@nokia.com>

Reported-by: Jere Leppanen <jere.leppanen@nokia.com>

> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 1b56fc4..2b55beb 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -88,8 +88,7 @@ static int sctp_send_asconf(struct sctp_association *asoc,
>  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
>  static int sctp_autobind(struct sock *sk);
>  static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> -			     struct sctp_association *assoc,
> -			     enum sctp_socket_type type);
> +			     struct sctp_association *assoc);
>  
>  static unsigned long sctp_memory_pressure;
>  static atomic_long_t sctp_memory_allocated;
> @@ -4965,7 +4964,7 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	error = sctp_sock_migrate(sk, newsk, asoc);
>  	if (error) {
>  		sk_common_release(newsk);
>  		newsk = NULL;
> @@ -5711,7 +5710,7 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>  		return -EINVAL;
>  
>  	/* Create a new socket.  */
> -	err = sock_create(sk->sk_family, SOCK_SEQPACKET, IPPROTO_SCTP, &sock);
> +	err = sock_create(sk->sk_family, SOCK_STREAM, IPPROTO_SCTP, &sock);
>  	if (err < 0)
>  		return err;
>  
> @@ -5727,8 +5726,7 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	err = sctp_sock_migrate(sk, sock->sk, asoc,
> -				SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	err = sctp_sock_migrate(sk, sock->sk, asoc);
>  	if (err) {
>  		sock_release(sock);
>  		sock = NULL;
> @@ -9453,8 +9451,7 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
>   * and its messages to the newsk.
>   */
>  static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> -			     struct sctp_association *assoc,
> -			     enum sctp_socket_type type)
> +			     struct sctp_association *assoc)
>  {
>  	struct sctp_sock *oldsp = sctp_sk(oldsk);
>  	struct sctp_sock *newsp = sctp_sk(newsk);
> @@ -9562,7 +9559,7 @@ static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	 * original UDP-style socket or created with the accept() call on a
>  	 * TCP-style socket..
>  	 */
> -	newsp->type = type;
> +	newsp->type = SCTP_SOCKET_TCP;
>  
>  	/* Mark the new socket "in-use" by the user so that any packets
>  	 * that may arrive on the association after we've moved it are
> -- 
> 2.1.0

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

[Xin Long]

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.

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

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.

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

[Jere Leppanen]

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.

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.

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

[David Laight]

From: Jere Leppanen
> Sent: 04 March 2020 17:13
> 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.
> 
> 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?

Because you don't want all the acks and retransmissions??

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Jere Leppanen]

On Thu, 5 Mar 2020, David Laight wrote:

> From: Jere Leppanen
> > Sent: 04 March 2020 17:13
> > 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.
> > 
> > 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?
> 
> Because you don't want all the acks and retransmissions??

I don't follow. The socket type and style have virtually no effect on the 
protocol side of things, I think.

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 
> 

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

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Mon,  2 Mar 2020 14:57:15 +0800

> 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.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Leppanen, Jere (Nokia - FI/Espoo) <jere.leppanen@nokia.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

I don't know what to do with this patch.

There seems to be some discussion about a potential alternative approach
to the fix, but there were problems with that suggestion.

Please advise, thank you.

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

[Marcelo Ricardo Leitner]

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

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

[Marcelo Ricardo Leitner]

On Mon, Mar 09, 2020 at 06:09:49PM -0700, David Miller wrote:
> From: Xin Long <lucien.xin@gmail.com>
> Date: Mon,  2 Mar 2020 14:57:15 +0800
> 
> > 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.
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Reported-by: Leppanen, Jere (Nokia - FI/Espoo) <jere.leppanen@nokia.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> 
> I don't know what to do with this patch.
> 
> There seems to be some discussion about a potential alternative approach
> to the fix, but there were problems with that suggestion.
> 
> Please advise, thank you.

Please drop it. As you noticed, we do need more discussions around
it. Thanks.

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

[Jere Leppanen]

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

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.