kfree_skb assumes that an skb is dropped after an failure and notes that. consume_skb should be used in non-failure situations. Such information is important for dropmonitor netlink which tells how many packets were dropped and where this drop happened.
Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/bat_iv_ogm.c | 13 ++++++++----- net/batman-adv/fragmentation.c | 20 ++++++++++++++------ net/batman-adv/network-coding.c | 24 +++++++++++++++--------- net/batman-adv/send.c | 27 +++++++++++++++++++-------- net/batman-adv/send.h | 3 ++- net/batman-adv/soft-interface.c | 2 +- 6 files changed, 59 insertions(+), 30 deletions(-)
diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c index a40cdf2..baf3d72 100644 --- a/net/batman-adv/bat_iv_ogm.c +++ b/net/batman-adv/bat_iv_ogm.c @@ -692,7 +692,7 @@ static void batadv_iv_ogm_aggregate_new(const unsigned char *packet_buff,
forw_packet_aggr->skb = netdev_alloc_skb_ip_align(NULL, skb_size); if (!forw_packet_aggr->skb) { - batadv_forw_packet_free(forw_packet_aggr); + batadv_forw_packet_free(forw_packet_aggr, true); return; }
@@ -1605,7 +1605,7 @@ out: if (hardif_neigh) batadv_hardif_neigh_put(hardif_neigh);
- kfree_skb(skb_priv); + consume_skb(skb_priv); }
/** @@ -1777,6 +1777,7 @@ static void batadv_iv_send_outstanding_bat_ogm_packet(struct work_struct *work) struct delayed_work *delayed_work; struct batadv_forw_packet *forw_packet; struct batadv_priv *bat_priv; + bool dropped = false;
delayed_work = to_delayed_work(work); forw_packet = container_of(delayed_work, struct batadv_forw_packet, @@ -1786,8 +1787,10 @@ static void batadv_iv_send_outstanding_bat_ogm_packet(struct work_struct *work) hlist_del(&forw_packet->list); spin_unlock_bh(&bat_priv->forw_bat_list_lock);
- if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING) + if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING) { + dropped = true; goto out; + }
batadv_iv_ogm_emit(forw_packet);
@@ -1804,7 +1807,7 @@ static void batadv_iv_send_outstanding_bat_ogm_packet(struct work_struct *work) batadv_iv_ogm_schedule(forw_packet->if_incoming);
out: - batadv_forw_packet_free(forw_packet); + batadv_forw_packet_free(forw_packet, dropped); }
static int batadv_iv_ogm_receive(struct sk_buff *skb, @@ -1845,7 +1848,7 @@ static int batadv_iv_ogm_receive(struct sk_buff *skb, ogm_packet = (struct batadv_ogm_packet *)packet_pos; }
- kfree_skb(skb); + consume_skb(skb); return NET_RX_SUCCESS; }
diff --git a/net/batman-adv/fragmentation.c b/net/batman-adv/fragmentation.c index 0934730..461b77d 100644 --- a/net/batman-adv/fragmentation.c +++ b/net/batman-adv/fragmentation.c @@ -42,17 +42,23 @@ /** * batadv_frag_clear_chain - delete entries in the fragment buffer chain * @head: head of chain with entries. + * @dropped: whether the chain is cleared because all fragments are dropped * * Free fragments in the passed hlist. Should be called with appropriate lock. */ -static void batadv_frag_clear_chain(struct hlist_head *head) +static void batadv_frag_clear_chain(struct hlist_head *head, bool dropped) { struct batadv_frag_list_entry *entry; struct hlist_node *node;
hlist_for_each_entry_safe(entry, node, head, list) { hlist_del(&entry->list); - kfree_skb(entry->skb); + + if (dropped) + kfree_skb(entry->skb); + else + consume_skb(entry->skb); + kfree(entry); } } @@ -73,7 +79,7 @@ void batadv_frag_purge_orig(struct batadv_orig_node *orig_node, spin_lock_bh(&chain->lock);
if (!check_cb || check_cb(chain)) { - batadv_frag_clear_chain(&chain->head); + batadv_frag_clear_chain(&chain->head, true); chain->size = 0; }
@@ -118,7 +124,7 @@ static bool batadv_frag_init_chain(struct batadv_frag_table_entry *chain, return false;
if (!hlist_empty(&chain->head)) - batadv_frag_clear_chain(&chain->head); + batadv_frag_clear_chain(&chain->head, true);
chain->size = 0; chain->seqno = seqno; @@ -220,7 +226,7 @@ out: * exceeds the maximum size of one merged packet. Don't allow * packets to have different total_size. */ - batadv_frag_clear_chain(&chain->head); + batadv_frag_clear_chain(&chain->head, true); chain->size = 0; } else if (ntohs(frag_packet->total_size) == chain->size) { /* All fragments received. Hand over chain to caller. */ @@ -254,6 +260,7 @@ batadv_frag_merge_packets(struct hlist_head *chain) struct batadv_frag_list_entry *entry; struct sk_buff *skb_out = NULL; int size, hdr_size = sizeof(struct batadv_frag_packet); + bool dropped = false;
/* Remove first entry, as this is the destination for the rest of the * fragments. @@ -270,6 +277,7 @@ batadv_frag_merge_packets(struct hlist_head *chain) if (pskb_expand_head(skb_out, 0, size - skb_out->len, GFP_ATOMIC) < 0) { kfree_skb(skb_out); skb_out = NULL; + dropped = true; goto free; }
@@ -291,7 +299,7 @@ batadv_frag_merge_packets(struct hlist_head *chain)
free: /* Locking is not needed, because 'chain' is not part of any orig. */ - batadv_frag_clear_chain(chain); + batadv_frag_clear_chain(chain, dropped); return skb_out; }
diff --git a/net/batman-adv/network-coding.c b/net/batman-adv/network-coding.c index 293ef4f..e924256 100644 --- a/net/batman-adv/network-coding.c +++ b/net/batman-adv/network-coding.c @@ -261,10 +261,16 @@ static void batadv_nc_path_put(struct batadv_nc_path *nc_path) /** * batadv_nc_packet_free - frees nc packet * @nc_packet: the nc packet to free + * @dropped: whether the packet is freed because is is dropped */ -static void batadv_nc_packet_free(struct batadv_nc_packet *nc_packet) +static void batadv_nc_packet_free(struct batadv_nc_packet *nc_packet, + bool dropped) { - kfree_skb(nc_packet->skb); + if (dropped) + kfree_skb(nc_packet->skb); + else + consume_skb(nc_packet->skb); + batadv_nc_path_put(nc_packet->nc_path); kfree(nc_packet); } @@ -577,7 +583,7 @@ static void batadv_nc_send_packet(struct batadv_nc_packet *nc_packet) { batadv_send_unicast_skb(nc_packet->skb, nc_packet->neigh_node); nc_packet->skb = NULL; - batadv_nc_packet_free(nc_packet); + batadv_nc_packet_free(nc_packet, false); }
/** @@ -611,7 +617,7 @@ static bool batadv_nc_sniffed_purge(struct batadv_priv *bat_priv,
/* purge nc packet */ list_del(&nc_packet->list); - batadv_nc_packet_free(nc_packet); + batadv_nc_packet_free(nc_packet, true);
res = true;
@@ -1210,11 +1216,11 @@ static bool batadv_nc_code_packets(struct batadv_priv *bat_priv, }
/* skb_src is now coded into skb_dest, so free it */ - kfree_skb(skb_src); + consume_skb(skb_src);
/* avoid duplicate free of skb from nc_packet */ nc_packet->skb = NULL; - batadv_nc_packet_free(nc_packet); + batadv_nc_packet_free(nc_packet, false);
/* Send the coded packet and return true */ batadv_send_unicast_skb(skb_dest, first_dest); @@ -1401,7 +1407,7 @@ static void batadv_nc_skb_store_before_coding(struct batadv_priv *bat_priv, /* batadv_nc_skb_store_for_decoding() clones the skb, so we must free * our ref */ - kfree_skb(skb); + consume_skb(skb); }
/** @@ -1725,7 +1731,7 @@ batadv_nc_skb_decode_packet(struct batadv_priv *bat_priv, struct sk_buff *skb, ether_addr_copy(unicast_packet->dest, orig_dest); unicast_packet->ttvn = ttvn;
- batadv_nc_packet_free(nc_packet); + batadv_nc_packet_free(nc_packet, false); return unicast_packet; }
@@ -1862,7 +1868,7 @@ static int batadv_nc_recv_coded_packet(struct sk_buff *skb, return batadv_recv_unicast_packet(skb, recv_if);
free_nc_packet: - batadv_nc_packet_free(nc_packet); + batadv_nc_packet_free(nc_packet, true); return NET_RX_DROP; }
diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c index 8d4e1f5..4f44ee2 100644 --- a/net/batman-adv/send.c +++ b/net/batman-adv/send.c @@ -451,13 +451,19 @@ int batadv_send_skb_via_gw(struct batadv_priv *bat_priv, struct sk_buff *skb, /** * batadv_forw_packet_free - free a forwarding packet * @forw_packet: The packet to free + * @dropped: whether the packet is freed because is is dropped * * This frees a forwarding packet and releases any resources it might * have claimed. */ -void batadv_forw_packet_free(struct batadv_forw_packet *forw_packet) +void batadv_forw_packet_free(struct batadv_forw_packet *forw_packet, + bool dropped) { - kfree_skb(forw_packet->skb); + if (dropped) + kfree_skb(forw_packet->skb); + else + consume_skb(forw_packet->skb); + if (forw_packet->if_incoming) batadv_hardif_put(forw_packet->if_incoming); if (forw_packet->if_outgoing) @@ -597,7 +603,7 @@ int batadv_add_bcast_packet_to_list(struct batadv_priv *bat_priv, return NETDEV_TX_OK;
err_packet_free: - batadv_forw_packet_free(forw_packet); + batadv_forw_packet_free(forw_packet, true); err: return NETDEV_TX_BUSY; } @@ -610,6 +616,7 @@ static void batadv_send_outstanding_bcast_packet(struct work_struct *work) struct sk_buff *skb1; struct net_device *soft_iface; struct batadv_priv *bat_priv; + bool dropped = false;
delayed_work = to_delayed_work(work); forw_packet = container_of(delayed_work, struct batadv_forw_packet, @@ -621,11 +628,15 @@ static void batadv_send_outstanding_bcast_packet(struct work_struct *work) hlist_del(&forw_packet->list); spin_unlock_bh(&bat_priv->forw_bcast_list_lock);
- if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING) + if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING) { + dropped = true; goto out; + }
- if (batadv_dat_drop_broadcast_packet(bat_priv, forw_packet)) + if (batadv_dat_drop_broadcast_packet(bat_priv, forw_packet)) { + dropped = true; goto out; + }
/* rebroadcast packet */ rcu_read_lock(); @@ -658,7 +669,7 @@ static void batadv_send_outstanding_bcast_packet(struct work_struct *work) }
out: - batadv_forw_packet_free(forw_packet); + batadv_forw_packet_free(forw_packet, dropped); }
void @@ -699,7 +710,7 @@ batadv_purge_outstanding_packets(struct batadv_priv *bat_priv,
if (pending) { hlist_del(&forw_packet->list); - batadv_forw_packet_free(forw_packet); + batadv_forw_packet_free(forw_packet, true); } } spin_unlock_bh(&bat_priv->forw_bcast_list_lock); @@ -726,7 +737,7 @@ batadv_purge_outstanding_packets(struct batadv_priv *bat_priv,
if (pending) { hlist_del(&forw_packet->list); - batadv_forw_packet_free(forw_packet); + batadv_forw_packet_free(forw_packet, true); } } spin_unlock_bh(&bat_priv->forw_bat_list_lock); diff --git a/net/batman-adv/send.h b/net/batman-adv/send.h index 999f786..e3968fe 100644 --- a/net/batman-adv/send.h +++ b/net/batman-adv/send.h @@ -27,7 +27,8 @@
struct sk_buff;
-void batadv_forw_packet_free(struct batadv_forw_packet *forw_packet); +void batadv_forw_packet_free(struct batadv_forw_packet *forw_packet, + bool dropped); struct batadv_forw_packet * batadv_forw_packet_alloc(struct batadv_hard_iface *if_incoming, struct batadv_hard_iface *if_outgoing, diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c index e508bf5..cb5b966 100644 --- a/net/batman-adv/soft-interface.c +++ b/net/batman-adv/soft-interface.c @@ -341,7 +341,7 @@ send: /* a copy is stored in the bcast list, therefore removing * the original skb. */ - kfree_skb(skb); + consume_skb(skb);
/* unicast packet */ } else {
A failure during the submission also causes dropped packets. batadv_interface_tx should therefore also increase the DROPPED counter for these returns.
Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/soft-interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c index cb5b966..5cf41e7 100644 --- a/net/batman-adv/soft-interface.c +++ b/net/batman-adv/soft-interface.c @@ -365,7 +365,7 @@ send: ret = batadv_send_skb_via_tt(bat_priv, skb, dst_hint, vid); } - if (ret == NET_XMIT_DROP) + if (ret != NET_XMIT_SUCCESS) goto dropped_freed; }
Sending functions in Linux consume the supplied skbuff. Doing the same in batadv_frag_send_packet avoids the hack of returning -1 (-EPERM) to signal the caller that he is responsible for cleaning up the skb.
Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/fragmentation.c | 50 ++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 21 deletions(-)
diff --git a/net/batman-adv/fragmentation.c b/net/batman-adv/fragmentation.c index 461b77d..4751141 100644 --- a/net/batman-adv/fragmentation.c +++ b/net/batman-adv/fragmentation.c @@ -20,6 +20,7 @@
#include <linux/atomic.h> #include <linux/byteorder/generic.h> +#include <linux/errno.h> #include <linux/etherdevice.h> #include <linux/fs.h> #include <linux/if_ether.h> @@ -441,8 +442,7 @@ err: * @orig_node: final destination of the created fragments * @neigh_node: next-hop of the created fragments * - * Return: the netdev tx status or -1 in case of error. - * When -1 is returned the skb is not consumed. + * Return: the netdev tx status or a negative errno code on a failure */ int batadv_frag_send_packet(struct sk_buff *skb, struct batadv_orig_node *orig_node, @@ -455,7 +455,7 @@ int batadv_frag_send_packet(struct sk_buff *skb, unsigned int mtu = neigh_node->if_incoming->net_dev->mtu; unsigned int header_size = sizeof(frag_header); unsigned int max_fragment_size, max_packet_size; - int ret = -1; + int ret;
/* To avoid merge and refragmentation at next-hops we never send * fragments larger than BATADV_FRAG_MAX_FRAG_SIZE @@ -465,13 +465,17 @@ int batadv_frag_send_packet(struct sk_buff *skb, max_packet_size = max_fragment_size * BATADV_FRAG_MAX_FRAGMENTS;
/* Don't even try to fragment, if we need more than 16 fragments */ - if (skb->len > max_packet_size) - goto out; + if (skb->len > max_packet_size) { + ret = -EAGAIN; + goto free_skb; + }
bat_priv = orig_node->bat_priv; primary_if = batadv_primary_if_get_selected(bat_priv); - if (!primary_if) - goto out; + if (!primary_if) { + ret = -EINVAL; + goto put_primary_if; + }
/* Create one header to be copied to all fragments */ frag_header.packet_type = BATADV_UNICAST_FRAG; @@ -496,34 +500,35 @@ int batadv_frag_send_packet(struct sk_buff *skb, /* Eat and send fragments from the tail of skb */ while (skb->len > max_fragment_size) { skb_fragment = batadv_frag_create(skb, &frag_header, mtu); - if (!skb_fragment) - goto out; + if (!skb_fragment) { + ret = -ENOMEM; + goto free_skb; + }
batadv_inc_counter(bat_priv, BATADV_CNT_FRAG_TX); batadv_add_counter(bat_priv, BATADV_CNT_FRAG_TX_BYTES, skb_fragment->len + ETH_HLEN); ret = batadv_send_unicast_skb(skb_fragment, neigh_node); if (ret != NET_XMIT_SUCCESS) { - /* return -1 so that the caller can free the original - * skb - */ - ret = -1; - goto out; + ret = NET_XMIT_DROP; + goto free_skb; }
frag_header.no++;
/* The initial check in this function should cover this case */ if (frag_header.no == BATADV_FRAG_MAX_FRAGMENTS - 1) { - ret = -1; - goto out; + ret = -EINVAL; + goto free_skb; } }
/* Make room for the fragment header. */ if (batadv_skb_head_push(skb, header_size) < 0 || - pskb_expand_head(skb, header_size + ETH_HLEN, 0, GFP_ATOMIC) < 0) - goto out; + pskb_expand_head(skb, header_size + ETH_HLEN, 0, GFP_ATOMIC) < 0) { + ret = -ENOMEM; + goto free_skb; + }
memcpy(skb->data, &frag_header, header_size);
@@ -532,10 +537,13 @@ int batadv_frag_send_packet(struct sk_buff *skb, batadv_add_counter(bat_priv, BATADV_CNT_FRAG_TX_BYTES, skb->len + ETH_HLEN); ret = batadv_send_unicast_skb(skb, neigh_node); + /* skb was consumed */ + skb = NULL;
-out: - if (primary_if) - batadv_hardif_put(primary_if); +put_primary_if: + batadv_hardif_put(primary_if); +free_skb: + kfree_skb(skb);
return ret; }
Sending functions in Linux consume the supplied skbuff. Doing the same in batadv_send_skb_to_orig avoids the hack of returning -1 (-EPERM) to signal the caller that he is responsible for cleaning up the skb.
Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/routing.c | 11 ++--------- net/batman-adv/send.c | 39 ++++++++++++++++++++++----------------- net/batman-adv/tp_meter.c | 6 ------ net/batman-adv/tvlv.c | 5 +---- 4 files changed, 25 insertions(+), 36 deletions(-)
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c index 610f2c4..49ff1cf 100644 --- a/net/batman-adv/routing.c +++ b/net/batman-adv/routing.c @@ -262,9 +262,6 @@ static int batadv_recv_my_icmp_packet(struct batadv_priv *bat_priv, icmph->ttl = BATADV_TTL;
res = batadv_send_skb_to_orig(skb, orig_node, NULL); - if (res == -1) - goto out; - ret = NET_RX_SUCCESS;
break; @@ -325,8 +322,7 @@ static int batadv_recv_icmp_ttl_exceeded(struct batadv_priv *bat_priv, icmp_packet->ttl = BATADV_TTL;
res = batadv_send_skb_to_orig(skb, orig_node, NULL); - if (res != -1) - ret = NET_RX_SUCCESS; + ret = NET_RX_SUCCESS;
out: if (primary_if) @@ -413,8 +409,7 @@ int batadv_recv_icmp_packet(struct sk_buff *skb,
/* route it */ res = batadv_send_skb_to_orig(skb, orig_node, recv_if); - if (res != -1) - ret = NET_RX_SUCCESS; + ret = NET_RX_SUCCESS;
out: if (orig_node) @@ -676,8 +671,6 @@ static int batadv_route_unicast_packet(struct sk_buff *skb,
len = skb->len; res = batadv_send_skb_to_orig(skb, orig_node, recv_if); - if (res == -1) - goto out;
/* translate transmit result into receive result */ if (res == NET_XMIT_SUCCESS) { diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c index 4f44ee2..4254172 100644 --- a/net/batman-adv/send.c +++ b/net/batman-adv/send.c @@ -165,11 +165,9 @@ int batadv_send_unicast_skb(struct sk_buff *skb, * host, NULL can be passed as recv_if and no interface alternating is * attempted. * - * Return: -1 on failure (and the skb is not consumed), -EINPROGRESS if the - * skb is buffered for later transmit or the NET_XMIT status returned by the + * Return: negative errno code on a failure, -EINPROGRESS if the skb is + * buffered for later transmit or the NET_XMIT status returned by the * lower routine if the packet has been passed down. - * - * If the returning value is not -1 the skb has been consumed. */ int batadv_send_skb_to_orig(struct sk_buff *skb, struct batadv_orig_node *orig_node, @@ -177,12 +175,14 @@ int batadv_send_skb_to_orig(struct sk_buff *skb, { struct batadv_priv *bat_priv = orig_node->bat_priv; struct batadv_neigh_node *neigh_node; - int ret = -1; + int ret;
/* batadv_find_router() increases neigh_nodes refcount if found. */ neigh_node = batadv_find_router(bat_priv, orig_node, recv_if); - if (!neigh_node) - goto out; + if (!neigh_node) { + ret = -EINVAL; + goto free_skb; + }
/* Check if the skb is too large to send in one piece and fragment * it if needed. @@ -191,8 +191,10 @@ int batadv_send_skb_to_orig(struct sk_buff *skb, skb->len > neigh_node->if_incoming->net_dev->mtu) { /* Fragment and send packet. */ ret = batadv_frag_send_packet(skb, orig_node, neigh_node); + /* skb was consumed */ + skb = NULL;
- goto out; + goto put_neigh_node; }
/* try to network code the packet, if it is received on an interface @@ -204,9 +206,13 @@ int batadv_send_skb_to_orig(struct sk_buff *skb, else ret = batadv_send_unicast_skb(skb, neigh_node);
-out: - if (neigh_node) - batadv_neigh_node_put(neigh_node); + /* skb was consumed */ + skb = NULL; + +put_neigh_node: + batadv_neigh_node_put(neigh_node); +free_skb: + kfree_skb(skb);
return ret; } @@ -327,7 +333,7 @@ int batadv_send_skb_unicast(struct batadv_priv *bat_priv, { struct batadv_unicast_packet *unicast_packet; struct ethhdr *ethhdr; - int res, ret = NET_XMIT_DROP; + int ret = NET_XMIT_DROP;
if (!orig_node) goto out; @@ -364,13 +370,12 @@ int batadv_send_skb_unicast(struct batadv_priv *bat_priv, if (batadv_tt_global_client_is_roaming(bat_priv, ethhdr->h_dest, vid)) unicast_packet->ttvn = unicast_packet->ttvn - 1;
- res = batadv_send_skb_to_orig(skb, orig_node, NULL); - if (res != -1) - ret = NET_XMIT_SUCCESS; + ret = batadv_send_skb_to_orig(skb, orig_node, NULL); + /* skb was consumed */ + skb = NULL;
out: - if (ret == NET_XMIT_DROP) - kfree_skb(skb); + kfree_skb(skb); return ret; }
diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c index 2333777..f156452 100644 --- a/net/batman-adv/tp_meter.c +++ b/net/batman-adv/tp_meter.c @@ -615,9 +615,6 @@ static int batadv_tp_send_msg(struct batadv_tp_vars *tp_vars, const u8 *src, batadv_tp_fill_prerandom(tp_vars, data, data_len);
r = batadv_send_skb_to_orig(skb, orig_node, NULL); - if (r == -1) - kfree_skb(skb); - if (r == NET_XMIT_SUCCESS) return 0;
@@ -1206,9 +1203,6 @@ static int batadv_tp_send_ack(struct batadv_priv *bat_priv, const u8 *dst,
/* send the ack */ r = batadv_send_skb_to_orig(skb, orig_node, NULL); - if (r == -1) - kfree_skb(skb); - if (unlikely(r < 0) || (r == NET_XMIT_DROP)) { ret = BATADV_TP_REASON_DST_UNREACHABLE; goto out; diff --git a/net/batman-adv/tvlv.c b/net/batman-adv/tvlv.c index 3d1cf0f..221efa3 100644 --- a/net/batman-adv/tvlv.c +++ b/net/batman-adv/tvlv.c @@ -591,7 +591,6 @@ void batadv_tvlv_unicast_send(struct batadv_priv *bat_priv, u8 *src, unsigned char *tvlv_buff; unsigned int tvlv_len; ssize_t hdr_len = sizeof(*unicast_tvlv_packet); - int res;
orig_node = batadv_orig_hash_find(bat_priv, dst); if (!orig_node) @@ -624,9 +623,7 @@ void batadv_tvlv_unicast_send(struct batadv_priv *bat_priv, u8 *src, tvlv_buff += sizeof(*tvlv_hdr); memcpy(tvlv_buff, tvlv_value, tvlv_value_len);
- res = batadv_send_skb_to_orig(skb, orig_node, NULL); - if (res == -1) - kfree_skb(skb); + batadv_send_skb_to_orig(skb, orig_node, NULL); out: batadv_orig_node_put(orig_node); }
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 sven@narfation.org --- 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 baf3d72..3c5cd90 100644 --- a/net/batman-adv/bat_iv_ogm.c +++ b/net/batman-adv/bat_iv_ogm.c @@ -1817,17 +1817,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, @@ -1848,8 +1849,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; }
/** diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c index 7d17001..0e5f901 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 6fbba4e..8bde16d 100644 --- a/net/batman-adv/bat_v_ogm.c +++ b/net/batman-adv/bat_v_ogm.c @@ -755,18 +755,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, @@ -787,6 +787,8 @@ int batadv_v_ogm_packet_recv(struct sk_buff *skb, }
ret = NET_RX_SUCCESS; + +free_skb: consume_skb(skb);
return ret; diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c index ef07e5b..e09d10f 100644 --- a/net/batman-adv/main.c +++ b/net/batman-adv/main.c @@ -400,6 +400,8 @@ void batadv_skb_set_priority(struct sk_buff *skb, int offset) static int batadv_recv_unhandled_packet(struct sk_buff *skb, struct batadv_hard_iface *recv_if) { + kfree_skb(skb); + return NET_RX_DROP; }
@@ -414,7 +416,6 @@ int batadv_batman_skb_recv(struct sk_buff *skb, struct net_device *dev, struct batadv_ogm_packet *batadv_ogm_packet; struct batadv_hard_iface *hard_iface; u8 idx; - int ret;
hard_iface = container_of(ptype, struct batadv_hard_iface, batman_adv_ptype); @@ -464,14 +465,8 @@ int batadv_batman_skb_recv(struct sk_buff *skb, struct net_device *dev, /* reset control block to avoid left overs from previous users */ memset(skb->cb, 0, sizeof(struct batadv_skb_cb));
- /* all receive handlers return whether they received or reused - * the supplied skb. if not, we have to free the skb. - */ idx = batadv_ogm_packet->packet_type; - ret = (*batadv_rx_handler[idx])(skb, hard_iface); - - if (ret == NET_RX_DROP) - kfree_skb(skb); + (*batadv_rx_handler[idx])(skb, hard_iface);
batadv_hardif_put(hard_iface);
diff --git a/net/batman-adv/network-coding.c b/net/batman-adv/network-coding.c index e924256..fd4410d 100644 --- a/net/batman-adv/network-coding.c +++ b/net/batman-adv/network-coding.c @@ -1821,11 +1821,11 @@ static int batadv_nc_recv_coded_packet(struct sk_buff *skb,
/* Check if network coding is enabled */ if (!atomic_read(&bat_priv->network_coding)) - return NET_RX_DROP; + goto free_skb;
/* Make sure we can access (and remove) header */ if (unlikely(!pskb_may_pull(skb, hdr_size))) - return NET_RX_DROP; + goto free_skb;
coded_packet = (struct batadv_coded_packet *)skb->data; ethhdr = eth_hdr(skb); @@ -1833,7 +1833,7 @@ static int batadv_nc_recv_coded_packet(struct sk_buff *skb, /* Verify frame is destined for us */ if (!batadv_is_my_mac(bat_priv, ethhdr->h_dest) && !batadv_is_my_mac(bat_priv, coded_packet->second_dest)) - return NET_RX_DROP; + goto free_skb;
/* Update stat counter */ if (batadv_is_my_mac(bat_priv, coded_packet->second_dest)) @@ -1843,7 +1843,7 @@ static int batadv_nc_recv_coded_packet(struct sk_buff *skb, coded_packet); if (!nc_packet) { batadv_inc_counter(bat_priv, BATADV_CNT_NC_DECODE_FAILED); - return NET_RX_DROP; + goto free_skb; }
/* Make skb's linear, because decoding accesses the entire buffer */ @@ -1869,6 +1869,9 @@ static int batadv_nc_recv_coded_packet(struct sk_buff *skb,
free_nc_packet: batadv_nc_packet_free(nc_packet, true); +free_skb: + kfree_skb(skb); + return NET_RX_DROP; }
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c index 49ff1cf..452b166 100644 --- a/net/batman-adv/routing.c +++ b/net/batman-adv/routing.c @@ -262,8 +262,11 @@ static int batadv_recv_my_icmp_packet(struct batadv_priv *bat_priv, icmph->ttl = BATADV_TTL;
res = batadv_send_skb_to_orig(skb, orig_node, NULL); - ret = NET_RX_SUCCESS; + if (res == NET_XMIT_SUCCESS) + ret = NET_RX_SUCCESS;
+ /* skb was consumed */ + skb = NULL; break; case BATADV_TP: if (!pskb_may_pull(skb, sizeof(struct batadv_icmp_tp_packet))) @@ -271,6 +274,8 @@ static int batadv_recv_my_icmp_packet(struct batadv_priv *bat_priv,
batadv_tp_meter_recv(bat_priv, skb); ret = NET_RX_SUCCESS; + /* skb was consumed */ + skb = NULL; goto out; default: /* drop unknown type */ @@ -281,6 +286,9 @@ out: batadv_hardif_put(primary_if); if (orig_node) batadv_orig_node_put(orig_node); + + kfree_skb(skb); + return ret; }
@@ -322,13 +330,20 @@ static int batadv_recv_icmp_ttl_exceeded(struct batadv_priv *bat_priv, icmp_packet->ttl = BATADV_TTL;
res = batadv_send_skb_to_orig(skb, orig_node, NULL); - ret = NET_RX_SUCCESS; + if (res == NET_RX_SUCCESS) + ret = NET_XMIT_SUCCESS; + + /* skb was consumed */ + skb = NULL;
out: if (primary_if) batadv_hardif_put(primary_if); if (orig_node) batadv_orig_node_put(orig_node); + + kfree_skb(skb); + return ret; }
@@ -345,21 +360,21 @@ int batadv_recv_icmp_packet(struct sk_buff *skb,
/* drop packet if it has not necessary minimum size */ if (unlikely(!pskb_may_pull(skb, hdr_size))) - goto out; + goto free_skb;
ethhdr = eth_hdr(skb);
/* packet with unicast indication but broadcast recipient */ if (is_broadcast_ether_addr(ethhdr->h_dest)) - goto out; + goto free_skb;
/* packet with broadcast sender address */ if (is_broadcast_ether_addr(ethhdr->h_source)) - goto out; + goto free_skb;
/* not for me */ if (!batadv_is_my_mac(bat_priv, ethhdr->h_dest)) - goto out; + goto free_skb;
icmph = (struct batadv_icmp_header *)skb->data;
@@ -368,17 +383,17 @@ int batadv_recv_icmp_packet(struct sk_buff *skb, icmph->msg_type == BATADV_ECHO_REQUEST) && (skb->len >= sizeof(struct batadv_icmp_packet_rr))) { if (skb_linearize(skb) < 0) - goto out; + goto free_skb;
/* create a copy of the skb, if needed, to modify it. */ if (skb_cow(skb, ETH_HLEN) < 0) - goto out; + goto free_skb;
ethhdr = eth_hdr(skb); icmph = (struct batadv_icmp_header *)skb->data; icmp_packet_rr = (struct batadv_icmp_packet_rr *)icmph; if (icmp_packet_rr->rr_cur >= BATADV_RR_LEN) - goto out; + goto free_skb;
ether_addr_copy(icmp_packet_rr->rr[icmp_packet_rr->rr_cur], ethhdr->h_dest); @@ -396,11 +411,11 @@ int batadv_recv_icmp_packet(struct sk_buff *skb, /* get routing information */ orig_node = batadv_orig_hash_find(bat_priv, icmph->dst); if (!orig_node) - goto out; + goto free_skb;
/* create a copy of the skb, if needed, to modify it. */ if (skb_cow(skb, ETH_HLEN) < 0) - goto out; + goto put_orig_node;
icmph = (struct batadv_icmp_header *)skb->data;
@@ -409,11 +424,18 @@ int batadv_recv_icmp_packet(struct sk_buff *skb,
/* route it */ res = batadv_send_skb_to_orig(skb, orig_node, recv_if); - ret = NET_RX_SUCCESS; + if (res == NET_XMIT_SUCCESS) + ret = NET_RX_SUCCESS;
-out: + /* skb was consumed */ + skb = NULL; + +put_orig_node: if (orig_node) batadv_orig_node_put(orig_node); +free_skb: + kfree_skb(skb); + return ret; }
@@ -636,18 +658,18 @@ static int batadv_route_unicast_packet(struct sk_buff *skb, if (unicast_packet->ttl < 2) { pr_debug("Warning - can't forward unicast packet from %pM to %pM: ttl exceeded\n", ethhdr->h_source, unicast_packet->dest); - goto out; + goto free_skb; }
/* get routing information */ orig_node = batadv_orig_hash_find(bat_priv, unicast_packet->dest);
if (!orig_node) - goto out; + goto free_skb;
/* create a copy of the skb, if needed, to modify it. */ if (skb_cow(skb, ETH_HLEN) < 0) - goto out; + goto put_orig_node;
/* decrement ttl */ unicast_packet = (struct batadv_unicast_packet *)skb->data; @@ -671,6 +693,11 @@ static int batadv_route_unicast_packet(struct sk_buff *skb,
len = skb->len; res = batadv_send_skb_to_orig(skb, orig_node, recv_if); + if (res == NET_XMIT_SUCCESS) + ret = NET_RX_SUCCESS; + + /* skb was consumed */ + skb = NULL;
/* translate transmit result into receive result */ if (res == NET_XMIT_SUCCESS) { @@ -680,11 +707,11 @@ static int batadv_route_unicast_packet(struct sk_buff *skb, len + ETH_HLEN); }
- ret = NET_RX_SUCCESS; +put_orig_node: + batadv_orig_node_put(orig_node); +free_skb: + kfree_skb(skb);
-out: - if (orig_node) - batadv_orig_node_put(orig_node); return ret; }
@@ -869,14 +896,18 @@ int batadv_recv_unhandled_unicast_packet(struct sk_buff *skb,
check = batadv_check_unicast_packet(bat_priv, skb, hdr_size); if (check < 0) - return NET_RX_DROP; + goto free_skb;
/* we don't know about this type, drop it. */ unicast_packet = (struct batadv_unicast_packet *)skb->data; if (batadv_is_my_mac(bat_priv, unicast_packet->dest)) - return NET_RX_DROP; + goto free_skb;
return batadv_route_unicast_packet(skb, recv_if); + +free_skb: + kfree_skb(skb); + return NET_RX_DROP; }
int batadv_recv_unicast_packet(struct sk_buff *skb, @@ -890,6 +921,7 @@ int batadv_recv_unicast_packet(struct sk_buff *skb, int check, hdr_size = sizeof(*unicast_packet); enum batadv_subtype subtype; bool is4addr; + int ret = NET_RX_DROP;
unicast_packet = (struct batadv_unicast_packet *)skb->data; unicast_4addr_packet = (struct batadv_unicast_4addr_packet *)skb->data; @@ -909,9 +941,9 @@ int batadv_recv_unicast_packet(struct sk_buff *skb, batadv_nc_skb_store_sniffed_unicast(bat_priv, skb);
if (check < 0) - return NET_RX_DROP; + goto free_skb; if (!batadv_check_unicast_ttvn(bat_priv, skb, hdr_size)) - return NET_RX_DROP; + goto free_skb;
/* packet for me */ if (batadv_is_my_mac(bat_priv, unicast_packet->dest)) { @@ -949,7 +981,14 @@ rx_success: return NET_RX_SUCCESS; }
- return batadv_route_unicast_packet(skb, recv_if); + ret = batadv_route_unicast_packet(skb, recv_if); + /* skb was consumed */ + skb = NULL; + +free_skb: + kfree_skb(skb); + + return ret; }
/** @@ -971,15 +1010,15 @@ int batadv_recv_unicast_tvlv(struct sk_buff *skb, int ret = NET_RX_DROP;
if (batadv_check_unicast_packet(bat_priv, skb, hdr_size) < 0) - return NET_RX_DROP; + goto free_skb;
/* the header is likely to be modified while forwarding */ if (skb_cow(skb, hdr_size) < 0) - return NET_RX_DROP; + goto free_skb;
/* packet needs to be linearized to access the tvlv content */ if (skb_linearize(skb) < 0) - return NET_RX_DROP; + goto free_skb;
unicast_tvlv_packet = (struct batadv_unicast_tvlv_packet *)skb->data;
@@ -987,17 +1026,21 @@ int batadv_recv_unicast_tvlv(struct sk_buff *skb, tvlv_buff_len = ntohs(unicast_tvlv_packet->tvlv_len);
if (tvlv_buff_len > skb->len - hdr_size) - return NET_RX_DROP; + goto free_skb;
ret = batadv_tvlv_containers_process(bat_priv, false, NULL, unicast_tvlv_packet->src, unicast_tvlv_packet->dst, tvlv_buff, tvlv_buff_len);
- if (ret != NET_RX_SUCCESS) + if (ret != NET_RX_SUCCESS) { ret = batadv_route_unicast_packet(skb, recv_if); - else - consume_skb(skb); + /* skb was consumed */ + skb = NULL; + } + +free_skb: + consume_skb(skb);
return ret; } @@ -1023,20 +1066,22 @@ int batadv_recv_frag_packet(struct sk_buff *skb,
if (batadv_check_unicast_packet(bat_priv, skb, sizeof(*frag_packet)) < 0) - goto out; + goto free_skb;
frag_packet = (struct batadv_frag_packet *)skb->data; orig_node_src = batadv_orig_hash_find(bat_priv, frag_packet->orig); if (!orig_node_src) - goto out; + goto free_skb;
skb->priority = frag_packet->priority + 256;
/* Route the fragment if it is not for us and too big to be merged. */ if (!batadv_is_my_mac(bat_priv, frag_packet->dest) && batadv_frag_skb_fwd(skb, recv_if, orig_node_src)) { + /* skb was consumed */ + skb = NULL; ret = NET_RX_SUCCESS; - goto out; + goto put_orig_node; }
batadv_inc_counter(bat_priv, BATADV_CNT_FRAG_RX); @@ -1044,20 +1089,24 @@ int batadv_recv_frag_packet(struct sk_buff *skb,
/* Add fragment to buffer and merge if possible. */ if (!batadv_frag_skb_buffer(&skb, orig_node_src)) - goto out; + goto put_orig_node;
/* Deliver merged packet to the appropriate handler, if it was * merged */ - if (skb) + if (skb) { batadv_batman_skb_recv(skb, recv_if->net_dev, &recv_if->batman_adv_ptype, NULL); + /* skb was consumed */ + skb = NULL; + }
ret = NET_RX_SUCCESS;
-out: - if (orig_node_src) - batadv_orig_node_put(orig_node_src); +put_orig_node: + batadv_orig_node_put(orig_node_src); +free_skb: + kfree_skb(skb);
return ret; } @@ -1076,35 +1125,35 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
/* drop packet if it has not necessary minimum size */ if (unlikely(!pskb_may_pull(skb, hdr_size))) - goto out; + goto free_skb;
ethhdr = eth_hdr(skb);
/* packet with broadcast indication but unicast recipient */ if (!is_broadcast_ether_addr(ethhdr->h_dest)) - goto out; + goto free_skb;
/* packet with broadcast sender address */ if (is_broadcast_ether_addr(ethhdr->h_source)) - goto out; + goto free_skb;
/* ignore broadcasts sent by myself */ if (batadv_is_my_mac(bat_priv, ethhdr->h_source)) - goto out; + goto free_skb;
bcast_packet = (struct batadv_bcast_packet *)skb->data;
/* ignore broadcasts originated by myself */ if (batadv_is_my_mac(bat_priv, bcast_packet->orig)) - goto out; + goto free_skb;
if (bcast_packet->ttl < 2) - goto out; + goto free_skb;
orig_node = batadv_orig_hash_find(bat_priv, bcast_packet->orig);
if (!orig_node) - goto out; + goto free_skb;
spin_lock_bh(&orig_node->bcast_seqno_lock);
@@ -1132,7 +1181,7 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
/* check whether this has been sent by another originator before */ if (batadv_bla_check_bcast_duplist(bat_priv, skb)) - goto out; + goto free_skb;
batadv_skb_set_priority(skb, sizeof(struct batadv_bcast_packet));
@@ -1143,7 +1192,7 @@ int batadv_recv_bcast_packet(struct sk_buff *skb, * from the same backbone. */ if (batadv_bla_is_backbone_gw(skb, orig_node, hdr_size)) - goto out; + goto free_skb;
if (batadv_dat_snoop_incoming_arp_request(bat_priv, skb, hdr_size)) goto rx_success; @@ -1159,6 +1208,8 @@ rx_success:
spin_unlock: spin_unlock_bh(&orig_node->bcast_seqno_lock); +free_skb: + kfree_skb(skb); out: if (orig_node) batadv_orig_node_put(orig_node);
No caller of batadv_send_skb_to_orig is expecting the results to be -1 (-EPERM) anymore when the skbuff was not consumed. They will instead expect that the skbuff is always consumed. Having such return code filter is therefore not needed anymore.
Signed-off-by: Sven Eckelmann sven@narfation.org --- net/batman-adv/send.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c index 4254172..2b946c1 100644 --- a/net/batman-adv/send.c +++ b/net/batman-adv/send.c @@ -64,8 +64,11 @@ static void batadv_send_outstanding_bcast_packet(struct work_struct *work); * If neigh_node is NULL, then the packet is broadcasted using hard_iface, * otherwise it is sent as unicast to the given neighbor. * - * Return: NET_TX_DROP in case of error or the result of dev_queue_xmit(skb) - * otherwise + * Regardless of the return value, the skb is consumed. + * + * Return: A negative errno code is returned on a failure. A success does not + * guarantee the frame will be transmitted as it may be dropped due + * to congestion or traffic shaping. */ int batadv_send_skb_packet(struct sk_buff *skb, struct batadv_hard_iface *hard_iface, @@ -73,7 +76,6 @@ int batadv_send_skb_packet(struct sk_buff *skb, { struct batadv_priv *bat_priv; struct ethhdr *ethhdr; - int ret;
bat_priv = netdev_priv(hard_iface->soft_iface);
@@ -111,15 +113,8 @@ int batadv_send_skb_packet(struct sk_buff *skb, /* dev_queue_xmit() returns a negative result on error. However on * congestion and traffic shaping, it drops and returns NET_XMIT_DROP * (which is > 0). This will not be treated as an error. - * - * a negative value cannot be returned because it could be interepreted - * as not consumed skb by callers of batadv_send_skb_to_orig. */ - ret = dev_queue_xmit(skb); - if (ret < 0) - ret = NET_XMIT_DROP; - - return ret; + return dev_queue_xmit(skb); send_skb_err: kfree_skb(skb); return NET_XMIT_DROP;
On Sunday, July 17, 2016 9:04:00 PM CEST Sven Eckelmann wrote:
kfree_skb assumes that an skb is dropped after an failure and notes that. consume_skb should be used in non-failure situations. Such information is important for dropmonitor netlink which tells how many packets were dropped and where this drop happened.
Signed-off-by: Sven Eckelmann sven@narfation.org
Applied the whole series in 59426a7..69fd4c0
Thanks, Simon
On Sun, Jul 17, 2016 at 09:04:00PM +0200, Sven Eckelmann wrote:
kfree_skb assumes that an skb is dropped after an failure and notes that. consume_skb should be used in non-failure situations. Such information is important for dropmonitor netlink which tells how many packets were dropped and where this drop happened.
Just a few, curious questions regarding why a kfree_skb() was chosen instead of a consume_skb() in a few places.
Especially so that I hopefully know which one best to use when rebasing the "batman-adv: fix race conditions on interface removal" patch :-).
Signed-off-by: Sven Eckelmann sven@narfation.org
net/batman-adv/bat_iv_ogm.c | 13 ++++++++----- net/batman-adv/fragmentation.c | 20 ++++++++++++++------ net/batman-adv/network-coding.c | 24 +++++++++++++++--------- net/batman-adv/send.c | 27 +++++++++++++++++++-------- net/batman-adv/send.h | 3 ++- net/batman-adv/soft-interface.c | 2 +- 6 files changed, 59 insertions(+), 30 deletions(-)
diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c index a40cdf2..baf3d72 100644 --- a/net/batman-adv/bat_iv_ogm.c +++ b/net/batman-adv/bat_iv_ogm.c @@ -1786,8 +1787,10 @@ static void batadv_iv_send_outstanding_bat_ogm_packet(struct work_struct *work) hlist_del(&forw_packet->list); spin_unlock_bh(&bat_priv->forw_bat_list_lock);
- if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING)
- if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING) {
dropped = true;- }
Is this reallly a failure case?
diff --git a/net/batman-adv/fragmentation.c b/net/batman-adv/fragmentation.c index 0934730..461b77d 100644 --- a/net/batman-adv/fragmentation.c +++ b/net/batman-adv/fragmentation.c @@ -42,17 +42,23 @@ @@ -73,7 +79,7 @@ void batadv_frag_purge_orig(struct batadv_orig_node *orig_node, spin_lock_bh(&chain->lock);
if (!check_cb || check_cb(chain)) {
batadv_frag_clear_chain(&chain->head);
batadv_frag_clear_chain(&chain->head, true); chain->size = 0;
Hm, have you chosen kfree_skb() over consume_skb() because it cannot easily be determined whether this call was from a failure case or not?
diff --git a/net/batman-adv/network-coding.c b/net/batman-adv/network-coding.c index 293ef4f..e924256 100644 --- a/net/batman-adv/network-coding.c +++ b/net/batman-adv/network-coding.c @@ -611,7 +617,7 @@ static bool batadv_nc_sniffed_purge(struct batadv_priv *bat_priv,
/* purge nc packet */ list_del(&nc_packet->list);
- batadv_nc_packet_free(nc_packet);
batadv_nc_packet_free(nc_packet, true);
res = true;
I could imagine, that with promiscious sniffing for coded packets, outdated, coded packets happen frequently and is not necessarilly a failure per se but to be expected.
On the other hand, missing a coding opportunity could have happened due to some failure elsewhere (a broken wifi driver, a noisy environment, ...).
In such an ambiguous case, should kfree_skb() be prefered over consume_skb()?
diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c index 8d4e1f5..4f44ee2 100644 --- a/net/batman-adv/send.c +++ b/net/batman-adv/send.c @@ -610,6 +616,7 @@ static void batadv_send_outstanding_bcast_packet(struct work_struct *work) struct sk_buff *skb1; struct net_device *soft_iface; struct batadv_priv *bat_priv;
bool dropped = false;
delayed_work = to_delayed_work(work); forw_packet = container_of(delayed_work, struct batadv_forw_packet,
@@ -621,11 +628,15 @@ static void batadv_send_outstanding_bcast_packet(struct work_struct *work) hlist_del(&forw_packet->list); spin_unlock_bh(&bat_priv->forw_bcast_list_lock);
- if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING)
- if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING) {
dropped = true;- }
Same as above, why is this considered a failure case?
- if (batadv_dat_drop_broadcast_packet(bat_priv, forw_packet))
- if (batadv_dat_drop_broadcast_packet(bat_priv, forw_packet)) {
dropped = true;- }
Why is this a failure? Isn't DAT supposed to drop things to avoid a failure case (that is duplicate broadcast packets on the client side)?
@@ -699,7 +710,7 @@ batadv_purge_outstanding_packets(struct batadv_priv *bat_priv,
if (pending) { hlist_del(&forw_packet->list);
batadv_forw_packet_free(forw_packet);
batadv_forw_packet_free(forw_packet, true);@@ -726,7 +737,7 @@ batadv_purge_outstanding_packets(struct batadv_priv *bat_priv,
if (pending) { hlist_del(&forw_packet->list);
batadv_forw_packet_free(forw_packet);
batadv_forw_packet_free(forw_packet, true);
These two above, again, why signaling a failure if it is a legitimate shutdown process?
Regards, Linus
On Samstag, 29. Oktober 2016 04:32:40 CEST Linus Lüssing wrote:
On Sun, Jul 17, 2016 at 09:04:00PM +0200, Sven Eckelmann wrote:
kfree_skb assumes that an skb is dropped after an failure and notes that. consume_skb should be used in non-failure situations. Such information is important for dropmonitor netlink which tells how many packets were
dropped
and where this drop happened.
Just a few, curious questions regarding why a kfree_skb() was chosen instead of a consume_skb() in a few places.
Especially so that I hopefully know which one best to use when rebasing the "batman-adv: fix race conditions on interface removal" patch :-).
Signed-off-by: Sven Eckelmann sven@narfation.org
net/batman-adv/bat_iv_ogm.c | 13 ++++++++----- net/batman-adv/fragmentation.c | 20 ++++++++++++++------ net/batman-adv/network-coding.c | 24 +++++++++++++++--------- net/batman-adv/send.c | 27 +++++++++++++++++++-------- net/batman-adv/send.h | 3 ++- net/batman-adv/soft-interface.c | 2 +- 6 files changed, 59 insertions(+), 30 deletions(-)
diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c index a40cdf2..baf3d72 100644 --- a/net/batman-adv/bat_iv_ogm.c +++ b/net/batman-adv/bat_iv_ogm.c @@ -1786,8 +1787,10 @@ static void
batadv_iv_send_outstanding_bat_ogm_packet(struct work_struct *work)
hlist_del(&forw_packet->list); spin_unlock_bh(&bat_priv->forw_bat_list_lock);
- if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING)
- if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING) {
dropped = true;- }
Is this reallly a failure case?
Hm, I would say it is not an extreme form of failure. But it is not a success either. So I've decided to use kfree_skb. The documentation is not really clear about it (or I missed the correct documentation). So this is my interpretation of it (which might be wrong).
diff --git a/net/batman-adv/fragmentation.c b/net/batman-adv/
fragmentation.c
index 0934730..461b77d 100644 --- a/net/batman-adv/fragmentation.c +++ b/net/batman-adv/fragmentation.c @@ -42,17 +42,23 @@ @@ -73,7 +79,7 @@ void batadv_frag_purge_orig(struct batadv_orig_node
*orig_node,
spin_lock_bh(&chain->lock); if (!check_cb || check_cb(chain)) {
batadv_frag_clear_chain(&chain->head);
batadv_frag_clear_chain(&chain->head, true); chain->size = 0;Hm, have you chosen kfree_skb() over consume_skb() because it cannot easily be determined whether this call was from a failure case or not?
My interpretation was that batadv_frag_purge_orig means that the fragments weren't successfully assembled. So it is some kind of soft failure.
diff --git a/net/batman-adv/network-coding.c b/net/batman-adv/network-
coding.c
index 293ef4f..e924256 100644 --- a/net/batman-adv/network-coding.c +++ b/net/batman-adv/network-coding.c @@ -611,7 +617,7 @@ static bool batadv_nc_sniffed_purge(struct batadv_priv
*bat_priv,
/* purge nc packet */ list_del(&nc_packet->list);
- batadv_nc_packet_free(nc_packet);
batadv_nc_packet_free(nc_packet, true);
res = true;
I could imagine, that with promiscious sniffing for coded packets, outdated, coded packets happen frequently and is not necessarilly a failure per se but to be expected.
On the other hand, missing a coding opportunity could have happened due to some failure elsewhere (a broken wifi driver, a noisy environment, ...).
In such an ambiguous case, should kfree_skb() be prefered over consume_skb()?
diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c index 8d4e1f5..4f44ee2 100644 --- a/net/batman-adv/send.c +++ b/net/batman-adv/send.c @@ -610,6 +616,7 @@ static void
batadv_send_outstanding_bcast_packet(struct work_struct *work)
struct sk_buff *skb1; struct net_device *soft_iface; struct batadv_priv *bat_priv;
bool dropped = false;
delayed_work = to_delayed_work(work); forw_packet = container_of(delayed_work, struct batadv_forw_packet,
@@ -621,11 +628,15 @@ static void
batadv_send_outstanding_bcast_packet(struct work_struct *work)
hlist_del(&forw_packet->list); spin_unlock_bh(&bat_priv->forw_bcast_list_lock);
- if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING)
- if (atomic_read(&bat_priv->mesh_state) == BATADV_MESH_DEACTIVATING) {
dropped = true;- }
Same as above, why is this considered a failure case?
Because it wasn't successful at fulfilling its task.
- if (batadv_dat_drop_broadcast_packet(bat_priv, forw_packet))
- if (batadv_dat_drop_broadcast_packet(bat_priv, forw_packet)) {
dropped = true;- }
Why is this a failure? Isn't DAT supposed to drop things to avoid a failure case (that is duplicate broadcast packets on the client side)?
Hm, good question. I think my idea behind it was that the original packet wasn't submitted.
@@ -699,7 +710,7 @@ batadv_purge_outstanding_packets(struct batadv_priv
*bat_priv,
if (pending) { hlist_del(&forw_packet->list);
batadv_forw_packet_free(forw_packet);
batadv_forw_packet_free(forw_packet, true);@@ -726,7 +737,7 @@ batadv_purge_outstanding_packets(struct batadv_priv
*bat_priv,
if (pending) { hlist_del(&forw_packet->list);
batadv_forw_packet_free(forw_packet);
batadv_forw_packet_free(forw_packet, true);These two above, again, why signaling a failure if it is a legitimate shutdown process?
Because the packet was not successfully forwarded.
Kind regards, Sven
b.a.t.m.a.n@lists.open-mesh.org
-
Linus Lüssing -
Simon Wunderlich -
Sven Eckelmann