netdev - Re: SCTP doesn't seem to let you 'cancel' a blocking accept()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 7 Jun 2024 10:53:38 -0400
From: Xin Long <lucien.xin@...il.com>
To: David Laight <David.Laight@...lab.com>
Cc: linux-sctp <linux-sctp@...r.kernel.org>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: SCTP doesn't seem to let you 'cancel' a blocking accept()

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