[PATCH 13/17] batman-adv: Consume skb in receive handlers

[Simon Wunderlich]

From: Sven Eckelmann 

Receiving functions in Linux consume the supplied skbuff. Doing the same in
the batadv_rx_handler functions makes the behavior more similar to the rest
of the Linux network code.

Signed-off-by: Sven Eckelmann 
Signed-off-by: Simon Wunderlich 
---
 net/batman-adv/bat_iv_ogm.c |  22 --
 net/batman-adv/bat_v_elp.c  |  30 
 net/batman-adv/bat_v_ogm.c  |  15 ++--
 net/batman-adv/main.c   |  11 +--
 net/batman-adv/network-coding.c |  11 +--
 net/batman-adv/routing.c| 149 +++-
 6 files changed, 153 insertions(+), 85 deletions(-)

diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
index 310f391..bd39247 100644
--- a/net/batman-adv/bat_iv_ogm.c
+++ b/net/batman-adv/bat_iv_ogm.c
@@ -1823,17 +1823,18 @@ static int batadv_iv_ogm_receive(struct sk_buff *skb,
struct batadv_ogm_packet *ogm_packet;
u8 *packet_pos;
int ogm_offset;
-   bool ret;
+   bool res;
+   int ret = NET_RX_DROP;
 
-   ret = batadv_check_management_packet(skb, if_incoming, BATADV_OGM_HLEN);
-   if (!ret)
-   return NET_RX_DROP;
+   res = batadv_check_management_packet(skb, if_incoming, BATADV_OGM_HLEN);
+   if (!res)
+   goto free_skb;
 
/* did we receive a B.A.T.M.A.N. IV OGM packet on an interface
 * that does not have B.A.T.M.A.N. IV enabled ?
 */
if (bat_priv->algo_ops->iface.enable != batadv_iv_ogm_iface_enable)
-   return NET_RX_DROP;
+   goto free_skb;
 
batadv_inc_counter(bat_priv, BATADV_CNT_MGMT_RX);
batadv_add_counter(bat_priv, BATADV_CNT_MGMT_RX_BYTES,
@@ -1854,8 +1855,15 @@ static int batadv_iv_ogm_receive(struct sk_buff *skb,
ogm_packet = (struct batadv_ogm_packet *)packet_pos;
}
 
-   consume_skb(skb);
-   return NET_RX_SUCCESS;
+   ret = NET_RX_SUCCESS;
+
+free_skb:
+   if (ret == NET_RX_SUCCESS)
+   consume_skb(skb);
+   else
+   kfree_skb(skb);
+
+   return ret;
 }
 
 #ifdef CONFIG_BATMAN_ADV_DEBUGFS
diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c
index ee08540..54bdd41 100644
--- a/net/batman-adv/bat_v_elp.c
+++ b/net/batman-adv/bat_v_elp.c
@@ -492,20 +492,21 @@ int batadv_v_elp_packet_recv(struct sk_buff *skb,
struct batadv_elp_packet *elp_packet;
struct batadv_hard_iface *primary_if;
struct ethhdr *ethhdr = (struct ethhdr *)skb_mac_header(skb);
-   bool ret;
+   bool res;
+   int ret = NET_RX_DROP;
 
-   ret = batadv_check_management_packet(skb, if_incoming, BATADV_ELP_HLEN);
-   if (!ret)
-   return NET_RX_DROP;
+   res = batadv_check_management_packet(skb, if_incoming, BATADV_ELP_HLEN);
+   if (!res)
+   goto free_skb;
 
if (batadv_is_my_mac(bat_priv, ethhdr->h_source))
-   return NET_RX_DROP;
+   goto free_skb;
 
/* did we receive a B.A.T.M.A.N. V ELP packet on an interface
 * that does not have B.A.T.M.A.N. V ELP enabled ?
 */
if (strcmp(bat_priv->algo_ops->name, "BATMAN_V") != 0)
-   return NET_RX_DROP;
+   goto free_skb;
 
elp_packet = (struct batadv_elp_packet *)skb->data;
 
@@ -516,14 +517,19 @@ int batadv_v_elp_packet_recv(struct sk_buff *skb,
 
primary_if = batadv_primary_if_get_selected(bat_priv);
if (!primary_if)
-   goto out;
+   goto free_skb;
 
batadv_v_elp_neigh_update(bat_priv, ethhdr->h_source, if_incoming,
  elp_packet);
 
-out:
-   if (primary_if)
-   batadv_hardif_put(primary_if);
-   consume_skb(skb);
-   return NET_RX_SUCCESS;
+   ret = NET_RX_SUCCESS;
+   batadv_hardif_put(primary_if);
+
+free_skb:
+   if (ret == NET_RX_SUCCESS)
+   consume_skb(skb);
+   else
+   kfree_skb(skb);
+
+   return ret;
 }
diff --git a/net/batman-adv/bat_v_ogm.c b/net/batman-adv/bat_v_ogm.c
index 9922ccd..38b9aab 100644
--- a/net/batman-adv/bat_v_ogm.c
+++ b/net/batman-adv/bat_v_ogm.c
@@ -810,18 +810,18 @@ int batadv_v_ogm_packet_recv(struct sk_buff *skb,
 * B.A.T.M.A.N. V enabled ?
 */
if (strcmp(bat_priv->algo_ops->name, "BATMAN_V") != 0)
-   return NET_RX_DROP;
+   goto free_skb;
 
if (!batadv_check_management_packet(skb, if_incoming, BATADV_OGM2_HLEN))
-   return NET_RX_DROP;
+   goto free_skb;
 
if (batadv_is_my_mac(bat_priv, ethhdr->h_source))
-   return NET_RX_DROP;
+   goto free_skb;
 
ogm_packet = (struct batadv_ogm2_packet *)skb->data;
 
if (batadv_is_my_mac(bat_priv, ogm_packet->orig))
-   return NET_RX_DROP;
+   goto free_skb;
 

Re: [PATCH 13/17] batman-adv: Consume skb in receive handlers

[Sven Eckelmann]

On Dienstag, 8. November 2016 09:43:01 CET Eric Dumazet wrote:
[...]
> Sure, but your patch 13/17 should address this right away.
[...]

Fair enough. I've asked Simon to resubmit the patches with the
"consume_skb -> conditional kfree_skb/consume_skb" patch squashed
into patch 13.

Kind regards,
Sven


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 13/17] batman-adv: Consume skb in receive handlers

[Eric Dumazet]

On Tue, 2016-11-08 at 18:28 +0100, Sven Eckelmann wrote:
> On Dienstag, 8. November 2016 08:59:49 CET Eric Dumazet wrote:
> [...]
> > > +free_skb:
> > >   consume_skb(skb);
> > > - return NET_RX_SUCCESS;
> > > +
> > > + return ret;
> > >  }
> > 
> > 
> > Okay, but we do have kfree_skb() and consume_skb() and they should be
> > used appropriately.
> 
> Yes, this patch is one part of reaching this goal. Some other parts are also
> in this patchset. But other changes like the one you've mention here (change
> some consume_skb partially back to kfree_skb) have still to be done. But
> first we have to clean up the main portion of the mess :)

