[B.A.T.M.A.N.] [PATCH next] batman-adv: Avoid skb free for batadv_send_skb_to_orig < -1

[B.A.T.M.A.N.] [PATCH next] batman-adv: Avoid skb free for batadv_send_skb_to_orig < -1

[Sven Eckelmann]

The tp_meter code frees the skb when the batadv_send_skb_to_orig returns <
0. But the batadv_send_skb_to_orig only defines -1 as return code for
failed submits with still valid skbs.

Fixes: 98d7a766b645 ("batman-adv: throughput meter implementation")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Interesting because a patch was submitted to net next to remove NET_XMIT_POLICED
and return -EINPROGRESS instead.

I will maybe later send more patches because the current way of handling
DROPPED/free'd skb/not-freed skb is quite confusing.

 net/batman-adv/tp_meter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c
index ed99afb..bf6bffb 100644
--- a/net/batman-adv/tp_meter.c
+++ b/net/batman-adv/tp_meter.c
@@ -615,7 +615,7 @@ static int batadv_tp_send_msg(struct batadv_tp_vars *tp_vars, const u8 *src,
 	batadv_tp_fill_prerandom(tp_vars, data, data_len);
 
 	r = batadv_send_skb_to_orig(skb, orig_node, NULL);
-	if (r < 0)
+	if (r == -1)
 		kfree_skb(skb);
 
 	if (r == NET_XMIT_SUCCESS)

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

[Sven Eckelmann]

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);
+
 	if (unlikely(r < 0) || (r == NET_XMIT_DROP)) {
 		ret = BATADV_TP_REASON_DST_UNREACHABLE;
 		goto out;

Re: [B.A.T.M.A.N.] [PATCH next] batman-adv: Avoid skb free for batadv_send_skb_to_orig < -1

[Marek Lindner]

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

On Friday, June 10, 2016 18:14:00 Sven Eckelmann wrote:
> The tp_meter code frees the skb when the batadv_send_skb_to_orig returns <
> 0. But the batadv_send_skb_to_orig only defines -1 as return code for
> failed submits with still valid skbs.
> 
> Fixes: 98d7a766b645 ("batman-adv: throughput meter implementation")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> Interesting because a patch was submitted to net next to remove
> NET_XMIT_POLICED and return -EINPROGRESS instead.
> 
> I will maybe later send more patches because the current way of handling
> DROPPED/free'd skb/not-freed skb is quite confusing.
> 
>  net/batman-adv/tp_meter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied in revision 1aaa327.

Thanks,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Antonio Quartulli]

[-- 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 --]

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

[Sven Eckelmann]

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

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Sven Eckelmann]

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

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Antonio Quartulli]

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

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.



-- 
Antonio Quartulli

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