[B.A.T.M.A.N.] [PATCH next v2 1/4] batman-adv: Free tp_meter ack skb when it was not consumed
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 --- v2: - rebased on current master - added patch to a common set of related patches
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;
batadv_send_skb_packet used by batadv_send_skb_to_orig and its return value is given directly to callers of batadv_send_skb_packet.
batadv_send_skb_to_orig -> batadv_send_unicast_skb -> batadv_send_skb_packet -> dev_queue_xmit
These callers of batadv_send_skb_to_orig expect that the skb isn't consumed when they receive a -1. But dev_queue_xmit may still have consumed it and still returned -1. Thus the free for the skb would be called twice.
Fixes: e3b8acbff9c8 ("batman-adv: return netdev status in the TX path") Signed-off-by: Sven Eckelmann sven@narfation.org --- v2: - rebased on current master - added patch to a common set of related patches
net/batman-adv/send.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c index 49836da..70a8c1d 100644 --- a/net/batman-adv/send.c +++ b/net/batman-adv/send.c @@ -72,6 +72,7 @@ int batadv_send_skb_packet(struct sk_buff *skb, { struct batadv_priv *bat_priv; struct ethhdr *ethhdr; + int ret;
bat_priv = netdev_priv(hard_iface->soft_iface);
@@ -109,8 +110,15 @@ int batadv_send_skb_packet(struct sk_buff *skb, /* dev_queue_xmit() returns a negative result on error. However on * congestion and traffic shaping, it drops and returns NET_XMIT_DROP * (which is > 0). This will not be treated as an error. + * + * a negative value cannot be returned because it could be interepreted + * as not consumed skb by callers of batadv_send_skb_to_orig. */ - return dev_queue_xmit(skb); + ret = dev_queue_xmit(skb); + if (ret < 0) + ret = NET_XMIT_DROP; + + return ret; send_skb_err: kfree_skb(skb); return NET_XMIT_DROP;
On Monday, June 20, 2016 19:53:28 Sven Eckelmann wrote:
batadv_send_skb_packet used by batadv_send_skb_to_orig and its return value is given directly to callers of batadv_send_skb_packet.
batadv_send_skb_to_orig -> batadv_send_unicast_skb -> batadv_send_skb_packet -> dev_queue_xmitThese callers of batadv_send_skb_to_orig expect that the skb isn't consumed when they receive a -1. But dev_queue_xmit may still have consumed it and still returned -1. Thus the free for the skb would be called twice.
Fixes: e3b8acbff9c8 ("batman-adv: return netdev status in the TX path") Signed-off-by: Sven Eckelmann sven@narfation.org
v2:
- rebased on current master
- added patch to a common set of related patches
net/batman-adv/send.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Applied in revision a20149c.
Thanks, Marek
From: Florian Westphal fw@strlen.de
BATMAN uses it as an intermediate return value to signal forwarding vs. buffering, but it did not return POLICED to callers outside of BATMAN.
Signed-off-by: Florian Westphal fw@strlen.de Signed-off-by: David S. Miller davem@davemloft.net [sven@narfation.org: rebased on top of TX path rewrite] Signed-off-by: Sven Eckelmann sven@narfation.org --- v2: - rebased on current master - added patch to a common set of related patches
net/batman-adv/send.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c index 70a8c1d..80208f1 100644 --- a/net/batman-adv/send.c +++ b/net/batman-adv/send.c @@ -20,6 +20,7 @@
#include <linux/atomic.h> #include <linux/byteorder/generic.h> +#include <linux/errno.h> #include <linux/etherdevice.h> #include <linux/fs.h> #include <linux/if.h> @@ -164,7 +165,7 @@ int batadv_send_unicast_skb(struct sk_buff *skb, * host, NULL can be passed as recv_if and no interface alternating is * attempted. * - * Return: -1 on failure (and the skb is not consumed), NET_XMIT_POLICED if the + * Return: -1 on failure (and the skb is not consumed), -EINPROGRESS if the * skb is buffered for later transmit or the NET_XMIT status returned by the * lower routine if the packet has been passed down. * @@ -199,7 +200,7 @@ int batadv_send_skb_to_orig(struct sk_buff *skb, * network coding fails, then send the packet as usual. */ if (recv_if && batadv_nc_skb_forward(skb, neigh_node)) - ret = NET_XMIT_POLICED; + ret = -EINPROGRESS; else ret = batadv_send_unicast_skb(skb, neigh_node);
On Monday, June 20, 2016 19:54:48 Sven Eckelmann wrote:
From: Florian Westphal fw@strlen.de
BATMAN uses it as an intermediate return value to signal forwarding vs. buffering, but it did not return POLICED to callers outside of BATMAN.
Signed-off-by: Florian Westphal fw@strlen.de Signed-off-by: David S. Miller davem@davemloft.net [sven@narfation.org: rebased on top of TX path rewrite] Signed-off-by: Sven Eckelmann sven@narfation.org
v2:
- rebased on current master
- added patch to a common set of related patches
net/batman-adv/send.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Applied in revision ec76a65.
Thanks, Marek
The dev_xmit_complete function is only necessary to check the return value for *_hard_xmit functions. But batman-adv only uses dev_queue_xmit to send data via the interface queue.
From the kerneldoc of __dev_queue_xmit:
Regardless of the return value, the skb is consumed, so it is currently difficult to retry a send to this method. (You can bump the ref count before sending to hold a reference for retry if you are careful.)
Fixes: e3b8acbff9c8 ("batman-adv: return netdev status in the TX path") Signed-off-by: Sven Eckelmann sven@narfation.org --- v2: - added this patch
net/batman-adv/routing.c | 10 ++++------ net/batman-adv/send.c | 2 +- net/batman-adv/tvlv.c | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c index 2bc9645..af8e119 100644 --- a/net/batman-adv/routing.c +++ b/net/batman-adv/routing.c @@ -274,8 +274,7 @@ static int batadv_recv_my_icmp_packet(struct batadv_priv *bat_priv, if (res == -1) goto out;
- if (dev_xmit_complete(res)) - ret = NET_RX_SUCCESS; + ret = NET_RX_SUCCESS;
break; case BATADV_TP: @@ -335,7 +334,7 @@ static int batadv_recv_icmp_ttl_exceeded(struct batadv_priv *bat_priv, icmp_packet->ttl = BATADV_TTL;
res = batadv_send_skb_to_orig(skb, orig_node, NULL); - if (res != -1 && dev_xmit_complete(res)) + if (res != -1) ret = NET_RX_SUCCESS;
out: @@ -423,7 +422,7 @@ int batadv_recv_icmp_packet(struct sk_buff *skb,
/* route it */ res = batadv_send_skb_to_orig(skb, orig_node, recv_if); - if (res != -1 && dev_xmit_complete(res)) + if (res != -1) ret = NET_RX_SUCCESS;
out: @@ -671,8 +670,7 @@ static int batadv_route_unicast_packet(struct sk_buff *skb, len + ETH_HLEN); }
- if (dev_xmit_complete(res)) - ret = NET_RX_SUCCESS; + ret = NET_RX_SUCCESS;
out: if (orig_node) diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c index 80208f1..3a10d87 100644 --- a/net/batman-adv/send.c +++ b/net/batman-adv/send.c @@ -366,7 +366,7 @@ int batadv_send_skb_unicast(struct batadv_priv *bat_priv, unicast_packet->ttvn = unicast_packet->ttvn - 1;
res = batadv_send_skb_to_orig(skb, orig_node, NULL); - if (res != -1 && dev_xmit_complete(res)) + if (res != -1) ret = NET_XMIT_SUCCESS;
out: diff --git a/net/batman-adv/tvlv.c b/net/batman-adv/tvlv.c index 8c59420..3d1cf0f 100644 --- a/net/batman-adv/tvlv.c +++ b/net/batman-adv/tvlv.c @@ -625,7 +625,7 @@ void batadv_tvlv_unicast_send(struct batadv_priv *bat_priv, u8 *src, memcpy(tvlv_buff, tvlv_value, tvlv_value_len);
res = batadv_send_skb_to_orig(skb, orig_node, NULL); - if (!(res != -1 && dev_xmit_complete(res))) + if (res == -1) kfree_skb(skb); out: batadv_orig_node_put(orig_node);
On Monday, June 20, 2016 19:54:49 Sven Eckelmann wrote:
The dev_xmit_complete function is only necessary to check the return value for *_hard_xmit functions. But batman-adv only uses dev_queue_xmit to send data via the interface queue.
From the kerneldoc of __dev_queue_xmit:
Regardless of the return value, the skb is consumed, so it is currently difficult to retry a send to this method. (You can bump the ref count before sending to hold a reference for retry if you are careful.)Fixes: e3b8acbff9c8 ("batman-adv: return netdev status in the TX path") Signed-off-by: Sven Eckelmann sven@narfation.org
v2:
- added this patch
net/batman-adv/routing.c | 10 ++++------ net/batman-adv/send.c | 2 +- net/batman-adv/tvlv.c | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-)
Applied in revision 1718050.
Thanks, Marek
On Monday, June 20, 2016 19:53:27 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
v2:
- rebased on current master
- added patch to a common set of related patches
net/batman-adv/tp_meter.c | 3 +++ 1 file changed, 3 insertions(+)
Applied in revision ae49875.
Thanks, Marek
b.a.t.m.a.n@lists.open-mesh.org
-
Marek Lindner -
Sven Eckelmann