diff mbox series

[2/2] ax25: Fix deadlock hang during concurrent read and write on socket.

Message ID 4a2f53386509164e60531750a02480a4c032d51a.1634069168.git.ralf@linux-mips.org (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [v2,1/2] ax25: Fix use of copy_from_sockptr() in ax25_setsockopt() | expand

Checks

Context Check Description
netdev/cover_letter success Single patches do not need cover letters
netdev/fixes_present success Fixes tag not required for -next series
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers warning 1 maintainers not CCed: jreuter@yaina.de
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/module_param success Was 0 now: 0
netdev/build_32bit fail Errors and warnings before: 0 this patch: 2
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success No Fixes tag
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 32 lines checked
netdev/build_allmodconfig_warn fail Errors and warnings before: 0 this patch: 2
netdev/header_inline success No static functions without inline keyword in header files

Commit Message

Ralf Baechle Oct. 12, 2021, 8:05 p.m. UTC 
From: Thomas Habets <thomas@habets.se>

Before this patch, this hangs, because the read(2) blocks the
write(2).

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]

Signed-off-by: Thomas Habets <thomas@habets.se>
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
---
 net/ax25/af_ax25.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Jakub Kicinski Oct. 12, 2021, 10:09 p.m. UTC | #1 
On Tue, 12 Oct 2021 22:05:30 +0200 Ralf Baechle wrote:
> From: Thomas Habets <thomas@habets.se>
> 
> Before this patch, this hangs, because the read(2) blocks the
> write(2).

Still build issues:

net/ax25/af_ax25.c: In function ‘ax25_recvmsg’:
net/ax25/af_ax25.c:1685:1: warning: label ‘out’ defined but not used [-Wunused-label]
 1685 | out:
      | ^~~
net/ax25/af_ax25.c:1685:1: warning: unused label 'out'
kernel test robot Oct. 13, 2021, 7:31 a.m. UTC | #2 
Hi Ralf,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[also build test WARNING on net-next/master horms-ipvs/master linus/master v5.15-rc5 next-20211012]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ralf-Baechle/ax25-Fix-use-of-copy_from_sockptr-in-ax25_setsockopt/20211013-042226
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 4d4a223a86afe658cd878800f09458e8bb54415d
config: x86_64-randconfig-a014-20211012 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project adf55ac6657693f7bfbe3087b599b4031a765a44)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/89cd241b1014e6501130d9116ea6ca367b10dc6a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ralf-Baechle/ax25-Fix-use-of-copy_from_sockptr-in-ax25_setsockopt/20211013-042226
        git checkout 89cd241b1014e6501130d9116ea6ca367b10dc6a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/ax25/af_ax25.c:1685:1: warning: unused label 'out' [-Wunused-label]
   out:
   ^~~~
   1 warning generated.


vim +/out +1685 net/ax25/af_ax25.c

