Page MenuHomeFreeBSD
Differential D30946

pf: getstates: avoid taking the hashrow lock if the row is empty
ClosedPublic
Actions

Authored by kp on Jun 29 2021, 7:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 30, 2:57 AM
Unknown Object (File)
Oct 16 2024, 6:03 AM
Unknown Object (File)
Oct 5 2024, 1:14 PM
Unknown Object (File)
Oct 4 2024, 2:42 PM
Unknown Object (File)
Oct 4 2024, 10:02 AM
Unknown Object (File)
Oct 1 2024, 8:52 PM
Unknown Object (File)
Oct 1 2024, 8:28 PM
Unknown Object (File)
Oct 1 2024, 1:05 PM
View All 56 Files
Subscribers
donner
farrokhi
imp
melifaro

Details

Reviewers
mjg
Group Reviewers
network
pfsense
Commits
rGb43db87c97dd: pf: getstates: avoid taking the hashrow lock if the row is empty
rG1882bdd192bc: pf: getstates: avoid taking the hashrow lock if the row is empty
rGa19ff8ce9b58: pf: getstates: avoid taking the hashrow lock if the row is empty
Summary

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 created this revision.Jun 29 2021, 7:38 PM
Herald added subscribers: melifaro, farrokhi, imp. · View Herald TranscriptJun 29 2021, 7:38 PM
kp requested review of this revision.Jun 29 2021, 7:38 PM
Harbormaster completed remote builds in B40173: Diff 91523.Jun 29 2021, 7:38 PM
kp added a parent revision: D30945: libpfctl: improve DIOCGETSTATESNV size estimation.Jun 29 2021, 7:38 PM
kp added a child revision: D30947: pf: support paging in the DIOCGETSTATESNV call.
mjg accepted this revision.Jun 30 2021, 2:04 PM
This revision is now accepted and ready to land.Jun 30 2021, 2:04 PM
donner added a subscriber: donner.Jun 30 2021, 3:29 PM
donner added inline comments.
sys/netpfil/pf/pf_ioctl.c
5058

Accessing this value without a lock ... may it cause a race condition?

mjg added inline comments.Jun 30 2021, 3:34 PM
sys/netpfil/pf/pf_ioctl.c
5058

It is a data race, but it is tolerable in this case. Absolute worst which can happen is that the row will be skipped when it should not be. But let's say garbage was read. The acquire fence provided by PF_HASHROW_LOCK below makes the previously read possibly bogus state irrelevant as it is forcibly refreshed.

mjg added inline comments.Jun 30 2021, 3:35 PM
sys/netpfil/pf/pf_ioctl.c
5058

That said ideally it would use a data structure which is safe to iterate in presence of modification. I have a rough sketch to implement this, so the state as proposed here should be temporary.

Closed by commit rGa19ff8ce9b58: pf: getstates: avoid taking the hashrow lock if the row is empty (authored by kp). · Explain WhyJul 2 2021, 1:50 PM
This revision was automatically updated to reflect the committed changes.
kp added a commit: rGa19ff8ce9b58: pf: getstates: avoid taking the hashrow lock if the row is empty.
kp removed a child revision: D30947: pf: support paging in the DIOCGETSTATESNV call.Jul 5 2021, 10:10 AM
kp added a commit: rG1882bdd192bc: pf: getstates: avoid taking the hashrow lock if the row is empty.Jul 9 2021, 1:24 PM
kp added a commit: rGb43db87c97dd: pf: getstates: avoid taking the hashrow lock if the row is empty.

Revision Contents
Changeset List

PathSize
sys/
netpfil/
pf/
4 lines

Diff 91665

View Options

sys/netpfil/pf/pf_ioctl.c

Loading...