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