Re: [B.A.T.M.A.N.] [PATCH 2/3] batman-adv: Receive fragmented packets and merge

[Marek Lindner <lindner_marek@yahoo.de>]

On Sunday, April 21, 2013 00:17:33 Martin Hundebøll wrote:
> @@ -0,0 +1,372 @@
> +/* Copyright (C) 2012 B.A.T.M.A.N. contributors:

The copyright year could be updated.


> +/**
> + * batadv_frag_clear_list() - Delete entries in the fragment buffer list.
> + * @head: head of list with entries.
> + *
> + * Frees fragments in the passed hlist. Should be called with appropriate
> lock. + */

The kernel doc should not contain "()" following the function name and no 
capital letter after the hyphen. You do this multiple times ..


> +static void batadv_frag_clear_list(struct hlist_head *head)
> +{
> +	struct batadv_frag_list_entry *entry;
> +	struct hlist_node *node, *node_tmp;
> +
> +	hlist_for_each_safe(node, node_tmp, head) {
> +		entry = hlist_entry(node, struct batadv_frag_list_entry, list);
> +		hlist_del(node);
> +		kfree_skb(entry->skb);
> +		kfree(entry);
> +	}
> +}

Why not hlist_for_each_entry_safe() ?


> +/**
> + * batadv_frag_clear_orig() - Free fragments associated to an orig.
> + * @orig_node: Originator to free fragments from.
> + */
> +void batadv_frag_clear_orig(struct batadv_orig_node *orig_node)
> +{
> +	uint8_t i;
> +
> +	for (i = 0; i < BATADV_FRAG_BUFFER_COUNT; i++) {
> +		spin_lock_bh(&orig_node->fragments[i].lock);
> +		batadv_frag_clear_list(&orig_node->fragments[i].head);
> +		orig_node->fragments[i].size = 0;
> +		spin_unlock_bh(&orig_node->fragments[i].lock);
> +	}
> +}
> +
> +/**
> + * batadv_frag_check_orig() - Free timed out fragments from an orig.
> + * @orig_node: originator to check fragments from.
> + *
> + * Check timestamp of every hlist in fragment table and delete all entries
> if + * timed out.
> + */
> +void batadv_frag_check_orig(struct batadv_orig_node *orig_node)
> +{
> +	struct batadv_frag_table_entry *frags;
> +	uint8_t i;
> +
> +	for (i = 0; i < BATADV_FRAG_BUFFER_COUNT; i++) {
> +		frags = &orig_node->fragments[i];
> +		spin_lock_bh(&frags->lock);
> +
> +		if (!hlist_empty(&frags->head) &&
> +		    batadv_has_timed_out(frags->timestamp, BATADV_FRAG_TIMEOUT))
> +			batadv_frag_clear_list(&frags->head);
> +
> +		spin_unlock_bh(&frags->lock);
> +	}
> +}

batadv_frag_clear_orig() and batadv_frag_check_orig() nearly do the same thing 
except for the timestamp check. Would it be feasible to merge these functions 
into one ?


> +/**
> + * batadv_frag_size_limit() - Maximum possible size of fragmented packet.
> + */

Kernel doc mentioning the return value ?


> +static inline int batadv_frag_size_limit(void)

No need to explicitely "inline" the function. The compiler will do this for 
us.


> +/**
> + * batadv_frag_init_list() - Check and prepare fragment buffer for new
> fragment. 

How about we call individual slices of the large packet "fragments" and the 
list of fragments we buffer "fragment chain" for the sake of clarity ?


> +/**
> + * batadv_frag_insert_packet() - Insert a fragment into an fragment
> buffer.

Same here. The "an" is a typo.


> +	struct batadv_frag_list_entry *new = NULL, *curr = NULL;
> +	struct batadv_frag_packet *packet;

There seems to be no apparent reason to initialize new and curr ?!
The naming of the three variables "new", "curr" and "packet" make this 
function hard to read. I understand you try to avoid confusion between the 
different types. Do you feel comfortable with "frag_entry_new", 
"frag_entry_curr" and "frag_packet" ?


> +/**
> + * batadv_frag_merge_packets() - Merge a list of fragments.
> + * @head: head if list with fragments.
> + * @skb: Packet with total size of skb after merging.
> + *
> + * Expands the first skb in the list and copies the content of the
> remaining + * skb's into the expanded one. After doing so, it clears the
> list. + *
> + * Returns NULL on error.
> + */

