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

[B.A.T.M.A.N.] [PATCH next v2 1/4] 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>
---
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;
-- 
2.8.1

[B.A.T.M.A.N.] [PATCH next v2 2/4] batman-adv: Don't propagate negative dev_queue_xmit return values

[Sven Eckelmann]

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;
-- 
2.8.1

Re: [B.A.T.M.A.N.] [PATCH next v2 2/4] batman-adv: Don't propagate negative dev_queue_xmit return values

[Marek Lindner]

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

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_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(-)

Applied in revision a20149c.

Thanks,
Marek

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

[B.A.T.M.A.N.] [PATCH next v2 3/4] batman-adv: remove NET_XMIT_POLICED

[Sven Eckelmann]

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);
 
-- 
2.8.1

Re: [B.A.T.M.A.N.] [PATCH next v2 3/4] batman-adv: remove NET_XMIT_POLICED

[Marek Lindner]

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

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

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

[B.A.T.M.A.N.] [PATCH next v2 4/4] batman-adv: Remove unnecessary call to dev_xmit_complete

[Sven Eckelmann]

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);
-- 
2.8.1

Re: [B.A.T.M.A.N.] [PATCH next v2 4/4] batman-adv: Remove unnecessary call to dev_xmit_complete

[Marek Lindner]

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

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

[-- 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 v2 1/4] batman-adv: Free tp_meter ack skb when it was not consumed

[Marek Lindner]

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

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

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