On Fri, Jun 7, 2024 at 4:36 AM David Laight [off-list ref] wrote:

From: Xin Long

quoted

Sent: 06 June 2024 21:15

On Mon, Jun 3, 2024 at 11:42 AM David Laight [off-list ref] wrote:

quoted

In a multithreaded program it is reasonable to have a thread blocked in accept().
With TCP a subsequent shutdown(listen_fd, SHUT_RDWR) causes the accept to fail.
But nothing happens for SCTP.

I think the 'magic' happens when tcp_disconnect() calls inet_csk_listen_stop(sk)
but sctp_disconnect() is an empty function and nothing happens.

I can't see any calls to inet_csk_listen_stop() in the sctp code - so I suspect
it isn't possible at all.

...

quoted

quoted

I also suspect that a blocking connect() can't be cancelled either?

For connecting socket, it calls sctp_shutdown() where SHUT_WR causes
the asoc to enter SHUTDOWN_SENT and cancel the blocking connect().

I'll test that later - the test I was running always connects.
I'm porting some kernel code that used signals to unblock synchronous
calls to userspace where you can't signal a thread.
The only problem with the kernel version is secure boot and driver
signing (especially for the windows build!).

quoted

quoted

Clearly the application can avoid the issue by using poll() and an
extra eventfd() for the wakeup - but it is all a faff for code that
otherwise straight forward.

I will try to prepare a patch to solve this for sctp accept() like:

I'll test it for you.

quoted

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index c67679a41044..f270a0a4c65d 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c

@@ -4834,10 +4834,13 @@ int sctp_inet_connect(struct socket *sock,

struct sockaddr *uaddr,
        return sctp_connect(sock->sk, uaddr, addr_len, flags);
 }

-/* FIXME: Write comments. */
 static int sctp_disconnect(struct sock *sk, int flags)
 {
-       return -EOPNOTSUPP; /* STUB */
+       if (!sctp_style(sk, TCP))
+               return -EOPNOTSUPP;
+
+       sk->sk_shutdown |= RCV_SHUTDOWN;
+       return 0;

I think you need to call something to unblock the thread as well
as changing the state.

shutdown() will call inet_shutdown()/sk_state_change(sk) to awake the
sleeping thread in sctp_accept()/connect().

In sctp_accept() it checks sk->sk_shutdown & RCV_SHUTDOWN to return,
while in sctp_connect() it check asoc->state >= SCTP_STATE_SHUTDOWN_PENDING
to return.

In inet_shutdown(), only with RCV_SHUTDOWN flag calls .disconnect() for
LISTEN sockets, and SCTP doesn't do many things for RCV_SHUTDOWN, I would
just set this flag to unlock the thread, and leave the real disconnection
to close(listen_sk);

...

quoted

-       if (!sctp_sstate(sk, LISTENING)) {

Any chance of making it much clearer that this is testing
                if (sk->sk_state == TCP_LISTEN)

The token-pasting though
        SCTP_SS_CLOSED         = TCP_CLOSE,
        SCTP_SS_LISTENING      = TCP_LISTEN,
        SCTP_SS_ESTABLISHING   = TCP_SYN_SENT,
        SCTP_SS_ESTABLISHED    = TCP_ESTABLISHED,
        SCTP_SS_CLOSING        = TCP_CLOSE_WAIT,
makes grepping for changes to sk_state pretty impossible.

You might argue that the sk_state values should be protocol neutral,
and that the wrapper gives strong typing - but together they make
the code hard to scan.

The strong typing could be maintained by changing the constants to
        SCTP_SS_TCP_CLOSE = TCP_CLOSE
(etc) so that grepping for the constants still works.

I understand.
I guess the author didn't want to have TCP named things in SCTP.
Maybe it's the inet layer that should use neutral names for states. :D

Thanks.

I keep thinking of ways to do strongly typed enum in C.
The main options seem to be embedding the value in a struct
or using a pointer to a struct.
Neither is ideal.

OTOH the compiler can't default to strongly typed enum.
Although perhaps that could be a per-enum attribute.

        David

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