[B.A.T.M.A.N.] [PATCH (maint?) 1/3] batamn-adv: fix elp packet data reservation

[B.A.T.M.A.N.] [PATCH (maint?) 1/3] batamn-adv: fix elp packet data reservation

[Linus Lüssing]

The skb_reserve() call only reserved headroom for the mac header, but
not the elp packet header itself.

Fixing this by using skb_put()'ing towards the skb tail instead of
skb_push()'ing towards the skb head.

Fixes: a4b88af77e28 ("batman-adv: ELP - adding basic infrastructure")
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>

---

Looking at some tests today, we might have been lucky so far:
dev_alloc_skb(size = 32) seems to actually round the head- and tailroom
reservation to 64 bytes. So there actually is enough headroom for the
skb_push(). Not sure whether this is always the case though, so
unsure whether this should go to maint/stable.

Also switching skb_push() with skb_pull() instead of simply increasing
skb_reserve() by ELP_HLEN, because of the next patch in this series.
---
 net/batman-adv/bat_v_elp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c
index df42eb1..ea463bf 100644
--- a/net/batman-adv/bat_v_elp.c
+++ b/net/batman-adv/bat_v_elp.c
@@ -323,7 +323,6 @@ out:
 int batadv_v_elp_iface_enable(struct batadv_hard_iface *hard_iface)
 {
 	struct batadv_elp_packet *elp_packet;
-	unsigned char *elp_buff;
 	u32 random_seqno;
 	size_t size;
 	int res = -ENOMEM;
@@ -334,8 +333,9 @@ int batadv_v_elp_iface_enable(struct batadv_hard_iface *hard_iface)
 		goto out;
 
 	skb_reserve(hard_iface->bat_v.elp_skb, ETH_HLEN + NET_IP_ALIGN);
-	elp_buff = skb_push(hard_iface->bat_v.elp_skb, BATADV_ELP_HLEN);
-	elp_packet = (struct batadv_elp_packet *)elp_buff;
+	skb_put(hard_iface->bat_v.elp_skb, BATADV_ELP_HLEN);
+	elp_packet = (struct batadv_elp_packet *)
+			hard_iface->bat_v.elp_skb->data;
 	memset(elp_packet, 0, BATADV_ELP_HLEN);
 
 	elp_packet->packet_type = BATADV_ELP;
-- 
2.1.4

[B.A.T.M.A.N.] [PATCH 2/3] batman-adv: elp: skb network header API instead of skb->data access

[Linus Lüssing]

Instead of playing with skb attributes directly, let's use the
appropriate, more descriptive API instead.

Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
 net/batman-adv/bat_v_elp.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c
index ea463bf..af414b0 100644
--- a/net/batman-adv/bat_v_elp.c
+++ b/net/batman-adv/bat_v_elp.c
@@ -263,7 +263,7 @@ static void batadv_v_elp_periodic_work(struct work_struct *work)
 	if (!skb)
 		goto restart_timer;
 
-	elp_packet = (struct batadv_elp_packet *)skb->data;
+	elp_packet = (struct batadv_elp_packet *)skb_network_header(skb);
 	elp_packet->seqno = htonl(atomic_read(&hard_iface->bat_v.elp_seqno));
 	elp_interval = atomic_read(&hard_iface->bat_v.elp_interval);
 	elp_packet->elp_interval = htonl(elp_interval);
@@ -323,19 +323,23 @@ out:
 int batadv_v_elp_iface_enable(struct batadv_hard_iface *hard_iface)
 {
 	struct batadv_elp_packet *elp_packet;
+	struct sk_buff *skb;
 	u32 random_seqno;
 	size_t size;
 	int res = -ENOMEM;
 
 	size = ETH_HLEN + NET_IP_ALIGN + BATADV_ELP_HLEN;
-	hard_iface->bat_v.elp_skb = dev_alloc_skb(size);
-	if (!hard_iface->bat_v.elp_skb)
+	skb = dev_alloc_skb(size);
+	hard_iface->bat_v.elp_skb = skb;
+
+	if (!skb)
 		goto out;
 
 	skb_reserve(hard_iface->bat_v.elp_skb, ETH_HLEN + NET_IP_ALIGN);
+	skb_reset_network_header(skb);
 	skb_put(hard_iface->bat_v.elp_skb, BATADV_ELP_HLEN);
-	elp_packet = (struct batadv_elp_packet *)
-			hard_iface->bat_v.elp_skb->data;
+
+	elp_packet = (struct batadv_elp_packet *)skb_network_header(skb);
 	memset(elp_packet, 0, BATADV_ELP_HLEN);
 
 	elp_packet->packet_type = BATADV_ELP;
@@ -392,7 +396,7 @@ void batadv_v_elp_iface_activate(struct batadv_hard_iface *primary_iface,
 		return;
 
 	skb = hard_iface->bat_v.elp_skb;
-	elp_packet = (struct batadv_elp_packet *)skb->data;
+	elp_packet = (struct batadv_elp_packet *)skb_network_header(skb);
 	ether_addr_copy(elp_packet->orig,
 			primary_iface->net_dev->dev_addr);
 }
@@ -506,7 +510,7 @@ int batadv_v_elp_packet_recv(struct sk_buff *skb,
 	if (strcmp(bat_priv->bat_algo_ops->name, "BATMAN_V") != 0)
 		return NET_RX_DROP;
 
-	elp_packet = (struct batadv_elp_packet *)skb->data;
+	elp_packet = (struct batadv_elp_packet *)skb_network_header(skb);
 
 	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
 		   "Received ELP packet from %pM seqno %u ORIG: %pM\n",
-- 
2.1.4

Re: [B.A.T.M.A.N.] [PATCH 2/3] batman-adv: elp: skb network header API instead of skb->data access

[Linus Lüssing]

On Sun, Aug 21, 2016 at 05:25:33AM +0200, Linus Lüssing wrote:
> Instead of playing with skb attributes directly, let's use the
> appropriate, more descriptive API instead.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---

Sorry, was on the maint branch when writing and
git-format-patch'ing this. While "git am *.patch" fails,
"git am --3way *.patch" works and looks good to me.

Re: [B.A.T.M.A.N.] [PATCH 2/3] batman-adv: elp: skb network header API instead of skb->data access

[Linus Lüssing]

On Sun, Aug 21, 2016 at 05:25:33AM +0200, Linus Lüssing wrote:
> Instead of playing with skb attributes directly, let's use the
> appropriate, more descriptive API instead.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---

Hm, wanted to fix up the aligment issues Sven mentioned in PATCH
1/3 wis this patch. But actually skb_put() assigment without
skb->data access works directly, let's go for this two bytes fix.

And ignore this [PATCH 2/3] for now, please.

[B.A.T.M.A.N.] [PATCH 3/3] batman-adv: fix batadv_forw_packet kerneldoc for list attribute

[Linus Lüssing]

The forw_packet list node is wrongly attributed to the icmp socket code.

Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
 net/batman-adv/types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index 74d865a..c25bec6 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -1237,7 +1237,7 @@ struct batadv_skb_cb {
 
 /**
  * struct batadv_forw_packet - structure for bcast packets to be sent/forwarded
- * @list: list node for batadv_socket_client::queue_list
+ * @list: list node for batadv_priv::forw_{bat,bcast}_list
  * @send_time: execution time for delayed_work (packet sending)
  * @own: bool for locally generated packets (local OGMs are re-scheduled after
  *  sending)
-- 
2.1.4

Re: [B.A.T.M.A.N.] [PATCH 3/3] batman-adv: fix batadv_forw_packet kerneldoc for list attribute

[Linus Lüssing]

On Sun, Aug 21, 2016 at 05:25:34AM +0200, Linus Lüssing wrote:
> The forw_packet list node is wrongly attributed to the icmp socket code.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---

Fixes: 2191c1bcbc64 ("batman-adv: kernel doc for types.h")

Re: [B.A.T.M.A.N.] [PATCH 3/3] batman-adv: fix batadv_forw_packet kerneldoc for list attribute

[Sven Eckelmann]

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

On Sonntag, 21. August 2016 05:25:34 CEST Linus Lüssing wrote:
> The forw_packet list node is wrongly attributed to the icmp socket code.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---

Reviewed-by: Sven Eckelmann <sven@narfation.org>

Kind regards,
	Sven

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

Re: [B.A.T.M.A.N.] [PATCH (maint?) 1/3] batamn-adv: fix elp packet data reservation

[Sven Eckelmann]

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

On Sonntag, 21. August 2016 05:25:32 CEST Linus Lüssing wrote:
[...]
> @@ -334,8 +333,9 @@ int batadv_v_elp_iface_enable(struct batadv_hard_iface *hard_iface)
>  		goto out;
>  
>  	skb_reserve(hard_iface->bat_v.elp_skb, ETH_HLEN + NET_IP_ALIGN);
> -	elp_buff = skb_push(hard_iface->bat_v.elp_skb, BATADV_ELP_HLEN);
> -	elp_packet = (struct batadv_elp_packet *)elp_buff;
> +	skb_put(hard_iface->bat_v.elp_skb, BATADV_ELP_HLEN);
> +	elp_packet = (struct batadv_elp_packet *)
> +			hard_iface->bat_v.elp_skb->data;
>  	memset(elp_packet, 0, BATADV_ELP_HLEN);
>  
>  	elp_packet->packet_type = BATADV_ELP;
> 

I don't get right now why you did the of split the skb_put into two different
"ugly" lines (skb_put + the elp_packet assignment without elp_buff). I fear 
that this weird (non)-alignment you've created will bite us when the patch is 
submitted upstream.

Maybe you can tell us more about why this removal of elp_buff is necessary.

Btw. please use the prefix "batman-adv" in the subject and not "batamn-adv" ;)

And it is at the moment not important whether it goes into next or
maint. Both will be submitted by Simon to net.git because we are currently
completely off with our timing (compared to the upstream submissions).
I personally would go for maint.

Kind regards,
	Sven

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