[B.A.T.M.A.N.] (no subject)

[B.A.T.M.A.N.] (no subject)

[Linus Lüssing]

-------------
Hi Linus,

On Wed, Aug 18, 2010 at 00:10 +0200, Linus Lüssing wrote:
> Hi Vasiliy,
>
> Also first of all thanks for your little reviewing of the batman code from
> me too. I've been the person submitting the patch to enable ebtables
> filtering of batman-adv's packets.Sorry, I'm not a kernel coding veteran, so
> I might possibly have missed something ;). However, I don't fully understand
> when the skb should be leaked, so hope you don't mind some more asking from
> my side :).
>
> Okay, I had a little closer look again.NF_HOOK returns -1 in case of a drop
> due to nf_hook_slow() and 1 in case of a success due to nf_hook_slow() + the
> ok function returning 1 too. And hey, yes, nf_hook_slow() can also return 0,
> passing it all up to the NF_HOOK which would lead to the goto err_out in
> batman-adv - and both the kernel module and netfilter won't free the skb!
> However, I think I'm not quite getting when nf_hook_slow() might return 0...

The thing is that code using NF_HOOK should be written in functional
paradigm: the recv() should be divided into 2 functions, before the NF_HOOK
and after. All after-nf work should be delegated to second part, not to code in
first part that is run after NF_HOOK return. It solves the problem of asyncronous
processing in hooks. It is perferctly seen in ipv4 ip_rcv() & rp_rcv_finish().

>
> What confuses me even more is, that of course if nf_hook_slow() could return
> a value other than 1 and without freeing the skb, the batman-adv kernel
> module would have to free the skb itself in send.c, too. In send.c the
> return value of NF_HOOK is being returned there immediately without a check
> at the moment, hoping that either netfilter or dev_queue_xmit() would free
> the skb. But then net/ipv4/arp.c's arp_xmit() would have the same problem,
> too, wouldn't it? It also does not check whether the
> NF_HOOK(/nf_hook_slow()) there might return 0 either, meaning that the skb
> has not yet been consumed/freed? So is there the same bug / possible memleak
> or what is the difference between ipv4's arp.c and batman-adv's
> send.c/hard_interface.c in the usage of the NF_HOOK?

No, there isn't ;) In fact, for the caller of NF_HOOK() three cases may
happed:


1) All hooks return NF_ACCEPT or similar (NF_STOP or through
 NF_QUEUE/NF_REPEAT with the same result). In this case ok_fn() is called
 and code after return from NF_HOOK() is run.

 In case of arp dev_queue_xmit() processes the skb and free it.

 In your case ok_fn() does nothing and code after return from NF_HOOK()
 finishes the processing of skb.


2) Some hook returns NF_DROP or similar (like (1), with the same
 effect for the caller). In this case nf_hook_slow() frees skb and NF_HOOK()
 returns -1. ok_fn() is not called at all.

 Arp and batman don't leak anything as skb is freed.


3) Some hook returns NF_STOLEN signaling that now it is responsible for
 skb delivery and freeing. It can be stolen until some long
 calculations end or even some network communication is finished (e.g.
 hook wants to know whether skb dest ip is google.com)

 a) After some time the hook frees skb and doesn't call ok_fn().

   arp and you don't leak anything and work exactly like NF_HOOK() retuned
   NF_DROPPED.

 b) After some time the hook finishes calcs and passes the skb to
 ok_fn(). From this time ok_fn() is responsible for the skb.

   In arp case it is dev_queue_xmit() that frees skb.

   In batman case it does NOTHING and skb is leaked.


Also see the comment from linux/netfilter.h:

/* Activate hook; either okfn or kfree_skb called, unless a hook
  returns NF_STOLEN (in which case, it's up to the hook to deal with
  the consequences).

  Returns -ERRNO if packet dropped.  Zero means queued, stolen or
  accepted.
*/

/* RR:
  > I don't want nf_hook to return anything because people might forget
  > about async and trust the return value to mean "packet was ok".

  AK:
  Just document it clearly, then you can expect some sense from kernel
  coders :)
*/


Thanks,
Vasiliy.

[B.A.T.M.A.N.] [PATCH] batman-adv: Don't rely on NF_HOOKs return value

[Linus Lüssing]

If a hook returns NF_STOLEN, neither batman_skb_recv nor
batman_skb_recv_finish free the skb due to the asynchronous netfilter
handling in the kernel. Therefore not batman_skb_recv but the ok
function batman_skb_recv_finish should do the freeing when a recv
subfunction returns NET_RX_DROP instead.

Reported-by: Vasiliy Kulikov <segooon@gmail.com>
Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 hard-interface.c |   80 +++++++++++++++++++++++++----------------------------
 1 files changed, 38 insertions(+), 42 deletions(-)

