| Message ID | 20250522195952.29265-2-linus.luessing@c0d3.blue |
|---|---|
| State | New |
| Headers | show |
| Series | net: bridge: propagate safe mcast snooping to switchdev + DSA | expand |
Hi Linus, kernel test robot noticed the following build errors: [auto build test ERROR on net/main] [also build test ERROR on linus/master v6.15-rc7 next-20250523] [cannot apply to net-next/main horms-ipvs/master] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Linus-L-ssing/net-bridge-mcast-explicitly-track-active-state/20250523-040914 base: net/main patch link: https://lore.kernel.org/r/20250522195952.29265-2-linus.luessing%40c0d3.blue patch subject: [PATCH net-next 1/5] net: bridge: mcast: explicitly track active state config: x86_64-buildonly-randconfig-004-20250523 (https://download.01.org/0day-ci/archive/20250523/202505231710.LnCUsJMF-lkp@intel.com/config) compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250523/202505231710.LnCUsJMF-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202505231710.LnCUsJMF-lkp@intel.com/ All errors (new ones prefixed by >>): >> net/bridge/br_multicast.c:1135:6: error: expected value in expression 1135 | #elif | ^ >> net/bridge/br_multicast.c:1180:11: error: no member named 'ip6_active' in 'struct net_bridge_mcast'; did you mean 'ip4_active'? 1180 | brmctx->ip6_active = ip6_active; | ^~~~~~~~~~ | ip4_active include/trace/events/../../../net/bridge/br_private.h:161:10: note: 'ip4_active' declared here 161 | bool ip4_active; | ^ 2 errors generated. vim +1135 net/bridge/br_multicast.c 1122 1123 static int br_ip6_multicast_check_active(struct net_bridge_mcast *brmctx, 1124 bool *active) 1125 { 1126 #if IS_ENABLED(CONFIG_IPV6) 1127 if (!__br_multicast_querier_exists(brmctx, &brmctx->ip6_other_query, 1128 true)) 1129 *active = false; 1130 1131 if (brmctx->ip6_active == *active) 1132 return false; 1133 1134 return true; > 1135 #elif 1136 *active = false; 1137 return false; 1138 #endif 1139 } 1140 1141 /** 1142 * __br_multicast_update_active() - update mcast active state 1143 * @brmctx: the bridge multicast context to check 1144 * @force_inactive: forcefully deactivate mcast active state 1145 * @extack: netlink extended ACK structure 1146 * 1147 * This (potentially) updates the IPv4/IPv6 multicast active state. And by 1148 * that enables or disables snooping of multicast payload traffic in fast 1149 * path. 1150 * 1151 * The multicast active state is set, per protocol family, if: 1152 * 1153 * - an IGMP/MLD querier is present 1154 * - for own IPv6 MLD querier: an IPv6 address is configured on the bridge 1155 * 1156 * And is unset otherwise. 1157 * 1158 * This function should be called by anything that changes one of the 1159 * above prerequisites. 1160 * 1161 * Return: 0 on success, a negative value otherwise. 1162 */ 1163 static int __br_multicast_update_active(struct net_bridge_mcast *brmctx, 1164 bool force_inactive, 1165 struct netlink_ext_ack *extack) 1166 { 1167 bool ip4_active, ip6_active, ip4_changed, ip6_changed; 1168 int ret = 0; 1169 1170 lockdep_assert_held_once(&brmctx->br->multicast_lock); 1171 1172 ip4_active = !force_inactive; 1173 ip6_active = !force_inactive; 1174 ip4_changed = br_ip4_multicast_check_active(brmctx, &ip4_active); 1175 ip6_changed = br_ip6_multicast_check_active(brmctx, &ip6_active); 1176 1177 if (ip4_changed) 1178 brmctx->ip4_active = ip4_active; 1179 if (ip6_changed) > 1180 brmctx->ip6_active = ip6_active; 1181 1182 return ret; 1183 } 1184
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software. On Thu, May 22, 2025 at 09:17:03PM +0200, Linus Lüssing wrote: > Combining and tracking the active state into new, per protocol family > variables. > > For one thing this slightly reduces the work and checks in fast path > for multicast payload traffic. For another this is in preparation to > be able to notify DSA/switchdev on the (in)applicability of multicast > snooping on multicast payload traffic. Especially on vanishing IGMP/MLD > queriers. Adding more code in the control path in order to simplify the data path makes sense, but IMO this patch is difficult to review and should be split into smaller patches. At the very least the patch can be split into a control path patch and a data path patch. The latter simplifies the data path by making use of the new multicast states maintained by the control path. The control path patch can be split into smaller patches where each patch updates the multicast states from the different places that affect them: 1. When the delay timer expires. 2. When we stop getting queries. 3. When the bridge gains or losses an IPv6 address. 4. When "mcast_querier" is changed (globally or per-VLAN). 5. When "mcast_snooping" is changed (globally or per-VLAN). Given the number of patches, consider splitting the offload changes into a separate patchset. It would probably be merged faster that way. See more comments below. > > Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue> > --- > net/bridge/br_device.c | 5 +- > net/bridge/br_input.c | 2 +- > net/bridge/br_multicast.c | 179 +++++++++++++++++++++++++++++++++++--- > net/bridge/br_private.h | 33 ++----- > 4 files changed, 180 insertions(+), 39 deletions(-) > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > index a818fdc22da9..315ed3d33406 100644 > --- a/net/bridge/br_device.c > +++ b/net/bridge/br_device.c > @@ -102,7 +102,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev) > > mdst = br_mdb_entry_skb_get(brmctx, skb, vid); > if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) && > - br_multicast_querier_exists(brmctx, eth_hdr(skb), mdst)) > + br_multicast_snooping_active(brmctx, eth_hdr(skb), mdst)) > br_multicast_flood(mdst, skb, brmctx, false, true); > else > br_flood(br, skb, BR_PKT_MULTICAST, false, true, vid); > @@ -168,7 +168,10 @@ static int br_dev_open(struct net_device *dev) > netdev_update_features(dev); > netif_start_queue(dev); > br_stp_enable_bridge(br); > + > + spin_lock_bh(&br->multicast_lock); > br_multicast_open(br); > + spin_unlock_bh(&br->multicast_lock); > > if (br_opt_get(br, BROPT_MULTICAST_ENABLED)) > br_multicast_join_snoopers(br); > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index 232133a0fd21..0c632655d66c 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -187,7 +187,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > case BR_PKT_MULTICAST: > mdst = br_mdb_entry_skb_get(brmctx, skb, vid); > if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) && > - br_multicast_querier_exists(brmctx, eth_hdr(skb), mdst)) { > + br_multicast_snooping_active(brmctx, eth_hdr(skb), mdst)) { > if ((mdst && mdst->host_joined) || > br_multicast_is_router(brmctx, skb)) { > local_rcv = true; > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c > index dcbf058de1e3..b66d2173e321 100644 > --- a/net/bridge/br_multicast.c > +++ b/net/bridge/br_multicast.c > @@ -1069,6 +1069,125 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge_mcast *brm > return skb; > } > > +static bool > +__br_multicast_querier_exists(struct net_bridge_mcast *brmctx, > + struct bridge_mcast_other_query *querier, > + const bool is_ipv6) Remove the const? > +{ > + bool own_querier_enabled; > + > + if (brmctx->multicast_querier) { > + if (is_ipv6 && !br_opt_get(brmctx->br, BROPT_HAS_IPV6_ADDR)) > + own_querier_enabled = false; > + else > + own_querier_enabled = true; > + } else { > + own_querier_enabled = false; > + } > + > + return !timer_pending(&querier->delay_timer) && > + (own_querier_enabled || timer_pending(&querier->timer)); > +} > + > +static bool br_multicast_querier_exists(struct net_bridge_mcast *brmctx, > + struct ethhdr *eth, Make this const or just pass the EtherType? > + const struct net_bridge_mdb_entry *mdb) > +{ > + switch (eth->h_proto) { > + case (htons(ETH_P_IP)): > + return __br_multicast_querier_exists(brmctx, > + &brmctx->ip4_other_query, false); > +#if IS_ENABLED(CONFIG_IPV6) > + case (htons(ETH_P_IPV6)): > + return __br_multicast_querier_exists(brmctx, > + &brmctx->ip6_other_query, true); > +#endif > + default: > + return !!mdb && br_group_is_l2(&mdb->addr); > + } > +} > + > +static bool br_ip4_multicast_check_active(struct net_bridge_mcast *brmctx, > + bool *active) > +{ > + if (!__br_multicast_querier_exists(brmctx, &brmctx->ip4_other_query, > + false)) > + *active = false; > + > + if (brmctx->ip4_active == *active) > + return false; > + > + return true; > +} > + > +static int br_ip6_multicast_check_active(struct net_bridge_mcast *brmctx, > + bool *active) > +{ > +#if IS_ENABLED(CONFIG_IPV6) > + if (!__br_multicast_querier_exists(brmctx, &brmctx->ip6_other_query, > + true)) > + *active = false; > + > + if (brmctx->ip6_active == *active) > + return false; > + > + return true; > +#elif > + *active = false; > + return false; > +#endif > +} > + > +/** > + * __br_multicast_update_active() - update mcast active state > + * @brmctx: the bridge multicast context to check > + * @force_inactive: forcefully deactivate mcast active state > + * @extack: netlink extended ACK structure > + * > + * This (potentially) updates the IPv4/IPv6 multicast active state. And by > + * that enables or disables snooping of multicast payload traffic in fast > + * path. > + * > + * The multicast active state is set, per protocol family, if: > + * > + * - an IGMP/MLD querier is present > + * - for own IPv6 MLD querier: an IPv6 address is configured on the bridge > + * > + * And is unset otherwise. > + * > + * This function should be called by anything that changes one of the > + * above prerequisites. > + * > + * Return: 0 on success, a negative value otherwise. > + */ > +static int __br_multicast_update_active(struct net_bridge_mcast *brmctx, > + bool force_inactive, > + struct netlink_ext_ack *extack) > +{ > + bool ip4_active, ip6_active, ip4_changed, ip6_changed; > + int ret = 0; > + > + lockdep_assert_held_once(&brmctx->br->multicast_lock); > + > + ip4_active = !force_inactive; > + ip6_active = !force_inactive; > + ip4_changed = br_ip4_multicast_check_active(brmctx, &ip4_active); > + ip6_changed = br_ip6_multicast_check_active(brmctx, &ip6_active); ip{4,6}_changed aren't really used in this patch. I suggest adding them when you need them. In addition, br_ip{4,6}_multicast_check_active() are quite confusing to me. They return a bool, but also modify a bool argument. It would be clearer to derive the existing states from brmctx->ip{4,6}_active and then derive the new states from something like br_ip{4,6}_multicast_can_activate(brmctx) > + > + if (ip4_changed) > + brmctx->ip4_active = ip4_active; > + if (ip6_changed) > + brmctx->ip6_active = ip6_active; > + > + return ret; > +} > + > +static int br_multicast_update_active(struct net_bridge_mcast *brmctx, > + struct netlink_ext_ack *extack) > +{ > + return __br_multicast_update_active(brmctx, false, extack); > +} > + > #if IS_ENABLED(CONFIG_IPV6) > static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge_mcast *brmctx, > struct net_bridge_mcast_port *pmctx, > @@ -1147,10 +1266,12 @@ static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge_mcast *brm > &ip6h->daddr, 0, &ip6h->saddr)) { > kfree_skb(skb); > br_opt_toggle(brmctx->br, BROPT_HAS_IPV6_ADDR, false); > + br_multicast_update_active(brmctx, NULL); > return NULL; > } > > br_opt_toggle(brmctx->br, BROPT_HAS_IPV6_ADDR, true); > + br_multicast_update_active(brmctx, NULL); > ipv6_eth_mc_map(&ip6h->daddr, eth->h_dest); > > hopopt = (u8 *)(ip6h + 1); > @@ -1762,10 +1883,28 @@ static void br_ip6_multicast_querier_expired(struct timer_list *t) > } > #endif > > -static void br_multicast_query_delay_expired(struct timer_list *t) > +static void br_ip4_multicast_query_delay_expired(struct timer_list *t) > { > + struct net_bridge_mcast *brmctx = from_timer(brmctx, t, > + ip4_other_query.delay_timer); > + > + spin_lock(&brmctx->br->multicast_lock); > + br_multicast_update_active(brmctx, NULL); > + spin_unlock(&brmctx->br->multicast_lock); > } > > +#if IS_ENABLED(CONFIG_IPV6) > +static void br_ip6_multicast_query_delay_expired(struct timer_list *t) > +{ > + struct net_bridge_mcast *brmctx = from_timer(brmctx, t, > + ip6_other_query.delay_timer); > + > + spin_lock(&brmctx->br->multicast_lock); > + br_multicast_update_active(brmctx, NULL); > + spin_unlock(&brmctx->br->multicast_lock); > +} > +#endif > + > static void br_multicast_select_own_querier(struct net_bridge_mcast *brmctx, > struct br_ip *ip, > struct sk_buff *skb) > @@ -3981,16 +4120,13 @@ static void br_multicast_query_expired(struct net_bridge_mcast *brmctx, > struct bridge_mcast_own_query *query, > struct bridge_mcast_querier *querier) > { > - spin_lock(&brmctx->br->multicast_lock); > if (br_multicast_ctx_vlan_disabled(brmctx)) > - goto out; > + return; > > if (query->startup_sent < brmctx->multicast_startup_query_count) > query->startup_sent++; > > br_multicast_send_query(brmctx, NULL, query); > -out: > - spin_unlock(&brmctx->br->multicast_lock); > } > > static void br_ip4_multicast_query_expired(struct timer_list *t) > @@ -3998,8 +4134,11 @@ static void br_ip4_multicast_query_expired(struct timer_list *t) > struct net_bridge_mcast *brmctx = from_timer(brmctx, t, > ip4_own_query.timer); > > + spin_lock(&brmctx->br->multicast_lock); > br_multicast_query_expired(brmctx, &brmctx->ip4_own_query, > &brmctx->ip4_querier); > + br_multicast_update_active(brmctx, NULL); > + spin_unlock(&brmctx->br->multicast_lock); > } > > #if IS_ENABLED(CONFIG_IPV6) > @@ -4008,8 +4147,11 @@ static void br_ip6_multicast_query_expired(struct timer_list *t) > struct net_bridge_mcast *brmctx = from_timer(brmctx, t, > ip6_own_query.timer); > > + spin_lock(&brmctx->br->multicast_lock); > br_multicast_query_expired(brmctx, &brmctx->ip6_own_query, > &brmctx->ip6_querier); > + br_multicast_update_active(brmctx, NULL); It is not clear to me why the new states are updated from br_ip{4,6}_multicast_query_expired(). These functions are called when the bridge is the querier and it is time to send a new query. Did you mean to place these in br_ip{4,6}_multicast_querier_expired() which are invoked when the other querier expired? > + spin_unlock(&brmctx->br->multicast_lock); > } > #endif > > @@ -4044,11 +4186,13 @@ void br_multicast_ctx_init(struct net_bridge *br, > brmctx->multicast_membership_interval = 260 * HZ; > > brmctx->ip4_querier.port_ifidx = 0; > + brmctx->ip4_active = 0; > seqcount_spinlock_init(&brmctx->ip4_querier.seq, &br->multicast_lock); > brmctx->multicast_igmp_version = 2; > #if IS_ENABLED(CONFIG_IPV6) > brmctx->multicast_mld_version = 1; > brmctx->ip6_querier.port_ifidx = 0; > + brmctx->ip6_active = 0; > seqcount_spinlock_init(&brmctx->ip6_querier.seq, &br->multicast_lock); > #endif > > @@ -4057,7 +4201,7 @@ void br_multicast_ctx_init(struct net_bridge *br, > timer_setup(&brmctx->ip4_other_query.timer, > br_ip4_multicast_querier_expired, 0); > timer_setup(&brmctx->ip4_other_query.delay_timer, > - br_multicast_query_delay_expired, 0); > + br_ip4_multicast_query_delay_expired, 0); > timer_setup(&brmctx->ip4_own_query.timer, > br_ip4_multicast_query_expired, 0); > #if IS_ENABLED(CONFIG_IPV6) > @@ -4066,7 +4210,7 @@ void br_multicast_ctx_init(struct net_bridge *br, > timer_setup(&brmctx->ip6_other_query.timer, > br_ip6_multicast_querier_expired, 0); > timer_setup(&brmctx->ip6_other_query.delay_timer, > - br_multicast_query_delay_expired, 0); > + br_ip6_multicast_query_delay_expired, 0); > timer_setup(&brmctx->ip6_own_query.timer, > br_ip6_multicast_query_expired, 0); > #endif > @@ -4171,6 +4315,8 @@ static void __br_multicast_open(struct net_bridge_mcast *brmctx) > #if IS_ENABLED(CONFIG_IPV6) > __br_multicast_open_query(brmctx->br, &brmctx->ip6_own_query); > #endif > + > + br_multicast_update_active(brmctx, NULL); > } > > void br_multicast_open(struct net_bridge *br) > @@ -4209,6 +4355,10 @@ static void __br_multicast_stop(struct net_bridge_mcast *brmctx) > timer_delete_sync(&brmctx->ip6_other_query.delay_timer); > timer_delete_sync(&brmctx->ip6_own_query.timer); > #endif > + > + spin_lock_bh(&brmctx->br->multicast_lock); > + __br_multicast_update_active(brmctx, true, NULL); > + spin_unlock_bh(&brmctx->br->multicast_lock); > } > > void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on) > @@ -4234,10 +4384,13 @@ void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on) > vlan->priv_flags ^= BR_VLFLAG_MCAST_ENABLED; > spin_unlock_bh(&br->multicast_lock); > > - if (on) > + if (on) { > + spin_lock_bh(&br->multicast_lock); > __br_multicast_open(&vlan->br_mcast_ctx); > - else > + spin_unlock_bh(&br->multicast_lock); > + } else { > __br_multicast_stop(&vlan->br_mcast_ctx); > + } > } else { > struct net_bridge_mcast *brmctx; > > @@ -4298,10 +4451,13 @@ int br_multicast_toggle_vlan_snooping(struct net_bridge *br, bool on, > br_opt_toggle(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED, on); > > /* disable/enable non-vlan mcast contexts based on vlan snooping */ > - if (on) > + if (on) { > __br_multicast_stop(&br->multicast_ctx); > - else > + } else { > + spin_lock_bh(&br->multicast_lock); > __br_multicast_open(&br->multicast_ctx); > + spin_unlock_bh(&br->multicast_lock); > + } > list_for_each_entry(p, &br->port_list, list) { > if (on) > br_multicast_disable_port(p); > @@ -4663,6 +4819,7 @@ int br_multicast_set_querier(struct net_bridge_mcast *brmctx, unsigned long val) > #endif > > unlock: > + br_multicast_update_active(brmctx, NULL); > spin_unlock_bh(&brmctx->br->multicast_lock); > > return 0; > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index d5b3c5936a79..3d05895a437f 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -158,12 +158,14 @@ struct net_bridge_mcast { > struct bridge_mcast_other_query ip4_other_query; > struct bridge_mcast_own_query ip4_own_query; > struct bridge_mcast_querier ip4_querier; > + bool ip4_active; > #if IS_ENABLED(CONFIG_IPV6) > struct hlist_head ip6_mc_router_list; > struct timer_list ip6_mc_router_timer; > struct bridge_mcast_other_query ip6_other_query; > struct bridge_mcast_own_query ip6_own_query; > struct bridge_mcast_querier ip6_querier; > + bool ip6_active; > #endif /* IS_ENABLED(CONFIG_IPV6) */ > #endif /* CONFIG_BRIDGE_IGMP_SNOOPING */ > }; > @@ -1144,37 +1146,16 @@ br_multicast_is_router(struct net_bridge_mcast *brmctx, struct sk_buff *skb) > } > > static inline bool > -__br_multicast_querier_exists(struct net_bridge_mcast *brmctx, > - struct bridge_mcast_other_query *querier, > - const bool is_ipv6) > -{ > - bool own_querier_enabled; > - > - if (brmctx->multicast_querier) { > - if (is_ipv6 && !br_opt_get(brmctx->br, BROPT_HAS_IPV6_ADDR)) > - own_querier_enabled = false; > - else > - own_querier_enabled = true; > - } else { > - own_querier_enabled = false; > - } > - > - return !timer_pending(&querier->delay_timer) && > - (own_querier_enabled || timer_pending(&querier->timer)); > -} > - > -static inline bool br_multicast_querier_exists(struct net_bridge_mcast *brmctx, > - struct ethhdr *eth, > - const struct net_bridge_mdb_entry *mdb) > +br_multicast_snooping_active(struct net_bridge_mcast *brmctx, > + struct ethhdr *eth, > + const struct net_bridge_mdb_entry *mdb) > { > switch (eth->h_proto) { > case (htons(ETH_P_IP)): > - return __br_multicast_querier_exists(brmctx, > - &brmctx->ip4_other_query, false); > + return brmctx->ip4_active; > #if IS_ENABLED(CONFIG_IPV6) > case (htons(ETH_P_IPV6)): > - return __br_multicast_querier_exists(brmctx, > - &brmctx->ip6_other_query, true); > + return brmctx->ip6_active; > #endif > default: > return !!mdb && br_group_is_l2(&mdb->addr); > -- > 2.49.0 >
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c index a818fdc22da9..315ed3d33406 100644 --- a/net/bridge/br_device.c +++ b/net/bridge/br_device.c @@ -102,7 +102,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev) mdst = br_mdb_entry_skb_get(brmctx, skb, vid); if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) && - br_multicast_querier_exists(brmctx, eth_hdr(skb), mdst)) + br_multicast_snooping_active(brmctx, eth_hdr(skb), mdst)) br_multicast_flood(mdst, skb, brmctx, false, true); else br_flood(br, skb, BR_PKT_MULTICAST, false, true, vid); @@ -168,7 +168,10 @@ static int br_dev_open(struct net_device *dev) netdev_update_features(dev); netif_start_queue(dev); br_stp_enable_bridge(br); + + spin_lock_bh(&br->multicast_lock); br_multicast_open(br); + spin_unlock_bh(&br->multicast_lock); if (br_opt_get(br, BROPT_MULTICAST_ENABLED)) br_multicast_join_snoopers(br); diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index 232133a0fd21..0c632655d66c 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -187,7 +187,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb case BR_PKT_MULTICAST: mdst = br_mdb_entry_skb_get(brmctx, skb, vid); if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) && - br_multicast_querier_exists(brmctx, eth_hdr(skb), mdst)) { + br_multicast_snooping_active(brmctx, eth_hdr(skb), mdst)) { if ((mdst && mdst->host_joined) || br_multicast_is_router(brmctx, skb)) { local_rcv = true; diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index dcbf058de1e3..b66d2173e321 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -1069,6 +1069,125 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge_mcast *brm return skb; } +static bool +__br_multicast_querier_exists(struct net_bridge_mcast *brmctx, + struct bridge_mcast_other_query *querier, + const bool is_ipv6) +{ + bool own_querier_enabled; + + if (brmctx->multicast_querier) { + if (is_ipv6 && !br_opt_get(brmctx->br, BROPT_HAS_IPV6_ADDR)) + own_querier_enabled = false; + else + own_querier_enabled = true; + } else { + own_querier_enabled = false; + } + + return !timer_pending(&querier->delay_timer) && + (own_querier_enabled || timer_pending(&querier->timer)); +} + +static bool br_multicast_querier_exists(struct net_bridge_mcast *brmctx, + struct ethhdr *eth, + const struct net_bridge_mdb_entry *mdb) +{ + switch (eth->h_proto) { + case (htons(ETH_P_IP)): + return __br_multicast_querier_exists(brmctx, + &brmctx->ip4_other_query, false); +#if IS_ENABLED(CONFIG_IPV6) + case (htons(ETH_P_IPV6)): + return __br_multicast_querier_exists(brmctx, + &brmctx->ip6_other_query, true); +#endif + default: + return !!mdb && br_group_is_l2(&mdb->addr); + } +} + +static bool br_ip4_multicast_check_active(struct net_bridge_mcast *brmctx, + bool *active) +{ + if (!__br_multicast_querier_exists(brmctx, &brmctx->ip4_other_query, + false)) + *active = false; + + if (brmctx->ip4_active == *active) + return false; + + return true; +} + +static int br_ip6_multicast_check_active(struct net_bridge_mcast *brmctx, + bool *active) +{ +#if IS_ENABLED(CONFIG_IPV6) + if (!__br_multicast_querier_exists(brmctx, &brmctx->ip6_other_query, + true)) + *active = false; + + if (brmctx->ip6_active == *active) + return false; + + return true; +#elif + *active = false; + return false; +#endif +} + +/** + * __br_multicast_update_active() - update mcast active state + * @brmctx: the bridge multicast context to check + * @force_inactive: forcefully deactivate mcast active state + * @extack: netlink extended ACK structure + * + * This (potentially) updates the IPv4/IPv6 multicast active state. And by + * that enables or disables snooping of multicast payload traffic in fast + * path. + * + * The multicast active state is set, per protocol family, if: + * + * - an IGMP/MLD querier is present + * - for own IPv6 MLD querier: an IPv6 address is configured on the bridge + * + * And is unset otherwise. + * + * This function should be called by anything that changes one of the + * above prerequisites. + * + * Return: 0 on success, a negative value otherwise. + */ +static int __br_multicast_update_active(struct net_bridge_mcast *brmctx, + bool force_inactive, + struct netlink_ext_ack *extack) +{ + bool ip4_active, ip6_active, ip4_changed, ip6_changed; + int ret = 0; + + lockdep_assert_held_once(&brmctx->br->multicast_lock); + + ip4_active = !force_inactive; + ip6_active = !force_inactive; + ip4_changed = br_ip4_multicast_check_active(brmctx, &ip4_active); + ip6_changed = br_ip6_multicast_check_active(brmctx, &ip6_active); + + if (ip4_changed) + brmctx->ip4_active = ip4_active; + if (ip6_changed) + brmctx->ip6_active = ip6_active; + + return ret; +} + +static int br_multicast_update_active(struct net_bridge_mcast *brmctx, + struct netlink_ext_ack *extack) +{ + return __br_multicast_update_active(brmctx, false, extack); +} + #if IS_ENABLED(CONFIG_IPV6) static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge_mcast *brmctx, struct net_bridge_mcast_port *pmctx, @@ -1147,10 +1266,12 @@ static struct sk_buff *br_ip6_multicast_alloc_query(struct net_bridge_mcast *brm &ip6h->daddr, 0, &ip6h->saddr)) { kfree_skb(skb); br_opt_toggle(brmctx->br, BROPT_HAS_IPV6_ADDR, false); + br_multicast_update_active(brmctx, NULL); return NULL; } br_opt_toggle(brmctx->br, BROPT_HAS_IPV6_ADDR, true); + br_multicast_update_active(brmctx, NULL); ipv6_eth_mc_map(&ip6h->daddr, eth->h_dest); hopopt = (u8 *)(ip6h + 1); @@ -1762,10 +1883,28 @@ static void br_ip6_multicast_querier_expired(struct timer_list *t) } #endif -static void br_multicast_query_delay_expired(struct timer_list *t) +static void br_ip4_multicast_query_delay_expired(struct timer_list *t) { + struct net_bridge_mcast *brmctx = from_timer(brmctx, t, + ip4_other_query.delay_timer); + + spin_lock(&brmctx->br->multicast_lock); + br_multicast_update_active(brmctx, NULL); + spin_unlock(&brmctx->br->multicast_lock); } +#if IS_ENABLED(CONFIG_IPV6) +static void br_ip6_multicast_query_delay_expired(struct timer_list *t) +{ + struct net_bridge_mcast *brmctx = from_timer(brmctx, t, + ip6_other_query.delay_timer); + + spin_lock(&brmctx->br->multicast_lock); + br_multicast_update_active(brmctx, NULL); + spin_unlock(&brmctx->br->multicast_lock); +} +#endif + static void br_multicast_select_own_querier(struct net_bridge_mcast *brmctx, struct br_ip *ip, struct sk_buff *skb) @@ -3981,16 +4120,13 @@ static void br_multicast_query_expired(struct net_bridge_mcast *brmctx, struct bridge_mcast_own_query *query, struct bridge_mcast_querier *querier) { - spin_lock(&brmctx->br->multicast_lock); if (br_multicast_ctx_vlan_disabled(brmctx)) - goto out; + return; if (query->startup_sent < brmctx->multicast_startup_query_count) query->startup_sent++; br_multicast_send_query(brmctx, NULL, query); -out: - spin_unlock(&brmctx->br->multicast_lock); } static void br_ip4_multicast_query_expired(struct timer_list *t) @@ -3998,8 +4134,11 @@ static void br_ip4_multicast_query_expired(struct timer_list *t) struct net_bridge_mcast *brmctx = from_timer(brmctx, t, ip4_own_query.timer); + spin_lock(&brmctx->br->multicast_lock); br_multicast_query_expired(brmctx, &brmctx->ip4_own_query, &brmctx->ip4_querier); + br_multicast_update_active(brmctx, NULL); + spin_unlock(&brmctx->br->multicast_lock); } #if IS_ENABLED(CONFIG_IPV6) @@ -4008,8 +4147,11 @@ static void br_ip6_multicast_query_expired(struct timer_list *t) struct net_bridge_mcast *brmctx = from_timer(brmctx, t, ip6_own_query.timer); + spin_lock(&brmctx->br->multicast_lock); br_multicast_query_expired(brmctx, &brmctx->ip6_own_query, &brmctx->ip6_querier); + br_multicast_update_active(brmctx, NULL); + spin_unlock(&brmctx->br->multicast_lock); } #endif @@ -4044,11 +4186,13 @@ void br_multicast_ctx_init(struct net_bridge *br, brmctx->multicast_membership_interval = 260 * HZ; brmctx->ip4_querier.port_ifidx = 0; + brmctx->ip4_active = 0; seqcount_spinlock_init(&brmctx->ip4_querier.seq, &br->multicast_lock); brmctx->multicast_igmp_version = 2; #if IS_ENABLED(CONFIG_IPV6) brmctx->multicast_mld_version = 1; brmctx->ip6_querier.port_ifidx = 0; + brmctx->ip6_active = 0; seqcount_spinlock_init(&brmctx->ip6_querier.seq, &br->multicast_lock); #endif @@ -4057,7 +4201,7 @@ void br_multicast_ctx_init(struct net_bridge *br, timer_setup(&brmctx->ip4_other_query.timer, br_ip4_multicast_querier_expired, 0); timer_setup(&brmctx->ip4_other_query.delay_timer, - br_multicast_query_delay_expired, 0); + br_ip4_multicast_query_delay_expired, 0); timer_setup(&brmctx->ip4_own_query.timer, br_ip4_multicast_query_expired, 0); #if IS_ENABLED(CONFIG_IPV6) @@ -4066,7 +4210,7 @@ void br_multicast_ctx_init(struct net_bridge *br, timer_setup(&brmctx->ip6_other_query.timer, br_ip6_multicast_querier_expired, 0); timer_setup(&brmctx->ip6_other_query.delay_timer, - br_multicast_query_delay_expired, 0); + br_ip6_multicast_query_delay_expired, 0); timer_setup(&brmctx->ip6_own_query.timer, br_ip6_multicast_query_expired, 0); #endif @@ -4171,6 +4315,8 @@ static void __br_multicast_open(struct net_bridge_mcast *brmctx) #if IS_ENABLED(CONFIG_IPV6) __br_multicast_open_query(brmctx->br, &brmctx->ip6_own_query); #endif + + br_multicast_update_active(brmctx, NULL); } void br_multicast_open(struct net_bridge *br) @@ -4209,6 +4355,10 @@ static void __br_multicast_stop(struct net_bridge_mcast *brmctx) timer_delete_sync(&brmctx->ip6_other_query.delay_timer); timer_delete_sync(&brmctx->ip6_own_query.timer); #endif + + spin_lock_bh(&brmctx->br->multicast_lock); + __br_multicast_update_active(brmctx, true, NULL); + spin_unlock_bh(&brmctx->br->multicast_lock); } void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on) @@ -4234,10 +4384,13 @@ void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on) vlan->priv_flags ^= BR_VLFLAG_MCAST_ENABLED; spin_unlock_bh(&br->multicast_lock); - if (on) + if (on) { + spin_lock_bh(&br->multicast_lock); __br_multicast_open(&vlan->br_mcast_ctx); - else + spin_unlock_bh(&br->multicast_lock); + } else { __br_multicast_stop(&vlan->br_mcast_ctx); + } } else { struct net_bridge_mcast *brmctx; @@ -4298,10 +4451,13 @@ int br_multicast_toggle_vlan_snooping(struct net_bridge *br, bool on, br_opt_toggle(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED, on); /* disable/enable non-vlan mcast contexts based on vlan snooping */ - if (on) + if (on) { __br_multicast_stop(&br->multicast_ctx); - else + } else { + spin_lock_bh(&br->multicast_lock); __br_multicast_open(&br->multicast_ctx); + spin_unlock_bh(&br->multicast_lock); + } list_for_each_entry(p, &br->port_list, list) { if (on) br_multicast_disable_port(p); @@ -4663,6 +4819,7 @@ int br_multicast_set_querier(struct net_bridge_mcast *brmctx, unsigned long val) #endif unlock: + br_multicast_update_active(brmctx, NULL); spin_unlock_bh(&brmctx->br->multicast_lock); return 0; diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index d5b3c5936a79..3d05895a437f 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -158,12 +158,14 @@ struct net_bridge_mcast { struct bridge_mcast_other_query ip4_other_query; struct bridge_mcast_own_query ip4_own_query; struct bridge_mcast_querier ip4_querier; + bool ip4_active; #if IS_ENABLED(CONFIG_IPV6) struct hlist_head ip6_mc_router_list; struct timer_list ip6_mc_router_timer; struct bridge_mcast_other_query ip6_other_query; struct bridge_mcast_own_query ip6_own_query; struct bridge_mcast_querier ip6_querier; + bool ip6_active; #endif /* IS_ENABLED(CONFIG_IPV6) */ #endif /* CONFIG_BRIDGE_IGMP_SNOOPING */ }; @@ -1144,37 +1146,16 @@ br_multicast_is_router(struct net_bridge_mcast *brmctx, struct sk_buff *skb) } static inline bool -__br_multicast_querier_exists(struct net_bridge_mcast *brmctx, - struct bridge_mcast_other_query *querier, - const bool is_ipv6) -{ - bool own_querier_enabled; - - if (brmctx->multicast_querier) { - if (is_ipv6 && !br_opt_get(brmctx->br, BROPT_HAS_IPV6_ADDR)) - own_querier_enabled = false; - else - own_querier_enabled = true; - } else { - own_querier_enabled = false; - } - - return !timer_pending(&querier->delay_timer) && - (own_querier_enabled || timer_pending(&querier->timer)); -} - -static inline bool br_multicast_querier_exists(struct net_bridge_mcast *brmctx, - struct ethhdr *eth, - const struct net_bridge_mdb_entry *mdb) +br_multicast_snooping_active(struct net_bridge_mcast *brmctx, + struct ethhdr *eth, + const struct net_bridge_mdb_entry *mdb) { switch (eth->h_proto) { case (htons(ETH_P_IP)): - return __br_multicast_querier_exists(brmctx, - &brmctx->ip4_other_query, false); + return brmctx->ip4_active; #if IS_ENABLED(CONFIG_IPV6) case (htons(ETH_P_IPV6)): - return __br_multicast_querier_exists(brmctx, - &brmctx->ip6_other_query, true); + return brmctx->ip6_active; #endif default: return !!mdb && br_group_is_l2(&mdb->addr);
Combining and tracking the active state into new, per protocol family variables. For one thing this slightly reduces the work and checks in fast path for multicast payload traffic. For another this is in preparation to be able to notify DSA/switchdev on the (in)applicability of multicast snooping on multicast payload traffic. Especially on vanishing IGMP/MLD queriers. Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue> --- net/bridge/br_device.c | 5 +- net/bridge/br_input.c | 2 +- net/bridge/br_multicast.c | 179 +++++++++++++++++++++++++++++++++++--- net/bridge/br_private.h | 33 ++----- 4 files changed, 180 insertions(+), 39 deletions(-)