Re: [B.A.T.M.A.N.] [PATCHv9 4/4] batman-adv: Forward IGMP/MLD reports to selected querier (only)

[Simon Wunderlich <sw@simonwunderlich.de>]

[-- Attachment #1: Type: text/plain, Size: 6050 bytes --]

Hi Linus,

On Sunday 07 September 2014 07:42:40 Linus Lüssing wrote:
> With this patch IGMP or MLD reports are only forwarded to the selected
> IGMP/MLD querier as RFC4541 suggests.
> 
> This is necessary to avoid multicast packet loss in bridged scenarios:
> An IGMPv2/MLDv1 querier does not actively join the multicast group the
> reports are sent to. Because of this, this leads to snooping
> bridges/switches not being able to learn of multicast listeners in the
> mesh and wrongly shutting down ports for multicast traffic to the mesh.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> ---
>  compat.h         |    7 +
>  multicast.c      |  375
> +++++++++++++++++++++++++++++++++++++++++++++++++++++- multicast.h      |  
>  8 ++
>  soft-interface.c |   11 ++
>  types.h          |    4 +
>  5 files changed, 399 insertions(+), 6 deletions(-)

As a general note: This patch adds ~400 lines for mainly processing IGMP/MLD 
stuff, right? Shouldn't this kind of IP/IGMP/MLD report detection and 
memorizing the Querier rather go in the bridge code instead of batman-adv, or 
is there any functionality there already? IMHO the preferred position would be 
the bridge code since its more general/used by more people on one hand, and 
does not bloat batman-adv on the other hand. :)

Could you give us some comments regarding who should do IGMP/MLD processing in 
the kernel so we are prepared for these kind of upstream questions?

Just some small notes regarding the code below:


> [...]
> --- a/multicast.c
> +++ b/multicast.c
> [...]
> @@ -512,10 +515,247 @@ out:
>  }
> 
>  /**
> + * batadv_mcast_is_valid_igmp - check for an IGMP packet
> + * @skb: the IPv4 packet to check
> + *
> + * Checks whether a given IPv4 packet is a valid IGMP packet and if so
> + * sets the skb transport header accordingly.
> + *
> + * The caller needs to ensure the correct setup of the skb network header.
> + * This call might reallocate skb data.
> + */
> +static bool batadv_mcast_is_valid_igmp(struct sk_buff *skb)
> +{
> +	struct iphdr *iphdr;
> +	unsigned int len = skb_network_offset(skb) + sizeof(*iphdr);
> +
> +	if (!pskb_may_pull(skb, len))
> +		return false;
> +
> +	iphdr = ip_hdr(skb);
> +
> +	if (iphdr->ihl < 5 || iphdr->version != 4)
> +		return false;
> +
> +	if (ntohs(iphdr->tot_len) < ip_hdrlen(skb))
> +		return false;
> +
> +	len += ip_hdrlen(skb) - sizeof(*iphdr);
> +	skb_set_transport_header(skb, len);
> +
> +	if (iphdr->protocol != IPPROTO_IGMP)
> +		return false;
> +
> +	/* TODO: verify checksums */
> +
> +	if (!pskb_may_pull(skb, len + sizeof(struct igmphdr)))
> +		return false;
> +
> +	/* RFC2236+RFC3376 (IGMPv2+IGMPv3) require the multicast link layer
> +	 * all-systems destination addresses (224.0.0.1) for general queries
> +	 */
> +	if (igmp_hdr(skb)->type == IGMP_HOST_MEMBERSHIP_QUERY &&
> +	    !igmp_hdr(skb)->group &&
> +	    ip_hdr(skb)->daddr != htonl(INADDR_ALLHOSTS_GROUP))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * batadv_mcast_is_report_ipv4 - check for IGMP reports (and queries)
> + * @bat_priv: the bat priv with all the soft interface information
> + * @skb: the ethernet frame destined for the mesh
> + * @orig: if IGMP report: to be set to the querier to forward the skb to
> + *
> + * Checks whether the given frame is an IGMP report and if so sets the
> + * orig pointer to the originator of the selected IGMP querier if one
> exists + * and returns true. Otherwise returns false.
> + *
> + * If the packet is a general IGMP query instead then we delete the
> memorized, + * foreign IGMP querier (if one exists): We became the selected
> querier and + * therefore do not need to forward reports into the mesh.
> + *
> + * This call might reallocate skb data.
> + */
> +static bool batadv_mcast_is_report_ipv4(struct batadv_priv *bat_priv,
> +					struct sk_buff *skb,
> +					struct batadv_orig_node **orig)
> +{
> +	struct batadv_mcast_querier_state *querier;
> +	struct batadv_orig_node *orig_node;
> +
> +	if (!batadv_mcast_is_valid_igmp(skb))
> +		return false;
> +
> +	querier = &bat_priv->mcast.querier_ipv4;
> +
> +	switch (igmp_hdr(skb)->type) {
> +	case IGMP_HOST_MEMBERSHIP_REPORT:
> +	case IGMPV2_HOST_MEMBERSHIP_REPORT:
> +	case IGMPV3_HOST_MEMBERSHIP_REPORT:
> +		rcu_read_lock();
> +		orig_node = rcu_dereference(querier->orig);
> +		if (orig_node && atomic_inc_not_zero(&orig_node->refcount)) {
> +			/* TODO: include multicast routers via MRD (RFC4286) */
> +			batadv_dbg(BATADV_DBG_MCAST, bat_priv,
> +				   "Redirecting IGMP Report to %pM\n",
> +				   orig_node->orig);
> +			*orig = orig_node;
> +		}
> +		rcu_read_unlock();
> +
> +		return true;
> +	case IGMP_HOST_MEMBERSHIP_QUERY:
> +		/* RFC4541, section 2.1.1.1.b) says:
> +		 * ignore general queries from 0.0.0.0
> +		 */
> +		if (!ip_hdr(skb)->saddr || igmp_hdr(skb)->group)
> +			break;

Why do we break here if group != 0? This looks wrong, maybe you meant && or 
group != 0?

> [...]
> diff --git a/soft-interface.c b/soft-interface.c
> index d4d892c..0f5fb30 100644
> --- a/soft-interface.c
> +++ b/soft-interface.c
> @@ -183,6 +183,7 @@ static int batadv_interface_tx(struct sk_buff *skb,
>  	if (atomic_read(&bat_priv->mesh_state) != BATADV_MESH_ACTIVE)
>  		goto dropped;
> 
> +	skb_set_network_header(skb, ETH_HLEN);
>  	soft_iface->trans_start = jiffies;
>  	vid = batadv_get_vid(skb, 0);
>  	ethhdr = eth_hdr(skb);
> @@ -397,7 +398,9 @@ void batadv_interface_rx(struct net_device *soft_iface,
>  	/* skb->dev & skb->pkt_type are set here */
>  	if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
>  		goto dropped;
> +
>  	skb->protocol = eth_type_trans(skb, soft_iface);
> +	skb_reset_network_header(skb);

Why do we need that header stuff here? And there is an extra newline too ... If 
you need that for the multicast stuff better move it there, otherwise it is set 
for every packet ...

Cheers,
   Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]