diff --git a/hard-interface.c b/hard-interface.c
index a437ec3..3e81d17 100644
--- a/hard-interface.c
+++ b/hard-interface.c
@@ -494,56 +494,27 @@ out:
 
 static int batman_skb_recv_finish(struct sk_buff *skb)
 {
-	return NF_ACCEPT;
-}
-
-/* receive a packet with the batman ethertype coming on a hard
- * interface */
-int batman_skb_recv(struct sk_buff *skb, struct net_device *dev,
-	struct packet_type *ptype, struct net_device *orig_dev)
-{
-	struct bat_priv *bat_priv;
 	struct batman_packet *batman_packet;
 	struct batman_if *batman_if;
+	struct bat_priv *bat_priv;
 	int ret;
 
-	batman_if = container_of(ptype, struct batman_if, batman_adv_ptype);
-	skb = skb_share_check(skb, GFP_ATOMIC);
-
-	/* skb was released by skb_share_check() */
-	if (!skb)
-		goto err_out;
-
-	/* if netfilter/ebtables wants to block incoming batman
-	 * packets then give them a chance to do so here */
-	ret = NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, dev, NULL,
-		      batman_skb_recv_finish);
-	if (ret != 1)
-		goto err_out;
-
-	/* packet should hold at least type and version */
-	if (unlikely(!pskb_may_pull(skb, 2)))
+	batman_if = get_batman_if_by_netdev(skb->dev);
+	if (!batman_if)
 		goto err_free;
 
-	/* expect a valid ethernet header here. */
-	if (unlikely(skb->mac_len != sizeof(struct ethhdr)
-				|| !skb_mac_header(skb)))
+	if (!batman_if->soft_iface)
 		goto err_free;
 
-	if (!batman_if->soft_iface)
+	/* discard frames on not active interfaces */
+	if (batman_if->if_status != IF_ACTIVE)
 		goto err_free;
 
 	bat_priv = netdev_priv(batman_if->soft_iface);
-
 	if (atomic_read(&bat_priv->mesh_state) != MESH_ACTIVE)
 		goto err_free;
 
-	/* discard frames on not active interfaces */
-	if (batman_if->if_status != IF_ACTIVE)
-		goto err_free;
-
 	batman_packet = (struct batman_packet *)skb->data;
-
 	if (batman_packet->version != COMPAT_VERSION) {
 		bat_dbg(DBG_BATMAN, bat_priv,
 			"Drop packet: incompatible batman version (%i)\n",
@@ -551,6 +522,7 @@ int batman_skb_recv(struct sk_buff *skb, struct net_device *dev,
 		goto err_free;
 	}
 
+
 	/* all receive handlers return whether they received or reused
 	 * the supplied skb. if not, we have to free the skb. */
 
@@ -589,18 +561,42 @@ int batman_skb_recv(struct sk_buff *skb, struct net_device *dev,
 	}
 
 	if (ret == NET_RX_DROP)
-		kfree_skb(skb);
+		goto err_free;
 
-	/* return NET_RX_SUCCESS in any case as we
-	 * most probably dropped the packet for
-	 * routing-logical reasons. */
+	return 0;
 
-	return NET_RX_SUCCESS;
+err_free:
+	kfree_skb(skb);
+	return 0;
+}
 
+/* receive a packet with the batman ethertype coming on a hard
+ * interface */
+int batman_skb_recv(struct sk_buff *skb, struct net_device *dev,
+	struct packet_type *ptype, struct net_device *orig_dev)
+{
+	skb = skb_share_check(skb, GFP_ATOMIC);
+
+	/* skb was released by skb_share_check() */
+	if (!skb)
+		return 0;
+
+	/* packet should hold at least type and version */
+	if (unlikely(!pskb_may_pull(skb, 2)))
+		goto err_free;
+
+	/* expect a valid ethernet header here. */
+	if (unlikely(skb->mac_len != sizeof(struct ethhdr)
+				|| !skb_mac_header(skb)))
+		goto err_free;
+
+	/* if netfilter/ebtables wants to block incoming batman
+	 * packets then give them a chance to do so here */
+	return NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, dev, NULL,
+		      batman_skb_recv_finish);
 err_free:
 	kfree_skb(skb);
-err_out:
-	return NET_RX_DROP;
+	return 0;
 }
 
 struct notifier_block hard_if_notifier = {
-- 
1.7.1

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Don't rely on NF_HOOKs return value

[Marek Lindner]

On Sunday 22 August 2010 22:53:20 Linus Lüssing wrote:
> If a hook returns NF_STOLEN, neither batman_skb_recv nor
> batman_skb_recv_finish free the skb due to the asynchronous netfilter
> handling in the kernel. Therefore not batman_skb_recv but the ok
> function batman_skb_recv_finish should do the freeing when a recv
> subfunction returns NET_RX_DROP instead.
> 
> Reported-by: Vasiliy Kulikov <segooon@gmail.com>
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

Patch has been applied (rev. 1780).

Thanks,
Marek