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

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

[Linus Lüssing]

Hi,

This second version adds a few more cases I could spot which look like trouble.

Secondly, instead of copying the destination mac on the stack it simply uses
eth_hdr().


Why do we need no skb_reset_mac_header() or skb_set_mac_header()? Because
batadv_skb_head_push() calls skb_headers_offset_update() after the reallocation
(via skb_cow_head()->__skb_cow()->pskb_expand_head()->
skb_headers_offset_update().

I also checked that the ether header in the newly allocated buffer is not going
to end up being fragmented. But looks like pskb_expand_head() actually reserves
enough space for both the old ethernet header plus the newly requested head
space.

So if the mac header was set correctly before, then eth_hdr() will point us
to the right one already.


This patch is maybe not so straight forward, you need to read quite skb
code. At least not as straight forward as the previous version of this patch.
So I leave it up to you to decide whether this is suitable for next or whether
you think there's a potential for unseen pitfalls, therefore making it
suitable for master only :).


This patch fixed the kernel panic I was reproduceably getting just as well. 
(Which was while sending multicast packets through the unicast functions
while having a bridge on top of bat0)


Cheers, Linus

[B.A.T.M.A.N.] [PATCHv2 next] batman-adv: fix potential kernel paging errors for unicast transmissions

[Linus Lüssing]

There are several functions which might reallocate skb data. Currently
some places keep reusing their old ethhdr pointer regardless of whether
they became invalid after such a reallocation or not. This potentially
leads to kernel paging errors.

This patch fixes these by refetching the ethdr pointer after the
potential reallocations.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 bridge_loop_avoidance.c |    2 ++
 gateway_client.c        |    6 +++++-
 gateway_client.h        |    3 +--
 soft-interface.c        |    8 +++++++-
 unicast.c               |   11 ++++++++---
 5 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/bridge_loop_avoidance.c b/bridge_loop_avoidance.c
index e14531f..264de88 100644
--- a/bridge_loop_avoidance.c
+++ b/bridge_loop_avoidance.c
@@ -1529,6 +1529,8 @@ out:
  * in these cases, the skb is further handled by this function and
  * returns 1, otherwise it returns 0 and the caller shall further
  * process the skb.
+ *
+ * This call might reallocate skb data.
  */
 int batadv_bla_tx(struct batadv_priv *bat_priv, struct sk_buff *skb,
 		  unsigned short vid)
diff --git a/gateway_client.c b/gateway_client.c
index f105219..30de529 100644
--- a/gateway_client.c
+++ b/gateway_client.c
@@ -568,6 +568,7 @@ out:
 	return ret;
 }
 
+/* this call might reallocate skb data */
 bool batadv_gw_is_dhcp_target(struct sk_buff *skb, unsigned int *header_len)
 {
 	struct ethhdr *ethhdr;
@@ -634,12 +635,14 @@ bool batadv_gw_is_dhcp_target(struct sk_buff *skb, unsigned int *header_len)
 	return true;
 }
 
