* [B.A.T.M.A.N.] [PATCH next v2 1/4] batman-adv: Free tp_meter ack skb when it was not consumed
@ 2016-06-20 17:53 Sven Eckelmann
2016-06-20 17:53 ` [B.A.T.M.A.N.] [PATCH next v2 2/4] batman-adv: Don't propagate negative dev_queue_xmit return values Sven Eckelmann
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Sven Eckelmann @ 2016-06-20 17:53 UTC (permalink / raw)
To: b.a.t.m.a.n
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [B.A.T.M.A.N.] [PATCH next v2 2/4] batman-adv: Don't propagate negative dev_queue_xmit return values
2016-06-20 17:53 [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
@ 2016-06-20 17:53 ` Sven Eckelmann
2016-07-05 8:38 ` Marek Lindner
2016-06-20 17:54 ` [B.A.T.M.A.N.] [PATCH next v2 3/4] batman-adv: remove NET_XMIT_POLICED Sven Eckelmann
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Sven Eckelmann @ 2016-06-20 17:53 UTC (permalink / raw)
To: b.a.t.m.a.n
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [B.A.T.M.A.N.] [PATCH next v2 3/4] batman-adv: remove NET_XMIT_POLICED
2016-06-20 17:53 [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
2016-06-20 17:53 ` [B.A.T.M.A.N.] [PATCH next v2 2/4] batman-adv: Don't propagate negative dev_queue_xmit return values Sven Eckelmann
@ 2016-06-20 17:54 ` Sven Eckelmann
2016-07-05 8:41 ` Marek Lindner
2016-06-20 17:54 ` [B.A.T.M.A.N.] [PATCH next v2 4/4] batman-adv: Remove unnecessary call to dev_xmit_complete Sven Eckelmann
2016-07-05 8:32 ` [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
3 siblings, 1 reply; 8+ messages in thread
From: Sven Eckelmann @ 2016-06-20 17:54 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Florian Westphal, David S . Miller
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [B.A.T.M.A.N.] [PATCH next v2 4/4] batman-adv: Remove unnecessary call to dev_xmit_complete
2016-06-20 17:53 [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
2016-06-20 17:53 ` [B.A.T.M.A.N.] [PATCH next v2 2/4] batman-adv: Don't propagate negative dev_queue_xmit return values Sven Eckelmann
2016-06-20 17:54 ` [B.A.T.M.A.N.] [PATCH next v2 3/4] batman-adv: remove NET_XMIT_POLICED Sven Eckelmann
@ 2016-06-20 17:54 ` Sven Eckelmann
2016-07-05 8:42 ` Marek Lindner
2016-07-05 8:32 ` [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
3 siblings, 1 reply; 8+ messages in thread
From: Sven Eckelmann @ 2016-06-20 17:54 UTC (permalink / raw)
To: b.a.t.m.a.n
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH next v2 1/4] batman-adv: Free tp_meter ack skb when it was not consumed
2016-06-20 17:53 [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
` (2 preceding siblings ...)
2016-06-20 17:54 ` [B.A.T.M.A.N.] [PATCH next v2 4/4] batman-adv: Remove unnecessary call to dev_xmit_complete Sven Eckelmann
@ 2016-07-05 8:32 ` Marek Lindner
3 siblings, 0 replies; 8+ messages in thread
From: Marek Lindner @ 2016-07-05 8:32 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH next v2 2/4] batman-adv: Don't propagate negative dev_queue_xmit return values
2016-06-20 17:53 ` [B.A.T.M.A.N.] [PATCH next v2 2/4] batman-adv: Don't propagate negative dev_queue_xmit return values Sven Eckelmann
@ 2016-07-05 8:38 ` Marek Lindner
0 siblings, 0 replies; 8+ messages in thread
From: Marek Lindner @ 2016-07-05 8:38 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH next v2 3/4] batman-adv: remove NET_XMIT_POLICED
2016-06-20 17:54 ` [B.A.T.M.A.N.] [PATCH next v2 3/4] batman-adv: remove NET_XMIT_POLICED Sven Eckelmann
@ 2016-07-05 8:41 ` Marek Lindner
0 siblings, 0 replies; 8+ messages in thread
From: Marek Lindner @ 2016-07-05 8:41 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Florian Westphal, David S . Miller
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH next v2 4/4] batman-adv: Remove unnecessary call to dev_xmit_complete
2016-06-20 17:54 ` [B.A.T.M.A.N.] [PATCH next v2 4/4] batman-adv: Remove unnecessary call to dev_xmit_complete Sven Eckelmann
@ 2016-07-05 8:42 ` Marek Lindner
0 siblings, 0 replies; 8+ messages in thread
From: Marek Lindner @ 2016-07-05 8:42 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-07-05 8:42 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-20 17:53 [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
2016-06-20 17:53 ` [B.A.T.M.A.N.] [PATCH next v2 2/4] batman-adv: Don't propagate negative dev_queue_xmit return values Sven Eckelmann
2016-07-05 8:38 ` Marek Lindner
2016-06-20 17:54 ` [B.A.T.M.A.N.] [PATCH next v2 3/4] batman-adv: remove NET_XMIT_POLICED Sven Eckelmann
2016-07-05 8:41 ` Marek Lindner
2016-06-20 17:54 ` [B.A.T.M.A.N.] [PATCH next v2 4/4] batman-adv: Remove unnecessary call to dev_xmit_complete Sven Eckelmann
2016-07-05 8:42 ` Marek Lindner
2016-07-05 8:32 ` [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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).