Re: [B.A.T.M.A.N.] [PATCH] batman-adv: generalize batman-adv icmp packet handling

[Marek Lindner <mareklindner@neomailbox.ch>]

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

On Friday 13 September 2013 18:08:10 Simon Wunderlich wrote:
> +/**
> + * batadv_socket_receive_packet - schedule an icmp packet to be sent to
> userspace + *  on an icmp socket.
> + * @socket_client: the socket this packet belongs to
> + * @icmph: pointer to the header of the icmp packet
> + * @icmp_len: total length of the icmp packet
> + */
>  static void batadv_socket_add_packet(struct batadv_socket_client
> *socket_client, -				     struct batadv_icmp_packet_rr 
*icmp_packet,
> +				     struct batadv_icmp_header *icmph,
>  				     size_t icmp_len)
>  {
>  	struct batadv_socket_packet *socket_packet;
> +	size_t len;
> 
>  	socket_packet = kmalloc(sizeof(*socket_packet), GFP_ATOMIC);
> 
>  	if (!socket_packet)
>  		return;
> 
> +	len = icmp_len;
> +	/* check the maximum length before filling the buffer */
> +	if (len > sizeof(socket_packet->icmp_packet))
> +		len = sizeof(socket_packet->icmp_packet);
> +
>  	INIT_LIST_HEAD(&socket_packet->list);
> -	memcpy(&socket_packet->icmp_packet, icmp_packet, icmp_len);
> +	memcpy(&socket_packet->icmp_packet, icmph, icmp_len);

Shouldn't "len" be used here ?

Besides, if we make everything generic batadv_socket_packet->icmp_packet 
should not be hard-coded to batadv_icmp_packet_rr but the largest available 
ICMP packet type ?


> +/**
> + * batadv_recv_my_icmp_packet - receive an icmp packet locally
> + * @bat_priv: the bat priv with all the soft interface information
> + * @skb: icmp packet to process
> + *
> + * Returns NET_RX_SUCCESS if the packet has been consumed or NET_RX_DROP
> + * otherwise.
> + */
>  static int batadv_recv_my_icmp_packet(struct batadv_priv *bat_priv,
> -				      struct sk_buff *skb, size_t icmp_len)
> +				      struct sk_buff *skb)
>  {
>  	struct batadv_hard_iface *primary_if = NULL;
>  	struct batadv_orig_node *orig_node = NULL;
> -	struct batadv_icmp_packet_rr *icmp_packet;
> +	struct batadv_icmp_header *icmph;
>  	int ret = NET_RX_DROP;
> 
> -	icmp_packet = (struct batadv_icmp_packet_rr *)skb->data;
> +	icmph = (struct batadv_icmp_header *)skb->data;
> 
>  	/* add data to device queue */
> -	if (icmp_packet->icmph.msg_type != BATADV_ECHO_REQUEST) {
> -		batadv_socket_receive_packet(icmp_packet, icmp_len);
> +	if (icmph->msg_type != BATADV_ECHO_REQUEST) {
> +		if (skb_linearize(skb) < 0)
> +			goto out;
> +
> +		batadv_socket_receive_packet(icmph, skb->len);
>  		goto out;
>  	}

Wouldn't it be better to dump unkown icmp types for us instead of copying 
everything to user space ?

Same is true for batadv_socket_write(). We should use the icmp header and not 
assume icmp echo.

Cheers,
Marek

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