e6000sw_write_xmdio() loaded the Clause 45 register address into the
SMI PHY data register a second time, where the value to write belongs,
so every Clause 45 write stored the register address.
Its only caller, e6000sw_serdes_power(), runs at attach for ports 9 and
10 of the 88E6190 and 88E6190X to power one SERDES interface up and the
other down. It wrote 0x2000 to the SGMII control register and 0x1000
to the 10GBASE-R PCS control register instead, so neither interface was
powered down and the rest of each register was overwritten.
Fixes: 9aba06377d ("Add support for the Marvell 88E6190 11 ports switch.")
Sponsored by: Rubicon Communications, LLC ("Netgate")
When a receive ring is full the hardware drops the packet and counts it
in RQDPC, a per-queue register the driver never read. Such drops were
invisible: MPC counts packet buffer overflows, not descriptor
exhaustion, so an overloaded queue lost packets with nothing to show for
it. With several receive queues this is the common overload signature,
since RSS loads the queues unevenly and one saturated queue drops while
the others idle.
Read RQDPC for each queue with the other statistics and account the
drops per queue as dev.igc.N.queue_rx_M.rx_drops and in total as the
interface's input queue drop counter (netstat -d, Idrop), as ixgbe does
with QPRDC. Write the register back to zero after reading; the I210
documents it as read/write rather than read-to-clear.
Reviewed by: kbowling
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D61004
ip6_tryforward() sends time exceeded for an expiring packet before the
inbound pfil hooks have seen it, whereas ip_tryforward() and
ip6_forward(), which ip6_input() calls after the hooks, check it
afterwards. On the fast path the router therefore answers packets the
firewall blocks inbound, and pf never handles the expiry itself: a
nat64 (af-to) packet with hop limit 1 gets a time exceeded error from
the router, which a "block" ruleset then drops on the way out, instead
of the one pf_translate_af() sends with the untranslated addresses.
Check the hop limit after the inbound hooks.
Reviewed by: kp
Fixes: 5a1842a2 ("Add ip6_tryforward() - a run to completion ...")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D61034
ip6_forward() has run the outbound hooks with pfil_mbuf_fwd() since
b52b61c0b6, and ip_tryforward() since 1f4c3887e3, but
ip6_tryforward() still calls pfil_mbuf_out(). pf therefore handles a
packet on the IPv6 fast path as locally generated: it skips the
packet-too-big error it sends with the untranslated addresses, and
pf_refragment6() sends reassembled packets out through ip6_output()
instead of ip6_forward(), so their fragments leave with the hop limit
not decremented.
Use pfil_mbuf_fwd() in ip6_tryforward() as well. The new
fragmentation_pass:v6_refrag_hlim test sends a packet that pf
reassembles and refragments through the fast path, and checks that its
fragments leave with the hop limit decremented once; before this change
they leave with it unchanged.
Reviewed by: kp
Fixes: b52b61c0 ("pf: distinguish forwarding and output cases for ...")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D61033
pf_state_key_attach() attaches the wire key first and then the stack
key, setting sks to NULL before the second pass. When the stack key
collides with an existing state, the error path removes the wire key
again but then frees sks, which is NULL by then: the stack key leaks.
pfsync hits this. A peer that gets a plain state in a bulk update
before a redirected state whose stack key is the plain state's key
fails to import the redirected state, leaks its stack key, and leaks
another one with every later update of that state.
Free sk, the key of the pass that collided (skw on the wire pass), and
add a test.
Reviewed by: kp
Fixes: 371bd29d4b ("pf: fix potential state key leak")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60951
hashalloc(9) rounds the requested size down to a power of two and writes
the result back, but pf_initialize() stored it into the mask, which the
next line overwrites from the unrounded size. With a
net.pf.source_nodes_hashsize that is not a power of two the mask is
larger than the table: pf_hashsrc() returns rows past its end, the row
walkers in pf_purge_expired_src_nodes() and the ioctl handlers run past
it, and pf_cleanup() frees it with the wrong size. The key and UDP
endpoint hashes already take the rounded size.
Reviewed by: glebius, kp
Fixes: cd5ff4e841 ("pf: use hashalloc(9) for key, id, src-node and udp-endpoint hashes")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60949
pf_init_pdesc() cleared the whole structure, 336 bytes on amd64, for
every packet and twice for a forwarded one. kif, m, pf_mtag, act and
the SCTP job list are set before they are read, so move them to the end
and clear only the 240 bytes before them. On amd64 that also stays
below the 256 bytes from which memset() switches to rep stosb.
Reviewed by: kp
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60948
iflib_parse_header() pulls every IPv4 packet up to the end of the TCP or
UDP header, including fragments at a non-zero offset, which carry no L4
header at all. A trailing fragment shorter than that header (1-7 bytes
of UDP, 1-19 bytes of TCP) makes m_pullup() fail and the frame is freed
without a trace in the interface counters.
Only parse the L4 header on the first fragment. TSO never produces
fragments, so assert that.
With a 1500-byte MTU every UDP datagram of 1473-1479 bytes is lost
behind a forwarding host whose ingress NIC sets receive checksum flags,
or on any VLAN with hardware tagging. SIP over UDP is a common victim.
Reported by: Alex Roberts
Reviewed by: gallatin
Fixes: c224b2ce7d ("iflib: don't pullup UDP payloads to the TCP header size")
See also: https://redmine.pfsense.org/issues/17196
MFC after: 3 days
Sponsored by: Rubicon Communications, LLC ("Netgate")
libpfctl sends a table definition in chunks of 256 addresses, each as
its own PFNL_CMD_INA_DEFINE. The kernel treated every chunk as a new
definition and replaced the shadow table, so only the last chunk
survived.
Mark the chunks after the first with PF_ID_APPEND and have the kernel
add them to the shadow table in progress. A request without the
attribute still replaces the shadow; one with it and no definition in
progress fails with EINVAL. Old kernels ignore the attribute, so both
halves are needed; the ioctl sends all addresses at once and is not
affected. Mask the flags userland passes to PFR_FLAG_ALLMASK, as
PFR_FLAG_APPEND is kernel-internal.
Add a test that defines a table of 1,000 addresses, also in an anchor.
Reviewed by: kp
Fixes: 219ce30638 ("pf: convert DIOCRINADEFINE to netlink")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60876
Since 83641335f9 only jail removal destroys exporters. Unloading
pflow with one left trips the assertion in vnet_pflowdetach() or,
without INVARIANTS, leaves pf calling into the freed module the next
time it removes a state that pflow exports.
Reviewed by: kp
Fixes: 83641335f9 ("pf: clean up pflow sockets on jail removal")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
pfctl_ina_define() returns an error number instead of setting errno, but
both of its callers print errno. A table that failed to load was
reported as "cannot define table t: No error: 0", or with a stale errno,
both in the main ruleset (process_tabledef()) and in anchors
(pfctl_load_tables()).
Reviewed by: kp
Fixes: 219ce30638 ("pf: convert DIOCRINADEFINE to netlink")
Sponsored by: Rubicon Communications, LLC ("Netgate")
pfctl_flush_fingerprints() returns an error number instead of setting
errno, so a failed flush was reported with whatever errno held: "No
error: 0" on a kernel with ALTQ, ENODEV from the ALTQ probe on one
without. Every "pfctl -f" flushes the fingerprints first, so on a
kernel that predates PFNL_CMD_OSFP_FLUSH every ruleset load fails this
way without saying why.
Reviewed by: kp
Fixes: 75c0a83762 ("pf: convert DIOCOSFPFLUSH DIOCOSFPGET DIOCOSFPADD to netlink")
Sponsored by: Rubicon Communications, LLC ("Netgate")
When the driver's isc_rxd_pkt_get() fails part way through a receive
pass, iflib_rxeof() jumps straight to the reset request. The frames it
already took off the ring and chained for if_input() are neither passed
up nor freed, so up to budget - 1 frames are lost and their mbufs and
clusters leak on every such error, and the frames queued to LRO in the
pass wait for the next one. Leave the loop instead, pass the chain up
and flush LRO as on the normal exit, then request the reset as before,
without refilling.
The path is reachable from several drivers: igc, em, igb and ixgbe
return EBADMSG for frames received with errors, which the hardware
passes up only with store-bad-packets enabled; ixl, iavf and ice for
every descriptor flagged RXE; aq, enetc, mgb and enic for descriptors
they cannot use.
Reviewed by: gallatin
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60805
pfr_remove_kentry() returns without removing an entry whose mark is
set. The mark is scratch space for the operations that handle a list
of addresses: each resets it before use and leaves it set on the
entries it kept, so after "pfctl -T replace" or "pfctl -T zero" of an
address the entry stays marked. An address a source limiter put in
its table is then never taken out again when the source falls below
the low-water mark.
Remove the entry whether it is marked or not; nothing else is in the
work queue.
Add a regression test.
Reviewed by: kp
Fixes: 4616481212 ("pf: introduce source and state limiters")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60530
epoch_drain_callbacks() bound the calling thread to each CPU in turn
to queue a callback on that CPU's record. Callers are often user
threads, such as kldunload or ifconfig destroy, and ULE does not
preempt a CPU-bound thread for a migrating thread of no better
priority, so each bind could wait out a time slice. On an 8-CPU VM
with six busy loops running, destroying an epair interface took 1.5 s
and kldunload pflow 0.8 s.
Instead, set a request flag on each record and enqueue that CPU's
callback task, which queues the callback before it polls the record.
The task runs on the CPU's softirq gtaskqueue thread at PI_SOFT, which
preempts a busy user thread, so the request is served at once; the
caller no longer migrates. Both operations now take under 10 ms, idle
or under load.
Reviewed by: markj
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60646
Cover the interface identifier selection in in6_ifadd(): a /64
link-local address provides the identifier, while a /128 link-local
address or no link-local address at all yields the EUI-64 identifier
derived from the MAC address. Each case asserts the exact
autoconfigured address. ndp_prefix_len_mismatch reaches the same
fallback with a /127 link-local address, but only checks that some
address was configured.
Stable addresses (RFC 7217) are the default and bypass both paths, so
the host disables them. The advertisement goes to all nodes, and an
interface only joins that group with its first address, so the case
without a link-local address gives the host a static address from
another prefix.
ra.py now takes the Ethernet source address from --sendif rather than
from a route lookup; for ff02::1 that lookup fails in scapy 2.7.0.
Reviewed by: pouria
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60520
pf_modevent() registers the generic netlink family at MOD_LOAD, before
vnet_pf_init() has set up the state hashes and locks the handlers use,
and pf_unload() unregisters it after vnet_pf_uninit() has torn them
down. Sending PFNL_CMD_GETSTATES while pf loads or unloads panics in
pf_handle_getstates() on a NULL or freed V_pf_idhash.
Register the family from a SYSINIT ordered after vnet_pf_init(), and
unregister it from the matching SYSUNINIT, before vnet_pf_uninit().
Reviewed by: kp
Fixes: 2cef62886d ("pf: convert state retrieval to netlink")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60651
pf_hash_pool() hashes the first address of a pool as a whole struct
pf_addr_wrap, including the union that holds the kernel's pfi_dynaddr
or pfr_ktable pointer.
An interface pool such as "-> ($ext_if)" gets a new pfi_dynaddr on
every load, so with "set keepcounters" the rule never matches its
predecessor and loses its counters on every reload. The pointers also
differ between hosts: once a filter rule in the main ruleset has an
interface or table pool, pfsync peers disagree on the ruleset checksum
and bind every synced state to the default rule.
Hash the address with that union cleared. The rest of the address,
including the prefix length of an interface address, is hashed as
before, and the hash of a rule with a plain address pool does not
change.
Reviewed by: kp
Fixes: c6bcf6e6fd ("pf: include all elements when hashing rules")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60370
When an ICMPv6 query or reply matches a translated state,
pf_test_state_icmp() copies the state's virtual id into icmp6_id. Only
echo messages have an identifier there. For all other types the
virtual id is 0 and those two bytes hold something else; in a neighbour
advertisement they hold the R, S and O flags. The checksum was not
updated either, so icmp6_input() dropped the advertisement as a bad
checksum.
Unicast NUD probes to a link-local neighbour, such as the default
router, are sent from the link-local address. With a nat rule that
matches it (e.g. "from any"), every advertisement answering a probe was
dropped: NUD failed, the neighbour cache entry was deleted, and traffic
to the neighbour stalled until it was resolved again.
Only rewrite the id of echo messages, as the ICMPv6 error path already
does, and update the checksum, as the IPv4 path does.
Add a regression test.
Reviewed by: kp
Fixes: f1ddd7f1da ("pf: add forgotten fixup for icmp6 id's when translating")
See also: https://redmine.pfsense.org/issues/16236
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60542
pfr_insert_kentry() returned the error of pfr_route_kentry() and
leaked the entry it had created; pfr_insert_kentries() stopped at the
first entry it could not route and leaked it and the rest of the work
queue, which no caller frees. Destroy an entry that cannot be routed
and go on with the next.
Reviewed by: kp
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60532
The GETSTATES dump sends PF_ST_ID as a u64, but the request parser
reads it as a u32 and rejects an id taken from the dump with EINVAL.
Nothing in base sends PF_ST_ID. Parse and document it as a u64.
Reviewed by: kp
Fixes: 2cef62886d ("pf: convert state retrieval to netlink")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60543
The kernel sends PF_STP_MAX_WIN and PF_STP_MSS for both peers of a
state, but struct pfctl_state_peer has no fields for them and libpfctl
dropped them. Add the fields and parse the attributes.
Reviewed by: kp
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60544
DIOCSETREASS stored V_pf_status.reass twice with no lock, first the
flags it was given and then, if reassembly was not among them, zero.
pf_normalize_ip() and pf_normalize_ip6() read the flags with the rules
lock held for reading, so a packet could see the intermediate value.
Compute the flags first and store them once, with the rules lock held
for writing, as DIOCSETDEBUG and DIOCSETHOSTID do.
Reviewed by: kp
Fixes: 39282ef356 ("pf: backport OpenBSD syntax of "scrub" option for "match" and "pass" rules")
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60531
ndp_prefix_len_mismatch exercises the fallback in in6_ifadd() for an
interface whose link-local address does not have the advertised prefix
length (9e792f7ef7). Since a2eb0894b7 made stable addresses
(RFC 7217) the default, in6_ifadd() takes the interface identifier from
in6_get_stableifid() and never looks at the link-local address, so the
case passes without reaching the fallback. Turn stable addresses off
on the interface.
Sponsored by: Rubicon Communications, LLC ("Netgate")
Readers check rms->writers without a fence, and rms_wunlock() drops
it with a plain store. rms_wlock() interrupts every CPU before the
first writer blocks readers, but nothing orders them when the last
writer leaves: on a weakly ordered CPU a reader can see the count
reach zero and still read what the writer replaced, such as callbacks
of a module unloaded under the lock.
Fence every CPU before the last writer drops the count: rename
rms_wlock_switch() to rms_block_readers() and add
rms_unblock_readers(), a wrapper for cpus_fence_seq_cst(). Readers
stay fence-free; the cost lands on the rare unlock.
Reviewed by: mjg
Discussed with: kib
Fixes: 1f162fef76 ("Add read-mostly sleepable locks")
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60658
pf_source_used() puts a source in the table of its source limiter when
the source has more states than the high-water mark, and
pf_source_rele() takes it out again below the low-water mark. Both
changed the table in place: pf_source_used() from pf_create_state(),
with the rules lock held for reading, and pf_source_rele() from
pf_remove_state(), with no rules lock at all. A table may only be
changed with the rules lock held for writing, as packets look addresses
up in its radix tree at the same time. With INVARIANTS the first
insertion panics ("Lock pf rulesets not exclusively locked" in
pfr_route_kentry()); without, the tree can be corrupted.
Queue the changes instead, as pf_overload_task() does for overload
tables, and make them from a task with the rules lock held for writing.
An entry names the limiter by its id and remembers its table: the
limiter, or its table, may be gone or replaced by the time the task
runs, and then the entry is dropped. pfsr_intable changes only once
the change is queued, so a failed allocation is retried the next time
the source crosses a mark; a removal is also retried when the source is
purged. Drain the task when a vnet goes away, after its last state.
Add a regression test.
Reviewed by: kp
Fixes: 4616481212 ("pf: introduce source and state limiters")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60526
pf_src_connlimit() queues an entry for pf_overload_task() when a
source exceeds max-src-conn or max-src-conn-rate and the rule has an
overload table. The entry holds a pointer to the rule, and the task
runs from taskqueue_swi with the vnet of the queue. Nothing drained
the task when the vnet went away: an entry queued just before could
be applied to a rule pf_unload_vnet_purge() had freed, or to a vnet
that was gone.
Drain the task at the start of pf_unload_vnet_purge(), before the
unlinked rules are freed. No packet can queue another by then, as the
hooks are already removed.
Reviewed by: kp
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60529
When a message outgrows the writer's buffer, _nlmsg_refill_buffer()
allocates a larger one and moves the unfinished message there. The new
buffer was sized for the data about to be added alone, so with enough of
the message already written the caller wrote past its end and left it
with datalen > buflen. A 20-byte message plus an attribute of 8176
bytes got a buffer of 8192 bytes for 8200 bytes of data, and reading it
panicked an INVARIANTS kernel in nl_soreceive().
Size the new buffer for both, and add a ktest for it.
Reviewed by: glebius
Fixes: 7e5bf68495 ("netlink: add netlink support")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60541
The function went when DIOCRINADEFINE was converted to netlink; pfctl
calls pfctl_ina_define() from libpfctl instead. Name that one in the
error message that pfctl_load_tables() prints when it fails.
Fixes: 219ce30638 ("pf: convert DIOCRINADEFINE to netlink")
Sponsored by: Rubicon Communications, LLC ("Netgate")
The counts of a table definition were stored only if the caller's
variables already held a non-zero value, and read uninitialised ones
otherwise. pfctl_ina_define() passes the address of two uninitialised
locals and sums them up.
Reviewed by: kp
Fixes: 219ce30638 ("pf: convert DIOCRINADEFINE to netlink")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60614
if_gre(4) registers net.link.gre with the static OID number IFT_TUNNEL.
That number is an interface type that several tunnel drivers share, not
one that belongs to gre(4). if_me(4) used it too, which made loading
both modules panic until 21f2245de2. Nothing addresses the node by
number, so use OID_AUTO as if_me(4) now does.
Suggested by: markj
MFC after: 3 days
Sponsored by: Rubicon Communications, LLC ("Netgate")
if_me(4) registers its net.link.me node with the static OID number
IFT_TUNNEL, the same number if_gre(4) has used for net.link.gre since
2003, so the two tunnel drivers collide under net.link. Until
d35c4cfad5 that only printed a warning and left two nodes with the
same number; since then sysctl_register_oid() panics, so loading if_me
after if_gre (or the other way round) takes the box down:
kldload if_gre
kldload if_me
panic: sysctl: OID number(131) is already in use for 'me'
Use OID_AUTO like the other tunnel drivers. Nothing addresses the node
by number.
Reviewed by: ae
MFC after: 3 days
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60536
Without a suitable link-local address, in6_ifadd() obtains the
interface identifier from in6_get_ifid(). It wrote the result into a
freshly allocated struct in6_ifaddr, M_NOWAIT since the function runs
in the network epoch, so SLAAC could fail under memory pressure over a
16 byte scratch buffer.
Write the identifier into the on-stack address the rest of the function
uses instead. Zero it first, in6_get_ifid() only fills in the low 64
bits, as the M_ZERO allocation did. Track which path provided the
identifier with a flag instead of a pointer into the ifaddr, so each
path releases its own reference.
Reviewed by: glebius
Fixes: 9e792f7ef7 ("sys/netinet6: Fix SLAAC for interfaces with no /64 LL address")
Fixes: f9fc93690a ("sys/netinet6: fix memory corruption in in6_ifadd")
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60488
Add an "autotrim" option taking "on" or "off". The value is stored in
the pool properties object alongside "bootfs", so the imported pool
starts trimming freed space automatically without a separate
"zpool set" step. When the option is omitted nothing is written and
the pool keeps the default ("off"), exactly as before.
A string option is used rather than makefs's boolean option type since
the latter ignores its value, which would make "autotrim=off" enable
the feature.
Define the spa_autotrim_t enumeration in the boot zfsimpl.h, copied
from OpenZFS's sys/spa.h, so that makefs can refer to SPA_AUTOTRIM_OFF
and SPA_AUTOTRIM_ON by name. The loader does not use it.
Document the option in makefs(8) and add a regression test.
Reviewed by: markj
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60420
dev.em.N.fc and dev.igb.N.fc read and wrote a function-static variable
shared by every em(4) and igb(4) device, so a read returned the last
value written to any of them (3 until the first write), not the state of
the device. The softc value started as 0, which is also the value of
"no flow control", while the hardware was set up for full flow control.
As a result:
- Writing 0 was taken for no change and did nothing, unless another
value had been written to that device before.
- em_reset() took a softc value of 0 for "not set", so a device set to 0
went back to full flow control on the next init.
- On igb(4) with more than one receive queue the driver enabled
per-queue drop (SRRCTL.DROP_EN), which is meant for a MAC that does
not send pause frames, although the MAC was told to send them.
- Values out of range were accepted and ignored.
A write only forced the MAC's flow control bits. The pause bits
advertised to the link partner, the pause thresholds and DROP_EN stayed
as the last init had set them, and the next link event resolved flow
control from the stale advertisement again. On 82577 and 82578 (pchlan)
a write could also turn on transmit flow control, which em_reset() keeps
off there as a workaround.
Start the softc value as full, report it, use it as it is in em_reset(),
reject invalid values, and apply a change by reinitializing the
interface, as the dmac and eee_control sysctls do. This is the same fix
as 68386f6b55 ("igc: Fix the flow control sysctl"); igc(4) inherited
the handler from this driver. On pchlan, remove transmit pause from the
requested mode instead of replacing the mode with receive pause, so that
the workaround stays and 0 still turns flow control off.
Reviewed by: kbowling
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60376
Invert the test so the doorbell write is no longer nested inside the
conditional, and wrap its comments to 80 columns. Fix a typo in one of
them.
No functional change intended.
Reviewed by: kbowling
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60371
For a lightly used ring iflib_txd_db_check() may defer zero descriptors,
so its "pending >= limit" test is true even when nothing has been queued
since the last doorbell. iflib_txq_drain() calls it before, inside and
after its loop, so a sender that drains its own packet wrote the tail
register three times per packet, twice with the value the hardware
already had.
The log of 81be655266 ("iflib: ensure that tx interrupts enabled and
cleanups") calls skipping the doorbell when db_pending is zero "an
obvious missing optimization"; the comparison against a limit of zero
defeated it. vmx(4) and mgb(4) have dropped such repeated requests in
the driver since 2019. Return early when nothing is pending.
Reviewed by: gallatin
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60290
dev.igc.N.fc read and wrote a function-static variable shared by every
igc device, so a read returned the last value written to any of them (3
until the first write), not the state of the device. The softc value
started as 0, which is also the value of "no flow control", while the
hardware was set up for full flow control. As a result:
- Writing 0 was taken for no change and did nothing, unless another
value had been written to that device before.
- igc_reset() took a softc value of 0 for "not set", so a device set to
0 went back to full flow control on the next init.
- With more than one receive queue the driver enabled per-queue drop
(SRRCTL.DROP_EN), which is meant for a MAC that does not send pause
frames, although the MAC was told to send them.
- Values out of range were accepted and ignored.
A write only forced the MAC's flow control bits. The pause bits
advertised to the link partner, the pause thresholds and DROP_EN stayed
as the last init had set them, and the next link event resolved flow
control from the stale advertisement again: a port set to 0 came back
honoring pause frames, for example.
Start the softc value as full, report it, use it as it is in
igc_reset(), reject invalid values, and apply a change by
reinitializing the interface, as the dmac and eee_control sysctls do.
Two things behave differently now. Changing the mode of a running
interface takes the link down to renegotiate. And with the default
mode and more than one receive queue DROP_EN is no longer set: a full
receive ring makes the MAC send pause frames, and holds up the other
queues, where it used to drop that queue's packets. Setting fc to 0 or
1 selects per-queue drop.
Tested on I226-V ports with four receive queues: for each mode, the
flow control bits in CTRL, the pause thresholds and DROP_EN were read
back, and the mode the link partner resolved, also after a down/up.
Reviewed by: gallatin
Fixes: 517904de5c ("igc(4): Introduce new driver for the Intel I225 Ethernet controller.")
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60363
Nothing in the tree calls any of the three. Remove them rather than
fix them: pfctl_get_states_h() does the same with the handle that the
caller has. pfctl_free_states(), struct pfctl_states and the list
entry in struct pfctl_state were there for pfctl_get_states() alone,
and go with it.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: 2a478dfc7f ("libpfctl: retrieve family id only once")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60248
rss_gethashconfig() is available without options RSS since d9c55b2e8c.
Use it to program MRQC, as ixl(4), ice(4) and iavf(4) do, instead of a
fixed field set that included UDP 4-tuple, which the configuration
excludes unless net.inet.rss.udp_4tuple is set. UDP is now hashed on
addresses only by default.
Also report the UDP hash types on receive; they were passed up as
M_HASHTYPE_NONE.
Reviewed by: kbowling
Fixes: 517904de5c ("igc(4): Introduce new driver for the Intel I225 Ethernet controller.")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60284
When softclock_call_cc() fails to acquire the lock of a CALLOUT_TRYLOCK
callout, it reschedules the callout half its precision after
cc_lastscan and halves the precision. Repeated failures shrink the
delay toward zero, and once the precision reaches 1 the callout is due
immediately: the timer fires again at once and softclock retries the
lock in a tight loop for as long as the lock is held.
If the lock owner runs on the callout's CPU and no other CPU is idle,
the softclock thread preempts it on every attempt, starving the thread
it is waiting on. On an 8-CPU arm64 VM, a test module holding the lock
saw 760,000 attempts per second, each with its own timer interrupt, and
progressed at 38% of its normal rate. On a 4-core amd64 system under
loopback TCP load, a netisr thread holding an inpcb lock made no
progress for 12 minutes while the TCP timer callout was retried 830,000
times per second.
Keep the half-precision retry, but never schedule it less than one
tick from now. Base the deadline on sbinuptime() rather than
cc_lastscan: cc_lastscan is the time of the last callout_process()
scan, which can be a tick or more in the past by the time softclock
runs the callout, leaving cc_lastscan + tick_sbt already expired.
Floor the precision at a tick as well. With a precision of 1 the
retries of many contended callouts cannot share a timer interrupt: the
timer is armed for the earliest retry, each interrupt collects only
those already due, and every callout_process() call walks all the
contended callouts in the callwheel bucket. On the same VM, with
10,000 callouts on one lock, the owner took 5.0 to 5.8 s to finish 1 s
of work with a precision of 1 and 1.3 s with the floor, and
callout_process() ran about 58,000 times against about 900. A single
contended callout is now retried about every two ticks.
Reviewed by: kib
Fixes: efcb2ec8cb ("callout: provide CALLOUT_TRYLOCK flag")
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60246
pf_handle_get_srcnodes() returns with the lock of a source hash row
held when it cannot start the message for a source node. Unlock the
row there, as the other error exit of the loop does.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: 9c12533672 ("pf: convert DIOCGETSRCNODES to netlink")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60251
The PFNL_CMD_TABLE_TEST_ADDRS reply carries only the match count, so
the per-address feedback from pfr_tst_addrs() is lost:
"pfctl -v -T test" lists nothing and "pfctl -vv -T test" reports
every address as "nomatch".
Return each address, as updated by pfr_tst_addrs(), in a nested
PF_TAS_ADDR attribute, and decode them into the caller's array in
libpfctl. PF_TA_ADDR is not reused: it shares its value with
PF_TAS_ASTATS, which older libpfctl would decode into an
uninitialised target. That target was also read when no reply was
parsed, so the match count could be garbage; initialise it.
Add a regression test.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: 281282e935 ("pf: convert DIOCRTSTADDRS to netlink")
See also: https://redmine.pfsense.org/issues/17135
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60146
When pf_sourcelim_add() finds the name of the new limiter taken, it
undoes the insertion into the id tree with RB_REMOVE() on the name tree,
which the limiter is not in, and then frees the limiter. The freed
limiter stays in the inactive id tree, and RB_REMOVE() of an element
with no links clears the root of the name tree, which loses every other
inactive limiter from it. pf_statelim_add() gets this right.
parse.y refuses duplicate names, so pfctl does not get here, but any
netlink client can.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: 4616481212 ("pf: introduce source and state limiters")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60189
pf_ioctl_addrule() allocates a counter_rate for every rule, whether it
has a max-pkt-rate or not, and pf_krule_free() never frees it.
Free it with the rest of the rule.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: ff11f1c8c7 ("pf: add a generic packet rate matching filter")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60190
pfctl -sr calls PFNL_CMD_GETRULE once per rule, and
pf_handle_getrule() takes the rules write lock each time, so listing a
ruleset of N rules stops packet processing N times. Only zeroing the
counters (pfctl -z) needs the write lock. Take the read lock
otherwise, as DIOCGETRULENV does.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: 777a4702c5 ("pf: implement addrule via netlink")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60161
pf_purge_thread() calls pf_purge_unlinked_rules() in the network
epoch, where sleeping is not allowed, and it takes pf_config_lock, an
sx lock. If a rule is being added at the time, the purge thread
can sleep on the lock, which panics with INVARIANTS.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: f92d9b1aad ("pflow: import from OpenBSD")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60160
When the PFNL_CMD_ADDRULE message fails to parse, pf_handle_addrule()
frees the rule with pf_free_rule(), which asserts the rules and config
locks (neither is held) and releases references that
pf_ioctl_addrule() has not taken yet. With INVARIANTS this panics on
any parse error; without, a rule address parsed as PF_ADDR_TABLE makes
pfr_detach_table() dereference NULL.
Use pf_krule_free(), as the ioctl paths do.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: e249f5daa4 ("pf: fix memory leak on rule add parse failure")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60104
The chunked table address functions (set, add, del, clr_astats) add
each chunk's result to the caller's counter without initialising it.
pfctl reuses nadd for the number of tables created, so a replace that
also creates the table is off by one:
pfctl -t foo -T replace 192.0.2.1
reports "2 addresses added".
Zero the counters first, as pfctl_test_addrs() already does. Remove
the workaround for the add case from pfctl (da64f6e047), which is
no longer needed.
Add a regression test.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: 08ed87a4a2 ("pf: convert DIOCRSETADDRS to netlink")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60174
The purge thread sets PFI_IFLAG_REFS on the interfaces that states
refer to without the rules lock, under which the other flags are
changed. The updates can interleave, so that a "set skip on" is lost,
or outlives its removal, until the next ruleset load.
Use atomic operations to modify the flags. In the purge thread, only
write if the flag is not already set.
Reviewed by: kp
Approved by: kp (mentor)
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60107
c2d03a920e rewrote the action printing in print_rule() from OpenBSD,
which has no natpass, and dropped the "pass" keyword. A ruleset
loaded from "pfctl -sn" output therefore lost its nat-pass semantics.
Add a parser test covering nat, rdr, rdr log and binat with pass.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: c2d03a920e ("pfctl: fix anchortypes bounds test")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60183
epoch_trace_report() assigned the return value of RB_INSERT() back to
the new element. When two threads report the same stack concurrently,
the loser's RB_INSERT() returns the element already in the tree, and
that element was freed while still linked, leaking the new allocation.
The next lookup touches freed memory; KASAN catches it as a
use-after-free.
Keep the return value separate and free the new element instead. The
thread that won the race prints the report, so return without printing
it a second time.
Reviewed by: markj
Fixes: 173c062a56 ("Improve EPOCH_TRACE")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60162
The netlink encoding of struct pfr_addr omits pfra_fback, so no
per-address feedback reaches userspace. In particular,
pfr_get_astats() marks entries of tables without counters with
PFR_FB_NOCOUNT, which pfctl uses to skip their counters, so
"pfctl -v -T show" prints zero counters for such tables.
Add PFR_A_FBACK, emit it from the kernel and decode it in libpfctl.
Add a regression test.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: 08f54dfca1 ("pf: convert DIOCRGETASTATS to netlink")
Sponsored by: Rubicon Communications, LLC ("Netgate")
pfi_initialize() registers pfi_rename_ifnet_event() on
ifnet_rename_event, but pfi_cleanup() never deregisters it. After
"kldunload pf", the next interface rename calls through a stale
pointer into the unloaded module:
kldload pf
kldunload pf
ifconfig epair create
ifconfig epair0a name foo
Reviewed by: kp
Approved by: kp (mentor)
Fixes: 349fcf079c ("net: add ifnet_rename_event EVENTHANDLER(9) for interface renaming")
Sponsored by: Rubicon Communications, LLC ("Netgate")
The kernel sends PFR_A_AF as a u32, although it is documented and
parsed as a u8, and libpfctl decoded it with snl_attr_get_uint32()
straight into the u8 pfra_af. That overwrote pfra_net, pfra_not and
pfra_fback, and left pfra_af zero on big-endian hosts.
Decode it via a temporary, and accept a u8 as well, so that the
kernel can be corrected later without breaking libpfctl.
Reviewed by: kp
Approved by: kp (mentor)
Fixes: f27e44e2e3 ("pf: convert DIOCRGETADDRS to netlink")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60110
pfr_clr_astats() looks up each address it is given and inserts the entry
it finds at the head of a work queue. If the same address is given more
than once, the entry is inserted twice and the second insertion makes it
its own successor. pfr_clstats_kentries() then walks the queue forever,
with the rules lock held for writing, so packet processing and every
other pf operation in that vnet stop as well. To reproduce:
pfctl -e
pfctl -t foo -T add 192.0.2.1
pfctl -t foo -T zero 192.0.2.1 192.0.2.1
Do as pfr_del_addrs() does: clear pfrke_mark on the entries named, then
queue an entry only the first time it is seen. An address given more
than once is cleared, and counted, once. Validate all addresses before
any entry is touched.
Add a regression test.
Reviewed by: kp
Approved by: kp (mentor)
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60103
Since DIOCRSETADDRS was converted to netlink, pf_handle_table_set_addrs()
calls pfr_set_addrs() with a NULL size2, as the netlink interface has no
buffer to return the deleted addresses in. pfr_set_addrs() only checked
size2 for NULL at the end of the function; with PFR_FLAG_FEEDBACK set it
dereferenced it unconditionally first. pfctl sets PFR_FLAG_FEEDBACK
when run with -v, so "pfctl -v -t foo -T replace ..." panicked the
kernel with a NULL pointer dereference. To reproduce:
pfctl -e
pfctl -t foo -T add 192.0.2.1
pfctl -v -t foo -T replace 192.0.2.2
Check size2 for NULL before dereferencing it, as is already done at the
end of the function. The per-address feedback for added and changed
addresses is still copied back as before; only the list of deleted
addresses, which the netlink caller has no room for, is skipped.
While here, compare size2 against NULL explicitly in the second check as
well, per style(9).
Add a regression test.
Approved by: kp (mentor)
Fixes: 08ed87a4a2 ("pf: convert DIOCRSETADDRS to netlink")
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D60096
pf_route() and pf_route6() forward broadcast and multicast traffic
when a route-to rule matches, without any check against the output
interface's broadcast domain. This is a deliberate property of the
route option code path, but it is not documented and the workaround
is non-obvious.
Document the behavior in pf.conf(5) with example block-out rules on
the target interface, scoped with the received-on qualifier so that
only forwarded traffic is dropped while the router's own broadcast
and multicast traffic continues to pass.
Add regression tests covering the full broadcast/multicast and
forwarded/local matrix on both IPv4 and IPv6.
Reviewed by: glebius, kp
Approved by: kp (mentor)
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D56559
Ensure that run_rc_script returns the true return code of the
rc command and not the return code of DebugOff
Reviewed by: sjg
Approved by: kp
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D46626
When a temporary/bootonce boot environment is renamed, we need to also
update the bootenv nvlist on-disk to reflect the new name. Additionally,
when a temporary/bootonce boot environment is destroyed, we also need to
clear out the on-disk state.
Reviewed by: kevans
Approved by: kp
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D43591
As a followup to D41330 and D41436, this patch introduces two new tests
for sbin/route: interface_route_v[46].
These tests fail without D41330.
Reviewed by: kp
Approved by: kp (mentor)
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
This is a follow-up patch to https://reviews.freebsd.org/D42459
that modifies the loader lua to use the correct loader variables
for determining ACPI availability.
This also fixes a bug where ACPI can be inadvertently disabled when
setting System Defaults at the loader menu.
Reviewed by: imp, kevans
Approved by: kp
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D42483
lua was previously unable to determine ACPI presence because this
probing was postponed until the final loading and execution of the
kernel.
This patch resolves that by detecting ACPI early (similar to
the order of operations in the legacy i386 loader).
Reviewed by: kevans
Approved by: kp
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D42459
