ipx: fix locking regression in ipx_sendmsg and ipx_recvmsg

This fixes an old regression introduced by commit
b0d0d915 (ipx: remove the BKL).

When a recvmsg syscall blocks waiting for new data, no data can be sent on the same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.

This breaks mars-nwe (NetWare emulator):
- the ncpserv process reads the request using recvmsg
- ncpserv forks and spawns nwconn
- ncpserv calls a (blocking) recvmsg and waits for new requests
- nwconn deadlocks in sendmsg on the same socket

Commit b0d0d915 has simply replaced BKL locking with lock_sock/release_sock. Unlike now, BKL got unlocked while sleeping, so a blocking recvmsg did not block a concurrent sendmsg.

Similarly, a potentially sleeping sendmsg() could block calls to recvmsg().

Only keep the socket locked while actually working with the socket data and release it prior to calling skb_recv_datagram() / ipxitf_send().

Signed-off-by Jiri Bohac <[email protected]>

net/ipx/af_ipx.c +1 -0

@@ -1745,12 +1745,16 @@ static int ipx_sendmsg(struct kiocb *iocb, struct socket *sock,
 		memcpy(usipx->sipx_node, ipxs->dest_addr.node, IPX_NODE_LEN);
 	}
 
+	/* releases sk */
 	rc = ipxrtr_route_packet(sk, usipx, msg->msg_iov, len,
 				 flags & MSG_DONTWAIT);
 	if (rc >= 0)

net/ipx/af_ipx.c +2 -2

@@ -1776,20 +1780,21 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
 #ifdef CONFIG_IPX_INTERN
 		rc = -ENETDOWN;
 		if (!ipxs->intrfc)
-			goto out; /* Someone zonked the iface */
+			goto out_release; /* Someone zonked the iface */
 		memcpy(uaddr.sipx_node, ipxs->intrfc->if_node, IPX_NODE_LEN);
 #endif	/* CONFIG_IPX_INTERN */
 
 		rc = __ipx_bind(sock, (struct sockaddr *)&uaddr,
 			      sizeof(struct sockaddr_ipx));
 		if (rc)
-			goto out;
+			goto out_release;
 	}

Does ipxrtr_route_packet() actually sleep while waiting for the network, or is it possible that you only need to change the recvmsg path?

Does ipxrtr_route_packet() actually sleep while waiting for the network, or is it possible that you only need to change the recvmsg path?

Quoted 4 lines

net/ipx/af_ipx.c +1 -1

 	if (sock_flag(sk, SOCK_ZAPPED))
-		goto out;
+		goto out_release;

net/ipx/af_ipx.c static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock, · line 1800

Quoted 4 lines

net/ipx/af_ipx.c +1 -0

