From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id CF4F7C433F5 for ; Thu, 17 Mar 2022 12:09:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233449AbiCQMKV (ORCPT ); Thu, 17 Mar 2022 08:10:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55756 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233438AbiCQMKU (ORCPT ); Thu, 17 Mar 2022 08:10:20 -0400 Received: from mail-lf1-x133.google.com (mail-lf1-x133.google.com [IPv6:2a00:1450:4864:20::133]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 96793171EE8 for ; Thu, 17 Mar 2022 05:09:02 -0700 (PDT) Received: by mail-lf1-x133.google.com with SMTP id b28so8654889lfc.4 for ; Thu, 17 Mar 2022 05:09:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=bb6LfXoL5Q5pGeuJSTt1JWFN2kWGULeSvMc4mkWgMSk=; b=mfc55g2doSzcgrkZU7D3W5YzqjOR9zqCTZFIPs6PLHWfw9YJaG3Av+VohLXRxAx9UX op9Z2H2XNPknID6XHncjKz9Q5vuUDI8/QzZ9K91cpgDS0V5BmKN3ju6D7sJ1VZe1dbnJ o57q+8xPbG7/8Kv73zlo29FD+lt7qgYU7Mg2KLB5v0TuV9Qznc8GquwCQwWMxDJj3vSA bxn/A+z2QDROSA/ME4hX429/qxAUqJRMoU91GYCQUrPMJvFaVOHm64C9J8BTkoy32kXC YkUUUwl8mfXsNaenvxzgtQgi6BcaOdEkRpqjvja7ARhhYDLm2mgTMLbipNKWIrPPgKw9 SH3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=bb6LfXoL5Q5pGeuJSTt1JWFN2kWGULeSvMc4mkWgMSk=; b=F8Dp/J3AEYL7w7oQbYcACmA2d2SagnH6jQ3kW8dnzUanoq0loD+C2pSA6dHgIU0fKu CgU/VTxOohouvbhXrISBXS/q53zMIdaZYSmfz4PBMerRuJMLM/adBasFVGLMkFnrdu68 4lIhVDEBwTuZV3oWgKyU2ewIlF6VLa/5s09nqfLMAvnfAL1blpkf4o4xnUsm0kUFiwhX D7R2BYKdAv1cvH3dET3GCRsKSR6WSYqVvac2RhaGDKJcT57uxo3dZ+nz1hMBtW2S7qNy 4ofJEbh94rs+AaFCdunIHLlHdCo+GFSjWnt6TC2FLwj3sRxyqHrHy1fJOQGZaxKoR4IX hLfg== X-Gm-Message-State: AOAM5311pXID0cd78lV3hkTds52eL8FfF6trCRopeHL0WGgEjDMK6iyZ yPn3tMKGs8YVMSywn7+Nt5o= X-Google-Smtp-Source: ABdhPJwuSxHF+b7nmFQihWxoK/I0ZUhAEugyTUOlY+LjhlufJAr0iAJveK8kc9jX1Lv9G7VBrXbEZg== X-Received: by 2002:ac2:4e42:0:b0:448:b502:8865 with SMTP id f2-20020ac24e42000000b00448b5028865mr2790364lfr.545.1647518940712; Thu, 17 Mar 2022 05:09:00 -0700 (PDT) Received: from [10.0.1.14] (h-98-128-237-157.A259.priv.bahnhof.se. [98.128.237.157]) by smtp.gmail.com with ESMTPSA id h21-20020a19ca55000000b004481d401734sm432914lfj.91.2022.03.17.05.08.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 17 Mar 2022 05:08:59 -0700 (PDT) Message-ID: Date: Thu, 17 Mar 2022 13:08:58 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH 2/5] net: bridge: Implement bridge flood flag Content-Language: en-US To: Vladimir Oltean Cc: Nikolay Aleksandrov , netdev@vger.kernel.org, "David S . Miller" , Jakub Kicinski , Andrew Lunn , Florian Fainelli , Vivien Didelot , Roopa Prabhu , Tobias Waldekranz , Joachim Wiberg References: <20220317065031.3830481-1-mattias.forsblad@gmail.com> <20220317065031.3830481-3-mattias.forsblad@gmail.com> <20220317111545.3e7dxu3oqocdmves@skbuf> <20220317115703.vg47gitbnbev4etu@skbuf> <20220317115900.5vfomgespvzvgzdt@skbuf> From: Mattias Forsblad In-Reply-To: <20220317115900.5vfomgespvzvgzdt@skbuf> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 2022-03-17 12:59, Vladimir Oltean wrote: > On Thu, Mar 17, 2022 at 01:57:03PM +0200, Vladimir Oltean wrote: >> On Thu, Mar 17, 2022 at 01:15:45PM +0200, Vladimir Oltean wrote: >>> On Thu, Mar 17, 2022 at 12:11:40PM +0200, Nikolay Aleksandrov wrote: >>>> On 17/03/2022 08:50, Mattias Forsblad 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. >>>>> This makes is possible to only forward selected traffic between the >>>>> port members of the bridge. >>>>> >>>>> Signed-off-by: Mattias Forsblad >>>>> --- >>>>> include/linux/if_bridge.h | 6 +++++ >>>>> include/uapi/linux/if_bridge.h | 9 ++++++- >>>>> net/bridge/br.c | 45 ++++++++++++++++++++++++++++++++++ >>>>> net/bridge/br_device.c | 3 +++ >>>>> net/bridge/br_input.c | 23 ++++++++++++++--- >>>>> net/bridge/br_private.h | 4 +++ >>>>> 6 files changed, 85 insertions(+), 5 deletions(-) >>>>> >>>> Please always CC bridge maintainers for bridge patches. >>>> I almost missed this one. I'll add my reply to Joachim's notes >>>> which are pretty spot on. >>> >>> And DSA maintainers for DSA patches ;) I was aimlessly scrolling through >>> patchwork when I happened to notice these, and the series is already at v3. >>> >>> As a matter of fact, I downloaded these patches from the mailing list >>> with the intention of giving them a spin on mv88e6xxx to see what >>> they're about, and to my surprise, this particular patch (I haven't even >>> reached the offloading part) breaks DHCP on my bridge, so it can no >>> longer get an IP address. I haven't toggled any bridge flag through >>> netlink, just booted the board with systemd-networkd. The same thing >>> happens with my LS1028A board. Further investigation to come, but this >>> isn't off to a good start, I'm afraid... >>> >>>> >>>>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h >>>>> index 3aae023a9353..fa8e000a6fb9 100644 >>>>> --- a/include/linux/if_bridge.h >>>>> +++ b/include/linux/if_bridge.h >>>>> @@ -157,6 +157,7 @@ static inline int br_vlan_get_info_rcu(const struct net_device *dev, u16 vid, >>>>> struct net_device *br_fdb_find_port(const struct net_device *br_dev, >>>>> const unsigned char *addr, >>>>> __u16 vid); >>>>> +bool br_flood_enabled(const struct net_device *dev); >>>>> void br_fdb_clear_offload(const struct net_device *dev, u16 vid); >>>>> bool br_port_flag_is_set(const struct net_device *dev, unsigned long flag); >>>>> u8 br_port_get_stp_state(const struct net_device *dev); >>>>> @@ -170,6 +171,11 @@ br_fdb_find_port(const struct net_device *br_dev, >>>>> return NULL; >>>>> } >>>>> >>>>> +static inline bool br_flood_enabled(const struct net_device *dev) >>>>> +{ >>>>> + return true; >>>>> +} >>>>> + >>>>> static inline void br_fdb_clear_offload(const struct net_device *dev, u16 vid) >>>>> { >>>>> } >>>>> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h >>>>> index 2711c3522010..765ed70c9b28 100644 >>>>> --- a/include/uapi/linux/if_bridge.h >>>>> +++ b/include/uapi/linux/if_bridge.h >>>>> @@ -72,6 +72,7 @@ struct __bridge_info { >>>>> __u32 tcn_timer_value; >>>>> __u32 topology_change_timer_value; >>>>> __u32 gc_timer_value; >>>>> + __u8 flood; >>>>> }; >>>>> >>>>> struct __port_info { >>>>> @@ -752,13 +753,19 @@ struct br_mcast_stats { >>>>> /* bridge boolean options >>>>> * BR_BOOLOPT_NO_LL_LEARN - disable learning from link-local packets >>>>> * BR_BOOLOPT_MCAST_VLAN_SNOOPING - control vlan multicast snooping >>>>> + * BR_BOOLOPT_FLOOD - control bridge flood flag >>>>> + * BR_BOOLOPT_MCAST_FLOOD - control bridge multicast flood flag >>>>> + * BR_BOOLOPT_BCAST_FLOOD - control bridge broadcast flood flag >>>>> * >>>>> * IMPORTANT: if adding a new option do not forget to handle >>>>> - * it in br_boolopt_toggle/get and bridge sysfs >>>>> + * it in br_boolopt_toggle/get >>>>> */ >>>>> enum br_boolopt_id { >>>>> BR_BOOLOPT_NO_LL_LEARN, >>>>> BR_BOOLOPT_MCAST_VLAN_SNOOPING, >>>>> + BR_BOOLOPT_FLOOD, >>>>> + BR_BOOLOPT_MCAST_FLOOD, >>>>> + BR_BOOLOPT_BCAST_FLOOD, >>>>> BR_BOOLOPT_MAX >>>>> }; >>>>> >>>>> diff --git a/net/bridge/br.c b/net/bridge/br.c >>>>> index b1dea3febeea..63a17bed6c63 100644 >>>>> --- a/net/bridge/br.c >>>>> +++ b/net/bridge/br.c >>>>> @@ -265,6 +265,11 @@ int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id opt, bool on, >>>>> case BR_BOOLOPT_MCAST_VLAN_SNOOPING: >>>>> err = br_multicast_toggle_vlan_snooping(br, on, extack); >>>>> break; >>>>> + case BR_BOOLOPT_FLOOD: >>>>> + case BR_BOOLOPT_MCAST_FLOOD: >>>>> + case BR_BOOLOPT_BCAST_FLOOD: >>>>> + err = br_flood_toggle(br, opt, on); >>>>> + break; >>>>> default: >>>>> /* shouldn't be called with unsupported options */ >>>>> WARN_ON(1); >>>>> @@ -281,6 +286,12 @@ int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt) >>>>> return br_opt_get(br, BROPT_NO_LL_LEARN); >>>>> case BR_BOOLOPT_MCAST_VLAN_SNOOPING: >>>>> return br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED); >>>>> + case BR_BOOLOPT_FLOOD: >>>>> + return br_opt_get(br, BROPT_FLOOD); >>>>> + case BR_BOOLOPT_MCAST_FLOOD: >>>>> + return br_opt_get(br, BROPT_MCAST_FLOOD); >>>>> + case BR_BOOLOPT_BCAST_FLOOD: >>>>> + return br_opt_get(br, BROPT_BCAST_FLOOD); >>>>> default: >>>>> /* shouldn't be called with unsupported options */ >>>>> WARN_ON(1); >>>>> @@ -325,6 +336,40 @@ void br_boolopt_multi_get(const struct net_bridge *br, >>>>> bm->optmask = GENMASK((BR_BOOLOPT_MAX - 1), 0); >>>>> } >>>>> >>>>> +int br_flood_toggle(struct net_bridge *br, enum br_boolopt_id opt, >>>>> + bool on) >>>>> +{ >>>>> + struct switchdev_attr attr = { >>>>> + .orig_dev = br->dev, >>>>> + .id = SWITCHDEV_ATTR_ID_BRIDGE_FLOOD, >>>>> + .flags = SWITCHDEV_F_DEFER, >>>>> + }; >>>>> + enum net_bridge_opts bropt; >>>>> + int ret; >>>>> + >>>>> + switch (opt) { >>>>> + case BR_BOOLOPT_FLOOD: >>>>> + bropt = BROPT_FLOOD; >>>>> + break; >>>>> + case BR_BOOLOPT_MCAST_FLOOD: >>>>> + bropt = BROPT_MCAST_FLOOD; >>>>> + break; >>>>> + case BR_BOOLOPT_BCAST_FLOOD: >>>>> + bropt = BROPT_BCAST_FLOOD; >>>>> + break; >>>>> + default: >>>>> + WARN_ON(1); >>>>> + return -EINVAL; >>>>> + } >>>>> + br_opt_toggle(br, bropt, on); >>>>> + >>>>> + attr.u.brport_flags.mask = BIT(bropt); >>>>> + attr.u.brport_flags.val = on << bropt; >>>>> + ret = switchdev_port_attr_set(br->dev, &attr, NULL); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> /* private bridge options, controlled by the kernel */ >>>>> void br_opt_toggle(struct net_bridge *br, enum net_bridge_opts opt, bool on) >>>>> { >>>>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c >>>>> index 8d6bab244c4a..fafaef9d4b3a 100644 >>>>> --- a/net/bridge/br_device.c >>>>> +++ b/net/bridge/br_device.c >>>>> @@ -524,6 +524,9 @@ void br_dev_setup(struct net_device *dev) >>>>> br->bridge_hello_time = br->hello_time = 2 * HZ; >>>>> br->bridge_forward_delay = br->forward_delay = 15 * HZ; >>>>> br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME; >>>>> + br_opt_toggle(br, BROPT_FLOOD, true); >>>>> + br_opt_toggle(br, BROPT_MCAST_FLOOD, true); >>>>> + br_opt_toggle(br, BROPT_BCAST_FLOOD, true); >>>>> dev->max_mtu = ETH_MAX_MTU; >>>>> >>>>> br_netfilter_rtable_init(br); >>>>> 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); >>>>> } 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; >>>>> } >>>>> } >>>>> >>>>> @@ -155,9 +156,13 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >>>>> local_rcv = true; >>>>> br->dev->stats.multicast++; >>>>> } >>>>> + if (!br_opt_get(br, BROPT_MCAST_FLOOD)) >>>>> + local_rcv = false; >>>>> break; >>>>> case BR_PKT_UNICAST: >>>>> dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid); >>>>> + if (!br_opt_get(br, BROPT_FLOOD)) >>>>> + local_rcv = false; >>>>> break; >>>>> default: >>>>> break; >>>>> @@ -166,7 +171,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >>>>> if (dst) { >>>>> unsigned long now = jiffies; >>>>> >>>>> - if (test_bit(BR_FDB_LOCAL, &dst->flags)) >>>>> + if (test_bit(BR_FDB_LOCAL, &dst->flags) && local_rcv) >> >> So this is the line that breaks local termination. Could you explain >> the reasoning for this change? For unicast packets matching a local FDB >> entry, local_rcv used to be irrelevant (and wasn't even set to true, >> unless the bridge device was promiscuous). > > Sorry, it wasn't obvious from the To: field, but the question was > targeted to Mattias and not to Nikolay. > It might as simple be that I've misunderstood that flow. /Mattias