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;
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",
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.
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.
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)
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")
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
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
b.a.t.m.a.n@lists.open-mesh.org
-
Linus Lüssing -
Sven Eckelmann