+	release_sock(sk);
 	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
 				flags & MSG_DONTWAIT, &rc);
 	if (!skb) {

Same thing here: I think your patch could be simplified if you just release the socket lock before calling skb_recv_datagram and get it back afterwards,

and it would be much simpler if you could show that the lock is not needed at all.

Quoted 2 lines · Arnd Bergmann

Does ipxrtr_route_packet() actually sleep while waiting for the network, or is it possible that you only need to change the recvmsg path?

You're right.
In fact, it can sleep in sock_alloc_send_skb(), but my patch does not fix this - it releases the lock after that.
So let's ignore that for now, I'll send a V2 modifying only ipx_recvmsg().

Quoted 4 lines

Quoted 4 lines

net/ipx/af_ipx.c +1 -1

 	if (sock_flag(sk, SOCK_ZAPPED))
-		goto out;
+		goto out_release;

Quoted 8 lines

net/ipx/af_ipx.c static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock, · line 1800

Quoted 4 lines

net/ipx/af_ipx.c +1 -0

+	release_sock(sk);
 	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
 				flags & MSG_DONTWAIT, &rc);
 	if (!skb) {

Same thing here: I think your patch could be simplified if you just release the socket lock before calling skb_recv_datagram and get it back afterwards,

It would simplify the code a little to just get the lock again. But do we really want to optimize for source code size at the cost of taking locks that are not necessarry?

Quoted 2 lines

and it would be much simpler if you could show that the lock is not needed at all.

At least the ipxitf_insert_socket() inside __ipx_bind() looks like it must be protected not to corrupt the intrfc->if_sklist. I am not familiar with the code, so there may be other things.

I'm more interested in the code structure, in particular this bit

net/ipx/af_ipx.c +4 -2

@@ -1807,8 +1812,10 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
 
        rc = skb_copy_datagram_iovec(skb, sizeof(struct ipxhdr), msg->msg_iov,
                                     copied);
-       if (rc)
-               goto out_free;
+       if (rc) {
+               skb_free_datagram(sk, skb);
+               goto out;
+       }
 
 

after your change mixes coding styles: in some failure cases you just goto a cleanup part, in other cases you do the cleanup before the goto.

If I'm reading the patch correctly, this change has introduced a leak

This fixes an old regression introduced by commit
b0d0d915 (ipx: remove the BKL).

When a recvmsg syscall blocks waiting for new data, no data can be sent on the same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.

This breaks mars-nwe (NetWare emulator):
- the ncpserv process reads the request using recvmsg
- ncpserv forks and spawns nwconn
- ncpserv calls a (blocking) recvmsg and waits for new requests
- nwconn deadlocks in sendmsg on the same socket

Commit b0d0d915 has simply replaced BKL locking with lock_sock/release_sock. Unlike now, BKL got unlocked while sleeping, so a blocking recvmsg did not block a concurrent sendmsg.

Only keep the socket locked while actually working with the socket data and release it prior to calling skb_recv_datagram().

Signed-off-by Jiri Bohac <[email protected]>

net/ipx/af_ipx.c +3 -0

index a0c7536..d0725d9 100644
@@ -1764,6 +1764,7 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
 	struct ipxhdr *ipx = NULL;
 	struct sk_buff *skb;
 	int copied, rc;
+	int locked = 1;
 
 	lock_sock(sk);
 	/* put the autobinding in */

@@ -1790,6 +1791,8 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
 	if (sock_flag(sk, SOCK_ZAPPED))
 		goto out;
 
+	release_sock(sk);
+	locked = 0;
 	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
 				flags & MSG_DONTWAIT, &rc);
 	if (!skb) {

This fixes an old regression introduced by commit
b0d0d915 (ipx: remove the BKL).

When a recvmsg syscall blocks waiting for new data, no data can be sent on the same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.

This breaks mars-nwe (NetWare emulator):
- the ncpserv process reads the request using recvmsg
- ncpserv forks and spawns nwconn
- ncpserv calls a (blocking) recvmsg and waits for new requests
- nwconn deadlocks in sendmsg on the same socket

Commit b0d0d915 has simply replaced BKL locking with lock_sock/release_sock. Unlike now, BKL got unlocked while sleeping, so a blocking recvmsg did not block a concurrent sendmsg.

Only keep the socket locked while actually working with the socket data and release it prior to calling skb_recv_datagram().

Signed-off-by Jiri Bohac <[email protected]>

net/ipx/af_ipx.c +0 -0

index a0c7536..d0725d9 100644

This fixes an old regression introduced by commit
b0d0d915 (ipx: remove the BKL).

When a recvmsg syscall blocks waiting for new data, no data can be sent on the same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.

This breaks mars-nwe (NetWare emulator):
- the ncpserv process reads the request using recvmsg
- ncpserv forks and spawns nwconn
- ncpserv calls a (blocking) recvmsg and waits for new requests
- nwconn deadlocks in sendmsg on the same socket

Commit b0d0d915 has simply replaced BKL locking with lock_sock/release_sock. Unlike now, BKL got unlocked while sleeping, so a blocking recvmsg did not block a concurrent sendmsg.

Only keep the socket locked while actually working with the socket data and release it prior to calling skb_recv_datagram().

Signed-off-by Jiri Bohac <[email protected]>

This fixes an old regression introduced by commit
b0d0d915 (ipx: remove the BKL).

When a recvmsg syscall blocks waiting for new data, no data can be sent on the same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.

This breaks mars-nwe (NetWare emulator):
- the ncpserv process reads the request using recvmsg
- ncpserv forks and spawns nwconn
- ncpserv calls a (blocking) recvmsg and waits for new requests
- nwconn deadlocks in sendmsg on the same socket

Commit b0d0d915 has simply replaced BKL locking with lock_sock/release_sock. Unlike now, BKL got unlocked while sleeping, so a blocking recvmsg did not block a concurrent sendmsg.

Only keep the socket locked while actually working with the socket data and release it prior to calling skb_recv_datagram().

Signed-off-by Jiri Bohac <[email protected]>

This fixes an old regression introduced by commit
b0d0d915 (ipx: remove the BKL).

When a recvmsg syscall blocks waiting for new data, no data can be sent on the same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.

This breaks mars-nwe (NetWare emulator):
- the ncpserv process reads the request using recvmsg
- ncpserv forks and spawns nwconn
- ncpserv calls a (blocking) recvmsg and waits for new requests
- nwconn deadlocks in sendmsg on the same socket

Commit b0d0d915 has simply replaced BKL locking with lock_sock/release_sock. Unlike now, BKL got unlocked while sleeping, so a blocking recvmsg did not block a concurrent sendmsg.

Only keep the socket locked while actually working with the socket data and release it prior to calling skb_recv_datagram().

Signed-off-by Jiri Bohac <[email protected]>