+/* this call might reallocate skb data */
 bool batadv_gw_out_of_range(struct batadv_priv *bat_priv,
-			    struct sk_buff *skb, struct ethhdr *ethhdr)
+			    struct sk_buff *skb)
 {
 	struct batadv_neigh_node *neigh_curr = NULL, *neigh_old = NULL;
 	struct batadv_orig_node *orig_dst_node = NULL;
 	struct batadv_gw_node *curr_gw = NULL;
+	struct ethhdr *ethhdr;
 	bool ret, out_of_range = false;
 	unsigned int header_len = 0;
 	uint8_t curr_tq_avg;
@@ -648,6 +651,7 @@ bool batadv_gw_out_of_range(struct batadv_priv *bat_priv,
 	if (!ret)
 		goto out;
 
+	ethhdr = eth_hdr(skb);
 	orig_dst_node = batadv_transtable_search(bat_priv, ethhdr->h_source,
 						 ethhdr->h_dest);
 	if (!orig_dst_node)
diff --git a/gateway_client.h b/gateway_client.h
index 039902d..1037d75 100644
--- a/gateway_client.h
+++ b/gateway_client.h
@@ -34,7 +34,6 @@ void batadv_gw_node_delete(struct batadv_priv *bat_priv,
 void batadv_gw_node_purge(struct batadv_priv *bat_priv);
 int batadv_gw_client_seq_print_text(struct seq_file *seq, void *offset);
 bool batadv_gw_is_dhcp_target(struct sk_buff *skb, unsigned int *header_len);
-bool batadv_gw_out_of_range(struct batadv_priv *bat_priv,
-			    struct sk_buff *skb, struct ethhdr *ethhdr);
+bool batadv_gw_out_of_range(struct batadv_priv *bat_priv, struct sk_buff *skb);
 
 #endif /* _NET_BATMAN_ADV_GATEWAY_CLIENT_H_ */
diff --git a/soft-interface.c b/soft-interface.c
index 700d0b4..51c6b5d 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -180,6 +180,8 @@ static int batadv_interface_tx(struct sk_buff *skb,
 	if (batadv_bla_tx(bat_priv, skb, vid))
 		goto dropped;
 
+	ethhdr = eth_hdr(skb);
+
 	/* Register the client MAC in the transtable */
 	if (!is_multicast_ether_addr(ethhdr->h_source))
 		batadv_tt_local_add(soft_iface, ethhdr->h_source, skb->skb_iif);
@@ -220,6 +222,10 @@ static int batadv_interface_tx(struct sk_buff *skb,
 		default:
 			break;
 		}
+
+		/* reminder: ethhdr might have become unusable from here on
+		 * (batadv_gw_is_dhcp_target() might have reallocated skb data)
+		 */
 	}
 
 	/* ethernet packet should be broadcasted */
@@ -266,7 +272,7 @@ static int batadv_interface_tx(struct sk_buff *skb,
 	/* unicast packet */
 	} else {
 		if (atomic_read(&bat_priv->gw_mode) != BATADV_GW_MODE_OFF) {
-			ret = batadv_gw_out_of_range(bat_priv, skb, ethhdr);
+			ret = batadv_gw_out_of_range(bat_priv, skb);
 			if (ret)
 				goto dropped;
 		}
diff --git a/unicast.c b/unicast.c
index 4c5a1aa..8378845 100644
--- a/unicast.c
+++ b/unicast.c
@@ -326,7 +326,9 @@ static bool batadv_unicast_push_and_fill_skb(struct sk_buff *skb, int hdr_size,
  * @skb: the skb containing the payload to encapsulate
  * @orig_node: the destination node
  *
- * Returns false if the payload could not be encapsulated or true otherwise
+ * Return false if the payload could not be encapsulated or true otherwise.
+ *
+ * This call might reallocate skb data.
  */
 static bool batadv_unicast_prepare_skb(struct sk_buff *skb,
 				       struct batadv_orig_node *orig_node)
@@ -343,7 +345,9 @@ static bool batadv_unicast_prepare_skb(struct sk_buff *skb,
  * @orig_node: the destination node
  * @packet_subtype: the batman 4addr packet subtype to use
  *
- * Returns false if the payload could not be encapsulated or true otherwise
+ * Return false if the payload could not be encapsulated or true otherwise.
+ *
+ * This call might reallocate skb data.
  */
 bool batadv_unicast_4addr_prepare_skb(struct batadv_priv *bat_priv,
 				      struct sk_buff *skb,
@@ -395,7 +399,7 @@ int batadv_unicast_generic_send_skb(struct batadv_priv *bat_priv,
 				    struct sk_buff *skb, int packet_type,
 				    int packet_subtype)
 {
-	struct ethhdr *ethhdr = (struct ethhdr *)skb->data;
+	struct ethhdr *ethhdr = eth_hdr(skb);
 	struct batadv_unicast_packet *unicast_packet;
 	struct batadv_orig_node *orig_node;
 	struct batadv_neigh_node *neigh_node;
@@ -444,6 +448,7 @@ find_router:
 	}
 
 	unicast_packet = (struct batadv_unicast_packet *)skb->data;
+	ethhdr = eth_hdr(skb);
 
 	/* inform the destination node that we are still missing a correct route
 	 * for this client. The destination will receive this packet and will
-- 
1.7.10.4

Re: [B.A.T.M.A.N.] [PATCHv2 next] batman-adv: fix potential kernel paging errors for unicast transmissions

[Marek Lindner]

On Friday, August 02, 2013 20:44:55 Linus Lüssing wrote:
> There are several functions which might reallocate skb data. Currently
> some places keep reusing their old ethhdr pointer regardless of whether
> they became invalid after such a reallocation or not. This potentially
> leads to kernel paging errors.
> 
> This patch fixes these by refetching the ethdr pointer after the
> potential reallocations.

As discussed on IRC, please fix a similar problem in 
batadv_gw_is_dhcp_target(). I know this bug wasn't created by your patch but I 
found it while reviewing your code and looking for similar cases. Should be 
simple to fix as well.


> @@ -326,7 +326,9 @@ static bool batadv_unicast_push_and_fill_skb(struct
> sk_buff *skb, int hdr_size, * @skb: the skb containing the payload to
> encapsulate
>   * @orig_node: the destination node
>   *
> - * Returns false if the payload could not be encapsulated or true
> otherwise + 
> * Return false if the payload could not be encapsulated or
> true otherwise. 

Don't change "returns" to "return".


> @@ -343,7 +345,9 @@ static bool batadv_unicast_prepare_skb(struct sk_buff
> *skb, * @orig_node: the destination node
>   * @packet_subtype: the batman 4addr packet subtype to use
>   *
> - * Returns false if the payload could not be encapsulated or true
> otherwise + 
> * Return false if the payload could not be encapsulated or
> true otherwise.

Same here.

Cheers,
Marek