vlan_capabilities(), used by the IFCAP ioctl, was not respecting the
IFCAP_LRO bit if it was masked by the requestor.
This prevented if_bridge(4) from automasking LRO with a message like:
bridge0: can't disable some capabilities on em3.11: 0x400
This also prevented manually disabling LRO from any vlan interface.
PR: 254596
Reported by: Paul Vixie <paul@redbarn.org>
MFC after: 1 week
This reverts commit 5f11a33cee.
As requested by Kevin Bowling. He explains:
> The subtle bug was that vlan_capabilities() in if_vlan was not obeying
> the requested mask from its IFCAP ioctl.
If the parent interface is not a bridge and can do LRO and
checksum offloading on VLANs, then guess it may do LRO on VLANs.
False positive here cost nothing, while false negative may lead
to some confusions. According to Wikipedia:
"LRO should not operate on machines acting as routers, as it breaks
the end-to-end principle and can significantly impact performance."
The same reasoning applies to machines acting as bridges.
PR: 254596
MFC after: 3 weeks
In iflib_init_locked(), sctx and scctx both point to the same value,
which is the ifc_softc_ctx field in the iflib softc. Remove the
declaration and assignment to sctx since scctx can be used instead, and
the name of scctx follows the naming convention used for local variables
that point to ifc_softc_ctx.
In theory there should be no functional impact with this change.
Signed-off-by: Eric Joyner <erj@FreeBSD.org>
Reviewed by: kbowling@
MFC after: 3 days
Sponsored by: Intel Corporation
Differential Revision: https://reviews.freebsd.org/D41325
This helps align some of the code with the rest of the style used in
iflib, but as marius@ points out, this is not style(9).
Signed-off-by: Eric Joyner <erj@FreeBSD.org>
Reviewed by: kbowling@
MFC after: 3 days
Sponsored by: Intel Corporation
Differential Revision: https://reviews.freebsd.org/D41324
This code was used by the first incarnation of wg(4) and is dead ever
since f187d6dfbf has removed the latter
again. Moreover, this code matched iflib(4) like a square peg fits in
a round hole, was incomplete and despite some hacks still tailored to
VPC and wg(4) but not generic. In effect, this reverts the following:
09f6ff4f1a (w/ its "ancillary changes")
9aeca213241f93e931d90f9544d03e0dd691b412
Reviewed by: erj, kbowling
Differential Revision: <https://reviews.freebsd.org/D41196>
During a driver reload stress test, after 50-300 reloads a panic occurs.
After adding sleeps in between loading and unloading the driver, the
issue does not occur. It's possible that loading/unloading too fast may
cause the gt_taskqueue pointer to be freed earlier than expected;
checking for a null pointer first fixes it.
Signed-off-by: Eric Joyner <erj@FreeBSD.org>
Reviewed by: erj@
Tested by: jeffrey.e.pieper@intel.com
MFC after: 3 days
Sponsored by: Intel Corporation
Differential Revision: https://reviews.freebsd.org/D39457
With the current implementation of if_bridge(4), bridge_enqueue()
calls gif_transmit() only. Ensure it stays that way so that the
expectations in both drivers are either met or changed accordingly.
PR: 227450
The code added in c89c8a1029 in order
to compensate possible misalignment caused by prepending the IP4/6
header with an EtherIP one got broken at some point by a rewrite of
gif(4). For better or worse, 8018ac153f
relaxed the alignment of struct ip from 32 bit to 16 bit, though. As
a result, a 16 bit offset of the IPv4 header induced by the addition
of the 16 bit EtherIP one no longer is a problem in the first place.
The alignment of struct ip6_hdr currently is even only 8 bit, making
it even less problematic with regards to possible misalignment.
Thus, remove the code for handling misalignment in in{,6}_gif_output()
altogether again.
While at it, replace the 3 bcopy(9) calls in gif(4) with memcpy(9) as
there's no need to handle overlap here.
Send an SCTP Abort message if we're refusing a connection, just like we
send a RST for TCP.
MFC after: 3 weeks
Sponsored by: Orange Business Services
Differential Revision: https://reviews.freebsd.org/D40864
Basic state tracking for SCTP. This means we scan through the packet to
identify the different chunks (so we can identify state changes).
MFC after: 3 weeks
Sponsored by: Orange Business Services
Differential Revision: https://reviews.freebsd.org/D40862
Panic occurs during loading driver using kldload. It exists since netlink is
enabled. There is problem with double locking ctx. This fix allows to call
ether_ifattach() without locked ctx.
Signed-off-by: Eric Joyner <erj@FreeBSD.org>
PR: 271768
Reviewed by: erj@, jhb@
MFC after: 1 day
Sponsored by: Intel Corporation
Differential Revision: https://reviews.freebsd.org/D40557
Actions applied to a processed packet come in case of stateless
firewalling from a rule or in case of statefull firewalling from a
state. The state obtains the actions from a rule when it is created by a
rule or by pfsync. The logic for deciding if actions come from a rule or
a state is spread across many places in pf.
There already is struct pf_rule_actions in struct pf_pdesc and thus it
can be used as a central place for storing actions and their parameters.
OpenBSD does something similar: they also store the actions in struct
pf_pdesc and have no variables in pf_test() but they use separate
variables instead of a structure. By using struct pf_rule_actions we can
simplify the code even further. Applying of actions is done *only* in
pf_rule_to_actions() no matter if for the legacy scrub rules or for the
normal match / pass rules. The logic of choosing if rule or state
actions are used is applied only once in pf_test() by copying the whole
struct.
Reviewed by: kp
Sponsored by: InnoGames GmbH
Differential Revision: https://reviews.freebsd.org/D41009
The variable storing the direction of a processed packet is passed
around to many functions. Most of those functions already have a pointer
to struct pf_pdesc which also contains the direction. By using the one
in struct pf_pdesc we can reduce the amount of arguments passed around.
Reviewed by: kp
Sponsored by: InnGames GmbH
Differential Revision: https://reviews.freebsd.org/D41008
- Add new DB_DEFINE_TABLE and DB_DECLARE_TABLE macros to define new
command tables. DB_DECLARE_TABLE is intended for use in headers
similar to MALLOC_DECLARE and SYSCTL_DECL.
DB_DEFINE_TABLE takes three arguments, the name of the parent table,
the command name, and the name of the table itself, e.g.
DB_DEFINE_TABLE(show, foo, show_foo) defines a new "show foo" table.
- DB_TABLE_COMMAND, DB_TABLE_COMMAND_FLAGS, DB_TABLE_ALIAS, and
DB_ALIAS_FLAGS allow new commands and aliases to be defined. These
are similar to the existing DB_COMMAND, etc. except that they take
an initial argument giving the name of the parent table, e.g.:
DB_TABLE_COMMAND(show_foo, bar, db_show_foo_bar)
defines a new "show foo bar" command.
This provides a cleaner interface than the ad-hoc use of internal
macros like _DB_SET that was required previously (e.g. in cxgbe(4)).
This retires DB_FUNC macro as well as the internal _DB_FUNC macro.
Reviewed by: melifaro, kib, markj
Differential Revision: https://reviews.freebsd.org/D40819
If we're called on an mbuf that's passed through codel before it may
already contain the MTAG_CODEL tag. The code accounts for this and does
not allocate a new mtag. However, it inserts the mtag unconditionally.
That is, it inserts the existing mtag a second time.
When the mbuf later gets freed we iterate over the list of mtags to fee
them one by one, and we'll end up freeing an mtag that's already been
freed.
Only insert the mtag if we've allocated a new one. If we found one
there's no need to insert it again.
See also: https://redmine.pfsense.org/issues/14497
Sponsored by: Rubicon Communications, LLC ("Netgate")
The value stored in pf_mtag->tag comes from "tag" and "match tag"
keywords in pf.conf and must not be abused for storing other
information. A ruleset with enough tags could set or remove the bits
responsible for PF_TAG_SYNCOOKIE_RECREATED.
Move this syncookie status to pf_mtag->flags. Rename this and other
related constants in a way that will prevent such mistakes in the
future. Move PF_REASSEMBLED constant to mbuf.h and rename accordingly
because it's not a flag stored in pf_mtag, but an identifier of a
different m_tag. Change the value of the constant to avoid conflicts
with other m_tags using MTAG_ABI_COMPAT.
Rename the variables in pf_build_tcp() and pf_send_tcp() in to reduce
confusion.
Reviewed by: kp
Sponsored by: InnoGames GmbH
Differential Revision: https://reviews.freebsd.org/D40587
Use the already populated PFE_SKIP_DST_ADDR and extend the skip
infrastructure to also skip on IP source/destination addresses.
This should make evaluating the rules slightly faster.
Reported by: R. Christian McDonald <rcm@rcm.sh>
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D40567
This change exports interface capabilities using the standard
Netlink attribute type, bitset, and switches `ifconfig(8)` to use
it when displaying interface data.
Bitset comes in two representations. The first one is "compact",
where the bits are exported via two arrays - "mask" listing the
"valid" bits and "values, providing the values for those bits.
The second one is more verbose, listing each bit as a separate item,
with its name, id and value. The latter option is handy when submitting
update requests.
The support for setting capabilities will be added in the upcoming diffs.
Differential Revision: https://reviews.freebsd.org/D40331
While if_epair has no issues doing this we should drop those packets
anyway, because it improves the fidelity of the automated tests.
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D40397
If we route-to (or dup-to/reply-to) we re-run pf_test(), which will also
create states for the connection.
This means that we may end up matching a different (i.e. not the state
that was created by the route-to rule) state, without the attributes
(such as dummynet pipes/queues) set by the route-to rule.
Address this by inheriting the pf_rule_actions from the route-to rule
while evaluating the connection again in pf_test(). That is, we set
default pf_rule_actions based on the route-to rule for the new
evaluation. The new rule may still overrule these, but if it does not
have such actions the route-to actions are applied.
Do the same for IPv6 rules in pf_test6()/pf_route6().
See also: https://redmine.pfsense.org/issues/14039
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D40340
Some context on the current IPv6 interface setup & address management:
There are two data path for IPv6 initialisation in context of assigning
LL addresses:
1) Userland explicitly requests IFF_UP for the interface w/o any addresses.
if_up() then calls in6_if_up(), which calls in6_ifattach().
The latter sets up some initial ND/IN6 state and disables IPv6 for the
interface if it’s not loopback. If the interface is loopback, then it
adds ::1/128 and LL addresses via in6_ifattach_loopback().
Then, devd notification is generated (if the VNET is the default one),
which triggers rc.network ifconfig_up(), causing ifdisabled to be removed
via SIOCSIFINFO_IN6 from ifconfig. The kernel SIOCSIFINFO_IN6 handler
calls in6_if_up() once again and it assigns the interface link-local address.
2) Userland adds IPv4 or IPv6 address to the interface. SIOCAIFADDR[_IN6]
kernel handler calls IPv4/IPv6 protocol handler to add the address.
Both then call if_ioctl() with SIOCSIFADDR. Ethernet/loopback ioctl handlers
silently sets IFF_UP for the interface. Finally, if.c:ifioctl() wrapper code
compares old and new interface flags and, if IFF_UP is added, it explicitly
calls in6_if_up(), which adds link-local address if either the original
address is IPv6 or the interface is loopback.
In the latter case, “formal” interface-up notifications are missing.
The kernel does not trigger event handler event, does not call carp hook
and does not provide any userland notification.
This diff unifies the event handling in both scenarios, providing the
necessary notifications to the kernel and userland.
Reviewed By: kp
Differential Revision: https://reviews.freebsd.org/D40332
MFC after: 2 weeks
b0e38a1373 improved if_bridge's ability to cope with different VLANs,
but it failed to update bridge_transmit() to cope with the new rule that
untagged packets are treated as having VLAN ID 0 (rather than 1, as used
to be the case).
Fix that oversight.
PR: 270559
Reviewed by: kp
tap(4) devices advertise themselves as just 'ethernet autoselect',
without duplex or speed capabilities.
This advertisement makes them unable to be aggregated into lacp-based
lagg(4):
- lacp code requires underlying interfaces to be full-duplex, else
interface will not participate in lacp at all
- lacp code requires underlying interface to have non-zero speed, else
this interface can not be selected as active aggregator
PR: 217374
Reported-by: Alexandre Snarskii <snar@snar.spb.ru>
Co-authored-by: Mina Galić <freebsd@igalic.co>
Reviewed-by: imp,karles
Pull-request: https://github.com/freebsd/freebsd-src/pull/745
Make struct pfsync_state contents configurable by sending out new
versions of the structure in separate subheader actions. Both old and
new version of struct pfsync_state can be understood, so replication of
states from a system running an older kernel is possible. The version
being sent out is configured using ifconfig pfsync0 … version XXXX. The
version is an user-friendly string - 1301 stands for FreeBSD 13.1 (I
have checked synchronization against a host running 13.1), 1400 stands
for 14.0.
A host running an older kernel will just ignore the messages and count
them as "packets discarded for bad action".
Reviewed by: kp
Sponsored by: InnoGames GmbH
Differential Revision: https://reviews.freebsd.org/D39392
- Fixes netgdb's double ack
- Moving ack responsibility to debugnet, decision to ack made by netdump/netgdb.
- Finish responsibility moved to debugnet, new finish handler.
- netgdb now prints the address to connect to in case the user doesn't have
access to the proxy machine.
Sponsored by: Dell EMC
Reviewed By: markj, bdrewery (earlier version)
Differential Revision: https://reviews.freebsd.org/D40064
The intent is to set the value to UINT32_MAX, not to |= UINT32_MAX.
Happily the intent (ensure that we do not send further packets) is
achieved either way.
Reported by: markj
Sponsored by: Rubicon Communications, LLC ("Netgate")
if_ovpn already notified userpsace when there was a risk of sequence
number re-use, but it trusted userspace to actually rotate the key.
Convert the internal sequence number counter to 64 bits so we can detect
overflows and then refuse to send packets.
Event: BSDCan 2023
Reviewed by: Leon Dang <ldang@netgate.com>
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D40187
When a new bridge_rtnode is added it is added with a NULL brt_dst. The
brt_dst is set after the entry is added. This means there's a small
window where another core could also attempt to add this node, leading
to the code attempting to log that the MAC addresses moved to a new
interface.
Aside from that being a spurious log entry it also panics, because
obif is NULL (and we attempt to dereference it).
Avoid this by settings brt_dst before we insert the bridge_rtnode.
Assert that obif is non-NULL, as an extra precaution.
Reported by: olivier@
Reviewed by: zlei@
Differential Revision: https://reviews.freebsd.org/D40147
Prepare for rtableid being included in struct pfsync_state where it will
be int32_t. Make variables which will be set to and from it the same
width.
Reviewed by: kp
Sponsored by: InnoGames GmbH
Differential Revision: https://reviews.freebsd.org/D40013
Nexthop groups can be referenced by the external code. The reference
can be released after the VNET destruction. Furthermore, nexthop
groups use a single per-rib lock, which is destroyed during the
VNET desctruction. To eliminate use-after-free problem, each nhg
is marked as "unlinked" during the VNET destruction stage, leaving
nhg_idx intact. Normally there should not be such nexthops, but if
there are any, the kernel will panic on 'gr_idx != 0' when the
last nhg reference is released.
Address this by using the assert checks only when the nexthop group
is destroyed during "valid" VNET lifetime.
MFC after: 3 days
State deletions are sent over pfsync using struct pfsync_del_c.
Remove the code for receiving state deletions using struct pfsync_state
as such deletions are never sent. Rename functions and constants so that
only the "compressed" versions remain.
Reviewed by: kp
Sponsored by: InnoGames GmbH
Differential Revision: https://reviews.freebsd.org/D40004
The SPDX folks have obsoleted the BSD-2-Clause-NetBSD identifier. Catch
up to that fact and revert to their recommended match of BSD-2-Clause.
Discussed with: pfg
MFC After: 3 days
Sponsored by: Netflix
The SPDX folks have obsoleted the BSD-2-Clause-FreeBSD identifier. Catch
up to that fact and revert to their recommended match of BSD-2-Clause.
Discussed with: pfg
MFC After: 3 days
Sponsored by: Netflix
Setting vlan flags needlessly takes the exclusive VLAN_XLOCK().
If we have stacked vlan devices (i.e. QinQ) and we set vlan flags (e.g.
IFF_PROMISC) we call rtnl_handle_ifevent() to send a notification about
the interface.
This ends up calling SIOCGIFMEDIA, which requires the VLAN_SLOCK().
Trying to take that one with the VLAN_XLOCK() held deadlocks us.
There's no need for the exclusive lock though, as we're only accessing
parent/trunk information, not modifying it, so a shared lock is
sufficient.
While here also add a test case for this issue.
Backtrace:
shared lock of (sx) vlan_sx @ /usr/src/sys/net/if_vlan.c:2192
while exclusively locked from /usr/src/sys/net/if_vlan.c:2307
panic: excl->share
cpuid = 29
time = 1683873033
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe015d4ad4b0
vpanic() at vpanic+0x152/frame 0xfffffe015d4ad500
panic() at panic+0x43/frame 0xfffffe015d4ad560
witness_checkorder() at witness_checkorder+0xcb5/frame 0xfffffe015d4ad720
_sx_slock_int() at _sx_slock_int+0x67/frame 0xfffffe015d4ad760
vlan_ioctl() at vlan_ioctl+0xf8/frame 0xfffffe015d4ad7c0
dump_iface() at dump_iface+0x12f/frame 0xfffffe015d4ad840
rtnl_handle_ifevent() at rtnl_handle_ifevent+0xab/frame 0xfffffe015d4ad8c0
if_setflag() at if_setflag+0xf6/frame 0xfffffe015d4ad930
ifpromisc() at ifpromisc+0x2a/frame 0xfffffe015d4ad960
vlan_setflags() at vlan_setflags+0x60/frame 0xfffffe015d4ad990
vlan_ioctl() at vlan_ioctl+0x216/frame 0xfffffe015d4ad9f0
if_setflag() at if_setflag+0xe4/frame 0xfffffe015d4ada60
ifpromisc() at ifpromisc+0x2a/frame 0xfffffe015d4ada90
bridge_ioctl_add() at bridge_ioctl_add+0x499/frame 0xfffffe015d4adb10
bridge_ioctl() at bridge_ioctl+0x328/frame 0xfffffe015d4adbc0
ifioctl() at ifioctl+0x972/frame 0xfffffe015d4adcc0
kern_ioctl() at kern_ioctl+0x1fe/frame 0xfffffe015d4add30
sys_ioctl() at sys_ioctl+0x154/frame 0xfffffe015d4ade00
amd64_syscall() at amd64_syscall+0x140/frame 0xfffffe015d4adf30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe015d4adf30
--- syscall (54, FreeBSD ELF64, ioctl), rip = 0x22b0f0ef8d8a, rsp = 0x22b0ec63f2c8, rbp = 0x22b0ec63f380 ---
KDB: enter: panic
[ thread pid 5715 tid 101132 ]
Sponsored by: Rubicon Communications, LLC ("Netgate")
OpenVPN uses the sequence number (as well as a userspace supplied nonce)
to build the IV. This means we should avoid re-using sequence numbers.
However, userspace doesn't know how many packets we've sent (and thus
what sequence number we're up to).
Notify userspace when we've used half of the available sequence numbers
to tell it that it's time for a key renegotiaton.
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D39570
