Page MenuHomeFreeBSD

Fix a race condition in udp_output()
ClosedPublic

Authored by bz on May 21 2019, 7:37 PM.

Details

Summary

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@syzkaller.appspotmail.com
MFC After: 2 months
Event: Waterloo Hackathon 2019

Test Plan

Added a printf, ran syzkaller again and found that after the lock was taken, with dst addr beign set, lport was also set:

...
2019/05/21 19:03:10 executed programs: 754306
2019/05/21 19:03:15 executed programs: 761776
XXX-BZ WE LOST THE RACE 0000000000 0x7b34
2019/05/21 19:03:20 executed programs: 769240
2019/05/21 19:03:25 executed programs: 776684
2019/05/21 19:03:30 executed programs: 784176
2019/05/21 19:03:35 executed programs: 791660
2019/05/21 19:03:40 executed programs: 799141
2019/05/21 19:03:45 executed programs: 806586
2019/05/21 19:03:50 executed programs: 814057
XXX-BZ WE LOST THE RACE 0000000000 0x1172
2019/05/21 19:03:55 executed programs: 821536
2019/05/21 19:04:00 executed programs: 829033
...

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 24363

Event Timeline

bz created this revision.May 21 2019, 7:37 PM
markj accepted this revision.May 22 2019, 2:45 PM

In a comment or in the commit log message, there should be a detailed explanation of why we don't want to take the write lock when this race is lost. It is not at all obvious when looking at the diff itself.

bz edited the summary of this revision. (Show Details)May 22 2019, 3:24 PM
bz removed a reviewer: transport.
This revision is now accepted and ready to land.May 22 2019, 3:24 PM