svn commit: r348494 - head/sys/netinet

[Bjoern A. Zeeb]

Author: bz
Date: Sat Jun  1 14:57:42 2019
New Revision: 348494
URL: https://svnweb.freebsd.org/changeset/base/348494

Log:
  After parts of the locking fixes in r346595, syzkaller found
  another one in udp_output(). This one is a race condition.
  We do check on the laddr and lport without holding a lock in
  order to determine whether we want a read or a write lock
  (this is in the "sendto/sendmsg" cases where addr (sin) is given).
  
  Instrumenting the kernel showed that after taking the lock, we
  had bound to a local port from a parallel thread on the same socket.
  
  If we find that case, unlock, and retry again. Taking the write
  lock would not be a problem in first place (apart from killing some
  parallelism). However the retry is needed as later on based on
  similar condition checks we do acquire the pcbinfo lock and if the
  conditions have changed, we might find ourselves with a lock
  inconsistency, hence at the end of the function when trying to
  unlock, hitting the KASSERT.
  
  Reported by:	syzbot+bdf4caa36f3ceeac198f at syzkaller.appspotmail.com
  Reviewed by:	markj
  MFC after:	6 weeks
  Event:		Waterloo Hackathon 2019

Modified:
  head/sys/netinet/udp_usrreq.c

Modified: head/sys/netinet/udp_usrreq.c
==============================================================================
--- head/sys/netinet/udp_usrreq.c	Sat Jun  1 14:39:12 2019	(r348493)
+++ head/sys/netinet/udp_usrreq.c	Sat Jun  1 14:57:42 2019	(r348494)
@@ -1156,9 +1156,23 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct s
 
 	src.sin_family = 0;
 	sin = (struct sockaddr_in *)addr;
+retry:
 	if (sin == NULL ||
 	    (inp->inp_laddr.s_addr == INADDR_ANY && inp->inp_lport == 0)) {
 		INP_WLOCK(inp);
+		/*
+		 * In case we lost a race and another thread bound addr/port
+		 * on the inp we cannot keep the wlock (which still would be
+		 * fine) as further down, based on these values we make
+		 * decisions for the pcbinfo lock.  If the locks are not in
+		 * synch the assertions on unlock will fire, hence we go for
+		 * one retry loop.
+		 */
+		if (sin != NULL && (inp->inp_laddr.s_addr != INADDR_ANY ||
+		    inp->inp_lport != 0)) {
+			INP_WUNLOCK(inp);
+			goto retry;
+		}
 		unlock_inp = UH_WLOCKED;
 	} else {
 		INP_RLOCK(inp);