Re: [B.A.T.M.A.N.] [PATCH next] batman-adv: Free tp_meter ack skb when it was not consumed

[Antonio Quartulli <a@unstable.cc>]

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

On Fri, Jun 10, 2016 at 06:14:01PM +0200, Sven Eckelmann wrote:
> batadv_send_skb_to_orig can return -1 to signal that the skb was not
> consumed. tp_meter has then to free the skb to avoid a memory leak.
> 
> Fixes: 98d7a766b645 ("batman-adv: throughput meter implementation")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> Antonio, this is not really tested. Could you please review it and tell me
> if I may have missed something.
> 
>  net/batman-adv/tp_meter.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c
> index bf6bffb..2333777 100644
> --- a/net/batman-adv/tp_meter.c
> +++ b/net/batman-adv/tp_meter.c
> @@ -1206,6 +1206,9 @@ static int batadv_tp_send_ack(struct batadv_priv *bat_priv, const u8 *dst,
>  
>  	/* send the ack */
>  	r = batadv_send_skb_to_orig(skb, orig_node, NULL);
> +	if (r == -1)
> +		kfree_skb(skb);
> +

Good catch, Sven !

However, how about changing the patch this way ?

--- a/net/batman-adv/tp_meter.c
+++ b/net/batman-adv/tp_meter.c
@@ -1206,7 +1206,7 @@ static int batadv_tp_send_ack(struct batadv_priv *bat_priv, const u8 *dst,
 
        /* send the ack */
        r = batadv_send_skb_to_orig(skb, orig_node, NULL);
-       if (r == -1)
+       if ((r == -1) || !dev_xmit_complete(res))
                kfree_skb(skb);
 
        if (unlikely(r < 0) || (r == NET_XMIT_DROP)) {

this is the same check we do in batadv_send_skb_unicast().

Cheers,

-- 
Antonio Quartulli

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