freebsd
if_bridge: avoid process non-local targeted packages via pf
if_bridge: avoid process non-local targeted packages via pf
Do not process multicast packages via pf that are not targeted to the local physical interface here.
details / discussion / bug report (2019): https://lists.freebsd.org/pipermail/freebsd-pf/2019-December/009275.html
Reported by: Dr. Andreas Longwitz Sponsored by: DSS GmbH
[...] the commit message must explain the /why/ of a commit. [...]
Correct, the commit message and title have been updated. Thx!
That seems to have only updated the message in the PR, not the commit itself (which is what'd end up in freebsd's commit log).
I'm also not sure I understand what the patch does.
V_pfil_local_phys is supposed to control whether the packet is passed to the pfil hooks or not, but this seems to about the local delivery of the packet. I don't see how the packet still gets locally delivered if V_pfil_local_phys is set, and I don't really see a duplicate call to the pfil hooks either.
I'm also not sure I understand what the patch does.
I may be mistaken, but the issue is that we are transmitting multicast/broadcast packets through an early exit opportunity, without going through the logic gate that unicast packets have to pass through later (line 2780).
/* Filter on the physical interface. */ \
if (V_pfil_local_phys && (PFIL_HOOKED_IN(V_inet_pfil_head) || \
PFIL_HOOKED_INET6)) { \
if (bridge_pfil(&m, NULL, ifp, \
PFIL_IN) != 0 || m == NULL) { \
return (NULL); \
} \
} \
(... and sorry for the delayed answer, ${DAYJOB})
So how does the packet get delivered locally if V_pfil_local_phys is set?
It might be useful (and it's generally very desirable anyway, close to outright required) to have a test case here. Demonstrating the problem would help a lot in understanding the point and correctness of this patch.
[last update: 202308080622 UTC ]
lab setup with the following participants
- (bridge) Interfaces: bridge0 with igb0 und igb1
- (sender) connected on igb0
- (receiver) connected on igb1
sysctl setup (bridge)
net.link.bridge.inherit_mac=1 net.link.bridge.pfil_bridge=0
sender :
echo "multicast packet" | /ssd/bin/socat -d -d UDP4 DATAGRAM:224.1.1.1:6666, bind=192.168.14.242:6666 - echo "broadcast packet" | /ssd/bin/socat -d -d UDP4-DATAGRAM:192.168.14.255:6667, bind=192.168.14.242:6667,broadcast -
receiver (for verification, always get the correct amount of packages, with or without fix) :
socat -d -d UDP4-DATAGRAM:224.1.1.1:6666,bind=:6666,ip-add-membership=224.1.1.1:$if - socat -d -d -u UDP4-RECVFROM:6667,bind='192.168.14.255',broadcast,fork -
bridge view after one broadcast and one multicast package send
multicast: pass in quick on igb0 inet proto udp from any to 224.1.1.1 port = 6666 no state tag BRIDGE [ Evaluations: 8623 Packets: 1 Bytes: 60 States: 0 ] pass in quick on bridge0 inet proto udp from any to 224.1.1.1 port = 6666 no state [ Evaluations: 7804 Packets: 1 Bytes: 60 States: 0 ] pass out quick on igb1 inet proto udp from any to 224.1.1.1 port = 6666 no state tagged BRIDGE [ Evaluations: 2666 Packets: 1 Bytes: 60 States: 0 ]
broadcast: pass in quick on igb0 inet proto udp from any to 192.168.14.255 port = 6667 no state [ Evaluations: 669 Packets: 1 Bytes: 60 States: 0 ] pass in quick on bridge0 inet proto udp from any to 192.168.14.255 port = 6667 no state [ Evaluations: 578 Packets: 1 Bytes: 60 States: 0 ] pass out quick on igb1 inet proto udp from any to 192.168.14.255 port = 6667 no state [ Evaluations: 2698 Packets: 1 Bytes: 60 States: 0 ]
Problem:
Without fix - pf sees (counts!) two packets on igb0 for every multicast or broadcast send, no problem with unicast traffic.
I was unclear: I meant an automated test case like the existing bridge tests in tests/sys/net/if_bridge_tests.sh.
I also still don't understand how the packet gets delivered locally if V_pfil_local_phys is set.
In general I consider filtering L3 on if_bridge to be a misfeature that should just be removed. This is, by far, not the worst bug in there.
In general I consider filtering L3 on if_bridge to be a misfeature that should just be removed. This is, by far, not the worst bug in there.
- I'm not sure if I would consider it a flaw, but with the recent integration of pf Layer2, there are other redundant solution approaches that are now possible.
- If the code segment remains, it would be unpleasant to ignore a well-reproducible bug and send other people days into digging into the source code rabbit hole again.
- I also believe that the comment line "/* Return the original packet for local processing. */" is misleading in this context. I did not verify this claim thoroughly enough, and the packet is not fully re-injected but rather double counted here.
Test script to reproduce the issue (provided by the original bug-reporter: Thank You Andreas!)
#!/bin/sh
my=bridge.sh
# pf.conf must include
# pass in quick on bridge0 inet proto udp from any to 224.1.1.1 port 6666 no state
# pass in quick on epair0a inet proto udp from any to 224.1.1.1 port 6666 no state
# pass out quick on epair1a inet proto udp from any to 224.1.1.1 port 6666 no state
# pass in quick on bridge0 inet proto udp from any to 192.0.2.255 port 6667 no state
# pass in quick on epair0a inet proto udp from any to 192.0.2.255 port 6667 no state
# pass out quick on epair1a inet proto udp from any to 192.0.2.255 port 6667 no state
# output of "bridge.sh create" on my testserver (2 packets on epair0a without patch)
# pass in quick on bridge0 inet proto udp from any to 192.0.2.255 port = 6667 no state
# [ Evaluations: 4 Packets: 1 Bytes: 60 States: 0 ]
# --
# pass in quick on epair0a inet proto udp from any to 192.0.2.255 port = 6667 no state
# [ Evaluations: 3 Packets: 2 Bytes: 120 States: 0 ]
# --
# pass out quick on epair1a inet proto udp from any to 192.0.2.255 port = 6667 no state
# [ Evaluations: 1 Packets: 1 Bytes: 60 States: 0 ]
# pass in quick on bridge0 inet proto udp from any to 224.1.1.1 port = 6666 no state
# [ Evaluations: 6 Packets: 1 Bytes: 60 States: 0 ]
# --
# pass in quick on epair0a inet proto udp from any to 224.1.1.1 port = 6666 no state
# [ Evaluations: 4 Packets: 2 Bytes: 120 States: 0 ]
# --
# pass out quick on epair1a inet proto udp from any to 224.1.1.1 port = 6666 no state
# [ Evaluations: 4 Packets: 1 Bytes: 60 States: 0 ]
#
opcode=$1
create_bridge() {
ifconfig epair create # gives epair0a/epair0b
ifconfig epair create # gives epair1a/epair1b
jail -c name=vnetjail1 persist vnet vnet.interface=epair0b
jail -c name=vnetjail2 persist vnet vnet.interface=epair1b
jexec vnetjail1 /sbin/ifconfig epair0b inet 192.0.2.1/24 up
jexec vnetjail2 /sbin/ifconfig epair1b inet 192.0.2.2/24 up
ifconfig epair0a up
ifconfig epair1a up
ifconfig bridge create # gives bridge0
ifconfig bridge0 inet 192.0.2.99/24 netmask 255.255.255.0 addm epair0a addm epair1a up
service pf resync
jexec vnetjail1 /bin/sh -c "echo broadcast_packet_from_vnetjail1 | socat UDP4-DATAGRAM:192.0.2.255:6667,bind=192.0.2.1:6667,broadcast -"
jexec vnetjail1 /sbin/route add 224.1.1.1 -iface epair0b
jexec vnetjail1 /bin/sh -c "echo multicast_packet_from_vnetjail1 | socat UDP4-DATAGRAM:224.1.1.1:6666,bind=192.0.2.1:6666 -"
pfctl -vsr | grep -A1 192.0.2.255
pfctl -vsr | grep -A1 224.1.1.1
}
destroy_bridge() {
jail -r vnetjail1
jail -r vnetjail2
ifconfig bridge0 destroy
ifconfig epair0a destroy
ifconfig epair1a destroy
}
#go
case ${opcode} in
create)
create_bridge
;;
destroy)
destroy_bridge
;;
*)
echo "--> usage: ${my} create/destroy"
exit 1
;;
esac
@kprovost Can you take another look? I'd like to either merge or close this and it looks stalled.
@kprovost Can you take another look? I'd like to either merge or close this and it looks stalled.
The questions raised in this review remain unanswered. It continues to lack an automated test case. We should close this.
As an aside, tagging people in GitHub is mostly useless, because the notification is lost in a sea of other GitHub notifications. I only saw this one by chance.
closing for now. This has stalled, has remaining objections and needs some more work. If that situation changed, please create a new pull request.. Thanks!