Re: [B.A.T.M.A.N.] [PATCH 3/7] batman-adv: split out router from orig_node

[Marek Lindner <mareklindner@neomailbox.ch>]

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

On Wednesday 09 October 2013 15:05:34 Simon Wunderlich wrote:
> diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
> index 2cdcb8b..a7aa7b9 100644
> --- a/bat_iv_ogm.c
> +++ b/bat_iv_ogm.c
> @@ -918,6 +918,7 @@ static void batadv_iv_ogm_schedule(struct
> batadv_hard_iface *hard_iface) static void
>  batadv_iv_ogm_orig_update(struct batadv_priv *bat_priv,
>  			  struct batadv_orig_node *orig_node,
> +			  struct batadv_orig_node_ifinfo *orig_node_ifinfo,
>  			  const struct ethhdr *ethhdr,
>  			  const struct batadv_ogm_packet *batadv_ogm_packet,
>  			  struct batadv_hard_iface *if_incoming,

Kernel doc ?


> +
> +/**
> + * batadv_iv_ogm_process_per_outif - process a batman iv OGM for an
> outgoing if + * @ethhdr: the original ethernet header of the sender
> + * @orig_node: the orig node for the originator of this packet
> + * @batadv_ogm_packet: pointer to the ogm packet
> + * @tt_buff: pointer to the tt buffer
> + * @if_incoming: the interface where this packet was receved
> + * @if_outgoing: the interface for which the packet should be considered
> + */
> +static void
> +batadv_iv_ogm_process_per_outif(const struct ethhdr *ethhdr,
> +				struct batadv_orig_node *orig_node,
> +				struct batadv_ogm_packet *batadv_ogm_packet,
> +				const unsigned char *tt_buff,
> +				struct batadv_hard_iface *if_incoming,
> +				struct batadv_hard_iface *if_outgoing)
>  {
>  	struct batadv_priv *bat_priv = netdev_priv(if_incoming->soft_iface);
> -	struct batadv_hard_iface *hard_iface;
> -	struct batadv_orig_node *orig_neigh_node, *orig_node, *orig_node_tmp;
>  	struct batadv_neigh_node *router = NULL, *router_router = NULL;
> +	struct batadv_orig_node *orig_neigh_node, *orig_node_tmp;
> +	struct batadv_orig_node_ifinfo *orig_node_ifinfo;
>  	struct batadv_neigh_node *orig_neigh_router = NULL;
>  	struct batadv_neigh_node_ifinfo *router_ifinfo = NULL;
> -	int has_directlink_flag;
> -	int is_my_addr = 0, is_my_orig = 0, is_my_oldorig = 0;
> -	int is_bidirect;
> -	bool is_single_hop_neigh = false;
> -	bool is_from_best_next_hop = false;
> -	int sameseq, similar_ttl;
> +	struct batadv_ogm_packet ogm_packet_backup;
> +	uint8_t *prev_sender, last_ttl, packet_ttl;
> +	uint32_t last_seqno, packet_seqno;
>  	enum batadv_dup_status dup_status;
> +	bool is_from_best_next_hop = false;
> +	bool is_single_hop_neigh = false;
> +	int is_bidirect;
> +
> +	/* some functions change tq value and/or flags. backup the ogm packet
> +	 * and restore it at the end to allow other interfaces to access the
> +	 * original data.
> +	 */
> +
> +	memcpy(&ogm_packet_backup, batadv_ogm_packet,
> +	       sizeof(ogm_packet_backup));
> +
> +	dup_status = batadv_iv_ogm_update_seqnos(ethhdr, batadv_ogm_packet,
> +						 if_incoming, if_outgoing);
> +	if (batadv_compare_eth(ethhdr->h_source, batadv_ogm_packet->orig))
> +		is_single_hop_neigh = true;
> +
> +	if (dup_status == BATADV_PROTECTED) {
> +		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +			   "Drop packet: packet within seqno protection time (sender: 
%pM)\n",
> +			   ethhdr->h_source);
> +		goto out;
> +	}
> +
> +	if (batadv_ogm_packet->tq == 0) {
> +		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +			   "Drop packet: originator packet with tq equal 0\n");
> +		goto out;
> +	}
> +
> +	rcu_read_lock();
> +	router = batadv_orig_node_get_router(orig_node, if_outgoing);
> +	if (router) {
> +		orig_node_tmp = orig_node_tmp;
> +		router_router = batadv_orig_node_get_router(router->orig_node,
> +							    if_outgoing);
> +		router_ifinfo = batadv_neigh_node_get_ifinfo(router,
> +							     if_outgoing);
> +	}
> +
> +       if ((router_ifinfo && router_ifinfo->bat_iv.tq_avg != 0) &&
> +           (batadv_compare_eth(router->addr, ethhdr->h_source)))
> +               is_from_best_next_hop = true;
> +       rcu_read_unlock();

orig_node_tmp = orig_node_tmp ??

What are we protecting against with the rcu_read_lock ? router_ifinfo ? How 
about using refcounting instead ? Every function calling 
batadv_neigh_node_get_ifinfo() would have to have rcu_read_lock() because an 
unprotected struct neigh_ifinfo is returned.


