Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: drop QinQ claim frames in bridge loop avoidance

[Marek Lindner <mareklindner@neomailbox.ch>]

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

On Friday 13 June 2014 11:15:50 Simon Wunderlich wrote:
> +/**
> + * batadv_is_encapsulated_claim - checks if a claim frame is encapsulated
> + * @bat_priv: the bat priv with all the soft interface information
> + * @skb: skb to be checked
> + *
> + * Check if this frame is a claim frame encapsulated in at least two layers
> + * of VLANs.

Maybe you mention QinQ here ? 'Two layers of VLANs' isn't that obvious 
(without the commit message).


> + * Returns 1 if this is an encapsulated claim frame, 0 otherwise.

Boolean functions should return bool instead of 0 / 1.


> +	 * At this point it is known that the first two protocols
> +	 * are VLAN headers, so start checking at the encapsulated protocol
> +	 * of the second header.

The first two protocols ? Are you trying to say that at this point we already 
know we are having QinQ ?


> +	 */
> +	headlen = VLAN_ETH_HLEN;
> +	do {
> +		vhdr_ptr = skb_header_pointer(skb, headlen, VLAN_HLEN, &vhdr);
> +		if (!vhdr_ptr)
> +			return 0;
> +
> +		headlen += VLAN_HLEN;
> +	} while (vhdr_ptr->h_vlan_encapsulated_proto == htons(ETH_P_8021Q));

Is there a length check somewhere in skb_header_pointer() ? We wouldn't want 
to read more than we have in the skb, right ?


> +	if (vhdr_ptr->h_vlan_encapsulated_proto != htons(ETH_P_ARP))
> +		return 0;
> +
> +	if (unlikely(!pskb_may_pull(skb, headlen + arp_hdr_len(skb->dev))))
> +		return 0;
> +
> +	/* pskb_may_pull() may have modified the pointers, get ethhdr again */
> +	ethhdr = eth_hdr(skb);
> +	arphdr = (struct arphdr *)((uint8_t *)ethhdr + headlen);
> +
> +	/* Check whether the ARP frame carries a valid IP information */
> +	if (arphdr->ar_hrd != htons(ARPHRD_ETHER))
> +		return 0;
> +	if (arphdr->ar_pro != htons(ETH_P_IP))
> +		return 0;
> +	if (arphdr->ar_hln != ETH_ALEN)
> +		return 0;
> +	if (arphdr->ar_pln != 4)
> +		return 0;
> +
> +	hw_src = (uint8_t *)arphdr + sizeof(struct arphdr);
> +	hw_dst = hw_src + ETH_ALEN + 4;
> +	bla_dst = (struct batadv_bla_claim_dst *)hw_dst;
> +	bla_dst_own = &bat_priv->bla.claim_dest;

Looks like copy & paste from batadv_bla_process_claim() ? How about using a 
shared function ?

Cheers,
Marek

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