Sure, but your patch 13/17 should address this right away.

You must not call consume_skb() if you are dropping a packet.

Prior to this patch, kfree_skb() was properly called, and after this
patch, consume_skb() is called instead.


-   ret = (*batadv_rx_handler[idx])(skb, hard_iface);
-
-   if (ret == NET_RX_DROP)
-   kfree_skb(skb);
+   (*batadv_rx_handler[idx])(skb, hard_iface);
 
You can not claim working on these issues and at the same time add them
back.

Re: [PATCH 13/17] batman-adv: Consume skb in receive handlers

[Sven Eckelmann]

On Dienstag, 8. November 2016 08:59:49 CET Eric Dumazet wrote:
[...]
> > +free_skb:
> > consume_skb(skb);
> > -   return NET_RX_SUCCESS;
> > +
> > +   return ret;
> >  }
> 
> 
> Okay, but we do have kfree_skb() and consume_skb() and they should be
> used appropriately.

Yes, this patch is one part of reaching this goal. Some other parts are also
in this patchset. But other changes like the one you've mention here (change
some consume_skb partially back to kfree_skb) have still to be done. But
first we have to clean up the main portion of the mess :)

Kind regards,
Sven

signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 13/17] batman-adv: Consume skb in receive handlers

[Eric Dumazet]

