Page MenuHomeFreeBSD

pf: release rules lock before passing the packet to dummynet
ClosedPublic

Authored by kp on May 11 2023, 4:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 4 2024, 3:44 AM
Unknown Object (File)
Mar 4 2024, 3:44 AM
Unknown Object (File)
Mar 4 2024, 3:44 AM
Unknown Object (File)
Mar 4 2024, 3:36 AM
Unknown Object (File)
Jan 10 2024, 2:57 AM
Unknown Object (File)
Dec 21 2023, 3:14 PM
Unknown Object (File)
Dec 12 2023, 6:49 AM
Unknown Object (File)
Nov 25 2023, 2:14 PM

Details

Summary

In the Ethernet rules we held the PF_RULES lock while we called
ip_dn_io_ptr() (i.e. dummynet). That meant that we could end up back in
pf while still holding the PF_RULES lock.
That's not immediately fatal, because that lock is recursive, but still
not ideal.

There also appear to be scenarios where this can actually trigger
deadlocks.

We don't need to hold the PF_RULES lock, as long as we make a local copy
of the data we need from the rule (in this case, the action and
bridge_to target). It's safe to keep the struct ifnet pointer around,
because we remain in NET_EPOCH.

See also: https://redmine.pfsense.org/issues/14373
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kp requested review of this revision.May 11 2023, 4:34 PM

perhaps recursion on read locking should be disallowed here?

This comment was removed by kp.
In D40067#912398, @mjg wrote:

perhaps recursion on read locking should be disallowed here?

Ideally yes. Right now we need it at least for syncookie support, and I wonder if there are scenarios with say gif tunnels where we'd see the same problem. It's something to be addressed separately as well.

This revision is now accepted and ready to land.May 17 2023, 10:17 AM