On Fri, Jun 7, 2024 at 4:36 AM David Laight <David.Laight@xxxxxxxxxx> wrote:
>
> From: Xin Long
> > Sent: 06 June 2024 21:15
> >
> > On Mon, Jun 3, 2024 at 11:42 AM David Laight <David.Laight@xxxxxxxxxx> wrote:
> > >
> > > 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.
> ...
> > >
> > > 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!).
>
> > > 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.
>
> > 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);
>
> ...
> > - 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)
