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 X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B2FDBC7618B for ; Thu, 25 Jul 2019 13:21:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6C219206A2 for ; Thu, 25 Jul 2019 13:21:25 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=cumulusnetworks.com header.i=@cumulusnetworks.com header.b="Po0NeTTx" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729984AbfGYNVY (ORCPT ); Thu, 25 Jul 2019 09:21:24 -0400 Received: from mail-wr1-f68.google.com ([209.85.221.68]:42633 "EHLO mail-wr1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725808AbfGYNVY (ORCPT ); Thu, 25 Jul 2019 09:21:24 -0400 Received: by mail-wr1-f68.google.com with SMTP id x1so859423wrr.9 for ; Thu, 25 Jul 2019 06:21:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cumulusnetworks.com; s=google; h=subject:from:to:references:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=sHoRY+mXic7NkNfPFucr+22Si1CXT/E0fQIjB3vP5E4=; b=Po0NeTTxb+nvdjvmvbUp8u6K6W2wJNfYZoiEEQgVBKy52QdPpqSOmtHg/zMCOS9AS5 xMbWSEyGg0+PXLtHosfvgCZq7V8YPbCVpoMQBn31WOosHTMSX5vlX2/GWwAt67ta0I6Y M0cyXxQMy7L4t7HVFJHileEePEdJtygUT3IGM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=sHoRY+mXic7NkNfPFucr+22Si1CXT/E0fQIjB3vP5E4=; b=POSpwyRxZ5faUmNTlhnLtVuTtQlLbeeJms3/ax2oulw/Wmb5MIBgL8BgMeodJ9H1XG nFpvdSRCr5Riyzkiqs24qLXQSYk+1otyZvTAEjPCErW9w+z7OroYT8yg955P+xhJH4x0 5tB/zyHF7huPGcd60pdugu3TUKWBC7m/sz/2HQKQdKHqu4IA1Q/I5kzlCb8nRxu+wsH9 x2sVOwIOykniUm0AQt6Z+Er7yZ+IaCqbwVqyV38qOqGOhZjLlz+iP052cjJWsX3kmo5b wSqZyB2DsZ5cZaTjnCodRfHPKmq6khRyunxOpQiPhUmI936Ja56w4gmWQ/xyk3ByCUn7 D3WQ== X-Gm-Message-State: APjAAAWRvTrzVC+kpTuD5hF9njSKSgdOZqro7tkUiOqRkYKQE57V56ko m5byyQbUAQWhjKGEBxggBNHIclzSaRo= X-Google-Smtp-Source: APXvYqzzn3dDK9Dsq4Rqf9c8rrDZiWfqE90XublDccXfwKAa0gI9nnKVLo/yjPHVttqGxqc2BBpLyQ== X-Received: by 2002:adf:ce88:: with SMTP id r8mr21287004wrn.42.1564060881337; Thu, 25 Jul 2019 06:21:21 -0700 (PDT) Received: from [192.168.0.107] (84-238-136-197.ip.btc-net.bg. [84.238.136.197]) by smtp.gmail.com with ESMTPSA id y6sm61292070wmd.16.2019.07.25.06.21.20 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Thu, 25 Jul 2019 06:21:20 -0700 (PDT) Subject: Re: [PATCH] net: bridge: Allow bridge to joing multicast groups From: Nikolay Aleksandrov To: Horatiu Vultur , roopa@cumulusnetworks.com, davem@davemloft.net, bridge@lists.linux-foundation.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, allan.nielsen@microchip.com References: <1564055044-27593-1-git-send-email-horatiu.vultur@microchip.com> <7e7a7015-6072-d884-b2ba-0a51177245ab@cumulusnetworks.com> Message-ID: Date: Thu, 25 Jul 2019 16:21:19 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <7e7a7015-6072-d884-b2ba-0a51177245ab@cumulusnetworks.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 25/07/2019 16:06, Nikolay Aleksandrov wrote: > On 25/07/2019 14:44, Horatiu Vultur wrote: >> There is no way to configure the bridge, to receive only specific link >> layer multicast addresses. From the description of the command 'bridge >> fdb append' is supposed to do that, but there was no way to notify the >> network driver that the bridge joined a group, because LLADDR was added >> to the unicast netdev_hw_addr_list. >> >> Therefore update fdb_add_entry to check if the NLM_F_APPEND flag is set >> and if the source is NULL, which represent the bridge itself. Then add >> address to multicast netdev_hw_addr_list for each bridge interfaces. >> And then the .ndo_set_rx_mode function on the driver is called. To notify >> the driver that the list of multicast mac addresses changed. >> >> Signed-off-by: Horatiu Vultur >> --- >> net/bridge/br_fdb.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 46 insertions(+), 3 deletions(-) >> > > Hi, > I'm sorry but this patch is wrong on many levels, some notes below. In general > NLM_F_APPEND is only used in vxlan, the bridge does not handle that flag at all. > FDB is only for *unicast*, nothing is joined and no multicast should be used with fdbs. > MDB is used for multicast handling, but both of these are used for forwarding. > The reason the static fdbs are added to the filter is for non-promisc ports, so they can > receive traffic destined for these FDBs for forwarding. > If you'd like to join any multicast group please use the standard way, if you'd like to join > it only on a specific port - join it only on that port (or ports) and the bridge and you'll And obviously this is for the case where you're not enabling port promisc mode (non-default). In general you'll only need to join the group on the bridge to receive traffic for it or add it as an mdb entry to forward it. > have the effect that you're describing. What do you mean there's no way ? > > In addition you're allowing a mix of mcast functions to be called with unicast addresses > and vice versa, it is not that big of a deal because the kernel will simply return an error > but still makes no sense. > > Nacked-by: Nikolay Aleksandrov > >> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c >> index b1d3248..d93746d 100644 >> --- a/net/bridge/br_fdb.c >> +++ b/net/bridge/br_fdb.c >> @@ -175,6 +175,29 @@ static void fdb_add_hw_addr(struct net_bridge *br, const unsigned char *addr) >> } >> } >> >> +static void fdb_add_hw_maddr(struct net_bridge *br, const unsigned char *addr) >> +{ >> + int err; >> + struct net_bridge_port *p; >> + >> + ASSERT_RTNL(); >> + >> + list_for_each_entry(p, &br->port_list, list) { >> + if (!br_promisc_port(p)) { >> + err = dev_mc_add(p->dev, addr); >> + if (err) >> + goto undo; >> + } >> + } >> + >> + return; >> +undo: >> + list_for_each_entry_continue_reverse(p, &br->port_list, list) { >> + if (!br_promisc_port(p)) >> + dev_mc_del(p->dev, addr); >> + } >> +} >> + >> /* When a static FDB entry is deleted, the HW address from that entry is >> * also removed from the bridge private HW address list and updates all >> * the ports with needed information. >> @@ -192,13 +215,27 @@ static void fdb_del_hw_addr(struct net_bridge *br, const unsigned char *addr) >> } >> } >> >> +static void fdb_del_hw_maddr(struct net_bridge *br, const unsigned char *addr) >> +{ >> + struct net_bridge_port *p; >> + >> + ASSERT_RTNL(); >> + >> + list_for_each_entry(p, &br->port_list, list) { >> + if (!br_promisc_port(p)) >> + dev_mc_del(p->dev, addr); >> + } >> +} >> + >> static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f, >> bool swdev_notify) >> { >> trace_fdb_delete(br, f); >> >> - if (f->is_static) >> + if (f->is_static) { >> fdb_del_hw_addr(br, f->key.addr.addr); >> + fdb_del_hw_maddr(br, f->key.addr.addr); > > Walking over all ports again for each static delete is a no-go. > >> + } >> >> hlist_del_init_rcu(&f->fdb_node); >> rhashtable_remove_fast(&br->fdb_hash_tbl, &f->rhnode, >> @@ -843,13 +880,19 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source, >> fdb->is_local = 1; >> if (!fdb->is_static) { >> fdb->is_static = 1; >> - fdb_add_hw_addr(br, addr); >> + if (flags & NLM_F_APPEND && !source) >> + fdb_add_hw_maddr(br, addr); >> + else >> + fdb_add_hw_addr(br, addr); >> } >> } else if (state & NUD_NOARP) { >> fdb->is_local = 0; >> if (!fdb->is_static) { >> fdb->is_static = 1; >> - fdb_add_hw_addr(br, addr); >> + if (flags & NLM_F_APPEND && !source) >> + fdb_add_hw_maddr(br, addr); >> + else >> + fdb_add_hw_addr(br, addr); >> } >> } else { >> fdb->is_local = 0; >> >