[Olsr-dev] [PATCH][BUILD] check_neighbor_link is wrong

Henning Rogge (spam-protected)
Tue Dec 11 11:02:43 CET 2012


Comments inline.

On 12/10/2012 08:34 PM, Giovanni Di Stasi wrote:
>> I see only two cases where this "mix up links between nodes" will happen:
>> a) a node use the same IPs for multiple interfaces
>> b) a node can see multiple interfaces of a different node
>>
>> The patch solves parts of case b) as long as no VLANs are involved. can
>> we use the interface index instead of the local IP?
>
> The intent of the patch was to fix case b) that  may easily arise if you change channels on the interfaces.
> I didn't consider the VLAN case, but from our point of view using the interface index is perfectly fine.

> diff --git a/src/hna_set.c b/src/hna_set.c
> index df8a82d..711e85a 100644
> --- a/src/hna_set.c
> +++ b/src/hna_set.c
> @@ -362,7 +362,7 @@ olsr_print_hna_set(void)
>   */
>
>  bool
> -olsr_input_hna(union olsr_message *m, struct interface *in_if __attribute__ ((unused)), union olsr_ip_addr *from_addr)
> +olsr_input_hna(union olsr_message *m, struct interface *in_if, union olsr_ip_addr *from_addr)
>  {
>
>    uint8_t olsr_msgtype;
> @@ -425,7 +425,7 @@ olsr_input_hna(union olsr_message *m, struct interface *in_if __attribute__ ((un
>     *      is not in the symmetric 1-hop neighborhood of this node, the
>     *      message MUST be discarded.
>     */
> -  if (check_neighbor_link(from_addr) != SYM_LINK) {
> +  if (check_neighbor_link(&in_if->ip_addr, from_addr) != SYM_LINK) {
>      OLSR_PRINTF(2, "Received HNA from NON SYM neighbor %s\n", olsr_ip_to_string(&buf, from_addr));
>      return false;
>    }
> diff --git a/src/link_set.c b/src/link_set.c
> index aa6a9ea..cba1855 100644
> --- a/src/link_set.c
> +++ b/src/link_set.c
> @@ -184,7 +184,7 @@ get_neighbor_status(const union olsr_ip_addr *address)
>    if (!(main_addr = mid_lookup_main_addr(address)))
>      main_addr = address;
>
> -  /* Loop trough local interfaces to check all possebilities */
> +  /* Loop trough local interfaces to check all possbilities */
>    for (ifs = ifnet; ifs != NULL; ifs = ifs->int_next) {
>      struct mid_address *aliases;
>      struct link_entry *lnk = lookup_link_entry(main_addr, NULL, ifs);
> @@ -216,10 +216,14 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
>    const union olsr_ip_addr *main_addr;
>    struct link_entry *walker, *good_link, *backup_link;
>    struct interface *tmp_if;
> -  int curr_metric = MAX_IF_METRIC;
> -  olsr_linkcost curr_lcost = LINK_COST_BROKEN;
> +  int curr_metric_good = MAX_IF_METRIC;
> +  int curr_metric_backup = MAX_IF_METRIC;
> +  olsr_linkcost curr_lcost_good = LINK_COST_BROKEN;
> +  olsr_linkcost curr_lcost_backup = LINK_COST_BROKEN;
> +
>    olsr_linkcost tmp_lc;
>
> +
>    /* main address lookup */
>    main_addr = mid_lookup_main_addr(remote);
>
> @@ -234,13 +238,12 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
>
>    /* loop through all links that we have */
>    OLSR_FOR_ALL_LINK_ENTRIES(walker) {
> -
> +
>      /* if this is not a link to the neighour in question, skip */
>      if (!ipequal(&walker->neighbor->neighbor_main_addr, main_addr))
>        continue;
> -
> +
>      if (olsr_cnf->lq_level == 0) {
> -
>        /*
>         * handle the non-LQ, RFC-compliant case
>         */
> @@ -257,20 +260,22 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
>
>        /*
>         * is this interface better than anything we had before ?
> -       * use the requested remote interface address as a tie-breaker
> +       * use the link interface index as a tie-breaker
>         */
> -      if ((tmp_if->int_metric < curr_metric) || ((tmp_if->int_metric == curr_metric) && ipequal(&walker->local_iface_addr, remote))) {
> -
> -        /* memorize the interface's metric */
> -        curr_metric = tmp_if->int_metric;
> -
> -        /* prefer symmetric links over asymmetric links */
> -        if (lookup_link_status(walker) == SYM_LINK) {
> -          good_link = walker;
> -        } else {
> -          backup_link = walker;
> -        }
> +      if (lookup_link_status(walker) == SYM_LINK) {
> +	if (tmp_if->int_metric < curr_metric_good ||
> +	    (tmp_if->int_metric == curr_metric_good && good_link && tmp_if->if_index < good_link->inter->if_index)){
> +	    good_link = walker;
> +            curr_metric_good = tmp_if->int_metric;
> +	  }
> +      } else {
> +	  if (tmp_if->int_metric < curr_metric_backup ||
> +	      (tmp_if->int_metric == curr_metric_backup && backup_link && tmp_if->if_index < backup_link->inter->if_index)){
> +	    backup_link = walker;
> +	    curr_metric_backup = tmp_if->int_metric;
> +	  }
>        }
> +
>      } else {
>
>        /*
> @@ -280,22 +285,31 @@ get_best_link_to_neighbor(const union olsr_ip_addr *remote)
>        /* get the link cost */
>        tmp_lc = walker->linkcost;
>
> +      /* find the interface for the link */
> +      tmp_if = walker->if_name ? if_ifwithname(walker->if_name) : if_ifwithaddr(&walker->local_iface_addr);
> +
> +      if (!tmp_if) {
> +        continue;
> +      }
> +
>        /*
>         * is this link better than anything we had before ?
> -       * use the requested remote interface address as a tie-breaker.
> +       * use the link interface index as a tie-breaker
>         */
> -      if ((tmp_lc < curr_lcost) || ((tmp_lc == curr_lcost) && ipequal(&walker->local_iface_addr, remote))) {
> -
> -        /* memorize the link quality */
> -        curr_lcost = tmp_lc;
> -
> -        /* prefer symmetric links over asymmetric links */
> -        if (lookup_link_status(walker) == SYM_LINK) {
> -          good_link = walker;
> -        } else {
> -          backup_link = walker;
> -        }
> +      if (lookup_link_status(walker) == SYM_LINK) {
> +	  if (tmp_lc < curr_lcost_good  ||
> +	     (tmp_lc == curr_lcost_good && good_link && tmp_if->if_index < good_link->inter->if_index)){
> +		good_link = walker;
> +		curr_lcost_good=tmp_lc;
> +	  }
> +      } else {
> +	   if (tmp_lc < curr_lcost_backup ||
> +    	      (tmp_lc == curr_lcost_backup && backup_link && tmp_if->if_index < backup_link->inter->if_index)){
> +	    backup_link = walker;
> +	    curr_lcost_backup=tmp_lc;
> +	  }
>        }
> +
>      }
>    }

I am a little bit scared to look up who use this function if it can 
return an asymmetric link. Asymmetric links are useless for outbound 
routing.

>    OLSR_FOR_ALL_LINK_ENTRIES_END(walker);
> @@ -327,7 +341,7 @@ set_loss_link_multiplier(struct link_entry *entry)
>      }
>    }
>    assert(cfg_inter);
> -
> +
>    /* create a null address for comparison */
>    memset(&null_addr, 0, sizeof(union olsr_ip_addr));
>
> @@ -627,12 +641,12 @@ add_link_entry(const union olsr_ip_addr *local, const union olsr_ip_addr *remote
>   * @return 1 of the link is symmertic 0 if not
>   */
>  int
> -check_neighbor_link(const union olsr_ip_addr *int_addr)
> +check_neighbor_link(const union olsr_ip_addr * local, const union olsr_ip_addr *remote)
>  {
>    struct link_entry *link;
>
>    OLSR_FOR_ALL_LINK_ENTRIES(link) {
> -    if (ipequal(int_addr, &link->neighbor_iface_addr)) {
> +    if (ipequal(remote, &link->neighbor_iface_addr)  && ipequal(local, &link->local_iface_addr)) {

I was talking about using the interface index (instead of the local IP) 
here to detect the "right" link.

>        return lookup_link_status(link);
>      }
>    }
> @@ -720,7 +734,7 @@ update_link_entry(const union olsr_ip_addr *local, const union olsr_ip_addr *rem
>    }
>
>    /* L_time = max(L_time, L_ASYM_time) */
> -  if (entry->link_timer == NULL || (entry->link_timer->timer_clock < entry->ASYM_time)) {
> +  if (entry->link_timer && (entry->link_timer->timer_clock < entry->ASYM_time)) {
>      olsr_set_link_timer(entry, TIME_DUE(entry->ASYM_time));
>    }
>
> diff --git a/src/link_set.h b/src/link_set.h
> index 9ae27f5..f7645e2 100644
> --- a/src/link_set.h
> +++ b/src/link_set.h
> @@ -133,7 +133,7 @@ struct link_entry *lookup_link_entry(const union olsr_ip_addr *, const union ols
>  struct link_entry *update_link_entry(const union olsr_ip_addr *, const union olsr_ip_addr *, const struct hello_message *,
>                                       const struct interface *);
>
> -int check_neighbor_link(const union olsr_ip_addr *);
> +int check_neighbor_link(const union olsr_ip_addr *, const union olsr_ip_addr *);
>  int replace_neighbor_link_set(const struct neighbor_entry *, struct neighbor_entry *);
>  int lookup_link_status(const struct link_entry *);
>  void olsr_update_packet_loss_hello_int(struct link_entry *, olsr_reltime);
> diff --git a/src/mid_set.c b/src/mid_set.c
> index c25e0bf..13d8b18 100644
> --- a/src/mid_set.c
> +++ b/src/mid_set.c
> @@ -561,7 +561,7 @@ olsr_print_mid_set(void)
>   */
>
>  bool
> -olsr_input_mid(union olsr_message *m, struct interface *in_if __attribute__ ((unused)), union olsr_ip_addr *from_addr)
> +olsr_input_mid(union olsr_message *m, struct interface *in_if, union olsr_ip_addr *from_addr)
>  {
>    struct ipaddr_str buf;
>    struct mid_alias *tmp_adr;
> @@ -584,7 +584,7 @@ olsr_input_mid(union olsr_message *m, struct interface *in_if __attribute__ ((un
>     *      message MUST be discarded.
>     */
>
> -  if (check_neighbor_link(from_addr) != SYM_LINK) {
> +  if (check_neighbor_link(&in_if->ip_addr, from_addr) != SYM_LINK) {
>      OLSR_PRINTF(2, "Received MID from NON SYM neighbor %s\n", olsr_ip_to_string(&buf, from_addr));
>      olsr_free_mid_packet(&message);
>      return false;
> diff --git a/src/tc_set.c b/src/tc_set.c
> index e1480d6..25e98d8 100644
> --- a/src/tc_set.c
> +++ b/src/tc_set.c
> @@ -768,7 +768,7 @@ olsr_calculate_tc_border(uint8_t lower_border, union olsr_ip_addr *lower_border_
>   * hence the spot we are looking at.
>   */
>  bool
> -olsr_input_tc(union olsr_message * msg, struct interface * input_if __attribute__ ((unused)), union olsr_ip_addr * from_addr)
> +olsr_input_tc(union olsr_message * msg, struct interface * input_if, union olsr_ip_addr * from_addr)
>  {
>    struct ipaddr_str buf;
>    uint16_t size, msg_seq, ansn;
> @@ -798,7 +798,7 @@ olsr_input_tc(union olsr_message * msg, struct interface * input_if __attribute_
>     * is not in the symmetric 1-hop neighborhood of this node, the
>     * message MUST be discarded.
>     */
> -  if (check_neighbor_link(from_addr) != SYM_LINK) {
> +  if (check_neighbor_link(&input_if->ip_addr, from_addr) != SYM_LINK) {
>      OLSR_PRINTF(2, "Received TC from NON SYM neighbor %s\n", olsr_ip_to_string(&buf, from_addr));
>      return false;
>    }



-- 
Diplom-Informatiker Henning Rogge , Fraunhofer-Institut für
Kommunikation, Informationsverarbeitung und Ergonomie FKIE
Kommunikationssysteme (KOM)
Fraunhofer Straße 20, 53343 Wachtberg, Germany
Telefon +49 228 9435-961,   Fax +49 228 9435 685
mailto:(spam-protected) http://www.fkie.fraunhofer.de

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 6169 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.olsr.org/pipermail/olsr-dev/attachments/20121211/08b7a207/attachment.bin>


More information about the Olsr-dev mailing list