Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Add multicast optimization support for bridged setups

["Linus Lüssing" <linus.luessing@web.de>]

On Tue, Jun 17, 2014 at 10:50:34PM +0800, Marek Lindner wrote:
> On Thursday 12 June 2014 02:00:27 Linus Lüssing wrote:
> > -	netif_addr_lock_bh(dev);
> > +	netif_addr_lock_bh(bridge ? bridge : dev);
> >  	netdev_for_each_mc_addr(mc_list_entry, dev) {
> >  		new = kmalloc(sizeof(*new), GFP_ATOMIC);
> >  		if (!new) {
> 
> Maybe I am missing something but shouldn't we fetch the entries from the 
> bridge interface instead of dev ?

Ups, right, good catch! The 'netdev_for_each...' should be for "bridge",
not "dev" if "bridge" exists.

> > +static void batadv_mcast_mla_br_addr_cpy(char *dst, const struct br_ip
> > *src) +{
> > +	if (src->proto == htons(ETH_P_IP)) {
> > +		/* RFC 1112 */
> > +		memcpy(dst, "\x01\x00\x5e", 3);
> > +		memcpy(dst + 3, ((char *)&src->u.ip4) + 1, ETH_ALEN - 3);
> > +		dst[3] &= 0x7F;
> > +	}
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +	else if (src->proto == htons(ETH_P_IPV6)) {
> > +		/* RFC 2464 */
> > +		memcpy(dst, "\x33\x33", 2);
> > +		memcpy(dst + 2, &src->u.ip6.s6_addr32[3],
> > +		       sizeof(src->u.ip6.s6_addr32[3]));
> > +	}
> > +#endif
> > +	else
> > +		memset(dst, 0, ETH_ALEN);
> > +}
> 
> Is there no cleaner way to do this ? "\x01\x00\x5e" and "\x33\x33" looks 
> pretty hackish. Are there no defines for mcast prefixes somewhere ?

Oh, indeed there is: The bridge code pointed me to
"ip_eth_mc_map()" and "ipv6_eth_mc_map()", making that a one-liner
each :).

> 
> 
> > +/**
> > + * batadv_mcast_mla_bridge_get - get bridged-in multicast listeners
> > + * @dev: a bridge slave whose bridge to collect multicast addresses from
> > + * @mcast_list: a list to put found addresses into
> > + *
> > + * Collects multicast addresses of the bridged-in multicast listeners
> > + * from the bridge on top of the given soft interface, dev, in the
> > + * given mcast_list.
> > + *
> > + * Returns -ENOMEM on memory allocation error or the number of
> > + * items added to the mcast_list otherwise.
> > + */
> > +static int batadv_mcast_mla_bridge_get(struct net_device *dev,
> > +				       struct hlist_head *mcast_list)
> > +{
> 
> That is probably where my confusion is coming from. Why do we query the bridge 
> interface again ?

A batman-adv node needs to know on/behind which other batman-adv
node a multicast listener exists to be able to know where it needs
to forward multicast packets to. A multicast listener can either
sit on bat0, on a bridge on top of bat0. Or in this case, that's
what "mla_bridge_get" is for, behind the bridge.

We usually don't have direct access to the kernel of weird Windows
machines behind a bridge. Luckily a protocol, Multicast Listener
Discovery, exists to detect such multicast listeners.

The bridge code has multicast snooping already, so it already
memorizes what multicast listeners it has behind it's bridge.
That's what we are fetching here. To add these to our local TT to
announce them through the mesh.

> 
> 
> > @@ -195,19 +306,18 @@ static bool batadv_mcast_mla_tvlv_update(struct
> > batadv_priv *bat_priv) mcast_data.flags = BATADV_NO_FLAGS;
> >  	memset(mcast_data.reserved, 0, sizeof(mcast_data.reserved));
> > 
> > -	/* Avoid attaching MLAs, if there is a bridge on top of our soft
> > -	 * interface, we don't support that yet (TODO)
> > -	 */
> > -	if (batadv_mcast_has_bridge(bat_priv)) {
> > -		if (bat_priv->mcast.enabled) {
> > -			batadv_tvlv_container_unregister(bat_priv,
> > -							 BATADV_TVLV_MCAST, 1);
> > -			bat_priv->mcast.enabled = false;
> > -		}
> > +	if (!batadv_mcast_has_bridge(bat_priv))
> > +		goto skip;
> > 
> > -		return false;
> > -	}
> > +	mcast_data.flags |= BATADV_MCAST_WANT_ALL_UNSNOOPABLES;
> > +
> > +	if (br_multicast_has_querier_adjacent(bat_priv->soft_iface, ETH_P_IP))
> > +		mcast_data.flags |= BATADV_MCAST_WANT_ALL_IPV4;
> > 
> > +	if (br_multicast_has_querier_adjacent(bat_priv->soft_iface, ETH_P_IPV6))
> > +		mcast_data.flags |= BATADV_MCAST_WANT_ALL_IPV6;
> > +
> > +skip:
> >  	if (!bat_priv->mcast.enabled ||
> >  	    mcast_data.flags != bat_priv->mcast.flags) {
> >  		batadv_tvlv_container_register(bat_priv, BATADV_TVLV_MCAST, 1,
> 
> Please find a more intuitive goto label ('skip' is quite generic).

Oki doki, good point.

> 
> 
> > @@ -233,13 +344,17 @@ void batadv_mcast_mla_update(struct batadv_priv
> > *bat_priv) int ret;
> > 
> >  	if (!batadv_mcast_mla_tvlv_update(bat_priv))
> > -		goto update;
> > +		goto skip;
> > 
> >  	ret = batadv_mcast_mla_softif_get(soft_iface, &mcast_list);
> >  	if (ret < 0)
> >  		goto out;
> > 
> > -update:
> > +	ret = batadv_mcast_mla_bridge_get(soft_iface, &mcast_list);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +skip:
> >  	batadv_mcast_mla_tt_retract(bat_priv, &mcast_list);
> >  	batadv_mcast_mla_tt_add(bat_priv, &mcast_list);
> 
> Again, 'skip' isn't very self-explanatory.

K, right!

> 
> Cheers,
> Marek
> 

Cheers, Linus