[PATCH] sctp: if backlog is 0, listening shall not be deactivated.

[PATCH] sctp: if backlog is 0, listening shall not be deactivated.

[Pierre Habouzit]

POSIX hints that when 0 is used for the listen backlog argument, the
kernel shall chose a default automatic value. TCP for example, works this
way.

Signed-off-by: Pierre Habouzit <pierre.habouzit@intersec.com>
---

 net/sctp/socket.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

    To put a bit of background, I stumbled against this while doing a code
    that basically did:

	struct sctp_event_subscribe events;
	/* ... */

	fd = socket(AF_INET, SOCK_SEQPACKETS, IPPROTO_SCTP);
	sctp_bindx(fd, ....);
	events = (struct sctp_event_subscribe){
	    .sctp_data_io_event     = 1,
	    .sctp_association_event = 1,
	};
	setsockopt(fd, SOL_SCTP, SCTP_EVENTS, &events, sizeof(events));
	listen(fd, 0);
	len = sctp_recvmsg(fd, .....);

    The latter call instead of blocking like I expected returned with
    errno == ENOTCONN.

    I know POSIX doesn't _require_ listen() to accept 0 as a valid backlog,
    but the other listen() implementation I have used in the kernel do, and
    it looks really surprising for the programmer (who really searches the
    error elsewhere).

    Fortunately I had another working code at hand and I managed to find the
    problem resorting to reverting the changes I made to the original code
    line per line (*sigh*).

    I'm unsure if the diff shouldn't do instead:

    if (!backlog)
        backlog = 1;

    I'm not really comfortable around the kernel core ;)


diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index ff0a8f8..da1d96a 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5866,16 +5866,6 @@ SCTP_STATIC int sctp_seqpacket_listen(struct sock *sk, int backlog)
 	if (!sctp_style(sk, UDP))
 		return -EINVAL;
 
-	/* If backlog is zero, disable listening. */
-	if (!backlog) {
-		if (sctp_sstate(sk, CLOSED))
-			return 0;
-
-		sctp_unhash_endpoint(ep);
-		sk->sk_state = SCTP_SS_CLOSED;
-		return 0;
-	}
-
 	/* Return if we are already listening. */
 	if (sctp_sstate(sk, LISTENING))
 		return 0;
@@ -5919,16 +5909,6 @@ SCTP_STATIC int sctp_stream_listen(struct sock *sk, int backlog)
 	struct sctp_sock *sp = sctp_sk(sk);
 	struct sctp_endpoint *ep = sp->ep;
 
-	/* If backlog is zero, disable listening. */
-	if (!backlog) {
-		if (sctp_sstate(sk, CLOSED))
-			return 0;
-
-		sctp_unhash_endpoint(ep);
-		sk->sk_state = SCTP_SS_CLOSED;
-		return 0;
-	}
-
 	if (sctp_sstate(sk, LISTENING))
 		return 0;
 
-- 
1.6.1.161.g5e07b.dirty

Re: [PATCH] sctp: if backlog is 0, listening shall not be deactivated.

[Vlad Yasevich]

Pierre Habouzit wrote:
> POSIX hints that when 0 is used for the listen backlog argument, the
> kernel shall chose a default automatic value. TCP for example, works this
> way.
> 

However SCTP API explicitly states that when the backlog is 0, listening is
disabled.  Here is an excerpt from the draft describing this:

   int listen(int sd,
              int backlog);

   and the arguments are
   sd:  The socket descriptor of the endpoint.
   backlog:  If backlog is non-zero, enable listening else disable
      listening.


So, this is something that spelled out in the draft.

NAK.

-vlad

