public inbox for b.a.t.m.a.n@lists.open-mesh.org
 help / color / mirror / Atom feed
From: "Linus Lüssing" <linus.luessing@web.de>
To: The list for a Better Approach To Mobile Ad-hoc Networking
	<b.a.t.m.a.n@lists.open-mesh.org>
Subject: Re: [B.A.T.M.A.N.] [PATCHv2 3/8] batman-adv: split out router from orig_node
Date: Tue, 5 Nov 2013 01:03:43 +0100	[thread overview]
Message-ID: <20131105000342.GU15496@Linus-Debian> (raw)
In-Reply-To: <1383141713-15820-4-git-send-email-sw@simonwunderlich.de>

Hi Simon,

I could only find one or two potential bugs, so I had to come up
with a few, additional style suggestions ;).


On Wed, Oct 30, 2013 at 03:01:48PM +0100, Simon Wunderlich wrote:
> From: Simon Wunderlich <simon@open-mesh.com>
> 
> For the network wide multi interface optimization there are different
> routers for each outgoing interface (outgoing from the OGM perspective,
> incoming for payload traffic). To reflect this, change the router and
> associated data to a list of routers.
> 
> While at it, rename batadv_orig_node_get_router() to
> batadv_orig_router_get() to follow the new naming scheme.
> 
> Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> ---
> Changes to PATCH:
>  * change orig_ifinfo locking from implicit rcu style to
>    refcount locking
>  * pass skb instead of buffers to the OGM processing functions
>  * remove unused tt_buff pointer from some bat_iv functions
>  * rename batadv_orig_node_ifinfo* to batadv_orig_ifinfo* - name
>    is still long enough
>  * rename batadv_orig_node_get_router to batadv_orig_router_get
>  * rename batadv_orig_node_get_ifinfo to batadv_orig_ifinfo_get
> 
> Changes to RFCv2:
>  * various style changes
>  * remove unneccesary batadv_orig_node_set_router prototype
> 
> Changes to RFC:
>  * rebase on current master
>  * remove useless goto
>  * split out batman_seqno_reset as well to avoid false seqno window
>    protections
> ---
>  bat_iv_ogm.c            |  456 ++++++++++++++++++++++++++++-------------------
>  distributed-arp-table.c |    3 +-
>  gateway_client.c        |   11 +-
>  icmp_socket.c           |    3 +-
>  network-coding.c        |    9 +-
>  originator.c            |  225 ++++++++++++++++++++++-
>  originator.h            |   12 +-
>  routing.c               |   35 +++-
>  routing.h               |    1 +
>  translation-table.c     |    3 +-
>  types.h                 |   31 +++-
>  11 files changed, 570 insertions(+), 219 deletions(-)
> 
> diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
> index 66ce1a7..3ba2402 100644
> --- a/bat_iv_ogm.c
> +++ b/bat_iv_ogm.c
> @@ -908,21 +908,21 @@ static void batadv_iv_ogm_schedule(struct batadv_hard_iface *hard_iface)
>   *  originator
>   * @bat_priv: the bat priv with all the soft interface information
>   * @orig_node: the orig node who originally emitted the ogm packet
> + * @orig_ifinfo: ifinfo for the outgoing interface of the orig_node
>   * @ethhdr: Ethernet header of the OGM
>   * @batadv_ogm_packet: the ogm packet
>   * @if_incoming: interface where the packet was received
>   * @if_outgoing: interface for which the retransmission should be considered
> - * @tt_buff: pointer to the tt buffer
>   * @dup_status: the duplicate status of this ogm packet.
>   */
>  static void
>  batadv_iv_ogm_orig_update(struct batadv_priv *bat_priv,
>  			  struct batadv_orig_node *orig_node,
> +			  struct batadv_orig_ifinfo *orig_ifinfo,
>  			  const struct ethhdr *ethhdr,
>  			  const struct batadv_ogm_packet *batadv_ogm_packet,
>  			  struct batadv_hard_iface *if_incoming,
>  			  struct batadv_hard_iface *if_outgoing,
> -			  const unsigned char *tt_buff,
>  			  enum batadv_dup_status dup_status)
>  {
>  	struct batadv_neigh_ifinfo *neigh_ifinfo = NULL;
> @@ -1004,22 +1004,30 @@ batadv_iv_ogm_orig_update(struct batadv_priv *bat_priv,
>  	spin_unlock_bh(&neigh_node->ifinfo_lock);
>  
>  	if (dup_status == BATADV_NO_DUP) {
> -		orig_node->last_ttl = batadv_ogm_packet->header.ttl;
> +		orig_ifinfo->last_ttl = batadv_ogm_packet->header.ttl;
>  		neigh_ifinfo->last_ttl = batadv_ogm_packet->header.ttl;
>  	}
>  
>  	/* if this neighbor already is our next hop there is nothing
>  	 * to change
>  	 */
> -	router = batadv_orig_node_get_router(orig_node);
> +	router = batadv_orig_router_get(orig_node, if_outgoing);
>  	if (router == neigh_node)
>  		goto out;
>  
> -	router_ifinfo = batadv_neigh_ifinfo_get(router, if_outgoing);
> -	/* if this neighbor does not offer a better TQ we won't consider it */
> -	if (router_ifinfo &&
> -	    (router_ifinfo->bat_iv.tq_avg > neigh_ifinfo->bat_iv.tq_avg))
> -		goto out;

Ok, if *router can actually be NULL, then the following
"if (router)" check, the diff block to follow, should probably have
been introduced in the previous patch already?

> +	if (router) {
> +		router_ifinfo = batadv_neigh_ifinfo_get(router, if_outgoing);

This comment might be better suited three lines later?

> +		/* if this neighbor does not offer a better TQ we won't
> +		 * consider it
> +		 */
> +		if (!router_ifinfo)
> +			goto out;
> +
> +		if (router_ifinfo->bat_iv.tq_avg > neigh_ifinfo->bat_iv.tq_avg)
> +			goto out;

This else-clause seems redundant:

> +	} else {
> +		router_ifinfo = NULL;
> +	}
>  
>  	/* if the TQ is the same and the link not more symmetric we
>  	 * won't consider it either
> @@ -1042,8 +1050,7 @@ batadv_iv_ogm_orig_update(struct batadv_priv *bat_priv,
>  			goto out;
>  	}
>  

Hm, if the TODO is solved within the same patchset, then maybe
it's not necessary to add the comment in the first place, in the previous
patch?

> -	/* TODO: pass if_outgoing later */
> -	batadv_update_route(bat_priv, orig_node, neigh_node);
> +	batadv_update_route(bat_priv, orig_node, if_outgoing, neigh_node);
>  	goto out;
>  
>  unlock:
> @@ -1204,6 +1211,7 @@ batadv_iv_ogm_update_seqnos(const struct ethhdr *ethhdr,
>  {
>  	struct batadv_priv *bat_priv = netdev_priv(if_incoming->soft_iface);
>  	struct batadv_orig_node *orig_node;
> +	struct batadv_orig_ifinfo *orig_ifinfo = NULL;
>  	struct batadv_neigh_node *neigh_node;
>  	struct batadv_neigh_ifinfo *neigh_ifinfo;
>  	int is_dup;
> @@ -1220,13 +1228,19 @@ batadv_iv_ogm_update_seqnos(const struct ethhdr *ethhdr,
>  	if (!orig_node)
>  		return BATADV_NO_DUP;
>  
> +	orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);
> +	if (WARN_ON(!orig_ifinfo)) {
> +		batadv_orig_node_free_ref(orig_node);
> +		return 0;
> +	}
> +
>  	spin_lock_bh(&orig_node->bat_iv.ogm_cnt_lock);
> -	seq_diff = seqno - orig_node->last_real_seqno;
> +	seq_diff = seqno - orig_ifinfo->last_real_seqno;
>  
>  	/* signalize caller that the packet is to be dropped. */
>  	if (!hlist_empty(&orig_node->neigh_list) &&
>  	    batadv_window_protected(bat_priv, seq_diff,
> -				    &orig_node->batman_seqno_reset)) {
> +				    &orig_ifinfo->batman_seqno_reset)) {
>  		ret = BATADV_PROTECTED;
>  		goto out;
>  	}
> @@ -1240,7 +1254,7 @@ batadv_iv_ogm_update_seqnos(const struct ethhdr *ethhdr,
>  
>  		neigh_addr = neigh_node->addr;
>  		is_dup = batadv_test_bit(neigh_ifinfo->bat_iv.real_bits,
> -					 orig_node->last_real_seqno,
> +					 orig_ifinfo->last_real_seqno,
>  					 seqno);
>  
>  		if (batadv_compare_eth(neigh_addr, ethhdr->h_source) &&
> @@ -1268,37 +1282,225 @@ batadv_iv_ogm_update_seqnos(const struct ethhdr *ethhdr,
>  
>  	if (need_update) {
>  		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -			   "updating last_seqno: old %u, new %u\n",
> -			   orig_node->last_real_seqno, seqno);
> -		orig_node->last_real_seqno = seqno;
> +			   "%s updating last_seqno: old %u, new %u\n",
> +			   if_outgoing ? if_outgoing->net_dev->name : "DEFAULT",
> +			   orig_ifinfo->last_real_seqno, seqno);
> +		orig_ifinfo->last_real_seqno = seqno;
>  	}
>  
>  out:
>  	spin_unlock_bh(&orig_node->bat_iv.ogm_cnt_lock);
>  	batadv_orig_node_free_ref(orig_node);
> +	if (orig_ifinfo)
> +		batadv_orig_ifinfo_free_ref(orig_ifinfo);
>  	return ret;
>  }
>  
> -static void batadv_iv_ogm_process(const struct ethhdr *ethhdr,
> -				  struct batadv_ogm_packet *batadv_ogm_packet,
> -				  const unsigned char *tt_buff,
> -				  struct batadv_hard_iface *if_incoming)
> +
> +/**
> + * batadv_iv_ogm_process_per_outif - process a batman iv OGM for an outgoing if
> + * @skb: the skb containing the OGM
> + * @orig_node: the (cached) orig node for the originator of this OGM

typo (receved vs. received):

> + * @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 sk_buff *skb, int ogm_offset,
> +				struct batadv_orig_node *orig_node,
> +				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_ifinfo *orig_ifinfo;
>  	struct batadv_neigh_node *orig_neigh_router = NULL;
>  	struct batadv_neigh_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;
>  	enum batadv_dup_status dup_status;
> -	uint32_t if_incoming_seqno;
> +	bool is_from_best_next_hop = false;
> +	bool is_single_hop_neigh = false;
> +	bool sameseq, similar_ttl;
> +	struct sk_buff *skb_priv;
> +	struct ethhdr *ethhdr;
>  	uint8_t *prev_sender;
> +	int is_bidirect;
> +
> +	/* create a private copy of the skb, as some functions change tq value
> +	 * and/or flags.
> +	 */
> +	skb_priv = skb_copy(skb, GFP_ATOMIC);
> +	if (!skb_priv)
> +		return;
> +
> +	ethhdr = eth_hdr(skb_priv);
> +	ogm_packet = (struct batadv_ogm_packet *)(skb_priv->data + ogm_offset);
> +
> +	dup_status = batadv_iv_ogm_update_seqnos(ethhdr, ogm_packet,
> +						 if_incoming, if_outgoing);

To avoid redundant code execution, 
move this if statement to the calling function maybe?

> +	if (batadv_compare_eth(ethhdr->h_source, 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;
> +	}
> +

Move this if statement to the calling function maybe?

> +	if (ogm_packet->tq == 0) {
> +		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +			   "Drop packet: originator packet with tq equal 0\n");
> +		goto out;
> +	}
> +
> +	router = batadv_orig_router_get(orig_node, if_outgoing);
> +	if (router) {
> +		orig_node_tmp = router->orig_node;
> +		router_router = batadv_orig_router_get(router->orig_node,
> +						       if_outgoing);
> +		router_ifinfo = batadv_neigh_ifinfo_get(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;
> +
> +	prev_sender = ogm_packet->prev_sender;
> +	/* avoid temporary routing loops */
> +	if (router && router_router &&
> +	    (batadv_compare_eth(router->addr, prev_sender)) &&
> +	    !(batadv_compare_eth(ogm_packet->orig, prev_sender)) &&
> +	    (batadv_compare_eth(router->addr, router_router->addr))) {
> +		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +			   "Drop packet: ignoring all rebroadcast packets that may make me loop (sender: %pM)\n",
> +			   ethhdr->h_source);
> +		goto out;
> +	}
> +

Move this to ...

> +	/* if sender is a direct neighbor the sender mac equals
> +	 * originator mac
> +	 */
> +	if (is_single_hop_neigh)
> +		orig_neigh_node = orig_node;
> +	else
> +		orig_neigh_node = batadv_iv_ogm_orig_get(bat_priv,
> +							 ethhdr->h_source);
> +
> +	if (!orig_neigh_node)
> +		goto out;
> +
> +	/* Update nc_nodes of the originator */
> +	batadv_nc_update_nc_node(bat_priv, orig_node, orig_neigh_node,
> +				 ogm_packet, is_single_hop_neigh);

... this to the calling function, maybe?

> +
> +	orig_neigh_router = batadv_orig_router_get(orig_neigh_node,
> +						   if_outgoing);
> +
> +	/* drop packet if sender is not a direct neighbor and if we
> +	 * don't route towards it
> +	 */
> +	if (!is_single_hop_neigh && (!orig_neigh_router)) {
> +		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +			   "Drop packet: OGM via unknown neighbor!\n");
> +		goto out_neigh;
> +	}
> +
> +	is_bidirect = batadv_iv_ogm_calc_tq(orig_node, orig_neigh_node,
> +					    ogm_packet, if_incoming,
> +					    if_outgoing);
> +
> +	/* update ranking if it is not a duplicate or has the same
> +	 * seqno and similar ttl as the non-duplicate
> +	 */
> +	orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);
> +	if (!orig_ifinfo)
> +		goto out_neigh;
> +
> +	sameseq = orig_ifinfo->last_real_seqno == ntohl(ogm_packet->seqno);
> +	similar_ttl = (orig_ifinfo->last_ttl - 3) <= ogm_packet->header.ttl;
> +	if (is_bidirect && ((dup_status == BATADV_NO_DUP) ||
> +			    (sameseq && similar_ttl))) {
> +		batadv_iv_ogm_orig_update(bat_priv, orig_node,
> +					  orig_ifinfo, ethhdr,
> +					  ogm_packet, if_incoming,
> +					  if_outgoing, dup_status);
> +	}
> +	batadv_orig_ifinfo_free_ref(orig_ifinfo);
> +
> +	/* is single hop (direct) neighbor */
> +	if (is_single_hop_neigh) {
> +		if ((ogm_packet->header.ttl <= 2) &&
> +		    (if_incoming != if_outgoing)) {
> +			batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +				   "Drop packet: OGM from secondary interface and wrong outgoing interface\n");
> +			goto out_neigh;
> +		}

The check above, is this just an optimization independant of
this patchset or is it somehow needed for the multi interface
patchset? Maybe a comment could be added?

> +		/* mark direct link on incoming interface */
> +		batadv_iv_ogm_forward(orig_node, ethhdr, ogm_packet,
> +				      is_single_hop_neigh,
> +				      is_from_best_next_hop, if_incoming);

Hmm, okay, now we are forwarding OGMs on an interface multiple
times. Though you are fixing this in "batman-adv: consider
outgoing interface in OGM sending" I guess, this is still a
regression here, isn't it?

> +
> +		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +			   "Forwarding packet: rebroadcast neighbor packet with direct link flag\n");
> +		goto out_neigh;
> +	}
> +
> +	/* multihop originator */
> +	if (!is_bidirect) {
> +		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +			   "Drop packet: not received via bidirectional link\n");
> +		goto out_neigh;
> +	}
> +
> +	if (dup_status == BATADV_NEIGH_DUP) {
> +		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +			   "Drop packet: duplicate packet received\n");
> +		goto out_neigh;
> +	}
> +
> +	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +		   "Forwarding packet: rebroadcast originator packet\n");
> +	batadv_iv_ogm_forward(orig_node, ethhdr, ogm_packet,
> +			      is_single_hop_neigh, is_from_best_next_hop,
> +			      if_incoming);
> +
> +out_neigh:
> +	if ((orig_neigh_node) && (!is_single_hop_neigh))
> +		batadv_orig_node_free_ref(orig_neigh_node);
> +out:
> +	if (router)
> +		batadv_neigh_node_free_ref(router);
> +	if (router_router)
> +		batadv_neigh_node_free_ref(router_router);
> +	if (orig_neigh_router)
> +		batadv_neigh_node_free_ref(orig_neigh_router);
> +
> +	kfree_skb(skb_priv);
> +}

