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

Hi,

On Mon, May 13, 2013 at 10:23:01PM +0200, Martin Hundebøll wrote:

[...]

+/**

• • batadv_frag_size_limit - maximum possible size of packet to be fragmented.
• • Returns the maximum size of payload that can be fragmented.
• */

+static int batadv_frag_size_limit(void) +{

• int limit = BATADV_FRAG_MAX_FRAG_SIZE;

a blank line should go here

• limit -= sizeof(struct batadv_frag_packet);
• limit *= BATADV_FRAG_MAX_FRAGMENTS;
• return limit;

+}

+/**

• • batadv_frag_init_chain - check and prepare fragment chain for new fragment.
• • @chain: Chain in fragments table to init.
• • @seqno: Sequence number of the received fragment.
• • Make chain ready for a fragment with sequence number "seqno". Delete existing
• • entries if they have an "old" sequence number.
• • Returns true if chain is empty and caller can just insert the new fragment
• • without searching for the right position.
• • Caller must hold chain->lock.
• */

+static bool batadv_frag_init_chain(struct batadv_frag_table_entry *chain,

• 		   uint16_t seqno)
  

+{

• if (chain->seqno == seqno)

• return false;
  

• if (!hlist_empty(&chain->head))

• batadv_frag_clear_chain(&chain->head);
  

• chain->size = 0;
• chain->seqno = seqno;
• return true;

+}

+/**

• • batadv_frag_insert_packet - insert a fragment into a fragment chain.
• • @orig_node: Originator that the fragment was received from.
• • @skb: skb to insert.
• • @head: head to attach complete chains of fragments to.
• • Insert a new fragment into the reverse ordered chain in the right table
• • entry. The hash table entry is cleared if "old" fragments exist in it.
• • Returns true if skb is buffered, false on error. If the chain has all the
• • fragments needed to merge the packet, the chain is moved to the passed head
• • to avoid locking the chain in the table.
• */

+static bool batadv_frag_insert_packet(struct batadv_orig_node *orig_node,

• 		      struct sk_buff *skb,
  

• 		      struct hlist_head *chain_out)
  

+{

• struct batadv_frag_table_entry *chain;
• struct batadv_frag_list_entry *frag_entry_new, *frag_entry_curr;
• struct batadv_frag_packet *frag_packet;
• uint8_t bucket;
• uint16_t seqno, hdr_size = sizeof(struct batadv_frag_packet);
• if (skb_linearize(skb) < 0)

• goto err;
  

I guess the skb is linearised here so that, later, during the merge, the skbs can be copied by means of memcpy. I think you should put a comment here to explain why this skb_linearise is done, otherwise it seems to be unrelated to everything since the skb data is not accessed in this function (I guess we do it here and not in the merge function so that we do one linearisation at a time instead of linearising all the fragments at once).

Moreover, I'll add a todo saying that we may want to optimise fragmented skbs manipulation in the future and avoid this linearisation at all.

• frag_packet = (struct batadv_frag_packet *)skb->data;
• seqno = ntohs(frag_packet->seqno);
• bucket = seqno % BATADV_FRAG_BUFFER_COUNT;
• frag_entry_new = kmalloc(sizeof(*frag_entry_new), GFP_ATOMIC);
• if (!frag_entry_new)

• goto err;
  

• frag_entry_new->skb = skb;
• frag_entry_new->no = frag_packet->no;
• /* Select entry in the "chain table" and delete any prior fragments

• * with another sequence number. batadv_frag_init_chain() returns true,
  

• * if the list is empty at return.
  

• */
  

• chain = &orig_node->fragments[bucket];
• spin_lock_bh(&chain->lock);
• if (batadv_frag_init_chain(chain, seqno)) {

• hlist_add_head(&frag_entry_new->list, &chain->head);
  

• chain->size = skb->len - hdr_size;
  

• chain->timestamp = jiffies;
  

• goto out;
  

• }
• /* Find the position for the new fragment. */
• hlist_for_each_entry(frag_entry_curr, &chain->head, list) {

• /* Drop packet if fragment already exists. */
  

• if (frag_entry_curr->no == frag_entry_new->no)
  

• 	goto err_unlock;
  

• /* Order fragments from highest to lowest. */
  

• if (frag_entry_curr->no < frag_entry_new->no) {
  

• 	hlist_add_before(&frag_entry_new->list,
  

• 			 &frag_entry_curr->list);
  

• 	chain->size += skb->len - hdr_size;
  

• 	chain->timestamp = jiffies;
  

• 	goto out;
  

• }
  

• }
• /* We reached the end of the list, so insert after 'frag_entry_curr'. */
• if (likely(frag_entry_curr)) {

• hlist_add_after(&frag_entry_curr->list, &frag_entry_new->list);
  

• chain->size += skb->len - hdr_size;
  

• chain->timestamp = jiffies;
  

• }

+out:

• if (chain->size > batadv_frag_size_limit() ||

•    ntohs(frag_packet->total_size) > batadv_frag_size_limit()) {
  

• /* Clear chain if total size of either the list or the packet
  

•  * exceeds the maximum size of one merged packet.
  

•  */
  

• batadv_frag_clear_chain(&chain->head);
  

• chain->size = 0;
  

• } else if (ntohs(frag_packet->total_size) == chain->size) {

• /* All fragments received. Hand over chain to caller. */
  

• hlist_move_list(&chain->head, chain_out);
  

• chain->size = 0;
  

• }
• spin_unlock_bh(&chain->lock);
• return true;

+err_unlock:

• spin_unlock_bh(&chain->lock);
• kfree(frag_entry_new);

+err:

• return false;

+}

