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)
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;
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,
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 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.
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 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
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Marek Lindner -
Sven Eckelmann