Skip to content

Commit

Permalink
After parts of the locking fixes in r346595, syzkaller found
Browse files Browse the repository at this point in the history
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@syzkaller.appspotmail.com
Reviewed by:	markj
MFC after:	6 weeks
Event:		Waterloo Hackathon 2019
  • Loading branch information
bz authored and bz committed Jun 1, 2019
1 parent 85107dc commit 4dc2772
Showing 1 changed file with 14 additions and 0 deletions.
14 changes: 14 additions & 0 deletions sys/netinet/udp_usrreq.c
Expand Up @@ -1156,9 +1156,23 @@ udp_output(struct inpcb *inp, struct mbuf *m, struct sockaddr *addr,

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);
Expand Down

0 comments on commit 4dc2772

Please sign in to comment.