On Tue, 2016-11-08 at 17:45 +0100, Simon Wunderlich wrote:
> From: Sven Eckelmann 
> 
> Receiving functions in Linux consume the supplied skbuff. Doing the same in
> the batadv_rx_handler functions makes the behavior more similar to the rest
> of the Linux network code.
> 
> Signed-off-by: Sven Eckelmann 
> Signed-off-by: Simon Wunderlich 
> ---
>  net/batman-adv/bat_iv_ogm.c |  17 +++--
>  net/batman-adv/bat_v_elp.c  |  25 ---
>  net/batman-adv/bat_v_ogm.c  |  10 +--
>  net/batman-adv/main.c   |  11 +--
>  net/batman-adv/network-coding.c |  11 +--
>  net/batman-adv/routing.c| 149 
> +++-
>  6 files changed, 141 insertions(+), 82 deletions(-)
> 
> diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
> index 310f391..b9941bf 100644
> --- a/net/batman-adv/bat_iv_ogm.c
> +++ b/net/batman-adv/bat_iv_ogm.c
> @@ -1823,17 +1823,18 @@ static int batadv_iv_ogm_receive(struct sk_buff *skb,
>   struct batadv_ogm_packet *ogm_packet;
>   u8 *packet_pos;
>   int ogm_offset;
> - bool ret;
> + bool res;
> + int ret = NET_RX_DROP;
>  
> - ret = batadv_check_management_packet(skb, if_incoming, BATADV_OGM_HLEN);
> - if (!ret)
> - return NET_RX_DROP;
> + res = batadv_check_management_packet(skb, if_incoming, BATADV_OGM_HLEN);
> + if (!res)
> + goto free_skb;
>  
>   /* did we receive a B.A.T.M.A.N. IV OGM packet on an interface
>* that does not have B.A.T.M.A.N. IV enabled ?
>*/
>   if (bat_priv->algo_ops->iface.enable != batadv_iv_ogm_iface_enable)
> - return NET_RX_DROP;
> + goto free_skb;
>  
>   batadv_inc_counter(bat_priv, BATADV_CNT_MGMT_RX);
>   batadv_add_counter(bat_priv, BATADV_CNT_MGMT_RX_BYTES,
> @@ -1854,8 +1855,12 @@ static int batadv_iv_ogm_receive(struct sk_buff *skb,
>   ogm_packet = (struct batadv_ogm_packet *)packet_pos;
>   }
>  
> + ret = NET_RX_SUCCESS;
> +
> +free_skb:
>   consume_skb(skb);
> - return NET_RX_SUCCESS;
> +
> + return ret;
>  }


Okay, but we do have kfree_skb() and consume_skb() and they should be
used appropriately.

[PATCH 13/17] batman-adv: Consume skb in receive handlers

[Simon Wunderlich]

From: Sven Eckelmann 

Receiving functions in Linux consume the supplied skbuff. Doing the same in
the batadv_rx_handler functions makes the behavior more similar to the rest
of the Linux network code.

Signed-off-by: Sven Eckelmann 
Signed-off-by: Simon Wunderlich 
---
 net/batman-adv/bat_iv_ogm.c |  17 +++--
 net/batman-adv/bat_v_elp.c  |  25 ---
 net/batman-adv/bat_v_ogm.c  |  10 +--
 net/batman-adv/main.c   |  11 +--
 net/batman-adv/network-coding.c |  11 +--
 net/batman-adv/routing.c| 149 +++-
 6 files changed, 141 insertions(+), 82 deletions(-)

diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
index 310f391..b9941bf 100644
--- a/net/batman-adv/bat_iv_ogm.c
+++ b/net/batman-adv/bat_iv_ogm.c
@@ -1823,17 +1823,18 @@ static int batadv_iv_ogm_receive(struct sk_buff *skb,
struct batadv_ogm_packet *ogm_packet;
u8 *packet_pos;
int ogm_offset;
-   bool ret;
+   bool res;
+   int ret = NET_RX_DROP;
 
-   ret = batadv_check_management_packet(skb, if_incoming, BATADV_OGM_HLEN);
-   if (!ret)
-   return NET_RX_DROP;
+   res = batadv_check_management_packet(skb, if_incoming, BATADV_OGM_HLEN);
+   if (!res)
+   goto free_skb;
 
/* did we receive a B.A.T.M.A.N. IV OGM packet on an interface
 * that does not have B.A.T.M.A.N. IV enabled ?
 */
if (bat_priv->algo_ops->iface.enable != batadv_iv_ogm_iface_enable)
-   return NET_RX_DROP;
+   goto free_skb;
 
batadv_inc_counter(bat_priv, BATADV_CNT_MGMT_RX);
batadv_add_counter(bat_priv, BATADV_CNT_MGMT_RX_BYTES,
@@ -1854,8 +1855,12 @@ static int batadv_iv_ogm_receive(struct sk_buff *skb,
ogm_packet = (struct batadv_ogm_packet *)packet_pos;
}
 
+   ret = NET_RX_SUCCESS;
+
+free_skb:
consume_skb(skb);
-   return NET_RX_SUCCESS;
+
+   return ret;
 }
 
 #ifdef CONFIG_BATMAN_ADV_DEBUGFS
diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c
index ee08540..81a0501 100644
--- a/net/batman-adv/bat_v_elp.c
+++ b/net/batman-adv/bat_v_elp.c
@@ -492,20 +492,21 @@ int batadv_v_elp_packet_recv(struct sk_buff *skb,
struct batadv_elp_packet *elp_packet;
struct batadv_hard_iface *primary_if;
struct ethhdr *ethhdr = (struct ethhdr *)skb_mac_header(skb);
-   bool ret;
+   bool res;
+   int ret;
 
-   ret = batadv_check_management_packet(skb, if_incoming, BATADV_ELP_HLEN);
-   if (!ret)
-   return NET_RX_DROP;
+   res = batadv_check_management_packet(skb, if_incoming, BATADV_ELP_HLEN);
+   if (!res)
+   goto free_skb;
 
if (batadv_is_my_mac(bat_priv, ethhdr->h_source))
-   return NET_RX_DROP;
+   goto free_skb;
 
/* did we receive a B.A.T.M.A.N. V ELP packet on an interface
 * that does not have B.A.T.M.A.N. V ELP enabled ?
 */
if (strcmp(bat_priv->algo_ops->name, "BATMAN_V") != 0)
-   return NET_RX_DROP;
+   goto free_skb;
 
elp_packet = (struct batadv_elp_packet *)skb->data;
 
@@ -516,14 +517,16 @@ int batadv_v_elp_packet_recv(struct sk_buff *skb,
 
primary_if = batadv_primary_if_get_selected(bat_priv);
if (!primary_if)
-   goto out;
+   goto free_skb;
 
batadv_v_elp_neigh_update(bat_priv, ethhdr->h_source, if_incoming,
  elp_packet);
 
-out:
-   if (primary_if)
-   batadv_hardif_put(primary_if);
+   ret = NET_RX_SUCCESS;
+   batadv_hardif_put(primary_if);
+
+free_skb:
consume_skb(skb);
-   return NET_RX_SUCCESS;
+
+   return ret;
 }
diff --git a/net/batman-adv/bat_v_ogm.c b/net/batman-adv/bat_v_ogm.c
index 9922ccd..ef3a88d 100644
--- a/net/batman-adv/bat_v_ogm.c
+++ b/net/batman-adv/bat_v_ogm.c
@@ -810,18 +810,18 @@ int batadv_v_ogm_packet_recv(struct sk_buff *skb,
 * B.A.T.M.A.N. V enabled ?
 */
if (strcmp(bat_priv->algo_ops->name, "BATMAN_V") != 0)
-   return NET_RX_DROP;
+   goto free_skb;
 
if (!batadv_check_management_packet(skb, if_incoming, BATADV_OGM2_HLEN))
-   return NET_RX_DROP;
+   goto free_skb;
 
if (batadv_is_my_mac(bat_priv, ethhdr->h_source))
-   return NET_RX_DROP;
+   goto free_skb;
 
ogm_packet = (struct batadv_ogm2_packet *)skb->data;
 
if (batadv_is_my_mac(bat_priv, ogm_packet->orig))
-   return NET_RX_DROP;
+   goto free_skb;
 
batadv_inc_counter(bat_priv, BATADV_CNT_MGMT_RX);
batadv_add_counter(bat_priv, BATADV_CNT_MGMT_RX_BYTES,
@@ -842,6 +842,8 @@ int batadv_v_ogm_packet_recv(struct sk_buff *skb,
}
 
ret = NET_RX_SUCCESS;
+
+free_skb: