Re: [B.A.T.M.A.N.] [PATCHv2 3/3] batman-adv: Fragment and send skbs larger than mtu

[Antonio Quartulli <ordex@autistici.org>]

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

Hi³, :)

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

[..]

> +
> +/**
> + * batadv_frag_create - create a fragment from skb.
> + * @skb: skb to create fragment from.
> + * @frag_head: header to use in new fragment.
> + * @mtu: Size of new fragment.
> + *
> + * Split the passed skb into two fragments: A new one with size matching the
> + * passed mtu and the old one with the rest. The new skb contains data from the
> + * tail of the old skb.
> + *
> + * Returns the new fragment, NULL on error.
> + */
> +static struct sk_buff *batadv_frag_create(struct sk_buff *skb,
> +					  struct batadv_frag_packet *frag_head,
> +					  unsigned int mtu)
> +{
> +	struct sk_buff *skb_fragment;
> +	unsigned header_size = sizeof(*frag_head);
> +	unsigned fragment_size = mtu - header_size - ETH_HLEN;
> +
> +	skb_fragment = dev_alloc_skb(mtu);

If I am not wrong, the external Ethernet header is not counted in the MTU of an
interface. Therefore if the MTU of eth0 is X, it should mean that eth0 will send
a packet of 1500 of payload + the external Ethernet encapsulation. isn't it?

> +	if (!skb_fragment)
> +		goto err;
> +
> +	/* Eat the last mtu-bytes of the skb */
> +	skb_reserve(skb_fragment, header_size);

why not reserving header_size + ETH_HLEN (ETH_HLEN would be the space for the
external Ethernet header).

> +	skb_split(skb, skb_fragment, skb->len - fragment_size);
> +
> +	/* Add the header */
> +	skb_push(skb_fragment, header_size);
> +	memcpy(skb_fragment->data, frag_head, header_size);
> +
> +err:
> +	return skb_fragment;
> +}
> +

[...]

> @@ -110,7 +109,19 @@ int batadv_send_skb_to_orig(struct sk_buff *skb,
>  	/* batadv_find_router() increases neigh_nodes refcount if found. */
>  	neigh_node = batadv_find_router(bat_priv, orig_node, recv_if);
>  	if (!neigh_node)
> -		return ret;
> +		goto out;
> +
> +	/* Check if the skb is too large to send in one piece and fragment
> +	 * it if needed.
> +	 */
> +	if (atomic_read(&bat_priv->fragmentation) &&
> +	    skb->len + ETH_HLEN > neigh_node->if_incoming->net_dev->mtu) {

same as before: the external Ethernet header should not be taken into
consideration while checking the MTU (no?).

My suggestion is to do:

"skb->len > neigh_node->if_incoming->net_dev->mtu) {"

> +		/* Fragment and send packet. */
> +		if (batadv_frag_send_packet(skb, orig_node, neigh_node))
> +			ret = NET_XMIT_SUCCESS;
> +
> +		goto out;
> +	}
>  
>  	/* try to network code the packet, if it is received on an interface
>  	 * (i.e. being forwarded). If the packet originates from this node or if

Thank you for your work Martin! :)

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

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