Page MenuHomeFreeBSD

ipfw: make the upper half lock sleepable
Needs ReviewPublic

Authored by glebius on Mon, Jan 5, 7:38 PM.
Tags
None
Referenced Files
F141981697: D54535.id.diff
Wed, Jan 14, 11:55 AM
Unknown Object (File)
Mon, Jan 12, 2:11 AM
Unknown Object (File)
Sat, Jan 10, 4:17 PM
Unknown Object (File)
Sat, Jan 10, 5:41 AM
Unknown Object (File)
Sat, Jan 10, 12:07 AM
Unknown Object (File)
Thu, Jan 8, 4:20 PM
Unknown Object (File)
Thu, Jan 8, 10:45 AM
Unknown Object (File)
Wed, Jan 7, 12:33 AM

Details

Reviewers
melifaro
ae
lytboris_gmail.com
Group Reviewers
network
Summary

The so called upper half ipfw lock is not used in the forwarding path. It
is used only during configuration changes and servicing system events like
interface arrival/departure or vnet creation. The original code drops the
lock before malloc(M_WAITOK) and then goes into great efforts to recover
from possible races. But the races still exist, e.g. create_table() would
first check for table existence, but then drop the lock. The change also
fixes unlock leak in check_table_space() in a branch that apparently was
never entered.

Changing to a sleepable lock we can reduce a lot of existing complexity
associated with race recovery, and as use the lock to cover other
configuration time allocations, like recently added per-rule bpf(4) taps.

This change doesn't remove much of a race recovery code, to ease bisection
in case of a regression. This will be done in a separate commit. This
change just removes lock drops during configuration events. The only
reduction is removal of get_map(), which is a straightforward reduce to a
simple malloc(9).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 69687
Build 66570: arc lint + arc unit

Event Timeline

  • Fix lock leak in ipfw_commit_rules().
The only sleepable context where the lock was acquired was dyn_tick(). The
comment said it is done to prevent parallel execution of
dyn_expire_states().  However, there is proper internal locking in there
and function should be safe to execute in parallel.  The real problem is
dyn_expire_states() called via userland to race with dyn_grow_hashtable()
called via dyn_tick().  Protect against this condition with the main chain
lock.
sys/netpfil/ipfw/ip_fw_table.c
1079

While you are here, it seems there is missing IPFW_UH_RUNLOCK.

sys/netpfil/ipfw/ip_fw_table.c
1079

I'd better assert that table implementation can find an entry :) Will make a separate commit.