^1da177e4c3f41 Linus Torvalds           2005-04-16  1618  
1b784140474e4f Ying Xue                 2015-03-02  1619  static int ax25_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
1b784140474e4f Ying Xue                 2015-03-02  1620  			int flags)
^1da177e4c3f41 Linus Torvalds           2005-04-16  1621  {
^1da177e4c3f41 Linus Torvalds           2005-04-16  1622  	struct sock *sk = sock->sk;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1623  	struct sk_buff *skb;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1624  	int copied;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1625  	int err = 0;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1626  
^1da177e4c3f41 Linus Torvalds           2005-04-16  1627  	/*
^1da177e4c3f41 Linus Torvalds           2005-04-16  1628  	 * 	This works for seqpacket too. The receiver has ordered the
^1da177e4c3f41 Linus Torvalds           2005-04-16  1629  	 *	queue for us! We do one quick check first though
^1da177e4c3f41 Linus Torvalds           2005-04-16  1630  	 */
^1da177e4c3f41 Linus Torvalds           2005-04-16  1631  	if (sk->sk_type == SOCK_SEQPACKET && sk->sk_state != TCP_ESTABLISHED) {
^1da177e4c3f41 Linus Torvalds           2005-04-16  1632  		err =  -ENOTCONN;
89cd241b1014e6 Thomas Habets            2021-10-12  1633  		goto out_nolock;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1634  	}
^1da177e4c3f41 Linus Torvalds           2005-04-16  1635  
^1da177e4c3f41 Linus Torvalds           2005-04-16  1636  	/* Now we can treat all alike */
^1da177e4c3f41 Linus Torvalds           2005-04-16  1637  	skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
^1da177e4c3f41 Linus Torvalds           2005-04-16  1638  				flags & MSG_DONTWAIT, &err);
^1da177e4c3f41 Linus Torvalds           2005-04-16  1639  	if (skb == NULL)
89cd241b1014e6 Thomas Habets            2021-10-12  1640  		goto out_nolock;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1641  
89cd241b1014e6 Thomas Habets            2021-10-12  1642  	lock_sock(sk);
3200392b88dd25 David Miller             2015-06-25  1643  	if (!sk_to_ax25(sk)->pidincl)
^1da177e4c3f41 Linus Torvalds           2005-04-16  1644  		skb_pull(skb, 1);		/* Remove PID */
^1da177e4c3f41 Linus Torvalds           2005-04-16  1645  
badff6d01a8589 Arnaldo Carvalho de Melo 2007-03-13  1646  	skb_reset_transport_header(skb);
^1da177e4c3f41 Linus Torvalds           2005-04-16  1647  	copied = skb->len;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1648  
^1da177e4c3f41 Linus Torvalds           2005-04-16  1649  	if (copied > size) {
^1da177e4c3f41 Linus Torvalds           2005-04-16  1650  		copied = size;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1651  		msg->msg_flags |= MSG_TRUNC;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1652  	}
^1da177e4c3f41 Linus Torvalds           2005-04-16  1653  
51f3d02b980a33 David S. Miller          2014-11-05  1654  	skb_copy_datagram_msg(skb, 0, msg, copied);
^1da177e4c3f41 Linus Torvalds           2005-04-16  1655  
f3d3342602f8bc Hannes Frederic Sowa     2013-11-21  1656  	if (msg->msg_name) {
^1da177e4c3f41 Linus Torvalds           2005-04-16  1657  		ax25_digi digi;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1658  		ax25_address src;
98e399f82ab3a6 Arnaldo Carvalho de Melo 2007-03-19  1659  		const unsigned char *mac = skb_mac_header(skb);
342dfc306fb321 Steffen Hurrle           2014-01-17  1660  		DECLARE_SOCKADDR(struct sockaddr_ax25 *, sax, msg->msg_name);
^1da177e4c3f41 Linus Torvalds           2005-04-16  1661  
ef3313e84acbf3 Mathias Krause           2013-04-07  1662  		memset(sax, 0, sizeof(struct full_sockaddr_ax25));
98e399f82ab3a6 Arnaldo Carvalho de Melo 2007-03-19  1663  		ax25_addr_parse(mac + 1, skb->data - mac - 1, &src, NULL,
98e399f82ab3a6 Arnaldo Carvalho de Melo 2007-03-19  1664  				&digi, NULL, NULL);
^1da177e4c3f41 Linus Torvalds           2005-04-16  1665  		sax->sax25_family = AF_AX25;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1666  		/* We set this correctly, even though we may not let the
^1da177e4c3f41 Linus Torvalds           2005-04-16  1667  		   application know the digi calls further down (because it
^1da177e4c3f41 Linus Torvalds           2005-04-16  1668  		   did NOT ask to know them).  This could get political... **/
^1da177e4c3f41 Linus Torvalds           2005-04-16  1669  		sax->sax25_ndigis = digi.ndigi;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1670  		sax->sax25_call   = src;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1671  
^1da177e4c3f41 Linus Torvalds           2005-04-16  1672  		if (sax->sax25_ndigis != 0) {
^1da177e4c3f41 Linus Torvalds           2005-04-16  1673  			int ct;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1674  			struct full_sockaddr_ax25 *fsa = (struct full_sockaddr_ax25 *)sax;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1675  
^1da177e4c3f41 Linus Torvalds           2005-04-16  1676  			for (ct = 0; ct < digi.ndigi; ct++)
^1da177e4c3f41 Linus Torvalds           2005-04-16  1677  				fsa->fsa_digipeater[ct] = digi.calls[ct];
^1da177e4c3f41 Linus Torvalds           2005-04-16  1678  		}
^1da177e4c3f41 Linus Torvalds           2005-04-16  1679  		msg->msg_namelen = sizeof(struct full_sockaddr_ax25);
^1da177e4c3f41 Linus Torvalds           2005-04-16  1680  	}
^1da177e4c3f41 Linus Torvalds           2005-04-16  1681  
^1da177e4c3f41 Linus Torvalds           2005-04-16  1682  	skb_free_datagram(sk, skb);
^1da177e4c3f41 Linus Torvalds           2005-04-16  1683  	err = copied;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1684  
^1da177e4c3f41 Linus Torvalds           2005-04-16 @1685  out:
^1da177e4c3f41 Linus Torvalds           2005-04-16  1686  	release_sock(sk);
89cd241b1014e6 Thomas Habets            2021-10-12  1687  out_nolock:
^1da177e4c3f41 Linus Torvalds           2005-04-16  1688  
^1da177e4c3f41 Linus Torvalds           2005-04-16  1689  	return err;
^1da177e4c3f41 Linus Torvalds           2005-04-16  1690  }
^1da177e4c3f41 Linus Torvalds           2005-04-16  1691  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 5e7ab76f7f9b..d2d0dd744bb4 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1624,22 +1624,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) {
 		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);
 	if (skb == NULL)
-		goto out;
+		goto out_nolock;
 
+	lock_sock(sk);
 	if (!sk_to_ax25(sk)->pidincl)
 		skb_pull(skb, 1);		/* Remove PID */
 
@@ -1684,6 +1684,7 @@  static int ax25_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
 
 out:
 	release_sock(sk);
+out_nolock:
 
 	return err;
 }