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

Mihail Costea
2013-08-14T13:51:32Z

On 10 August 2013 06:20, Antonio Quartulli <ordex@autistici.org> wrote:
> 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.
>

I agree with calling IPs keys and mac + extra stuff to become generic
data. Also that generic data
should have its own struct if it has more than 1 member.

>>
>> 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).
>

So both router flag and override flag are always sent with NA package.
That means when
we snoop a NA we also get those.
For router, we can get that flag if we snoop NDP router packages.

For override flag, I think it might be possible to calculate it when
we create NA. Unfortunately I didn't
understand well how ndisc code was calculating it so I couldn't port
it here. Override flag must NOT be set
for proxy advertisements and anycast addresses. I don't really
understand what proxy advertisements are
and how anycast addresses are calculated. For anycast address I'm
pretty sure we should be able to calculate it,
because from what I understand that address is used by more different
nodes, so it should have information
known by more nodes.

>> 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.
>

I still don't know what generated some style problems. Alignments
problems shouldn't be here because I already
used --strict (but that didn't say anything about comments) and sent
the patches with git mail. That is weird.

>
> 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

Thanks,
Mihail