Re: [PATCH v4 3/5] batman-adv: mcast: implement multicast packet reception and forwarding

["Linus Lüssing" <linus.luessing@c0d3.blue>]

On Sun, Jan 15, 2023 at 06:56:37PM +0100, Sven Eckelmann wrote:
> [...]
> > +static void
> > +batadv_mcast_forw_scrub_dests(struct batadv_priv *bat_priv,
> > +			      struct batadv_neigh_node *comp_neigh, u8 *dest,
> > +			      u8 *next_dest, u16 num_dests)
> > +{
> > +	struct batadv_neigh_node *next_neigh;
> > +
> > +	/* skip first entry, this is what we are comparing with */
> > +	eth_zero_addr(dest);
> > +	dest += ETH_ALEN;
> > +	next_dest += ETH_ALEN;
> > +	num_dests--;
> > +
> > +	batadv_mcast_forw_tracker_for_each_dest(next_dest, num_dests) {
> > +		if (is_zero_ether_addr(next_dest))
> > +			goto scrub_next;
> > +
> > +		if (is_multicast_ether_addr(next_dest)) {
> > +			eth_zero_addr(dest);
> > +			eth_zero_addr(next_dest);
> > +			goto scrub_next;
> > +		}
> > +
> > +		next_neigh = batadv_mcast_forw_orig_to_neigh(bat_priv,
> > +							     next_dest);
> > +		if (!next_neigh) {
> > +			eth_zero_addr(next_dest);
> 
> Why is the original skb not touched in this case?
> 
> It might not be a problem because you are also doing the 
> batadv_mcast_forw_orig_to_neigh check in batadv_mcast_forw_packet. But I was 
> just wondering about it

I actually thought the same at some point, to potentially reduce the
number of neighbor node lookups, and tried it. And then
realized that it can break local reception. The destination
address might be our own one and then the neighbor node lookup
will return NULL, too.

So instead of adding another batadv_is_my_mac() check in yet
another place and branching here in scrub_dests(), I thought I'd keep
it simple.

I could also add these two lines in scrub_dests(), the check and zeroing,
I'm just not quite sure if we would overall gain something with it, as
batadv_is_my_mac() can be a bit expensive at the moment when there are
many interfaces?

> 
> 
> Btw. it could happen that you send out a packet with zero destinations in the 
> TVLV because the neighbor disappeared between the 
> batadv_mcast_forw_orig_to_neigh in batadv_mcast_forw_packet and in 
> batadv_mcast_forw_scrub_dests

Ah good point. Hm, the protocol should be robust enough on
the receiver side to handle it. And it should overall happen very rarely.
I'm indifferent to adding two more lines of code to check that.

> 
> Kind regards,
> 	Sven