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);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,