Returns the merged skb or NULL on error.


> +	/* Make room for the rest of the fragments. */
> +	if (pskb_expand_head(skb_out, 0, size - skb->len, GFP_ATOMIC) < 0) {
> +		kfree_skb(skb_out);
> +		goto free;
> +	}

If pskb_expand_head() really fails this function will lead to a crash because 
skb_out points to undefined memory.

Btw, is pskb_expand_head() enough to handle all cases ? Did you try this with 
a large (4000 bytes or more) packet ? 


> diff --git a/fragmentation.h b/fragmentation.h
> new file mode 100644
> index 0000000..16d1a62
> --- /dev/null
> +++ b/fragmentation.h
> @@ -0,0 +1,31 @@
> +/* Copyright (C) 2012 B.A.T.M.A.N. contributors:

Copyright year ...


>  #define BATADV_GW_THRESHOLD	50
> +#define BATADV_FRAG_BUFFER_COUNT 8
> +#define BATADV_FRAG_MAX_FRAGMENTS 16
> +#define BATADV_FRAG_MAX_FRAG_SIZE 1400
> +#define BATADV_FRAG_TIMEOUT 10000

Some comments explaining the meaning of these defines would be nice.


> +/**
> + * struct batadv_frag_packet - Network structure for fragments.
> + * @header: Common batman-adv header with type, compatibility version, and
> ttl. + * @dest: Final destination used when routing fragments.
> + * @orig: Originator of the fragment used when merging the packet.
> + * @no: Fragment number within this sequence.
> + * @reserved: Unused
> + * @seqno: Sequence identification.
> + * @total_size: Size of the merged packet.
> + */
> +struct batadv_frag_packet {
> +	struct  batadv_header header;
> +	uint8_t dest[ETH_ALEN];
> +	uint8_t orig[ETH_ALEN];
> +	uint8_t no:4;
> +	uint8_t reserved:4;
> +	__be16  seqno;
> +	__be16  total_size;
> +} __packed;

If you moved the no/reserved byte between header and dest the packet would be 
properly aligned. Furthermore, the bitfield requires manual endianess 
"support" (as we recently learned). You might want to check the tvlv patches 
as reference.

Last but not least, Simon proposed patches to route unicast packets without 
knowing their type. This requires the dest field to be in the right place. 
That is one of the reasons for the BUILD_BUG_ON() macro. You removed the old 
one but forgot to add the new fragment check.


> +int batadv_recv_frag_packet(struct sk_buff *skb,
> +			    struct batadv_hard_iface *recv_if)
> +{
> +	struct batadv_priv *bat_priv = netdev_priv(recv_if->soft_iface);
> +	struct batadv_orig_node *orig_node_src = NULL;
> +	struct batadv_frag_packet *packet;

Same nitpick here: packet isn't that clear - frag_packet would be better.


>@@ -178,6 +206,7 @@ struct batadv_orig_node {
>        spinlock_t in_coding_list_lock; /* Protects in_coding_list */
>        spinlock_t out_coding_list_lock; /* Protects out_coding_list */
> #endif
>+       struct batadv_frag_table_entry fragments[BATADV_FRAG_BUFFER_COUNT];
> };

Kernel doc ?


> @@ -307,6 +336,10 @@ enum batadv_counters {
>  	BATADV_CNT_MGMT_TX_BYTES,
>  	BATADV_CNT_MGMT_RX,
>  	BATADV_CNT_MGMT_RX_BYTES,
> +	BATADV_CNT_FRAG_RX,
> +	BATADV_CNT_FRAG_RX_BYTES,
> +	BATADV_CNT_FRAG_FWD,
> +	BATADV_CNT_FRAG_FWD_BYTES,
>  	BATADV_CNT_TT_REQUEST_TX,
>  	BATADV_CNT_TT_REQUEST_RX,
>  	BATADV_CNT_TT_RESPONSE_TX,

Kernel doc ?

Cheers,
Marek