Re: [B.A.T.M.A.N.] [PATCH] batman-adv: receive packets directly using skbs

[Sven Eckelmann <sven.eckelmann@gmx.de>]

[-- Attachment #1: Type: Text/Plain, Size: 4371 bytes --]

Simon Wunderlich wrote:
> +/* receive a packet with the batman ethertype coming on a hard
> + * interface */
> +int batman_skb_recv(struct sk_buff *skb, struct net_device *dev,
> +	struct packet_type *ptype, struct net_device *orig_dev)
> +{
> +	struct batman_packet *batman_packet;
> +	struct batman_if *batman_if;
> +	struct net_device_stats *stats;
> +	int ret;
> +
> +    skb = skb_share_check(skb, GFP_ATOMIC);
> +
> +    if (skb == NULL)
> +		goto err_free;
> +
> +	/* packet should hold at least type and version */
> +	if (unlikely(skb->len < 2))
> +		goto err_free;

Len field includes the length of the data in the main socket buffer and 
possible fragments attached to the main socket buffer. This length is changed 
when you move from layer to layer. So it is ok to test for 2 here to check if 
it is enough data for version and type.

> +
> +	batman_if = find_batman_if(skb->dev);
> +	if (!batman_if)
> +		goto err_free;
> +
> +    stats = &skb->dev->stats;
> +    stats->rx_packets++;
> +    stats->rx_bytes += skb->len;
> +
> +
> +	/* push back, we want to look at the ethernet header as well. */
> +	skb_push(skb, sizeof(struct ethhdr));
> +	skb_reset_mac_header(skb);
> +
> +	batman_packet = (struct batman_packet *)(skb->data
> +					+ sizeof(struct ethhdr));
> +
> +	if (batman_packet->version != COMPAT_VERSION) {
> +		bat_dbg(DBG_BATMAN,
> +			"Drop packet: incompatible batman version (%i)\n",
> +			batman_packet->version);
> +		goto err_free;
> +	}
Here you try to to access data which may be fragmented over different buffers. 
This could for example happen if we send our data over a layer which fragments 
it.

In the "normal" scenario of sending it directly over ethernet this shouldn't 
happen, but that doesn't mean that nobody wants to do something "special".

There are different other positions like recv_bat_packet where this is 
ignored.

Maybe someone knows why this could never happen, but everything I have read 
was more like "you have to deal with it".

> -static void recv_icmp_packet(struct ethhdr *ethhdr,
> -			     unsigned char *packet_buff,
> -			     int result,
> -			     struct batman_if *batman_if)
> +int recv_icmp_packet(struct sk_buff *skb)
>  {
>  	struct icmp_packet *icmp_packet;
> +	struct ethhdr *ethhdr;
>  	struct orig_node *orig_node;
> +	int hdr_size = sizeof(struct icmp_packet) + sizeof(struct ethhdr);
> +	int ret;
> 
> +	/* drop packet if it has not necessary minimum size */
> +	if (skb->len < hdr_size)
> +		return NET_RX_DROP;
> +
> +	ethhdr = (struct ethhdr *)skb_mac_header(skb);
> +
>  	/* packet with unicast indication but broadcast recipient */
>  	if (is_bcast(ethhdr->h_dest))
> -		return;
> +		return NET_RX_DROP;
> 
>  	/* packet with broadcast sender address */
>  	if (is_bcast(ethhdr->h_source))
> -		return;
> +		return NET_RX_DROP;
> 
>  	/* not for me */
>  	if (!is_my_mac(ethhdr->h_dest))
> -		return;
> +		return NET_RX_DROP;
> 
> -	/* drop packet if it has not necessary minimum size */
> -	if (result < sizeof(struct ethhdr) + sizeof(struct icmp_packet))
> -		return;
> -
>  	icmp_packet = (struct icmp_packet *)
> -		(packet_buff + sizeof(struct ethhdr));
> +			(skb->data + sizeof(struct ethhdr));
> 
>  	/* packet for me */
>  	if (is_my_mac(icmp_packet->dst))
> -		recv_my_icmp_packet(ethhdr, icmp_packet, packet_buff, result);
> +		return recv_my_icmp_packet(skb);
> 
>  	/* TTL exceeded */
>  	if (icmp_packet->ttl < 2) {
> -		recv_icmp_ttl_exceeded(icmp_packet, ethhdr, packet_buff, result,
> -				       batman_if);
> -		return;
> +		return recv_icmp_ttl_exceeded(skb);
> 
>  	}
> 
> +	ret = NET_RX_DROP;
> +
>  	/* get routing information */
>  	spin_lock(&orig_hash_lock);
>  	orig_node = ((struct orig_node *)
> @@ -725,49 +721,51 @@
>  		icmp_packet->ttl--;

You are modify the data inside the socket buffer (not the control structure 
but the real data), but I cannot see where you created a private copy of it. 
Data of cloned copies is not allowed to be changed. You must use (p)skb_copy 
(it clones the the data too and not only the sk_buff structure).

This is explained in "Understanding Linux Network Internals", "Christian 
Benvenuti", "December 2005, O'Reilly", "2.1.5.5. Cloning and copying buffers"


Best regards,
	Sven

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