[...]

+/*

• • batadv_recv_frag_packet() - Process received fragment.
• • @skb: The received fragment.
• • @recv_if: Interface that the skb is received on.
• • This function does one of the three following things: 1) Forward fragment, if
• • the assembled packet will exceed our MTU; 2) Buffer fragment, if we till
• • lack further fragments; 3) Merge fragments, if we have all needed parts.
• • Returns NET_RX_DROP if the skb is not consumed, NET_RX_SUCCESS otherwise.
• */

+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 *frag_packet;
• int ret = NET_RX_DROP;
• if (batadv_check_unicast_packet(bat_priv, skb,

• 			sizeof(*frag_packet)) < 0)
  

• goto out;
  

• frag_packet = (struct batadv_frag_packet *)skb->data;
• orig_node_src = batadv_orig_hash_find(bat_priv, frag_packet->orig);
• if (!orig_node_src)

• goto out;
  

• /* Route the fragment if it is not for us and too big to be merged. */
• if (!batadv_is_my_mac(bat_priv, frag_packet->dest) &&

•    batadv_frag_skb_fwd(skb, recv_if, orig_node_src)) {
  

• ret = NET_RX_SUCCESS;
  

• goto out;
  

• }
• batadv_inc_counter(bat_priv, BATADV_CNT_FRAG_RX);
• batadv_add_counter(bat_priv, BATADV_CNT_FRAG_RX_BYTES,

• 	   skb->len + ETH_HLEN);
  

why adding ETH_HLEN ? skb->len already counts the outer Ethernet header, no?

• /* Add fragment to buffer and merge if possible. */
• if (!batadv_frag_skb_buffer(&skb, orig_node_src))

• goto out;
  

• /* Deliver merged packet to the appropriate handler, if it was

• * merged
  

• */
  

• if (skb)

• batadv_batman_skb_recv(skb, recv_if->net_dev,
  

• 		       &recv_if->batman_adv_ptype, NULL);
  

• ret = NET_RX_SUCCESS;

+out:

• if (orig_node_src)

• batadv_orig_node_free_ref(orig_node_src);
  

• return ret;

+}

Cheers,