> +	last_seqno = orig_node_ifinfo->last_real_seqno;
> +	last_ttl = orig_node_ifinfo->last_ttl;
> +	packet_seqno = ntohl(batadv_ogm_packet->seqno);
> +	packet_ttl = batadv_ogm_packet->header.ttl;
> +	if (is_bidirect && ((dup_status == BATADV_NO_DUP) ||
> +			    ((last_seqno == packet_seqno) &&
> +			     (last_ttl - 3) <= packet_ttl)))
> +		batadv_iv_ogm_orig_update(bat_priv, orig_node,
> +					  orig_node_ifinfo, ethhdr,
> +					  batadv_ogm_packet, if_incoming,
> +					  if_outgoing, tt_buff, dup_status);

I don't think this if statement will pass. The complexity has been there 
before but there was an attempt to make it readable ...


> -/* increases the refcounter of a found router */
> +/**
> + * batadv_orig_node_get_router - router to the originator depending on
> iface + * @orig_node: the orig node for the router
> + * @if_received: the interface where the packet to be transmitted was
> received + *
> + * Returns the neighbor which should be router for this orig_node/iface.
> + */
>  struct batadv_neigh_node *
> -batadv_orig_node_get_router(struct batadv_orig_node *orig_node)
> +batadv_orig_node_get_router(struct batadv_orig_node *orig_node,
> +			    const struct batadv_hard_iface *if_received)
>  {

Would you mind cleaning up the API while you are at it. We have:
 * batadv_orig_node_vlan_get()
 * batadv_orig_node_vlan_new()
 * batadv_orig_node_new()
 * batadv_iv_ogm_process()
 * etc

Therefore, I suggest renaming this function to batadv_orig_node_router_get().
The kernel doc could mention that the refcount is increased by this function 
call.


>  /**
> + * batadv_orig_node_get_ifinfo - gets the ifinfo from an orig_node
> + * @orig_node: the orig node to be queried
> + * @if_received: the interface for which the ifinfo should be acquired
> + *
> + * Returns the requested orig_node_ifinfo or NULL if not found.
> + *
> + * Note: this function must be called under rcu lock
> + */
> +struct batadv_orig_node_ifinfo *
> +batadv_orig_node_get_ifinfo(struct batadv_orig_node *orig_node,
> +			    struct batadv_hard_iface *if_received)
> +{
> +	struct batadv_orig_node_ifinfo *tmp, *orig_ifinfo = NULL;
> +	unsigned long reset_time;
> +
> +	hlist_for_each_entry_rcu(tmp, &orig_node->ifinfo_list,
> +				 list) {
> +		if (tmp->if_outgoing != if_received)
> +			continue;
> +		orig_ifinfo = tmp;
> +		break;
> +	}
> +
> +	spin_lock_bh(&orig_node->neigh_list_lock);
> +	if (!orig_ifinfo) {
> +		orig_ifinfo = kzalloc(sizeof(*orig_ifinfo), GFP_ATOMIC);
> +		if (!orig_ifinfo)
> +			goto out;
> +
> +		if (if_received &&
> +		    !atomic_inc_not_zero(&if_received->refcount)) {
> +			kfree(orig_ifinfo);
> +			orig_ifinfo = NULL;
> +			goto out;
> +		}
> +		reset_time = jiffies - 1;
> +		reset_time -= msecs_to_jiffies(BATADV_RESET_PROTECTION_MS);
> +		orig_ifinfo->batman_seqno_reset = reset_time;
> +		orig_ifinfo->if_outgoing = if_received;
> +		INIT_HLIST_NODE(&orig_ifinfo->list);
> +		hlist_add_head_rcu(&orig_ifinfo->list,
> +				   &orig_node->ifinfo_list);
> +	}
> +out:
> +	spin_unlock_bh(&orig_node->neigh_list_lock);
> +	return orig_ifinfo;
> +}

Same function name change is desirable.

Moreover, why do we require the calling functions to hold the rcu_lock ? I see 
no technical reason but it can easily be forgotten like in 
batadv_iv_ogm_update_seqnos(). Also, why is the spinlock acquired for no 
reason ? It could be moved down just around the hlist_add_head_rcu() call.


> +static void batadv_orig_node_ifinfo_free_rcu(struct rcu_head *rcu)
> +{
> +	struct batadv_orig_node_ifinfo *orig_node_ifinfo;
> +
> +	orig_node_ifinfo = container_of(rcu, struct batadv_orig_node_ifinfo,
> +					rcu);
> +
> +	/* We are in an rcu callback here, therefore we cannot use
> +	 * batadv_hardif_free_ref() and its call_rcu():
> +	 * An rcu_barrier() wouldn't wait for that to finish
> +	 */
> +	if (orig_node_ifinfo->if_outgoing)
> +		batadv_hardif_free_ref_now(orig_node_ifinfo->if_outgoing);
> +
> +	kfree(orig_node_ifinfo);
> +}

Kernel doc ?

Cheers,
Marek

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