Re: [B.A.T.M.A.N.] [RFC 6/6] batman-adv: Adds snooping of router and override flags for NA creation

[Antonio Quartulli <ordex@autistici.org>]

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

On Mon, Jul 08, 2013 at 03:12:45AM +0300, mihail.costea2005@gmail.com wrote:
> From: Mihail Costea <mihail.costea90@gmail.com>
> 
> Snoops router and override flags for Neighbor Advertisement in order to
> use it in NA creation. This information is stored in a extra member
> in the dat entry. This is done separately from normal data,
> because data is used for DHT comparison, while this flags can change
> their value over time.

Ok, therefore the extra member is not part of the "key" ( that we wrongly called
data till now), but it is part of the "data" (that was a mac address only till
now).

I think that at this point it is better to generalise the data part too. We are
not storing MAC addresses only anymore.

For IPv4 we have data = { mac_addr }
For IPv6 now we have data = { mac_addr, extra_stuff }.

(and in the future we might have something else).
I thought about not generalising the data field, but if we are going to
introduce new (and IPv6 specific) fields than we have to do it anyway.

I think that having a generic void *data and data_size where we can store the
struct we want is much cleaner.

What do you think? you think it is a too big work? In this case we could leave
as you implemented it here and generalise later...Actually you "only" have to
bring the mac_addr field inside the extra_data and rename extra_data to data.

> 
> Comments:
> For now router and override are initialized to the values used in most
> case scenarios. This can be changed easily if we want to avoid the
> NA creation until we get the flags.

when do we get this flags? when we create the entry don't we get the flags too
from the snooped packet? (sorry but I don't entirely know the ND protocol).

> Also, the router flag can be calculated easily using the Router Advertisement.
> Any node that sends that package is a router, so there isn't a lot of snooping
> in that case. This feature can be added easily.
> The problem is that I have no other idea how to get the override flag,
> with the exception of the current implemented mechanism.
> 
> Signed-off-by: Mihail Costea <mihail.costea90@gmail.com>
> Signed-off-by: Stefan Popa <Stefan.A.Popa@intel.com>
> Reviewed-by: Stefan Popa <Stefan.A.Popa@intel.com>
> 
> ---
>  distributed-arp-table.c |  139 +++++++++++++++++++++++++++++++++++------------
>  types.h                 |   21 ++++++-
>  2 files changed, 124 insertions(+), 36 deletions(-)
> 
> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
> index 1a5749b..ce5c13d 100644
> --- a/distributed-arp-table.c
> +++ b/distributed-arp-table.c
> @@ -39,7 +39,8 @@ static bool batadv_dat_snoop_arp_pkt(struct batadv_priv *bat_priv,
>  				     struct sk_buff *skb, int hdr_size,
>  				     uint16_t pkt_type, uint8_t pkt_dir_type,
>  				     uint8_t **hw_src, void **ip_src,
> -				     uint8_t **hw_dst, void **ip_dst);
> +				     uint8_t **hw_dst, void **ip_dst,
> +				     void **extra_data);
>  
>  static struct sk_buff *
>  batadv_dat_create_arp_reply(struct batadv_priv *bat_priv,
> @@ -56,7 +57,8 @@ static bool batadv_dat_snoop_ndisc_pkt(struct batadv_priv *bat_priv,
>  				       struct sk_buff *skb, int hdr_size,
>  				       uint16_t pkt_type, uint8_t pkt_dir_type,
>  				       uint8_t **hw_src, void **ip_src,
> -				       uint8_t **hw_dst, void **ip_dst);
> +				       uint8_t **hw_dst, void **ip_dst,
> +				       void **extra_data);
>  
>  static struct sk_buff *
>  batadv_dat_create_ndisc_na(struct batadv_priv *bat_priv,
> @@ -68,6 +70,7 @@ batadv_dat_create_ndisc_na(struct batadv_priv *bat_priv,
>  static struct batadv_dat_type_info batadv_dat_types_info[] = {
>  	{
>  		.size = sizeof(__be32),
> +		.extra_size = 0,
>  		.str_fmt = "%pI4",
>  		.snoop_pkt = batadv_dat_snoop_arp_pkt,
>  		.create_skb = batadv_dat_create_arp_reply,
> @@ -75,6 +78,7 @@ static struct batadv_dat_type_info batadv_dat_types_info[] = {
>  #if IS_ENABLED(CONFIG_IPV6)
>  	{
>  		.size = sizeof(struct in6_addr),
> +		.extra_size = sizeof(struct batadv_dat_ipv6_extra_data),
>  		.str_fmt = "%pI6c",
>  		.snoop_pkt = batadv_dat_snoop_ndisc_pkt,
>  		.create_skb = batadv_dat_create_ndisc_na,
> @@ -119,6 +123,7 @@ static void batadv_dat_entry_free_ref_rcu(struct rcu_head *rcu)
>  
>  	dat_entry = container_of(rcu, struct batadv_dat_entry, rcu);
>  	kfree(dat_entry->data);
> +	kfree(dat_entry->extra_data);
>  	kfree(dat_entry);
>  }
>  
> @@ -363,18 +368,20 @@ batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, void *data,
>   * batadv_dat_entry_add - add a new dat entry or update it if already exists
>   * @bat_priv: the bat priv with all the soft interface information
>   * @data: the data to add/edit
> + * @extra_data: the extra data for data param (can be NULL if none)
>   * @data_type: type of the data added to DAT
>   * @mac_addr: mac address to assign to the given data
>   * @vid: VLAN identifier
>   */
>  static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *data,
> -				 uint8_t data_type, uint8_t *mac_addr,
> -				 unsigned short vid)
> +				 void *extra_data, uint8_t data_type,
> +				 uint8_t *mac_addr, unsigned short vid)
>  {
>  	struct batadv_dat_entry *dat_entry;
>  	int hash_added;
>  	char dbg_data[BATADV_DAT_DATA_MAX_LEN];
>  	size_t data_size = batadv_dat_types_info[data_type].size;
> +	size_t extra_data_size = batadv_dat_types_info[data_type].extra_size;
>  
>  	dat_entry = batadv_dat_entry_hash_find(bat_priv, data, data_type, vid);
>  	/* if this entry is already known, just update it */
> @@ -382,22 +389,40 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *data,
>  		if (!batadv_compare_eth(dat_entry->mac_addr, mac_addr))
>  			memcpy(dat_entry->mac_addr, mac_addr, ETH_ALEN);
>  		dat_entry->last_update = jiffies;
> -		batadv_dbg(BATADV_DBG_DAT, bat_priv, "Entry updated: %s %pM (vid: %u)\n",
> +		if (extra_data)
> +			memcpy(dat_entry->extra_data, extra_data,
> +			       extra_data_size);
> +
> +		batadv_dbg(BATADV_DBG_DAT, bat_priv,
> +			   "Entry updated: %s %pM (vid: %u)\n",
>  			   batadv_dat_data_to_str(dat_entry->data, data_type,
>  						  dbg_data, sizeof(dbg_data)),
>  			   dat_entry->mac_addr, vid);
>  		goto out;
>  	}
>  
> +	/* alloc entry */
>  	dat_entry = kmalloc(sizeof(*dat_entry), GFP_ATOMIC);
>  	if (!dat_entry)
>  		goto out;
> +	/* some entries don't have an extra data and useful if allocation for
> +	 * data fails */

this comment has to be indented

/* like
 * this
 */



There are other style issues in this patch, but they mostly concern what I
already pointed out in the other comments.

Remember to always check your patch with checkpatch.pl --strict in order to find
problems before sending the patches.


But overall is a very good job! I think we are close to the real patch series ;)

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]