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

Marek Lindner
2013-10-15T08:04:31Z

[-- 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 --]