> Signed-off-by: Pierre Habouzit <pierre.habouzit@intersec.com>
> ---
> 
>  net/sctp/socket.c |   20 --------------------
>  1 files changed, 0 insertions(+), 20 deletions(-)
> 
>     To put a bit of background, I stumbled against this while doing a code
>     that basically did:
> 
> 	struct sctp_event_subscribe events;
> 	/* ... */
> 
> 	fd = socket(AF_INET, SOCK_SEQPACKETS, IPPROTO_SCTP);
> 	sctp_bindx(fd, ....);
> 	events = (struct sctp_event_subscribe){
> 	    .sctp_data_io_event     = 1,
> 	    .sctp_association_event = 1,
> 	};
> 	setsockopt(fd, SOL_SCTP, SCTP_EVENTS, &events, sizeof(events));
> 	listen(fd, 0);
> 	len = sctp_recvmsg(fd, .....);
> 
>     The latter call instead of blocking like I expected returned with
>     errno == ENOTCONN.
> 
>     I know POSIX doesn't _require_ listen() to accept 0 as a valid backlog,
>     but the other listen() implementation I have used in the kernel do, and
>     it looks really surprising for the programmer (who really searches the
>     error elsewhere).
> 
>     Fortunately I had another working code at hand and I managed to find the
>     problem resorting to reverting the changes I made to the original code
>     line per line (*sigh*).
> 
>     I'm unsure if the diff shouldn't do instead:
> 
>     if (!backlog)
>         backlog = 1;
> 
>     I'm not really comfortable around the kernel core ;)
> 
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index ff0a8f8..da1d96a 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -5866,16 +5866,6 @@ SCTP_STATIC int sctp_seqpacket_listen(struct sock *sk, int backlog)
>  	if (!sctp_style(sk, UDP))
>  		return -EINVAL;
>  
> -	/* If backlog is zero, disable listening. */
> -	if (!backlog) {
> -		if (sctp_sstate(sk, CLOSED))
> -			return 0;
> -
> -		sctp_unhash_endpoint(ep);
> -		sk->sk_state = SCTP_SS_CLOSED;
> -		return 0;
> -	}
> -
>  	/* Return if we are already listening. */
>  	if (sctp_sstate(sk, LISTENING))
>  		return 0;
> @@ -5919,16 +5909,6 @@ SCTP_STATIC int sctp_stream_listen(struct sock *sk, int backlog)
>  	struct sctp_sock *sp = sctp_sk(sk);
>  	struct sctp_endpoint *ep = sp->ep;
>  
> -	/* If backlog is zero, disable listening. */
> -	if (!backlog) {
> -		if (sctp_sstate(sk, CLOSED))
> -			return 0;
> -
> -		sctp_unhash_endpoint(ep);
> -		sk->sk_state = SCTP_SS_CLOSED;
> -		return 0;
> -	}
> -
>  	if (sctp_sstate(sk, LISTENING))
>  		return 0;
>  

Re: [PATCH] sctp: if backlog is 0, listening shall not be deactivated.

[Alan Cox]

> However SCTP API explicitly states that when the backlog is 0, listening is
> disabled.  Here is an excerpt from the draft describing this:

POSIX is an established standard, SCTP is a draft proposal. POSIX should
win. The SCTP developers need to bring their draft API into alignment with
POSIX.

They need to fix their draft to use a sockopt or similar to
enable/disable listening.

Re: [PATCH] sctp: if backlog is 0, listening shall not be deactivated.

[Vlad Yasevich]

Alan Cox wrote:
>> However SCTP API explicitly states that when the backlog is 0, listening is
>> disabled.  Here is an excerpt from the draft describing this:
> 
> POSIX is an established standard, SCTP is a draft proposal. POSIX should
> win. The SCTP developers need to bring their draft API into alignment with
> POSIX.
> 
> They need to fix their draft to use a sockopt or similar to
> enable/disable listening.
> 

Here is what POSIX says:

A backlog argument of 0 may allow the socket to accept connections, in which
case the length of the listen queue may be set to an implementation-defined
minimum value.


SCTP API simply chooses to ignore the "may".  It is still fully compliant
with POSIX in this regard.

-vlad

Re: [PATCH] sctp: if backlog is 0, listening shall not be deactivated.

[Alan Cox]

> SCTP API simply chooses to ignore the "may".  It is still fully compliant
> with POSIX in this regard.

Linux chooses the interpretation that zero means one connection. Having a
single protocol variant do different things is not nice because  a lot
of code is designed to handle multiple address families and expect the
same non address handling behaviour.

So while it may be able to claim posix compliance, its not Linux
behaviour, and its relying on a specific interpretation of posix not
being used by the OS.

At the very least the current behaviour of the SCTP code is plain rude.

Re: [PATCH] sctp: if backlog is 0, listening shall not be deactivated.

[Vlad Yasevich]

Alan Cox wrote:
>> SCTP API simply chooses to ignore the "may".  It is still fully compliant
>> with POSIX in this regard.
> 
> Linux chooses the interpretation that zero means one connection. Having a
> single protocol variant do different things is not nice because  a lot
> of code is designed to handle multiple address families and expect the
> same non address handling behaviour.
> 
> So while it may be able to claim posix compliance, its not Linux
> behaviour, and its relying on a specific interpretation of posix not
> being used by the OS.
> 
> At the very least the current behaviour of the SCTP code is plain rude.
> 

I will submit a requirest to change this behavior in the spec, but I am not
terribly optimistic.  This has been specified for a very long time and there
might be applications taking advantage of the ability to shut listing off.

At this time, let's leave this as is.  A well written application should specify
the listen backlog anyway, otherwise it's depending on the "may" language in the
Posix spec and will not get consistent behavior across different systems.

-vlad

Re: [PATCH] sctp: if backlog is 0, listening shall not be deactivated.

[Alan Cox]

> I will submit a requirest to change this behavior in the spec, but I am not
> terribly optimistic.  This has been specified for a very long time and there
> might be applications taking advantage of the ability to shut listing off.
> 
> At this time, let's leave this as is.  A well written application should specify
> the listen backlog anyway, otherwise it's depending on the "may" language in the
> Posix spec and will not get consistent behavior across different systems.

Seems fair enough.