Re: [PATCH 2/5] net: bridge: Implement bridge flood flag

[Ido Schimmel <idosch@idosch.org>]

On Thu, Mar 17, 2022 at 01:42:55PM +0200, Nikolay Aleksandrov wrote:
> On 17/03/2022 13:39, Mattias Forsblad wrote:
> > On 2022-03-17 10:07, Joachim Wiberg wrote:
> >> On Thu, Mar 17, 2022 at 07:50, Mattias Forsblad <mattias.forsblad@gmail.com> wrote:
> >>> This patch implements the bridge flood flags. There are three different
> >>> flags matching unicast, multicast and broadcast. When the corresponding
> >>> flag is cleared packets received on bridge ports will not be flooded
> >>> towards the bridge.
> >>
> >> If I've not completely misunderstood things, I believe the flood and
> >> mcast_flood flags operate on unknown unicast and multicast.  With that
> >> in mind I think the hot path in br_input.c needs a bit more eyes.  I'll
> >> add my own comments below.
> >>
> >> Happy incident I saw this patch set, I have a very similar one for these
> >> flags to the bridge itself, with the intent to improve handling of all
> >> classes of multicast to/from the bridge itself.
> >>
> >>> [snip]
> >>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> >>> index e0c13fcc50ed..fcb0757bfdcc 100644
> >>> --- a/net/bridge/br_input.c
> >>> +++ b/net/bridge/br_input.c
> >>> @@ -109,11 +109,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
> >>>  		/* by definition the broadcast is also a multicast address */
> >>>  		if (is_broadcast_ether_addr(eth_hdr(skb)->h_dest)) {
> >>>  			pkt_type = BR_PKT_BROADCAST;
> >>> -			local_rcv = true;
> >>> +			local_rcv = true && br_opt_get(br, BROPT_BCAST_FLOOD);
> >>
> >> Minor comment, I believe the preferred style is more like this:
> >>
> >> 	if (br_opt_get(br, BROPT_BCAST_FLOOD))
> >>         	local_rcv = true;
> >>
> >>>  		} else {
> >>>  			pkt_type = BR_PKT_MULTICAST;
> >>> -			if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
> >>> -				goto drop;
> >>> +			if (br_opt_get(br, BROPT_MCAST_FLOOD))
> >>> +				if (br_multicast_rcv(&brmctx, &pmctx, vlan, skb, vid))
> >>> +					goto drop;
> >>
> >> Since the BROPT_MCAST_FLOOD flag should only control uknown multicast,
> >> we cannot bypass the call to br_multicast_rcv(), which helps with the
> >> classifcation.  E.g., we want IGMP/MLD reports to be forwarded to all
> >> router ports, while the mdb lookup (below) is what an tell us if we
> >> have uknown multicast and there we can check the BROPT_MCAST_FLOOD
> >> flag for the bridge itself.
> > 
> > The original flag was name was local_receive to separate it from being
> > mistaken for the flood unknown flags. However the comment I've got was
> > to align it with the existing (port) flags. These flags have nothing to do with
> > the port flood unknown flags. Imagine the setup below:
> > 
> >            vlan1
> >              |
> >             br0             br1
> >            /   \           /   \
> >          swp1 swp2       swp3 swp4
> > 
> > We want to have swp1/2 as member of a normal vlan filtering bridge br0 /w learning on. 
> > On br1 we want to just forward packets between swp3/4 and disable learning. 
> > Additional we don't want this traffic to impact the CPU. 
> > If we disable learning on swp3/4 all traffic will be unknown and if we also 
> > have flood unknown on the CPU-port because of requirements for br0 it will
> > impact the traffic to br1. Thus we want to restrict traffic between swp3/4<->CPU port
> > with the help of the PVT.
> > 
> > /Mattias
> 
> The feedback was correct and we all assumed unknown traffic control.
> If you don't want any local receive then use filtering rules. Don't add unnecessary flags.

Yep. Very easy with tc:

# tc qdisc add dev br1 clsact
# tc filter add dev br1 ingress pref 1 proto all matchall action drop

This can be fully implemented inside the relevant device driver, no
changes needed in the bridge driver.