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

Antonio Quartulli
2013-10-15T19:20:23Z

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

On Tue, Oct 15, 2013 at 04:04:31PM +0800, Marek Lindner wrote:
> 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 ?

or we dynamically allocate a buffer of size 'len'? In this way we don't need to
change icmp_packet each time (hopefully not so many but still..) the
"largest available ICMP packet type" changes.

>
>
> > +/**
> > + * 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 ?

dump == drop ? in that case I agree. Delivering unknown packets to batctl may
also be dangerous.

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

do you mean using the ICMP header to understand what packet it is and then
behave accordingly? Also in this case I agree with you :)

Moving to a packet type based "check" was also part of the original idea of this
generalisation (if I remember correctly), but not really needed when looking at
avoiding further compatibility breakage (this should be the reason why this
"feature" is not part of this patch).

I am not replying on Simon's behalf, I was just eager to share my 2 cents.

Cheers,

--
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]