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

[Antonio Quartulli <antonio@meshcoding.com>]

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