Although this is mostly just code moved around, maybe it would be
a good idea to split it into several functions now? Would make it
easier to debug and find potential regressions introduced by this
patch later, wouldn't it?

Maybe batadv_iv_ogm_process() would benefit from some more splits,
too?

> +
> +/**
> + * batadv_iv_ogm_process - process an incoming batman iv OGM
> + * @skb: the skb containing the OGM
> + * @ogm_offset: offset to the OGM which should be processed (for aggregates)
> + * @if_incoming: the interface where this packet was receved
> + */
> +static void batadv_iv_ogm_process(const struct sk_buff *skb, int ogm_offset,
> +				  struct batadv_hard_iface *if_incoming)
> +{
> +	struct batadv_priv *bat_priv = netdev_priv(if_incoming->soft_iface);
> +	struct batadv_orig_node *orig_neigh_node, *orig_node;
> +	struct batadv_hard_iface *hard_iface;
> +	struct batadv_ogm_packet *ogm_packet;
> +	uint32_t if_incoming_seqno;
> +	int has_directlink_flag;
> +	struct ethhdr *ethhdr;

maybe change these three to bool while we are at it? and
has_directlink_flag, too?

> +	int is_my_oldorig = 0;
> +	int is_my_addr = 0;
> +	int is_my_orig = 0;
> +
> +	ogm_packet = (struct batadv_ogm_packet *)(skb->data + ogm_offset);
> +	ethhdr = eth_hdr(skb);
>  
>  	/* Silently drop when the batman packet is actually not a
>  	 * correct packet.
> @@ -1312,28 +1514,24 @@ static void batadv_iv_ogm_process(const struct ethhdr *ethhdr,
>  	 * packet in an aggregation.  Here we expect that the padding
>  	 * is always zero (or not 0x01)
>  	 */
> -	if (batadv_ogm_packet->header.packet_type != BATADV_IV_OGM)
> +	if (ogm_packet->header.packet_type != BATADV_IV_OGM)
>  		return;
>  
>  	/* could be changed by schedule_own_packet() */
>  	if_incoming_seqno = atomic_read(&if_incoming->bat_iv.ogm_seqno);
>  
> -	if (batadv_ogm_packet->flags & BATADV_DIRECTLINK)
> +	if (ogm_packet->flags & BATADV_DIRECTLINK)
>  		has_directlink_flag = 1;
>  	else
>  		has_directlink_flag = 0;
>  
> -	if (batadv_compare_eth(ethhdr->h_source, batadv_ogm_packet->orig))
> -		is_single_hop_neigh = true;
> -
>  	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
>  		   "Received BATMAN packet via NB: %pM, IF: %s [%pM] (from OG: %pM, via prev OG: %pM, seqno %u, tq %d, TTL %d, V %d, IDF %d)\n",
>  		   ethhdr->h_source, if_incoming->net_dev->name,
> -		   if_incoming->net_dev->dev_addr, batadv_ogm_packet->orig,
> -		   batadv_ogm_packet->prev_sender,
> -		   ntohl(batadv_ogm_packet->seqno), batadv_ogm_packet->tq,
> -		   batadv_ogm_packet->header.ttl,
> -		   batadv_ogm_packet->header.version, has_directlink_flag);
> +		   if_incoming->net_dev->dev_addr, ogm_packet->orig,
> +		   ogm_packet->prev_sender, ntohl(ogm_packet->seqno),
> +		   ogm_packet->tq, ogm_packet->header.ttl,
> +		   ogm_packet->header.version, has_directlink_flag);
>  
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(hard_iface, &batadv_hardif_list, list) {
> @@ -1347,11 +1545,11 @@ static void batadv_iv_ogm_process(const struct ethhdr *ethhdr,
>  				       hard_iface->net_dev->dev_addr))
>  			is_my_addr = 1;
>  
> -		if (batadv_compare_eth(batadv_ogm_packet->orig,
> +		if (batadv_compare_eth(ogm_packet->orig,
>  				       hard_iface->net_dev->dev_addr))
>  			is_my_orig = 1;
>  
> -		if (batadv_compare_eth(batadv_ogm_packet->prev_sender,
> +		if (batadv_compare_eth(ogm_packet->prev_sender,
>  				       hard_iface->net_dev->dev_addr))
>  			is_my_oldorig = 1;
>  	}
> @@ -1382,14 +1580,14 @@ static void batadv_iv_ogm_process(const struct ethhdr *ethhdr,
>  		 */
>  		if (has_directlink_flag &&
>  		    batadv_compare_eth(if_incoming->net_dev->dev_addr,
> -				       batadv_ogm_packet->orig)) {
> +				       ogm_packet->orig)) {
>  			if_num = if_incoming->if_num;
>  			offset = if_num * BATADV_NUM_WORDS;
>  
>  			spin_lock_bh(&orig_neigh_node->bat_iv.ogm_cnt_lock);
>  			word = &(orig_neigh_node->bat_iv.bcast_own[offset]);
>  			bit_pos = if_incoming_seqno - 2;
> -			bit_pos -= ntohl(batadv_ogm_packet->seqno);
> +			bit_pos -= ntohl(ogm_packet->seqno);
>  			batadv_set_bit(word, bit_pos);
>  			weight = &orig_neigh_node->bat_iv.bcast_own_sum[if_num];
>  			*weight = bitmap_weight(word,
> @@ -1410,144 +1608,34 @@ static void batadv_iv_ogm_process(const struct ethhdr *ethhdr,
>  		return;
>  	}
>  
> -	if (batadv_ogm_packet->flags & BATADV_NOT_BEST_NEXT_HOP) {
> +	if (ogm_packet->flags & BATADV_NOT_BEST_NEXT_HOP) {
>  		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
>  			   "Drop packet: ignoring all packets not forwarded from the best next hop (sender: %pM)\n",
>  			   ethhdr->h_source);
>  		return;
>  	}
>  
> -	orig_node = batadv_iv_ogm_orig_get(bat_priv, batadv_ogm_packet->orig);
> +	orig_node = batadv_iv_ogm_orig_get(bat_priv, ogm_packet->orig);
>  	if (!orig_node)
>  		return;
>  
> -	dup_status = batadv_iv_ogm_update_seqnos(ethhdr, batadv_ogm_packet,
> -						 if_incoming,
> -						 BATADV_IF_DEFAULT);
> +	batadv_tvlv_ogm_receive(bat_priv, ogm_packet, orig_node);

Hmm, moving the tvlv_ogm processing upwards, in front of all the
duplicate/protected/etc. checks seems like a notable, functional change.
Though there don't seem to be any problems with the OGM TVLVs we
are currently having as far as I can tell, maybe this change could
be mentioned in the commit message though?

>  
> -	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;
> -	}
> -
> -	router = batadv_orig_node_get_router(orig_node);
> -	if (router) {
> -		orig_node_tmp = router->orig_node;
> -		router_router = batadv_orig_node_get_router(orig_node_tmp);
> -		router_ifinfo = batadv_neigh_ifinfo_get(router,
> -							BATADV_IF_DEFAULT);
> -	}
> -
> -	if ((router && router_ifinfo->bat_iv.tq_avg != 0) &&
> -	    (batadv_compare_eth(router->addr, ethhdr->h_source)))
> -		is_from_best_next_hop = true;
> -
> -	prev_sender = batadv_ogm_packet->prev_sender;
> -	/* avoid temporary routing loops */
> -	if (router && router_router &&
> -	    (batadv_compare_eth(router->addr, prev_sender)) &&
> -	    !(batadv_compare_eth(batadv_ogm_packet->orig, prev_sender)) &&
> -	    (batadv_compare_eth(router->addr, router_router->addr))) {
> -		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -			   "Drop packet: ignoring all rebroadcast packets that may make me loop (sender: %pM)\n",
> -			   ethhdr->h_source);
> -		goto out;
> -	}
> -
> -	batadv_tvlv_ogm_receive(bat_priv, batadv_ogm_packet, orig_node);
> -
> -	/* if sender is a direct neighbor the sender mac equals
> -	 * originator mac
> -	 */
> -	if (is_single_hop_neigh)
> -		orig_neigh_node = orig_node;
> -	else
> -		orig_neigh_node = batadv_iv_ogm_orig_get(bat_priv,
> -							 ethhdr->h_source);
> -
> -	if (!orig_neigh_node)
> -		goto out;
> +	batadv_iv_ogm_process_per_outif(skb, ogm_offset, orig_node,
> +					if_incoming, BATADV_IF_DEFAULT);
>  
> -	/* Update nc_nodes of the originator */
> -	batadv_nc_update_nc_node(bat_priv, orig_node, orig_neigh_node,
> -				 batadv_ogm_packet, is_single_hop_neigh);
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(hard_iface, &batadv_hardif_list, list) {
> +		if (hard_iface->if_status != BATADV_IF_ACTIVE)
> +			continue;
>  
> -	orig_neigh_router = batadv_orig_node_get_router(orig_neigh_node);
> +		if (hard_iface->soft_iface != bat_priv->soft_iface)
> +			continue;
>  
> -	/* drop packet if sender is not a direct neighbor and if we
> -	 * don't route towards it
> -	 */
> -	if (!is_single_hop_neigh && (!orig_neigh_router)) {
> -		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -			   "Drop packet: OGM via unknown neighbor!\n");
> -		goto out_neigh;
> +		batadv_iv_ogm_process_per_outif(skb, ogm_offset, orig_node,
> +						if_incoming, hard_iface);
>  	}
> -
> -	is_bidirect = batadv_iv_ogm_calc_tq(orig_node, orig_neigh_node,
> -					    batadv_ogm_packet, if_incoming,
> -					    BATADV_IF_DEFAULT);
> -
> -	/* update ranking if it is not a duplicate or has the same
> -	 * seqno and similar ttl as the non-duplicate
> -	 */
> -	sameseq = orig_node->last_real_seqno == ntohl(batadv_ogm_packet->seqno);
> -	similar_ttl = orig_node->last_ttl - 3 <= batadv_ogm_packet->header.ttl;
> -	if (is_bidirect && ((dup_status == BATADV_NO_DUP) ||
> -			    (sameseq && similar_ttl)))
> -		batadv_iv_ogm_orig_update(bat_priv, orig_node, ethhdr,
> -					  batadv_ogm_packet, if_incoming,
> -					  BATADV_IF_DEFAULT, tt_buff,
> -					  dup_status);
> -
> -	/* is single hop (direct) neighbor */
> -	if (is_single_hop_neigh) {
> -		/* mark direct link on incoming interface */
> -		batadv_iv_ogm_forward(orig_node, ethhdr, batadv_ogm_packet,
> -				      is_single_hop_neigh,
> -				      is_from_best_next_hop, if_incoming);
> -
> -		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -			   "Forwarding packet: rebroadcast neighbor packet with direct link flag\n");
> -		goto out_neigh;
> -	}
> -
> -	/* multihop originator */
> -	if (!is_bidirect) {
> -		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -			   "Drop packet: not received via bidirectional link\n");
> -		goto out_neigh;
> -	}
> -
> -	if (dup_status == BATADV_NEIGH_DUP) {
> -		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -			   "Drop packet: duplicate packet received\n");
> -		goto out_neigh;
> -	}
> -
> -	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> -		   "Forwarding packet: rebroadcast originator packet\n");
> -	batadv_iv_ogm_forward(orig_node, ethhdr, batadv_ogm_packet,
> -			      is_single_hop_neigh, is_from_best_next_hop,
> -			      if_incoming);
> -
> -out_neigh:
> -	if ((orig_neigh_node) && (!is_single_hop_neigh))
> -		batadv_orig_node_free_ref(orig_neigh_node);
> -out:
> -	if (router)
> -		batadv_neigh_node_free_ref(router);
> -	if (router_router)
> -		batadv_neigh_node_free_ref(router_router);
> -	if (orig_neigh_router)
> -		batadv_neigh_node_free_ref(orig_neigh_router);
> +	rcu_read_unlock();
>  
>  	batadv_orig_node_free_ref(orig_node);
>  }
> @@ -1556,11 +1644,9 @@ static int batadv_iv_ogm_receive(struct sk_buff *skb,
>  				 struct batadv_hard_iface *if_incoming)
>  {
>  	struct batadv_priv *bat_priv = netdev_priv(if_incoming->soft_iface);
> -	struct batadv_ogm_packet *batadv_ogm_packet;
> -	struct ethhdr *ethhdr;
> -	int buff_pos = 0, packet_len;
> -	unsigned char *tvlv_buff, *packet_buff;
> +	struct batadv_ogm_packet *ogm_packet;
>  	uint8_t *packet_pos;
> +	int ogm_offset;
>  	bool ret;
>  
>  	ret = batadv_check_management_packet(skb, if_incoming, BATADV_OGM_HLEN);
> @@ -1577,24 +1663,19 @@ static int batadv_iv_ogm_receive(struct sk_buff *skb,
>  	batadv_add_counter(bat_priv, BATADV_CNT_MGMT_RX_BYTES,
>  			   skb->len + ETH_HLEN);
>  
> -	packet_len = skb_headlen(skb);
> -	ethhdr = eth_hdr(skb);
> -	packet_buff = skb->data;
> -	batadv_ogm_packet = (struct batadv_ogm_packet *)packet_buff;
> +	ogm_offset = 0;
> +	ogm_packet = (struct batadv_ogm_packet *)skb->data;
>  
>  	/* unpack the aggregated packets and process them one by one */
> -	while (batadv_iv_ogm_aggr_packet(buff_pos, packet_len,
> -					 batadv_ogm_packet->tvlv_len)) {
> -		tvlv_buff = packet_buff + buff_pos + BATADV_OGM_HLEN;
> +	while (batadv_iv_ogm_aggr_packet(ogm_offset, skb_headlen(skb),
> +					 ogm_packet->tvlv_len)) {
> +		batadv_iv_ogm_process(skb, ogm_offset, if_incoming);
>  
> -		batadv_iv_ogm_process(ethhdr, batadv_ogm_packet,
> -				      tvlv_buff, if_incoming);
> +		ogm_offset += BATADV_OGM_HLEN;
> +		ogm_offset += ntohs(ogm_packet->tvlv_len);
>  
> -		buff_pos += BATADV_OGM_HLEN;
> -		buff_pos += ntohs(batadv_ogm_packet->tvlv_len);
> -
> -		packet_pos = packet_buff + buff_pos;
> -		batadv_ogm_packet = (struct batadv_ogm_packet *)packet_pos;
> +		packet_pos = skb->data + ogm_offset;
> +		ogm_packet = (struct batadv_ogm_packet *)packet_pos;
>  	}
>  
>  	kfree_skb(skb);
> @@ -1657,7 +1738,8 @@ static void batadv_iv_ogm_orig_print(struct batadv_priv *bat_priv,
>  
>  		rcu_read_lock();
>  		hlist_for_each_entry_rcu(orig_node, head, hash_entry) {
> -			neigh_node = batadv_orig_node_get_router(orig_node);
> +			neigh_node = batadv_orig_router_get(orig_node,
> +							    BATADV_IF_DEFAULT);
>  			if (!neigh_node)
>  				continue;
>  
> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
> index 6c8c393..ae0404e 100644
> --- a/distributed-arp-table.c
> +++ b/distributed-arp-table.c
> @@ -591,7 +591,8 @@ static bool batadv_dat_send_data(struct batadv_priv *bat_priv,
>  		if (cand[i].type == BATADV_DAT_CANDIDATE_NOT_FOUND)
>  			continue;
>  
> -		neigh_node = batadv_orig_node_get_router(cand[i].orig_node);
> +		neigh_node = batadv_orig_router_get(cand[i].orig_node,
> +						    BATADV_IF_DEFAULT);
>  		if (!neigh_node)
>  			goto free_orig;
>  
> diff --git a/gateway_client.c b/gateway_client.c
> index 71759d0..722a051 100644
> --- a/gateway_client.c
> +++ b/gateway_client.c
> @@ -131,7 +131,7 @@ batadv_gw_get_best_gw_node(struct batadv_priv *bat_priv)
>  			continue;
>  
>  		orig_node = gw_node->orig_node;
> -		router = batadv_orig_node_get_router(orig_node);
> +		router = batadv_orig_router_get(orig_node, BATADV_IF_DEFAULT);
>  		if (!router)
>  			continue;
>  
> @@ -246,7 +246,8 @@ void batadv_gw_election(struct batadv_priv *bat_priv)
>  	if (next_gw) {
>  		sprintf(gw_addr, "%pM", next_gw->orig_node->orig);
>  
> -		router = batadv_orig_node_get_router(next_gw->orig_node);
> +		router = batadv_orig_router_get(next_gw->orig_node,
> +						BATADV_IF_DEFAULT);
>  		if (!router) {
>  			batadv_gw_deselect(bat_priv);
>  			goto out;
> @@ -315,7 +316,7 @@ void batadv_gw_check_election(struct batadv_priv *bat_priv,
>  	if (!curr_gw_orig)
>  		goto deselect;
>  
> -	router_gw = batadv_orig_node_get_router(curr_gw_orig);
> +	router_gw = batadv_orig_router_get(curr_gw_orig, BATADV_IF_DEFAULT);
>  	if (!router_gw)
>  		goto deselect;
>  
> @@ -328,7 +329,7 @@ void batadv_gw_check_election(struct batadv_priv *bat_priv,
>  	if (curr_gw_orig == orig_node)
>  		goto out;
>  
> -	router_orig = batadv_orig_node_get_router(orig_node);
> +	router_orig = batadv_orig_router_get(orig_node, BATADV_IF_DEFAULT);
>  	if (!router_orig)
>  		goto out;
>  
> @@ -556,7 +557,7 @@ static int batadv_write_buffer_text(struct batadv_priv *bat_priv,
>  	struct batadv_neigh_ifinfo *router_ifinfo;
>  	int ret = -1;
>  
> -	router = batadv_orig_node_get_router(gw_node->orig_node);
> +	router = batadv_orig_router_get(gw_node->orig_node, BATADV_IF_DEFAULT);
>  	if (!router)
>  		goto out;
>  
> diff --git a/icmp_socket.c b/icmp_socket.c
> index 29ae4ef..6b15efc 100644
> --- a/icmp_socket.c
> +++ b/icmp_socket.c
> @@ -217,7 +217,8 @@ static ssize_t batadv_socket_write(struct file *file, const char __user *buff,
>  		if (!orig_node)
>  			goto dst_unreach;
>  
> -		neigh_node = batadv_orig_node_get_router(orig_node);
> +		neigh_node = batadv_orig_router_get(orig_node,
> +						    BATADV_IF_DEFAULT);
>  		if (!neigh_node)
>  			goto dst_unreach;
>  
> diff --git a/network-coding.c b/network-coding.c
> index 351e199..792a072 100644
> --- a/network-coding.c
> +++ b/network-coding.c
> @@ -1019,12 +1019,17 @@ static bool batadv_nc_code_packets(struct batadv_priv *bat_priv,
>  	int coded_size = sizeof(*coded_packet);
>  	int header_add = coded_size - unicast_size;
>  
> -	router_neigh = batadv_orig_node_get_router(neigh_node->orig_node);
> +	/* TODO: do we need to consider the outgoing interface for
> +	 * coded packets?
> +	 */
> +	router_neigh = batadv_orig_router_get(neigh_node->orig_node,
> +					      BATADV_IF_DEFAULT);
>  	if (!router_neigh)
>  		goto out;
>  
>  	neigh_tmp = nc_packet->neigh_node;
> -	router_coding = batadv_orig_node_get_router(neigh_tmp->orig_node);
> +	router_coding = batadv_orig_router_get(neigh_tmp->orig_node,
> +					       BATADV_IF_DEFAULT);
>  	if (!router_coding)
>  		goto out;
>  
> diff --git a/originator.c b/originator.c
> index acbd254..62691d0 100644
> --- a/originator.c
> +++ b/originator.c
> @@ -227,14 +227,30 @@ void batadv_neigh_node_free_ref(struct batadv_neigh_node *neigh_node)
>  		call_rcu(&neigh_node->rcu, batadv_neigh_node_free_rcu);
>  }
>  
> -/* 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

Yet another naming for "if_incoming"? (there are few more
occurences of "if_received" introduced in this patch)

> + *
> + * Returns the neighbor which should be router for this orig_node/iface.
> + *
> + * The object is returned with refcounter increased by 1.
> + */
>  struct batadv_neigh_node *
> -batadv_orig_node_get_router(struct batadv_orig_node *orig_node)
> +batadv_orig_router_get(struct batadv_orig_node *orig_node,
> +		       const struct batadv_hard_iface *if_received)
>  {
> -	struct batadv_neigh_node *router;
> +	struct batadv_orig_ifinfo *orig_ifinfo;
> +	struct batadv_neigh_node *router = NULL;
>  
>  	rcu_read_lock();
> -	router = rcu_dereference(orig_node->router);
> +	hlist_for_each_entry_rcu(orig_ifinfo, &orig_node->ifinfo_list, list) {
> +		if (orig_ifinfo->if_outgoing != if_received)
> +			continue;
> +
> +		router = rcu_dereference(orig_ifinfo->router);
> +		break;
> +	}
>  
>  	if (router && !atomic_inc_not_zero(&router->refcount))
>  		router = NULL;
> @@ -244,6 +260,86 @@ batadv_orig_node_get_router(struct batadv_orig_node *orig_node)
>  }
>  
>  /**
> + * batadv_orig_ifinfo_get - find 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_ifinfo or NULL if not found.
> + *
> + * The object is returned with refcounter increased by 1.
> + */
> +struct batadv_orig_ifinfo *
> +batadv_orig_ifinfo_get(struct batadv_orig_node *orig_node,
> +		       struct batadv_hard_iface *if_received)
> +{
> +	struct batadv_orig_ifinfo *tmp, *orig_ifinfo = NULL;
> +
> +	rcu_read_lock();
> +	hlist_for_each_entry_rcu(tmp, &orig_node->ifinfo_list,
> +				 list) {
> +		if (tmp->if_outgoing != if_received)
> +			continue;
> +
> +		if (!atomic_inc_not_zero(&tmp->refcount))
> +			continue;
> +
> +		orig_ifinfo = tmp;
> +		break;
> +	}
> +	rcu_read_unlock();
> +
> +	return orig_ifinfo;
> +}
> +
> +/**
> + * batadv_orig_ifinfo_new - search and possibly create an orig_ifinfo object
> + * @orig_node: the orig node to be queried
> + * @if_received: the interface for which the ifinfo should be acquired
> + *
> + * Returns NULL in case of failure or the orig_ifinfo object for the if_outgoing
> + * interface otherwise. The object is created and added to the list
> + * if it does not exist.
> + *
> + * The object is returned with refcounter increased by 1.
> + */
> +struct batadv_orig_ifinfo *
> +batadv_orig_ifinfo_new(struct batadv_orig_node *orig_node,
> +		       struct batadv_hard_iface *if_received)
> +{
> +	struct batadv_orig_ifinfo *orig_ifinfo = NULL;
> +	unsigned long reset_time;
> +
> +	spin_lock_bh(&orig_node->neigh_list_lock);
> +
> +	orig_ifinfo = batadv_orig_ifinfo_get(orig_node, if_received);
> +	if (orig_ifinfo)
> +		goto out;
> +
> +	orig_ifinfo = kzalloc(sizeof(*orig_ifinfo), GFP_ATOMIC);
> +	if (!orig_ifinfo)
> +		goto out;
> +
> +	if (if_received != BATADV_IF_DEFAULT &&
> +	    !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);
> +	atomic_set(&orig_ifinfo->refcount, 2);
> +	hlist_add_head_rcu(&orig_ifinfo->list,
> +			   &orig_node->ifinfo_list);
> +out:
> +	spin_unlock_bh(&orig_node->neigh_list_lock);
> +	return orig_ifinfo;
> +}
> +
> +/**
>   * batadv_neigh_ifinfo_get - find the ifinfo from an neigh_node
>   * @neigh_node: the neigh node to be queried
>   * @if_outgoing: the interface for which the ifinfo should be acquired
> @@ -356,11 +452,50 @@ out:
>  	return neigh_node;
>  }
>  
> +/* batadv_orig_ifinfo_free_rcu - free the orig_ifinfo object
> + * @rcu: rcu pointer of the orig_ifinfo object
> + */
> +static void batadv_orig_ifinfo_free_rcu(struct rcu_head *rcu)
> +{
> +	struct batadv_orig_ifinfo *orig_ifinfo;
> +
> +	orig_ifinfo = container_of(rcu, struct batadv_orig_ifinfo, rcu);
> +
> +	if (orig_ifinfo->if_outgoing != BATADV_IF_DEFAULT)
> +		batadv_hardif_free_ref_now(orig_ifinfo->if_outgoing);
> +
> +	kfree(orig_ifinfo);
> +}
> +
> +/**
> + * batadv_orig_ifinfo_free_ref - decrement the refcounter and possibly free
> + *  the orig_ifinfo (without rcu callback)
> + * @orig_ifinfo: the orig_ifinfo object to release
> + */
> +static void
> +batadv_orig_ifinfo_free_ref_now(struct batadv_orig_ifinfo *orig_ifinfo)
> +{
> +	if (atomic_dec_and_test(&orig_ifinfo->refcount))
> +		batadv_orig_ifinfo_free_rcu(&orig_ifinfo->rcu);
> +}
> +
> +/**
> + * batadv_orig_ifinfo_free_ref - decrement the refcounter and possibly free
> + *  the orig_ifinfo
> + * @orig_ifinfo: the orig_ifinfo object to release
> + */
> +void batadv_orig_ifinfo_free_ref(struct batadv_orig_ifinfo *orig_ifinfo)
> +{
> +	if (atomic_dec_and_test(&orig_ifinfo->refcount))
> +		call_rcu(&orig_ifinfo->rcu, batadv_orig_ifinfo_free_rcu);
> +}
> +
>  static void batadv_orig_node_free_rcu(struct rcu_head *rcu)
>  {
>  	struct hlist_node *node_tmp;
>  	struct batadv_neigh_node *neigh_node;
>  	struct batadv_orig_node *orig_node;
> +	struct batadv_orig_ifinfo *orig_ifinfo;
>  
>  	orig_node = container_of(rcu, struct batadv_orig_node, rcu);
>  
> @@ -373,6 +508,11 @@ static void batadv_orig_node_free_rcu(struct rcu_head *rcu)
>  		batadv_neigh_node_free_ref_now(neigh_node);
>  	}
>  
> +	hlist_for_each_entry_safe(orig_ifinfo, node_tmp,
> +				  &orig_node->ifinfo_list, list) {
> +		hlist_del_rcu(&orig_ifinfo->list);
> +		batadv_orig_ifinfo_free_ref_now(orig_ifinfo);
> +	}
>  	spin_unlock_bh(&orig_node->neigh_list_lock);
>  
>  	/* Free nc_nodes */
> @@ -470,6 +610,7 @@ struct batadv_orig_node *batadv_orig_node_new(struct batadv_priv *bat_priv,
>  
>  	INIT_HLIST_HEAD(&orig_node->neigh_list);
>  	INIT_LIST_HEAD(&orig_node->vlan_list);
> +	INIT_HLIST_HEAD(&orig_node->ifinfo_list);
>  	spin_lock_init(&orig_node->bcast_seqno_lock);
>  	spin_lock_init(&orig_node->neigh_list_lock);
>  	spin_lock_init(&orig_node->tt_buff_lock);
> @@ -485,13 +626,11 @@ struct batadv_orig_node *batadv_orig_node_new(struct batadv_priv *bat_priv,
>  	orig_node->bat_priv = bat_priv;
>  	memcpy(orig_node->orig, addr, ETH_ALEN);
>  	batadv_dat_init_orig_node_addr(orig_node);
> -	orig_node->router = NULL;
>  	atomic_set(&orig_node->last_ttvn, 0);
>  	orig_node->tt_buff = NULL;
>  	orig_node->tt_buff_len = 0;
>  	reset_time = jiffies - 1 - msecs_to_jiffies(BATADV_RESET_PROTECTION_MS);
>  	orig_node->bcast_seqno_reset = reset_time;
> -	orig_node->batman_seqno_reset = reset_time;
>  
>  	/* create a vlan object for the "untagged" LAN */
>  	vlan = batadv_orig_node_vlan_new(orig_node, BATADV_NO_FLAGS);
> @@ -516,6 +655,52 @@ free_orig_node:
>  }
>  
>  /**
> + * batadv_purge_orig_ifinfo - purge ifinfo entries from originator
> + * @bat_priv: the bat priv with all the soft interface information
> + * @orig_node: orig node which is to be checked
> + *
> + * Returns true if any ifinfo entry was purged, false otherwise.
> + */
> +static bool
> +batadv_purge_orig_ifinfo(struct batadv_priv *bat_priv,
> +			 struct batadv_orig_node *orig_node)
> +{
> +	struct batadv_orig_ifinfo *orig_ifinfo;
> +	struct batadv_hard_iface *if_outgoing;
> +	struct hlist_node *node_tmp;
> +	bool ifinfo_purged = false;
> +
> +	spin_lock_bh(&orig_node->neigh_list_lock);
> +
> +	/* for all neighbors towards this originator ... */
> +	hlist_for_each_entry_safe(orig_ifinfo, node_tmp,
> +				  &orig_node->ifinfo_list, list) {
> +		if_outgoing = orig_ifinfo->if_outgoing;
> +		if (!if_outgoing)
> +			continue;
> +
> +		if ((if_outgoing->if_status != BATADV_IF_INACTIVE) &&
> +		    (if_outgoing->if_status != BATADV_IF_NOT_IN_USE) &&
> +		    (if_outgoing->if_status != BATADV_IF_TO_BE_REMOVED))
> +			continue;
> +

Hm, if we aren't purging them now, when are we going to do that
instead? Later we can't because the orig_node and its ifinfo list is
gone, can we?

> +		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> +			   "router/ifinfo purge: originator %pM, iface: %s\n",
> +			   orig_node->orig, if_outgoing->net_dev->name);
> +
> +		ifinfo_purged = true;
> +
> +		hlist_del_rcu(&orig_ifinfo->list);
> +		batadv_orig_ifinfo_free_ref(orig_ifinfo);
> +	}
> +
> +	spin_unlock_bh(&orig_node->neigh_list_lock);
> +
> +	return ifinfo_purged;
> +}
> +
> +
> +/**
>   * batadv_purge_orig_neighbors - purges neighbors from originator
>   * @bat_priv: the bat priv with all the soft interface information
>   * @orig_node: orig node which is to be checked
> @@ -599,6 +784,8 @@ static bool batadv_purge_orig_node(struct batadv_priv *bat_priv,
>  				   struct batadv_orig_node *orig_node)
>  {
>  	struct batadv_neigh_node *best_neigh_node;
> +	struct batadv_hard_iface *hard_iface;
> +	bool changed;
>  
>  	if (batadv_has_timed_out(orig_node->last_seen,
>  				 2 * BATADV_PURGE_TIMEOUT)) {
> @@ -608,12 +795,34 @@ static bool batadv_purge_orig_node(struct batadv_priv *bat_priv,
>  			   jiffies_to_msecs(orig_node->last_seen));
>  		return true;
>  	}
> -	if (!batadv_purge_orig_neighbors(bat_priv, orig_node))
> +	changed = batadv_purge_orig_ifinfo(bat_priv, orig_node);
> +	changed = changed || batadv_purge_orig_neighbors(bat_priv, orig_node);
> +
> +	if (!changed)
>  		return false;
>  
> +	/* first for NULL ... */
>  	best_neigh_node = batadv_find_best_neighbor(bat_priv, orig_node,
>  						    BATADV_IF_DEFAULT);
> -	batadv_update_route(bat_priv, orig_node, best_neigh_node);
> +	batadv_update_route(bat_priv, orig_node, BATADV_IF_DEFAULT,
> +			    best_neigh_node);
> +
> +	/* ... then for all other interfaces. */
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(hard_iface, &batadv_hardif_list, list) {
> +		if (hard_iface->if_status != BATADV_IF_ACTIVE)
> +			continue;
> +
> +		if (hard_iface->soft_iface != bat_priv->soft_iface)
> +			continue;
> +
> +		best_neigh_node = batadv_find_best_neighbor(bat_priv,
> +							    orig_node,
> +							    hard_iface);
> +		batadv_update_route(bat_priv, orig_node, hard_iface,
> +				    best_neigh_node);
> +	}
> +	rcu_read_unlock();
>  
>  	return false;
>  }
> diff --git a/originator.h b/originator.h
> index 76a8de6..404f7cb 100644
> --- a/originator.h
> +++ b/originator.h
> @@ -36,7 +36,8 @@ batadv_neigh_node_new(struct batadv_hard_iface *hard_iface,
>  		      struct batadv_orig_node *orig_node);
>  void batadv_neigh_node_free_ref(struct batadv_neigh_node *neigh_node);
>  struct batadv_neigh_node *
> -batadv_orig_node_get_router(struct batadv_orig_node *orig_node);
> +batadv_orig_router_get(struct batadv_orig_node *orig_node,
> +		       const struct batadv_hard_iface *if_received);
>  struct batadv_neigh_ifinfo *
>  batadv_neigh_ifinfo_new(struct batadv_neigh_node *neigh,
>  			struct batadv_hard_iface *if_outgoing);
> @@ -44,6 +45,15 @@ struct batadv_neigh_ifinfo *
>  batadv_neigh_ifinfo_get(struct batadv_neigh_node *neigh,
>  			struct batadv_hard_iface *if_outgoing);
>  void batadv_neigh_ifinfo_free_ref(struct batadv_neigh_ifinfo *neigh_ifinfo);
> +
> +struct batadv_orig_ifinfo *
> +batadv_orig_ifinfo_get(struct batadv_orig_node *orig_node,
> +		       struct batadv_hard_iface *if_received);
> +struct batadv_orig_ifinfo *
> +batadv_orig_ifinfo_new(struct batadv_orig_node *orig_node,
> +		       struct batadv_hard_iface *if_received);
> +void batadv_orig_ifinfo_free_ref(struct batadv_orig_ifinfo *orig_ifinfo);
> +
>  int batadv_orig_seq_print_text(struct seq_file *seq, void *offset);
>  int batadv_orig_hash_add_if(struct batadv_hard_iface *hard_iface,
>  			    int max_if_num);
> diff --git a/routing.c b/routing.c
> index 360a5b9..da6741a 100644
> --- a/routing.c
> +++ b/routing.c
> @@ -35,13 +35,29 @@
>  static int batadv_route_unicast_packet(struct sk_buff *skb,
>  				       struct batadv_hard_iface *recv_if);
>  
> +/* _batadv_update_route - set the router for this originator
> + * @bat_priv: the bat priv with all the soft interface information
> + * @orig_node: orig node which is to be configured
> + * @recv_if: the receive interface for which this route is set
> + * @neigh_node: neighbor which should be the next router
> + *
> + * This function does not perform any error checks
> + */
>  static void _batadv_update_route(struct batadv_priv *bat_priv,
>  				 struct batadv_orig_node *orig_node,
> +				 struct batadv_hard_iface *recv_if,
>  				 struct batadv_neigh_node *neigh_node)
>  {
> +	struct batadv_orig_ifinfo *orig_ifinfo;
>  	struct batadv_neigh_node *curr_router;
>  
> -	curr_router = batadv_orig_node_get_router(orig_node);
> +	orig_ifinfo = batadv_orig_ifinfo_get(orig_node, recv_if);
> +	if (!orig_ifinfo)
> +		return;
> +

Looks like there is an rcu_read_lock() missing for the following
three lines:

> +	curr_router = rcu_dereference(orig_ifinfo->router);
> +	if (curr_router && !atomic_inc_not_zero(&curr_router->refcount))
> +		curr_router = NULL;
>  
>  	/* route deleted */
>  	if ((curr_router) && (!neigh_node)) {
> @@ -71,16 +87,25 @@ static void _batadv_update_route(struct batadv_priv *bat_priv,
>  		neigh_node = NULL;
>  
>  	spin_lock_bh(&orig_node->neigh_list_lock);
> -	rcu_assign_pointer(orig_node->router, neigh_node);
> +	rcu_assign_pointer(orig_ifinfo->router, neigh_node);
>  	spin_unlock_bh(&orig_node->neigh_list_lock);
> +	batadv_orig_ifinfo_free_ref(orig_ifinfo);
>  
>  	/* decrease refcount of previous best neighbor */
>  	if (curr_router)
>  		batadv_neigh_node_free_ref(curr_router);
>  }
>  
> +/**
> + * batadv_update_route - set the router for this originator
> + * @bat_priv: the bat priv with all the soft interface information
> + * @orig_node: orig node which is to be configured
> + * @recv_if: the receive interface for which this route is set
> + * @neigh_node: neighbor which should be the next router
> + */
>  void batadv_update_route(struct batadv_priv *bat_priv,
>  			 struct batadv_orig_node *orig_node,
> +			 struct batadv_hard_iface *recv_if,
>  			 struct batadv_neigh_node *neigh_node)
>  {
>  	struct batadv_neigh_node *router = NULL;
> @@ -88,10 +113,10 @@ void batadv_update_route(struct batadv_priv *bat_priv,
>  	if (!orig_node)
>  		goto out;
>  
> -	router = batadv_orig_node_get_router(orig_node);
> +	router = batadv_orig_router_get(orig_node, recv_if);
>  
>  	if (router != neigh_node)
> -		_batadv_update_route(bat_priv, orig_node, neigh_node);
> +		_batadv_update_route(bat_priv, orig_node, recv_if, neigh_node);
>  
>  out:
>  	if (router)
> @@ -408,7 +433,7 @@ batadv_find_router(struct batadv_priv *bat_priv,
>  	if (!orig_node)
>  		return NULL;
>  
> -	router = batadv_orig_node_get_router(orig_node);
> +	router = batadv_orig_router_get(orig_node, recv_if);
>  
>  	/* TODO: fill this later with new bonding mechanism */
>  
> diff --git a/routing.h b/routing.h
> index b8fed80..7a7c6e9 100644
> --- a/routing.h
> +++ b/routing.h
> @@ -25,6 +25,7 @@ bool batadv_check_management_packet(struct sk_buff *skb,
>  				    int header_len);
>  void batadv_update_route(struct batadv_priv *bat_priv,
>  			 struct batadv_orig_node *orig_node,
> +			 struct batadv_hard_iface *recv_if,
>  			 struct batadv_neigh_node *neigh_node);
>  int batadv_recv_icmp_packet(struct sk_buff *skb,
>  			    struct batadv_hard_iface *recv_if);
> diff --git a/translation-table.c b/translation-table.c
> index 135c165..1b2a615 100644
> --- a/translation-table.c
> +++ b/translation-table.c
> @@ -1385,7 +1385,8 @@ batadv_transtable_best_orig(struct batadv_priv *bat_priv,
>  
>  	head = &tt_global_entry->orig_list;
>  	hlist_for_each_entry_rcu(orig_entry, head, list) {
> -		router = batadv_orig_node_get_router(orig_entry->orig_node);
> +		router = batadv_orig_router_get(orig_entry->orig_node,
> +						BATADV_IF_DEFAULT);
>  		if (!router)
>  			continue;
>  
> diff --git a/types.h b/types.h
> index 11d448f..167c3a5 100644
> --- a/types.h
> +++ b/types.h
> @@ -90,6 +90,27 @@ struct batadv_hard_iface {
>  	struct work_struct cleanup_work;
>  };
>  
> +/* struct batadv_orig_ifinfo - originator info per outgoing interface
> + * @list: list node for orig_node::ifinfo_list
> + * @if_outgoing: pointer to outgoing hard interface
> + * @router: router that should be used to reach this originator
> + * @last_real_seqno: last and best known sequence number
> + * @last_ttl: ttl of last received packet
> + * @batman_seqno_reset: time when the batman seqno window was reset
> + * @refcount: number of contexts the object is used
> + * @rcu: struct used for freeing in an RCU-safe manner
> + */
> +struct batadv_orig_ifinfo {
> +	struct hlist_node list;
> +	struct batadv_hard_iface *if_outgoing;
> +	struct batadv_neigh_node __rcu *router; /* rcu protected pointer */
> +	uint32_t last_real_seqno;
> +	uint8_t last_ttl;
> +	unsigned long batman_seqno_reset;
> +	atomic_t refcount;
> +	struct rcu_head rcu;
> +};
> +
>  /**
>   * struct batadv_frag_table_entry - head in the fragment buffer table
>   * @head: head of list with fragments
> @@ -165,11 +186,10 @@ struct batadv_orig_bat_iv {
>   * struct batadv_orig_node - structure for orig_list maintaining nodes of mesh
>   * @orig: originator ethernet address
>   * @primary_addr: hosts primary interface address
> - * @router: router that should be used to reach this originator
> + * @ifinfo_list: list for routers per outgoing interface
>   * @batadv_dat_addr_t:  address of the orig node in the distributed hash
>   * @last_seen: time when last packet from this node was received
>   * @bcast_seqno_reset: time when the broadcast seqno window was reset
> - * @batman_seqno_reset: time when the batman seqno window was reset
>   * @capabilities: announced capabilities of this originator
>   * @last_ttvn: last seen translation table version number
>   * @tt_buff: last tt changeset this node received from the orig node
> @@ -182,8 +202,6 @@ struct batadv_orig_bat_iv {
>   *  made up by two operations (data structure update and metdata -CRC/TTVN-
>   *  recalculation) and they have to be executed atomically in order to avoid
>   *  another thread to read the table/metadata between those.
> - * @last_real_seqno: last and best known sequence number
> - * @last_ttl: ttl of last received packet
>   * @bcast_bits: bitfield containing the info which payload broadcast originated
>   *  from this orig node this host already has seen (relative to
>   *  last_bcast_seqno)
> @@ -208,13 +226,12 @@ struct batadv_orig_bat_iv {
>  struct batadv_orig_node {
>  	uint8_t orig[ETH_ALEN];
>  	uint8_t primary_addr[ETH_ALEN];
> -	struct batadv_neigh_node __rcu *router; /* rcu protected pointer */
> +	struct hlist_head ifinfo_list;
>  #ifdef CONFIG_BATMAN_ADV_DAT
>  	batadv_dat_addr_t dat_addr;
>  #endif
>  	unsigned long last_seen;
>  	unsigned long bcast_seqno_reset;
> -	unsigned long batman_seqno_reset;
>  	uint8_t capabilities;
>  	atomic_t last_ttvn;
>  	unsigned char *tt_buff;
> @@ -223,8 +240,6 @@ struct batadv_orig_node {
>  	bool tt_initialised;
>  	/* prevents from changing the table while reading it */
>  	spinlock_t tt_lock;
> -	uint32_t last_real_seqno;
> -	uint8_t last_ttl;
>  	DECLARE_BITMAP(bcast_bits, BATADV_TQ_LOCAL_WINDOW_SIZE);
>  	uint32_t last_bcast_seqno;
>  	struct hlist_head neigh_list;
> -- 
> 1.7.10.4
> 

Cheers, Linus

  reply	other threads:[~2013-11-05  0:03 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-30 14:01 [B.A.T.M.A.N.] [PATCHv2 0/8] add network wide multi interface optimization Simon Wunderlich
2013-10-30 14:01 ` [B.A.T.M.A.N.] [PATCHv2 1/8] batman-adv: remove bonding and interface alternating Simon Wunderlich
2013-10-30 14:01 ` [B.A.T.M.A.N.] [PATCHv2 2/8] batman-adv: split tq information in neigh_node struct Simon Wunderlich
2013-10-30 16:33   ` Marek Lindner
2013-11-01  5:53   ` Linus Lüssing
2013-11-05 11:16     ` Simon Wunderlich
2013-11-05  0:13   ` Linus Lüssing
2013-10-30 14:01 ` [B.A.T.M.A.N.] [PATCHv2 3/8] batman-adv: split out router from orig_node Simon Wunderlich
2013-11-05  0:03   ` Linus Lüssing [this message]
2013-11-05 13:14     ` Simon Wunderlich
2013-11-05  0:17   ` Linus Lüssing
2013-10-30 14:01 ` [B.A.T.M.A.N.] [PATCHv2 4/8] batman-adv: add WiFi penalty Simon Wunderlich
2013-10-30 14:01 ` [B.A.T.M.A.N.] [PATCHv2 5/8] batman-adv: consider outgoing interface in OGM sending Simon Wunderlich
2013-10-30 14:01 ` [B.A.T.M.A.N.] [PATCHv2 6/8] batman-adv: add bonding again Simon Wunderlich
2013-10-30 14:01 ` [B.A.T.M.A.N.] [PATCHv2 7/8] batman-adv: add debugfs structure for information per interface Simon Wunderlich
2013-10-30 14:01 ` [B.A.T.M.A.N.] [PATCHv2 8/8] batman-adv: add debugfs support to view multiif tables Simon Wunderlich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131105000342.GU15496@Linus-Debian \
    --to=linus.luessing@web.de \
    --cc=b.a.t.m.a.n@lists.open-mesh.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox