pull request [net]: batman-adv-0160426

pull request [net]: batman-adv-0160426

[Antonio Quartulli]

Hi David,

this is a batch intended for net. Changes are quite small, therefore I
hope it is not a big deal to include them at this point of the release cycle.

In this patchset you can find the following fixes:

1) check skb size to avoid reading beyond its border when delivering
   payloads, by Sven Eckelmann
2) initialize last_seen time in neigh_node object to prevent cleanup
   routine from accidentally purge it, by Marek Lindner
3) release "recently added" slave interfaces upon virtual/batman
   interface shutdown, by Sven Eckelmann
4) properly decrease router object reference counter upon routing table
   update, by Sven Eckelmann
5) release queue slots when purging OGM packets of deactivating slave
   interface, by Linus Lüssing

Patch 2 and 3 have no "Fixes:" tag because the offending commits date
back to when batman-adv was not yet officially in the net tree.

Note that all these changes are fixing very old commits and therefore
it would be nice if you could queue them for *stable*.

Please pull or let me know of any issue!

Thanks a lot,
	Antonio




The following changes since commit 5f44abd041c5f3be76d57579ab254d78e601315b:

  Merge tag 'rtc-4.6-3' of git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux (2016-04-21 15:41:13 -0700)

are available in the git repository at:

  git://git.open-mesh.org/linux-merge.git tags/batman-adv-fix-for-davem

for you to fetch changes up to c4fdb6cff2aa0ae740c5f19b6f745cbbe786d42f:

  batman-adv: Fix broadcast/ogm queue limit on a removed interface (2016-04-24 15:41:56 +0800)

----------------------------------------------------------------
In this patchset you can find the following fixes:

1) check skb size to avoid reading beyond its border when delivering
   payloads, by Sven Eckelmann
2) initialize last_seen time in neigh_node object to prevent cleanup
   routine from accidentally purge it, by Marek Lindner
3) release "recently added" slave interfaces upon virtual/batman
   interface shutdown, by Sven Eckelmann
4) properly decrease router object reference counter upon routing table
   update, by Sven Eckelmann
5) release queue slots when purging OGM packets of deactivating slave
   interface, by Linus Lüssing

Patch 2 and 3 have no "Fixes:" tag because the offending commits date
back to when batman-adv was not yet officially in the net tree.

----------------------------------------------------------------
Linus Lüssing (1):
      batman-adv: Fix broadcast/ogm queue limit on a removed interface

Marek Lindner (1):
      batman-adv: init neigh node last seen field

Sven Eckelmann (3):
      batman-adv: Check skb size before using encapsulated ETH+VLAN header
      batman-adv: Deactivate TO_BE_ACTIVATED hardif on shutdown
      batman-adv: Reduce refcnt of removed router when updating route

 net/batman-adv/hard-interface.c | 3 +--
 net/batman-adv/originator.c     | 1 +
 net/batman-adv/routing.c        | 9 +++++++++
 net/batman-adv/send.c           | 6 ++++++
 net/batman-adv/soft-interface.c | 8 ++++++--
 5 files changed, 23 insertions(+), 4 deletions(-)

[PATCH 1/5] batman-adv: Check skb size before using encapsulated ETH+VLAN header

[Antonio Quartulli]

From: Sven Eckelmann <sven@narfation.org>

The encapsulated ethernet and VLAN header may be outside the received
ethernet frame. Thus the skb buffer size has to be checked before it can be
parsed to find out if it encapsulates another batman-adv packet.

Fixes: 420193573f11 ("batman-adv: softif bridge loop avoidance")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 net/batman-adv/soft-interface.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index 0710379491bf..8a136b6a1ff0 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -408,11 +408,17 @@ void batadv_interface_rx(struct net_device *soft_iface,
 	 */
 	nf_reset(skb);
 
+	if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
+		goto dropped;
+
 	vid = batadv_get_vid(skb, 0);
 	ethhdr = eth_hdr(skb);
 
 	switch (ntohs(ethhdr->h_proto)) {
 	case ETH_P_8021Q:
+		if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
+			goto dropped;
+
 		vhdr = (struct vlan_ethhdr *)skb->data;
 
 		if (vhdr->h_vlan_encapsulated_proto != ethertype)
@@ -424,8 +430,6 @@ void batadv_interface_rx(struct net_device *soft_iface,
 	}
 
 	/* skb->dev & skb->pkt_type are set here */
-	if (unlikely(!pskb_may_pull(skb, ETH_HLEN)))
-		goto dropped;
 	skb->protocol = eth_type_trans(skb, soft_iface);
 
 	/* should not be necessary anymore as we use skb_pull_rcsum()
-- 
2.8.1

[PATCH 2/5] batman-adv: init neigh node last seen field

[Antonio Quartulli]

From: Marek Lindner <mareklindner@neomailbox.ch>

Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
[sven@narfation.org: fix conflicts with current version]
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 net/batman-adv/originator.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c
index e4cbb0753e37..d52f67a0c057 100644
--- a/net/batman-adv/originator.c
+++ b/net/batman-adv/originator.c
@@ -663,6 +663,7 @@ batadv_neigh_node_new(struct batadv_orig_node *orig_node,
 	ether_addr_copy(neigh_node->addr, neigh_addr);
 	neigh_node->if_incoming = hard_iface;
 	neigh_node->orig_node = orig_node;
+	neigh_node->last_seen = jiffies;
 
 	/* extra reference for return */
 	kref_init(&neigh_node->refcount);
-- 
2.8.1

[PATCH 3/5] batman-adv: Deactivate TO_BE_ACTIVATED hardif on shutdown

[Antonio Quartulli]

From: Sven Eckelmann <sven@narfation.org>

The shutdown of an batman-adv interface can happen with one of its slave
interfaces still being in the BATADV_IF_TO_BE_ACTIVATED state. A possible
reason for it is that the routing algorithm BATMAN_V was selected and
batadv_schedule_bat_ogm was not yet called for this interface. This slave
interface still has to be set to BATADV_IF_INACTIVE or the batman-adv
interface will never reduce its usage counter and thus never gets shutdown.

This problem can be simulated via:

    $ modprobe dummy
    $ modprobe batman-adv routing_algo=BATMAN_V
    $ ip link add bat0 type batadv
    $ ip link set dummy0 master bat0
    $ ip link set dummy0 up
    $ ip link del bat0
    unregister_netdevice: waiting for bat0 to become free. Usage count = 3

Reported-by: Matthias Schiffer <mschiffer@universe-factory.net>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 net/batman-adv/hard-interface.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
index b22b2775a0a5..c61d5b0b24d2 100644
--- a/net/batman-adv/hard-interface.c
+++ b/net/batman-adv/hard-interface.c
@@ -572,8 +572,7 @@ void batadv_hardif_disable_interface(struct batadv_hard_iface *hard_iface,
 	struct batadv_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
 	struct batadv_hard_iface *primary_if = NULL;
 
-	if (hard_iface->if_status == BATADV_IF_ACTIVE)
-		batadv_hardif_deactivate_interface(hard_iface);
+	batadv_hardif_deactivate_interface(hard_iface);
 
 	if (hard_iface->if_status != BATADV_IF_INACTIVE)
 		goto out;
-- 
2.8.1

[PATCH 4/5] batman-adv: Reduce refcnt of removed router when updating route

[Antonio Quartulli]

From: Sven Eckelmann <sven@narfation.org>

_batadv_update_route rcu_derefences orig_ifinfo->router outside of a
spinlock protected region to print some information messages to the debug
log. But this pointer is not checked again when the new pointer is assigned
in the spinlock protected region. Thus is can happen that the value of
orig_ifinfo->router changed in the meantime and thus the reference counter
of the wrong router gets reduced after the spinlock protected region.

Just rcu_dereferencing the value of orig_ifinfo->router inside the spinlock
protected region (which also set the new pointer) is enough to get the
correct old router object.

Fixes: e1a5382f978b ("batman-adv: Make orig_node->router an rcu protected pointer")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 net/batman-adv/routing.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 4dd646a52f1a..b781bf753250 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -105,6 +105,15 @@ static void _batadv_update_route(struct batadv_priv *bat_priv,
 		neigh_node = NULL;
 
 	spin_lock_bh(&orig_node->neigh_list_lock);
+	/* curr_router used earlier may not be the current orig_ifinfo->router
+	 * anymore because it was dereferenced outside of the neigh_list_lock
+	 * protected region. After the new best neighbor has replace the current
+	 * best neighbor the reference counter needs to decrease. Consequently,
+	 * the code needs to ensure the curr_router variable contains a pointer
+	 * to the replaced best neighbor.
+	 */
+	curr_router = rcu_dereference_protected(orig_ifinfo->router, true);
+
 	rcu_assign_pointer(orig_ifinfo->router, neigh_node);
 	spin_unlock_bh(&orig_node->neigh_list_lock);
 	batadv_orig_ifinfo_put(orig_ifinfo);
-- 
2.8.1

Re: [PATCH 4/5] batman-adv: Reduce refcnt of removed router when updating route

[Sergei Shtylyov]

Hello.

On 4/26/2016 6:27 AM, Antonio Quartulli wrote:

> From: Sven Eckelmann <sven-KaDOiPu9UxWEi8DpZVb4nw@public.gmane.org>
>
> _batadv_update_route rcu_derefences orig_ifinfo->router outside of a
> spinlock protected region to print some information messages to the debug
> log. But this pointer is not checked again when the new pointer is assigned
> in the spinlock protected region. Thus is can happen that the value of

    Thus is can? :-)

> orig_ifinfo->router changed in the meantime and thus the reference counter
> of the wrong router gets reduced after the spinlock protected region.
>
> Just rcu_dereferencing the value of orig_ifinfo->router inside the spinlock
> protected region (which also set the new pointer) is enough to get the
> correct old router object.
>
> Fixes: e1a5382f978b ("batman-adv: Make orig_node->router an rcu protected pointer")
> Signed-off-by: Sven Eckelmann <sven-KaDOiPu9UxWEi8DpZVb4nw@public.gmane.org>
> Signed-off-by: Marek Lindner <mareklindner-rVWd3aGhH2z5bpWLKbzFeg@public.gmane.org>
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
>  net/batman-adv/routing.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
> index 4dd646a52f1a..b781bf753250 100644
> --- a/net/batman-adv/routing.c
> +++ b/net/batman-adv/routing.c
> @@ -105,6 +105,15 @@ static void _batadv_update_route(struct batadv_priv *bat_priv,
>  		neigh_node = NULL;
>
>  	spin_lock_bh(&orig_node->neigh_list_lock);
> +	/* curr_router used earlier may not be the current orig_ifinfo->router
> +	 * anymore because it was dereferenced outside of the neigh_list_lock
> +	 * protected region. After the new best neighbor has replace the current

    Replaced.

[...]

MBR, Sergei

Re: [PATCH 4/5] batman-adv: Reduce refcnt of removed router when updating route

[Sven Eckelmann]

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

On Tuesday 26 April 2016 17:42:54 Sergei Shtylyov wrote:
> > _batadv_update_route rcu_derefences orig_ifinfo->router outside of a
> > spinlock protected region to print some information messages to the debug
> > log. But this pointer is not checked again when the new pointer is assigned
> > in the spinlock protected region. Thus is can happen that the value of
> 
>     Thus is can? :-)

Yes, my fault. s/is/it/.

[...]
> >  	spin_lock_bh(&orig_node->neigh_list_lock);
> > +	/* curr_router used earlier may not be the current orig_ifinfo->router
> > +	 * anymore because it was dereferenced outside of the neigh_list_lock
> > +	 * protected region. After the new best neighbor has replace the current
> 
>     Replaced.
> 
> [...]

This one looks like one of Marek's modifications [1] to the patch. But I would
guess that he has nothing against adding a 'd'.

Should Antonio resent all the patches or is a different approach preferred?

Kind regards,
	Sven

[1] https://patchwork.open-mesh.org/patch/15940/

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

[PATCH 5/5] batman-adv: Fix broadcast/ogm queue limit on a removed interface

[Antonio Quartulli]

From: Linus Lüssing <linus.luessing@c0d3.blue>

When removing a single interface while a broadcast or ogm packet is
still pending then we will free the forward packet without releasing the
queue slots again.

This patch is supposed to fix this issue.

Fixes: 6d5808d4ae1b ("batman-adv: Add missing hardif_free_ref in forw_packet_free")
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
[sven@narfation.org: fix conflicts with current version]
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 net/batman-adv/send.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c
index 3ce06e0a91b1..76417850d3fc 100644
--- a/net/batman-adv/send.c
+++ b/net/batman-adv/send.c
@@ -675,6 +675,9 @@ batadv_purge_outstanding_packets(struct batadv_priv *bat_priv,
 
 		if (pending) {
 			hlist_del(&forw_packet->list);
+			if (!forw_packet->own)
+				atomic_inc(&bat_priv->bcast_queue_left);
+
 			batadv_forw_packet_free(forw_packet);
 		}
 	}
@@ -702,6 +705,9 @@ batadv_purge_outstanding_packets(struct batadv_priv *bat_priv,
 
 		if (pending) {
 			hlist_del(&forw_packet->list);
+			if (!forw_packet->own)
+				atomic_inc(&bat_priv->batman_queue_left);
+
 			batadv_forw_packet_free(forw_packet);
 		}
 	}
-- 
2.8.1

Re: pull request [net]: batman-adv-0160426

[David Miller]

From: Antonio Quartulli <a@unstable.cc>
Date: Tue, 26 Apr 2016 11:27:14 +0800

> In this patchset you can find the following fixes:

Pulled, even though there were some typos in the commit messages.

> Patch 2 and 3 have no "Fixes:" tag because the offending commits date
> back to when batman-adv was not yet officially in the net tree.

This is not correct.  Instead, in the future, you should provide a
Fixes: tag that indicates the commit that merged batman-adv into the
upstream tree initially.

Thanks.

Re: pull request [net]: batman-adv-0160426

[Antonio Quartulli]

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

On Thu, Apr 28, 2016 at 04:43:51PM -0400, David Miller wrote:
> > Patch 2 and 3 have no "Fixes:" tag because the offending commits date
> > back to when batman-adv was not yet officially in the net tree.
> 
> This is not correct.  Instead, in the future, you should provide a
> Fixes: tag that indicates the commit that merged batman-adv into the
> upstream tree initially.

makes sense. Thanks for the suggestion!

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]