Page MenuHomeFreeBSD

Fix udp_output() locking strategy in one case

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



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

Original report can be found here:

Test Plan

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

Diff Detail

rS FreeBSD src repository - subversion
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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?

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

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
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 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
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.