Page MenuHomeFreeBSD

Fix udp_output() locking strategy in one case
ClosedPublic

Authored by bz on Mar 15 2019, 4:09 PM.

Details

Summary

Fix udp_output() lock inconsistency.

In r297225 the initial INP_RLOCK() was replaced by an early
acquisition of an r- or w-lock depending on input variables
possibly extending the write locked area for reasons not entirely
clear but possibly to avoid a later case of unlock and relock
leading to a possible race condition and possibly in order to
allow the route cache to work for connected sockets.

Unfortunately the conditions were not 1:1 replicated (probably
because of the route cache needs). While this would not be a
problem the legacy IP code compared to IPv6 has an extra case
when dealing with IP_SENDSRCADDR. In a particular case we were
holding an exclusive inp lock and acquired the shared udbinfo
lock (now epoch).
When then running into an error case, the locking assertions
on release fired as the udpinfo and inp lock levels did not match.

Break up the special case and in that particular case acquire
and udpinfo lock depending on the exclusitivity of the inp lock.

MFC After: 1 week
Reported-by: syzbot+1f5c6800e4f99bdb1a48@syzkaller.appspotmail.com

Original report can be found here:
https://groups.google.com/d/msg/syzkaller-freebsd-bugs/MHUQhKBFhHA/kBsbeOy-AwAJ

Test Plan

NOT TESTED AT ALL by me, @tuexen said he did; thank you!

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bz created this revision.Mar 15 2019, 4:09 PM

Original commit Phabricator link rS297225

emaste added inline comments.Mar 20 2019, 2:13 PM
sys/netinet/udp_usrreq.c
1275 ↗(On Diff #55107)

I think this comment does not reduce confusion. Is the statement that hash locking update is not correct and that the entire block comment above is wrong?

bz added a reviewer: tuexen.Apr 21 2019, 8:00 PM
tuexen accepted this revision.Apr 22 2019, 10:32 AM

I tested that this patch fixes the issue reported by syzkaller.

sys/netinet/udp_usrreq.c
1280 ↗(On Diff #55107)

No code above this changes addr or sin. So I would just remove the duplicated assignment instead of changing it to a KASSERT.

This revision is now accepted and ready to land.Apr 22 2019, 10:32 AM
karels added inline comments.Apr 22 2019, 12:21 PM
sys/netinet/udp_usrreq.c
1292 ↗(On Diff #55107)

I haven't thought this all the way through, but should this block be earlier, after line 1284? I.e. can both tests be true; if so, maybe we need the write lock?

bz updated this revision to Diff 56487.Apr 22 2019, 2:45 PM
bz edited the summary of this revision. (Show Details)
bz edited the test plan for this revision. (Show Details)

@emaste update the comment a bit.
@tuexen removed the KASSERT and left the sin = to be removed in a separate commit (as it is noise here)
@karels I think with the change we are fine as splitting up the case was for IP_SENDSRCADDR.

This revision now requires review to proceed.Apr 22 2019, 2:45 PM
tuexen accepted this revision.Apr 22 2019, 8:31 PM
This revision is now accepted and ready to land.Apr 22 2019, 8:31 PM
This revision was automatically updated to reflect the committed changes.