patch title has a typo : there is no skb lock.
On 7/9/20 12:49 PM, Thomas Habets wrote:
> Here's a test program that illustrates the problem:
> https://github.com/ThomasHabets/radiostuff/blob/master/ax25/axftp/examples/client_lockcheck.cc
>
> Before this patch, this hangs, because the read(2) blocks the
> write(2).
>
> I see that calling skb_recv_datagram without this lock is done in
> pep_sock_accept() and atalk_recvmsg() and others, which is what makes
> me think it's safe to do here too. But I'm far from an expert on skb
> lock semantics.
Again, do not mix skb and socket ;)
>
> I see some other socket types are also locking during
> read. E.g. qrtr_recvmsg. Maybe they need to be fixed too.
>
> Before:
> strace -f -eread,write ./examples/client_lockcheck M0THC-9 M0THC-0 M0THC-2
> strace: Process 3888 attached
> [pid 3888] read(3, <unfinished ...>
> [pid 3887] write(3, "hello world", 11
> [hang]
>
> After:
> strace -f -eread,write ./examples/client_lockcheck M0THC-9 M0THC-0 M0THC-2
> strace: Process 2433 attached
> [pid 2433] read(3, <unfinished ...>
> [pid 2432] write(3, "hello world", 11) = 11
> [pid 2433] <... read resumed> "yo", 1000) = 2
> [pid 2433] write(1, "yo\n", 3yo
> ) = 3
> [successful exit]
>
>
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index fd91cd34f25e..378ee132e4d0 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -1617,22 +1617,22 @@ static int ax25_recvmsg(struct socket *sock,
> struct msghdr *msg, size_t size,
> int copied;
> int err = 0;
>
> - lock_sock(sk);
> /*
> * This works for seqpacket too. The receiver has ordered the
> * queue for us! We do one quick check first though
> */
> if (sk->sk_type == SOCK_SEQPACKET && sk->sk_state != TCP_ESTABLISHED)
> {
This would be racy, but maybe we do not care.
> err = -ENOTCONN;
> - goto out;
> + goto out_nolock;
> }
>
> /* Now we can treat all alike */
> skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
> flags & MSG_DONTWAIT, &err);
Or you could use a smaller change and make this look like net/x25/af_x25.c ?
In any case, this will open gates for syzbot fun I am sure ;)
