[next] batman-adv: Free tp_meter ack skb when it was not consumed

  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 ?


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

Cheers,
  

On Tuesday 21 June 2016 00:41:15 Antonio Quartulli wrote:
[...]
> 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);

Wouldn't this cause a double free when r != -1 and !dev_xmit_complete
is true?  dev_queue_xmit would have consumed it anyway, right?

And did you mean r and not res?

Kind regards,
	Sven
  

On Tuesday 21 June 2016 00:41:15 Antonio Quartulli wrote:
[...]
> --- 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().

Why are we doing the check in batadv_send_skb_unicast,
batadv_recv_my_icmp_packet, batadv_recv_icmp_ttl_exceeded,
batadv_recv_icmp_packet, batadv_route_unicast_packet and
batadv_tvlv_unicast_send? The doc for this functions says that it is only for
(dev_)hard_start_xmit [1]. So it is for scheduler functions and things which
directly want to send to the hw via netdev_start_xmit.

Or did I miss something?

Kind regards,
	Sven

[1] http://lxr.free-electrons.com/source/include/linux/netdevice.h#L117
  

On Mon, Jun 20, 2016 at 06:55:49PM +0200, Sven Eckelmann wrote:
> On Tuesday 21 June 2016 00:41:15 Antonio Quartulli wrote:
> [...]
> > 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);
> 
> Wouldn't this cause a double free when r != -1 and !dev_xmit_complete
> is true?  dev_queue_xmit would have consumed it anyway, right?
> 
> And did you mean r and not res?

Yes, I meant 'r'. 'res' was the result of a copy/paste error. Sorry.