[B.A.T.M.A.N.] [RFC 1/6] batman-adv: Generalize DAT in order to support any type of data, not only IPv4
From: Mihail Costea mihail.costea90@gmail.com
Mades DAT support more types by making its data a void*, adding type field to dat_entry and adding data_type to necessary functions. This change is needed in order to make DAT support any type of data, like IPv6 too.
Adds generic function for transforming DAT data to string. The function is used in order to avoid defining different debug messages for different DAT data types. For example, if we had IPv6 as a DAT data, then "%pI4" should be "%pI6c", but all the other text of the debug message would be the same. Also everything is memorized in a struct in order to avoid further switch cases for all types.
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 | 197 +++++++++++++++++++++++++++++++++++------------ distributed-arp-table.h | 1 + types.h | 24 +++++- 3 files changed, 169 insertions(+), 53 deletions(-)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index f2543c2..90565d0 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -31,9 +31,32 @@ #include "types.h" #include "translation-table.h"
+static struct batadv_dat_type_info batadv_dat_types_info[] = { + { + .size = sizeof(__be32), + .str_fmt = "%pI4", + }, +}; + static void batadv_dat_purge(struct work_struct *work);
/** + * batadv_dat_data_to_str: transforms DAT data to string + * @data: the DAT data + * @type: type of data + * @buf: the buf where the data string is stored + * @buf_len: buf length + * + * Returns buf. + */ +static char *batadv_dat_data_to_str(void *data, uint8_t type, + char *buf, size_t buf_len) +{ + snprintf(buf, buf_len, batadv_dat_types_info[type].str_fmt, data); +return buf; +} + +/** * batadv_dat_start_timer - initialise the DAT periodic worker * @bat_priv: the bat priv with all the soft interface information */ @@ -45,6 +68,19 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv) }
/** + * batadv_dat_entry_free_ref_rcu - free a dat entry using its rcu + * @rcu: the dat entry rcu + */ +static void batadv_dat_entry_free_ref_rcu(struct rcu_head *rcu) +{ + struct batadv_dat_entry *dat_entry; + + dat_entry = container_of(rcu, struct batadv_dat_entry, rcu); + kfree(dat_entry->data); + kfree(dat_entry); +} + +/** * batadv_dat_entry_free_ref - decrement the dat_entry refcounter and possibly * free it * @dat_entry: the entry to free @@ -52,7 +88,7 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv) static void batadv_dat_entry_free_ref(struct batadv_dat_entry *dat_entry) { if (atomic_dec_and_test(&dat_entry->refcount)) - kfree_rcu(dat_entry, rcu); + call_rcu(&dat_entry->rcu, batadv_dat_entry_free_ref_rcu); }
/** @@ -136,12 +172,21 @@ static void batadv_dat_purge(struct work_struct *work) * * Returns 1 if the two entries are the same, 0 otherwise. */ -static int batadv_compare_dat(const struct hlist_node *node, const void *data2) +static int batadv_compare_dat(const struct hlist_node *node, const void *data2) { - const void *data1 = container_of(node, struct batadv_dat_entry, - hash_entry); + struct batadv_dat_entry *dat_entry1 = + container_of(node, struct batadv_dat_entry, + hash_entry); + struct batadv_dat_entry *dat_entry2 = + container_of(data2, + struct batadv_dat_entry, data); + size_t data_size = batadv_dat_types_info[dat_entry1->type].size;
- return (memcmp(data1, data2, sizeof(__be32)) == 0 ? 1 : 0); + if (dat_entry1->type != dat_entry2->type) + return 0; + + return (memcmp(dat_entry1->data, dat_entry2->data, + data_size) == 0 ? 1 : 0); }
/** @@ -198,8 +243,9 @@ static __be32 batadv_arp_ip_dst(struct sk_buff *skb, int hdr_size) }
/** - * batadv_hash_dat - compute the hash value for an IP address + * batadv_hash_dat - compute the hash value for a DAT data * @data: data to hash + * @data_type: type of data * @size: size of the hash table * * Returns the selected index in the hash table for the given data. @@ -209,7 +255,8 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size) uint32_t hash = 0; const struct batadv_dat_entry *dat = data;
- hash = batadv_hash_bytes(hash, &dat->ip, sizeof(dat->ip)); + hash = batadv_hash_bytes(hash, dat->data, + batadv_dat_types_info[dat->type].size); hash = batadv_hash_bytes(hash, &dat->vid, sizeof(dat->vid));
hash += (hash << 3); @@ -223,32 +270,40 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size) * batadv_dat_entry_hash_find - look for a given dat_entry in the local hash * table * @bat_priv: the bat priv with all the soft interface information - * @ip: search key + * @data: search key + * @data_type: type of data * @vid: VLAN identifier * * Returns the dat_entry if found, NULL otherwise. */ static struct batadv_dat_entry * -batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip, - unsigned short vid) +batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, void *data, + uint8_t data_type, unsigned short vid) { struct hlist_head *head; struct batadv_dat_entry to_find, *dat_entry, *dat_entry_tmp = NULL; struct batadv_hashtable *hash = bat_priv->dat.hash; - uint32_t index; + uint32_t index, data_size = batadv_dat_types_info[data_type].size;
if (!hash) return NULL;
- to_find.ip = ip; + to_find.data = kmalloc(data_size, GFP_ATOMIC); + if (!to_find.data) + return NULL; + memcpy(to_find.data, data, data_size); + to_find.type = data_type; to_find.vid = vid;
index = batadv_hash_dat(&to_find, hash->size); head = &hash->table[index]; + kfree(to_find.data);
rcu_read_lock(); hlist_for_each_entry_rcu(dat_entry, head, hash_entry) { - if (dat_entry->ip != ip) + if (dat_entry->type != data_type) + continue; + if (memcmp(dat_entry->data, data, data_size)) continue;
if (!atomic_inc_not_zero(&dat_entry->refcount)) @@ -265,25 +320,30 @@ batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip, /** * 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 - * @ip: ipv4 to add/edit - * @mac_addr: mac address to assign to the given ipv4 + * @data: the data to add/edit + * @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, __be32 ip, - uint8_t *mac_addr, unsigned short vid) +static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *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;
- dat_entry = batadv_dat_entry_hash_find(bat_priv, ip, vid); + dat_entry = batadv_dat_entry_hash_find(bat_priv, data, data_type, vid); /* if this entry is already known, just update it */ if (dat_entry) { 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: %pI4 %pM (vid: %u)\n", - &dat_entry->ip, dat_entry->mac_addr, vid); + 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; }
@@ -291,7 +351,12 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip, if (!dat_entry) goto out;
- dat_entry->ip = ip; + dat_entry->data = kmalloc(data_size, GFP_ATOMIC); + if (!dat_entry->data) + goto out; + memcpy(dat_entry->data, data, data_size); + + dat_entry->type = data_type; dat_entry->vid = vid; memcpy(dat_entry->mac_addr, mac_addr, ETH_ALEN); dat_entry->last_update = jiffies; @@ -307,8 +372,10 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip, goto out; }
- batadv_dbg(BATADV_DBG_DAT, bat_priv, "New entry added: %pI4 %pM (vid: %u)\n", - &dat_entry->ip, dat_entry->mac_addr, vid); + batadv_dbg(BATADV_DBG_DAT, bat_priv, "New entry added: %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);
out: if (dat_entry) @@ -520,7 +587,8 @@ static void batadv_choose_next_candidate(struct batadv_priv *bat_priv, * batadv_dat_select_candidates - select the nodes which the DHT message has to * be sent to * @bat_priv: the bat priv with all the soft interface information - * @ip_dst: ipv4 to look up in the DHT + * @data: data to look up in the DHT + * @data_type: type of data * * An originator O is selected if and only if its DHT_ID value is one of three * closest values (from the LEFT, with wrap around if needed) then the hash @@ -529,11 +597,15 @@ static void batadv_choose_next_candidate(struct batadv_priv *bat_priv, * Returns the candidate array of size BATADV_DAT_CANDIDATE_NUM. */ static struct batadv_dat_candidate * -batadv_dat_select_candidates(struct batadv_priv *bat_priv, __be32 ip_dst) +batadv_dat_select_candidates(struct batadv_priv *bat_priv, void *data, + uint8_t data_type) { int select; - batadv_dat_addr_t last_max = BATADV_DAT_ADDR_MAX, ip_key; + batadv_dat_addr_t last_max = BATADV_DAT_ADDR_MAX, data_key; struct batadv_dat_candidate *res; + struct batadv_dat_entry to_find; + char dbg_data[BATADV_DAT_DATA_MAX_LEN]; + size_t data_size = batadv_dat_types_info[data_type].size;
if (!bat_priv->orig_hash) return NULL; @@ -542,15 +614,23 @@ batadv_dat_select_candidates(struct batadv_priv *bat_priv, __be32 ip_dst) if (!res) return NULL;
- ip_key = (batadv_dat_addr_t)batadv_hash_dat(&ip_dst, - BATADV_DAT_ADDR_MAX); + to_find.data = kmalloc(data_size, GFP_ATOMIC); + if (!to_find.data) + return NULL; + memcpy(to_find.data, data, data_size); + to_find.type = data_type; + data_key = (batadv_dat_addr_t)batadv_hash_dat(&to_find, + BATADV_DAT_ADDR_MAX); + kfree(to_find.data);
batadv_dbg(BATADV_DBG_DAT, bat_priv, - "dat_select_candidates(): IP=%pI4 hash(IP)=%u\n", &ip_dst, - ip_key); + "dat_select_candidates(): DATA=%s hash(DATA)=%u\n", + batadv_dat_data_to_str(data, data_type, dbg_data, + sizeof(dbg_data)), + data_key);
for (select = 0; select < BATADV_DAT_CANDIDATES_NUM; select++) - batadv_choose_next_candidate(bat_priv, res, select, ip_key, + batadv_choose_next_candidate(bat_priv, res, select, data_key, &last_max);
return res; @@ -560,7 +640,8 @@ batadv_dat_select_candidates(struct batadv_priv *bat_priv, __be32 ip_dst) * batadv_dat_send_data - send a payload to the selected candidates * @bat_priv: the bat priv with all the soft interface information * @skb: payload to send - * @ip: the DHT key + * @data: the DHT key + * @data_type: type of data * @packet_subtype: unicast4addr packet subtype to use * * This function copies the skb with pskb_copy() and is sent as unicast packet @@ -570,8 +651,8 @@ batadv_dat_select_candidates(struct batadv_priv *bat_priv, __be32 ip_dst) * otherwise. */ static bool batadv_dat_send_data(struct batadv_priv *bat_priv, - struct sk_buff *skb, __be32 ip, - int packet_subtype) + struct sk_buff *skb, void *data, + uint8_t data_type, int packet_subtype) { int i; bool ret = false; @@ -579,12 +660,15 @@ static bool batadv_dat_send_data(struct batadv_priv *bat_priv, struct batadv_neigh_node *neigh_node = NULL; struct sk_buff *tmp_skb; struct batadv_dat_candidate *cand; + char dbg_data[BATADV_DAT_DATA_MAX_LEN];
- cand = batadv_dat_select_candidates(bat_priv, ip); + cand = batadv_dat_select_candidates(bat_priv, data, data_type); if (!cand) goto out;
- batadv_dbg(BATADV_DBG_DAT, bat_priv, "DHT_SEND for %pI4\n", &ip); + batadv_dbg(BATADV_DBG_DAT, bat_priv, "DHT_SEND for %s\n", + batadv_dat_data_to_str(data, data_type, dbg_data, + sizeof(dbg_data)));
for (i = 0; i < BATADV_DAT_CANDIDATES_NUM; i++) { if (cand[i].type == BATADV_DAT_CANDIDATE_NOT_FOUND) @@ -754,6 +838,7 @@ int batadv_dat_cache_seq_print_text(struct seq_file *seq, void *offset) unsigned long last_seen_jiffies; int last_seen_msecs, last_seen_secs, last_seen_mins; uint32_t i; + char dbg_data[BATADV_DAT_DATA_MAX_LEN];
primary_if = batadv_seq_print_text_primary_if_get(seq); if (!primary_if) @@ -774,8 +859,12 @@ int batadv_dat_cache_seq_print_text(struct seq_file *seq, void *offset) last_seen_msecs = last_seen_msecs % 60000; last_seen_secs = last_seen_msecs / 1000;
- seq_printf(seq, " * %15pI4 %14pM %6i:%02i\n", - &dat_entry->ip, dat_entry->mac_addr, + seq_printf(seq, " * %15s %14pM %6i:%02i\n", + batadv_dat_data_to_str(dat_entry->data, + dat_entry->type, + dbg_data, + sizeof(dbg_data)), + dat_entry->mac_addr, last_seen_mins, last_seen_secs); } rcu_read_unlock(); @@ -926,9 +1015,10 @@ bool batadv_dat_snoop_outgoing_arp_request(struct batadv_priv *bat_priv, hw_src = batadv_arp_hw_src(skb, 0); ip_dst = batadv_arp_ip_dst(skb, 0);
- batadv_dat_entry_add(bat_priv, ip_src, hw_src, vid); + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src, vid);
- dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst, vid); + dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst, + BATADV_DAT_IPV4, vid); if (dat_entry) { /* If the ARP request is destined for a local client the local * client will answer itself. DAT would only generate a @@ -962,7 +1052,8 @@ bool batadv_dat_snoop_outgoing_arp_request(struct batadv_priv *bat_priv, ret = true; } else { /* Send the request to the DHT */ - ret = batadv_dat_send_data(bat_priv, skb, ip_dst, + ret = batadv_dat_send_data(bat_priv, skb, &ip_dst, + BATADV_DAT_IPV4, BATADV_P_DAT_DHT_GET); } out: @@ -1008,9 +1099,10 @@ bool batadv_dat_snoop_incoming_arp_request(struct batadv_priv *bat_priv, batadv_dbg_arp(bat_priv, skb, type, hdr_size, "Parsing incoming ARP REQUEST");
- batadv_dat_entry_add(bat_priv, ip_src, hw_src, vid); + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src, vid);
- dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst, vid); + dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst, + BATADV_DAT_IPV4, vid); if (!dat_entry) goto out;
@@ -1074,14 +1166,16 @@ void batadv_dat_snoop_outgoing_arp_reply(struct batadv_priv *bat_priv, hw_dst = batadv_arp_hw_dst(skb, hdr_size); ip_dst = batadv_arp_ip_dst(skb, hdr_size);
- batadv_dat_entry_add(bat_priv, ip_src, hw_src, vid); - batadv_dat_entry_add(bat_priv, ip_dst, hw_dst, vid); + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src, vid); + batadv_dat_entry_add(bat_priv, &ip_dst, BATADV_DAT_IPV4, hw_dst, vid);
/* Send the ARP reply to the candidates for both the IP addresses that * the node obtained from the ARP reply */ - batadv_dat_send_data(bat_priv, skb, ip_src, BATADV_P_DAT_DHT_PUT); - batadv_dat_send_data(bat_priv, skb, ip_dst, BATADV_P_DAT_DHT_PUT); + batadv_dat_send_data(bat_priv, skb, &ip_src, BATADV_DAT_IPV4, + BATADV_P_DAT_DHT_PUT); + batadv_dat_send_data(bat_priv, skb, &ip_dst, BATADV_DAT_IPV4, + BATADV_P_DAT_DHT_PUT); } /** * batadv_dat_snoop_incoming_arp_reply - snoop the ARP reply and fill the local @@ -1119,8 +1213,8 @@ bool batadv_dat_snoop_incoming_arp_reply(struct batadv_priv *bat_priv, /* Update our internal cache with both the IP addresses the node got * within the ARP reply */ - batadv_dat_entry_add(bat_priv, ip_src, hw_src, vid); - batadv_dat_entry_add(bat_priv, ip_dst, hw_dst, vid); + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src, vid); + batadv_dat_entry_add(bat_priv, &ip_dst, BATADV_DAT_IPV4, hw_dst, vid);
/* if this REPLY is directed to a client of mine, let's deliver the * packet to the interface @@ -1167,7 +1261,8 @@ bool batadv_dat_drop_broadcast_packet(struct batadv_priv *bat_priv, goto out;
ip_dst = batadv_arp_ip_dst(forw_packet->skb, hdr_size); - dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst, vid); + dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst, + BATADV_DAT_IPV4, vid); /* check if the node already got this entry */ if (!dat_entry) { batadv_dbg(BATADV_DBG_DAT, bat_priv, diff --git a/distributed-arp-table.h b/distributed-arp-table.h index 60d853b..557bab9 100644 --- a/distributed-arp-table.h +++ b/distributed-arp-table.h @@ -28,6 +28,7 @@ #include <linux/if_arp.h>
#define BATADV_DAT_ADDR_MAX ((batadv_dat_addr_t)~(batadv_dat_addr_t)0) +#define BATADV_DAT_DATA_MAX_LEN 16
void batadv_dat_status_update(struct net_device *net_dev); bool batadv_dat_snoop_outgoing_arp_request(struct batadv_priv *bat_priv, diff --git a/types.h b/types.h index 20a1bef..69c187e 100644 --- a/types.h +++ b/types.h @@ -931,7 +931,8 @@ struct batadv_algo_ops { /** * struct batadv_dat_entry - it is a single entry of batman-adv ARP backend. It * is used to stored ARP entries needed for the global DAT cache - * @ip: the IPv4 corresponding to this DAT/ARP entry + * @data: the data corresponding to this DAT entry + * @type: the type corresponding to this DAT entry * @mac_addr: the MAC address associated to the stored IPv4 * @vid: the vlan ID associated to this entry * @last_update: time in jiffies when this entry was refreshed last time @@ -940,7 +941,8 @@ struct batadv_algo_ops { * @rcu: struct used for freeing in an RCU-safe manner */ struct batadv_dat_entry { - __be32 ip; + void *data; + uint8_t type; uint8_t mac_addr[ETH_ALEN]; unsigned short vid; unsigned long last_update; @@ -950,6 +952,24 @@ struct batadv_dat_entry { };
/** + * batadv_dat_types - types used in batadv_dat_entry for IP + * @BATADV_DAT_IPv4: IPv4 address type + */ +enum batadv_dat_types { + BATADV_DAT_IPV4 = 0, +}; + +/** + * batadv_dat_type_info - info needed for a DAT type data + * @size: the size of the type data + * @str_fmt: string format used by the data + */ +struct batadv_dat_type_info { + size_t size; + char *str_fmt; +}; + +/** * struct batadv_dat_candidate - candidate destination for DAT operations * @type: the type of the selected candidate. It can one of the following: * - BATADV_DAT_CANDIDATE_NOT_FOUND
From: Mihail Costea mihail.costea90@gmail.com
Renames snooping functions in order to suggest that they should work on more protocols than ARP.
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 | 16 ++++++++-------- distributed-arp-table.h | 16 ++++++++-------- routing.c | 8 ++++---- soft-interface.c | 6 +++--- 4 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index 90565d0..b2a4fe5 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -976,7 +976,7 @@ static unsigned short batadv_dat_get_vid(struct sk_buff *skb, int *hdr_size) }
/** - * batadv_dat_snoop_outgoing_arp_request - snoop the ARP request and try to + * batadv_dat_snoop_outgoing_pkt_request - snoop the ARP request and try to * answer using DAT * @bat_priv: the bat priv with all the soft interface information * @skb: packet to check @@ -985,7 +985,7 @@ static unsigned short batadv_dat_get_vid(struct sk_buff *skb, int *hdr_size) * otherwise. In case of a positive return value the message has to be enqueued * to permit the fallback. */ -bool batadv_dat_snoop_outgoing_arp_request(struct batadv_priv *bat_priv, +bool batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, struct sk_buff *skb) { uint16_t type = 0; @@ -1063,7 +1063,7 @@ out: }
/** - * batadv_dat_snoop_incoming_arp_request - snoop the ARP request and try to + * batadv_dat_snoop_incoming_pkt_request - snoop the ARP request and try to * answer using the local DAT storage * @bat_priv: the bat priv with all the soft interface information * @skb: packet to check @@ -1071,7 +1071,7 @@ out: * * Returns true if the request has been answered, false otherwise. */ -bool batadv_dat_snoop_incoming_arp_request(struct batadv_priv *bat_priv, +bool batadv_dat_snoop_incoming_pkt_request(struct batadv_priv *bat_priv, struct sk_buff *skb, int hdr_size) { uint16_t type; @@ -1137,11 +1137,11 @@ out: }
/** - * batadv_dat_snoop_outgoing_arp_reply - snoop the ARP reply and fill the DHT + * batadv_dat_snoop_outgoing_pkt_reply - snoop the ARP reply and fill the DHT * @bat_priv: the bat priv with all the soft interface information * @skb: packet to check */ -void batadv_dat_snoop_outgoing_arp_reply(struct batadv_priv *bat_priv, +void batadv_dat_snoop_outgoing_pkt_reply(struct batadv_priv *bat_priv, struct sk_buff *skb) { uint16_t type; @@ -1178,13 +1178,13 @@ void batadv_dat_snoop_outgoing_arp_reply(struct batadv_priv *bat_priv, BATADV_P_DAT_DHT_PUT); } /** - * batadv_dat_snoop_incoming_arp_reply - snoop the ARP reply and fill the local + * batadv_dat_snoop_incoming_pkt_reply - snoop the ARP reply and fill the local * DAT storage only * @bat_priv: the bat priv with all the soft interface information * @skb: packet to check * @hdr_size: size of the encapsulation header */ -bool batadv_dat_snoop_incoming_arp_reply(struct batadv_priv *bat_priv, +bool batadv_dat_snoop_incoming_pkt_reply(struct batadv_priv *bat_priv, struct sk_buff *skb, int hdr_size) { uint16_t type; diff --git a/distributed-arp-table.h b/distributed-arp-table.h index 557bab9..21d7b24 100644 --- a/distributed-arp-table.h +++ b/distributed-arp-table.h @@ -31,13 +31,13 @@ #define BATADV_DAT_DATA_MAX_LEN 16
void batadv_dat_status_update(struct net_device *net_dev); -bool batadv_dat_snoop_outgoing_arp_request(struct batadv_priv *bat_priv, +bool batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, struct sk_buff *skb); -bool batadv_dat_snoop_incoming_arp_request(struct batadv_priv *bat_priv, +bool batadv_dat_snoop_incoming_pkt_request(struct batadv_priv *bat_priv, struct sk_buff *skb, int hdr_size); -void batadv_dat_snoop_outgoing_arp_reply(struct batadv_priv *bat_priv, +void batadv_dat_snoop_outgoing_pkt_reply(struct batadv_priv *bat_priv, struct sk_buff *skb); -bool batadv_dat_snoop_incoming_arp_reply(struct batadv_priv *bat_priv, +bool batadv_dat_snoop_incoming_pkt_reply(struct batadv_priv *bat_priv, struct sk_buff *skb, int hdr_size); bool batadv_dat_drop_broadcast_packet(struct batadv_priv *bat_priv, struct batadv_forw_packet *forw_packet); @@ -105,28 +105,28 @@ static inline void batadv_dat_status_update(struct net_device *net_dev) }
static inline bool -batadv_dat_snoop_outgoing_arp_request(struct batadv_priv *bat_priv, +batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, struct sk_buff *skb) { return false; }
static inline bool -batadv_dat_snoop_incoming_arp_request(struct batadv_priv *bat_priv, +batadv_dat_snoop_incoming_pkt_request(struct batadv_priv *bat_priv, struct sk_buff *skb, int hdr_size) { return false; }
static inline bool -batadv_dat_snoop_outgoing_arp_reply(struct batadv_priv *bat_priv, +batadv_dat_snoop_outgoing_pkt_reply(struct batadv_priv *bat_priv, struct sk_buff *skb) { return false; }
static inline bool -batadv_dat_snoop_incoming_arp_reply(struct batadv_priv *bat_priv, +batadv_dat_snoop_incoming_pkt_reply(struct batadv_priv *bat_priv, struct sk_buff *skb, int hdr_size) { return false; diff --git a/routing.c b/routing.c index e63b05d..d4fae86 100644 --- a/routing.c +++ b/routing.c @@ -937,10 +937,10 @@ int batadv_recv_unicast_packet(struct sk_buff *skb, orig_node = batadv_orig_hash_find(bat_priv, orig_addr); }
- if (batadv_dat_snoop_incoming_arp_request(bat_priv, skb, + if (batadv_dat_snoop_incoming_pkt_request(bat_priv, skb, hdr_size)) goto rx_success; - if (batadv_dat_snoop_incoming_arp_reply(bat_priv, skb, + if (batadv_dat_snoop_incoming_pkt_reply(bat_priv, skb, hdr_size)) goto rx_success;
@@ -1142,9 +1142,9 @@ int batadv_recv_bcast_packet(struct sk_buff *skb, if (batadv_bla_is_backbone_gw(skb, orig_node, hdr_size)) goto out;
- if (batadv_dat_snoop_incoming_arp_request(bat_priv, skb, hdr_size)) + if (batadv_dat_snoop_incoming_pkt_request(bat_priv, skb, hdr_size)) goto rx_success; - if (batadv_dat_snoop_incoming_arp_reply(bat_priv, skb, hdr_size)) + if (batadv_dat_snoop_incoming_pkt_reply(bat_priv, skb, hdr_size)) goto rx_success;
/* broadcast for me */ diff --git a/soft-interface.c b/soft-interface.c index d897194..d294e36 100644 --- a/soft-interface.c +++ b/soft-interface.c @@ -245,7 +245,7 @@ static int batadv_interface_tx(struct sk_buff *skb, * packet, instead we first wait for DAT to try to retrieve the * correct ARP entry */ - if (batadv_dat_snoop_outgoing_arp_request(bat_priv, skb)) + if (batadv_dat_snoop_outgoing_pkt_request(bat_priv, skb)) brd_delay = msecs_to_jiffies(ARP_REQ_DELAY);
if (batadv_skb_head_push(skb, sizeof(*bcast_packet)) < 0) @@ -284,10 +284,10 @@ static int batadv_interface_tx(struct sk_buff *skb, goto dropped; }
- if (batadv_dat_snoop_outgoing_arp_request(bat_priv, skb)) + if (batadv_dat_snoop_outgoing_pkt_request(bat_priv, skb)) goto dropped;
- batadv_dat_snoop_outgoing_arp_reply(bat_priv, skb); + batadv_dat_snoop_outgoing_pkt_reply(bat_priv, skb);
ret = batadv_send_skb_unicast(bat_priv, skb, vid); if (ret != 0)
From: Mihail Costea mihail.costea90@gmail.com
Adds IPv6 functionality to the generic struct.
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 | 12 +++++++++--- distributed-arp-table.h | 2 +- types.h | 6 +++++- 3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index b2a4fe5..f941913 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -36,6 +36,12 @@ static struct batadv_dat_type_info batadv_dat_types_info[] = { .size = sizeof(__be32), .str_fmt = "%pI4", }, +#if IS_ENABLED(CONFIG_IPV6) + { + .size = sizeof(struct in6_addr), + .str_fmt = "%pI6c", + }, +#endif };
static void batadv_dat_purge(struct work_struct *work); @@ -845,8 +851,8 @@ int batadv_dat_cache_seq_print_text(struct seq_file *seq, void *offset) goto out;
seq_printf(seq, "Distributed ARP Table (%s):\n", net_dev->name); - seq_printf(seq, " %-7s %-13s %5s\n", "IPv4", "MAC", - "last-seen"); + seq_printf(seq, " %-26s %-15s %5s\n", + "IPv4/IPv6", "MAC", "last-seen");
for (i = 0; i < hash->size; i++) { head = &hash->table[i]; @@ -859,7 +865,7 @@ int batadv_dat_cache_seq_print_text(struct seq_file *seq, void *offset) last_seen_msecs = last_seen_msecs % 60000; last_seen_secs = last_seen_msecs / 1000;
- seq_printf(seq, " * %15s %14pM %6i:%02i\n", + seq_printf(seq, " * %40s %15pM %6i:%02i\n", batadv_dat_data_to_str(dat_entry->data, dat_entry->type, dbg_data, diff --git a/distributed-arp-table.h b/distributed-arp-table.h index 21d7b24..d029b4b 100644 --- a/distributed-arp-table.h +++ b/distributed-arp-table.h @@ -28,7 +28,7 @@ #include <linux/if_arp.h>
#define BATADV_DAT_ADDR_MAX ((batadv_dat_addr_t)~(batadv_dat_addr_t)0) -#define BATADV_DAT_DATA_MAX_LEN 16 +#define BATADV_DAT_DATA_MAX_LEN 40
void batadv_dat_status_update(struct net_device *net_dev); bool batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, diff --git a/types.h b/types.h index 69c187e..60d2d64 100644 --- a/types.h +++ b/types.h @@ -953,10 +953,14 @@ struct batadv_dat_entry {
/** * batadv_dat_types - types used in batadv_dat_entry for IP - * @BATADV_DAT_IPv4: IPv4 address type + * @BATADV_DAT_IPV4: IPv4 address type + * @BATADV_DAT_IPV6: IPv6 address type */ enum batadv_dat_types { BATADV_DAT_IPV4 = 0, +#if IS_ENABLED(CONFIG_IPV6) + BATADV_DAT_IPV6 = 1, +#endif };
/**
On Mon, Jul 08, 2013 at 03:12:42AM +0300, mihail.costea2005@gmail.com wrote:
From: Mihail Costea mihail.costea90@gmail.com
Adds IPv6 functionality to the generic struct.
You can be a bit more verbose here :) Not everybody will understand what this patch is meant for.
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 | 12 +++++++++--- distributed-arp-table.h | 2 +- types.h | 6 +++++- 3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index b2a4fe5..f941913 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -36,6 +36,12 @@ static struct batadv_dat_type_info batadv_dat_types_info[] = { .size = sizeof(__be32), .str_fmt = "%pI4", }, +#if IS_ENABLED(CONFIG_IPV6)
- {
.size = sizeof(struct in6_addr),.str_fmt = "%pI6c",- },
+#endif };
static void batadv_dat_purge(struct work_struct *work); @@ -845,8 +851,8 @@ int batadv_dat_cache_seq_print_text(struct seq_file *seq, void *offset) goto out;
seq_printf(seq, "Distributed ARP Table (%s):\n", net_dev->name);
- seq_printf(seq, " %-7s %-13s %5s\n", "IPv4", "MAC",
"last-seen");
- seq_printf(seq, " %-26s %-15s %5s\n",
"IPv4/IPv6", "MAC", "last-seen");
At this point I'd suggest to switch from "IPv4/IPv6" to "Key".
for (i = 0; i < hash->size; i++) { head = &hash->table[i]; @@ -859,7 +865,7 @@ int batadv_dat_cache_seq_print_text(struct seq_file *seq, void *offset) last_seen_msecs = last_seen_msecs % 60000; last_seen_secs = last_seen_msecs / 1000;
seq_printf(seq, " * %15s %14pM %6i:%02i\n",
seq_printf(seq, " * %40s %15pM %6i:%02i\n", batadv_dat_data_to_str(dat_entry->data, dat_entry->type, dbg_data,diff --git a/distributed-arp-table.h b/distributed-arp-table.h index 21d7b24..d029b4b 100644 --- a/distributed-arp-table.h +++ b/distributed-arp-table.h @@ -28,7 +28,7 @@ #include <linux/if_arp.h>
#define BATADV_DAT_ADDR_MAX ((batadv_dat_addr_t)~(batadv_dat_addr_t)0) -#define BATADV_DAT_DATA_MAX_LEN 16 +#define BATADV_DAT_DATA_MAX_LEN 40
void batadv_dat_status_update(struct net_device *net_dev); bool batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, diff --git a/types.h b/types.h index 69c187e..60d2d64 100644 --- a/types.h +++ b/types.h @@ -953,10 +953,14 @@ struct batadv_dat_entry {
/**
- batadv_dat_types - types used in batadv_dat_entry for IP
- @BATADV_DAT_IPv4: IPv4 address type
- @BATADV_DAT_IPV4: IPv4 address type
You should use the correct string in the previous patch already (I think you overlooked the comment while fixing the var names).
*/
- @BATADV_DAT_IPV6: IPv6 address type
enum batadv_dat_types { BATADV_DAT_IPV4 = 0, +#if IS_ENABLED(CONFIG_IPV6)
- BATADV_DAT_IPV6 = 1,
+#endif };
/**
1.7.10.4
From: Mihail Costea mihail.costea90@gmail.com
Adds functions needed for NDP snooping, like getting the IPv6 addresses or getting the target HW address from an Neighbor Advertisement (NA). Also added functions to create NA for Neighbor Solicitations that have already the HW address in DAT.
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 | 319 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 319 insertions(+)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index f941913..d0b9e95 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -20,7 +20,9 @@ #include <linux/if_ether.h> #include <linux/if_arp.h> #include <linux/if_vlan.h> +#include <net/addrconf.h> #include <net/arp.h> +#include <net/ipv6.h>
#include "main.h" #include "hash.h" @@ -981,6 +983,323 @@ static unsigned short batadv_dat_get_vid(struct sk_buff *skb, int *hdr_size) return vid; }
+#if IS_ENABLED(CONFIG_IPV6) +/** + * batadv_ndisc_hw_src - get source hw address from a packet + * @skb: packet to check + * @hdr_size: size of the encapsulation header + * + * Returns source hw address of the skb packet. + */ +static uint8_t *batadv_ndisc_hw_src(struct sk_buff *skb, int hdr_size) +{ + struct ethhdr *ethhdr; + ethhdr = (struct ethhdr *)(skb->data + hdr_size); + return (uint8_t *)ethhdr->h_source; +} + +/** + * batadv_ndisc_hw_dst - get destination hw address from a packet + * @skb: packet to check + * @hdr_size: size of the encapsulation header + * + * Returns destination hw address of the skb packet. + */ +static uint8_t *batadv_ndisc_hw_dst(struct sk_buff *skb, int hdr_size) +{ + struct ethhdr *ethhdr; + ethhdr = (struct ethhdr *)(skb->data + hdr_size); + return (uint8_t *)ethhdr->h_dest; +} + +/** + * batadv_ndisc_ipv6_src - get source IPv6 address from a packet + * @skb: packet to check + * @hdr_size: size of the encapsulation header + * + * Returns source IPv6 address of the skb packet. + */ +static struct in6_addr *batadv_ndisc_ipv6_src(struct sk_buff *skb, + int hdr_size) +{ + struct ipv6hdr *ipv6hdr; + ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN); + return &ipv6hdr->saddr; +} + +/** + * batadv_ndisc_ipv6_dst - get destination IPv6 address from a packet + * @skb: packet to check + * @hdr_size: size of the encapsulation header + * + * Returns destination IPv6 address of the skb packet. + */ +static struct in6_addr *batadv_ndisc_ipv6_dst(struct sk_buff *skb, + int hdr_size) +{ + struct ipv6hdr *ipv6hdr; + ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN); + return &ipv6hdr->daddr; +} + +/** + * batadv_ndisc_ipv6_target - get target IPv6 address from a NS/NA packet + * @skb: packet to check + * @hdr_size: size of the encapsulation header + * + * Returns target IPv6 address of the skb packet. + */ +static struct in6_addr *batadv_ndisc_ipv6_target(struct sk_buff *skb, + int hdr_size) +{ + return (struct in6_addr *)(skb->data + hdr_size + ETH_HLEN + + sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr)); +} + +/** + * batadv_ndisc_hw_opt - get hw address from NS/NA packet options + * @skb: packet to check + * @hdr_size: size of the encapsulation header + * @type: type of the address (ND_OPT_SOURCE_LL_ADDR or ND_OPT_TARGET_LL_ADDR) + * + * The address can be either the source link-layer address + * or the target link-layer address. + * For more info see RFC2461. + * + * Returns hw address from packet options. + */ +static uint8_t *batadv_ndisc_hw_opt(struct sk_buff *skb, int hdr_size, + uint8_t type) +{ + unsigned char *opt_addr; + + opt_addr = skb->data + hdr_size + ETH_HLEN + + sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr) + + sizeof(struct in6_addr); + + /* test if option header is ok */ + if (*opt_addr != type || *(opt_addr + 1) != 1) + return NULL; + return (uint8_t *)(opt_addr + 2); +} + +/** + * batadv_ndisc_get_type - get icmp6 packet type + * @bat_priv: the bat priv with all the soft interface information + * @skb: packet to check + * @hdr_size: size of the encapsulation header + * + * Returns the icmp6 type, or 0 if packet is not a valid icmp6 packet. + */ +static __u8 batadv_ndisc_get_type(struct batadv_priv *bat_priv, + struct sk_buff *skb, int hdr_size) +{ + struct ethhdr *ethhdr; + struct ipv6hdr *ipv6hdr; + struct icmp6hdr *icmp6hdr; + __u8 type = 0; + + /* pull headers */ + if (unlikely(!pskb_may_pull(skb, hdr_size + ETH_HLEN + + sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr)))) + goto out; + + /* get the ethernet header */ + ethhdr = (struct ethhdr *)(skb->data + hdr_size); + if (ethhdr->h_proto != htons(ETH_P_IPV6)) + goto out; + + /* get the ipv6 header */ + ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN); + if (ipv6hdr->nexthdr != IPPROTO_ICMPV6) + goto out; + + /* get the icmpv6 header */ + icmp6hdr = (struct icmp6hdr *)(skb->data + hdr_size + ETH_HLEN + + sizeof(struct ipv6hdr)); + + /* check whether the ndisc packet carries a valid icmp6 header */ + if (ipv6hdr->hop_limit != 255) + goto out; + + if (icmp6hdr->icmp6_code != 0) + goto out; + + type = icmp6hdr->icmp6_type; +out: + return type; +} + +/** + * batadv_ndisc_is_valid - check if a ndisc packet is valid + * @bat_priv: the bat priv with all the soft interface information + * @skb: packet to check + * @hdr_size: size of the encapsulation header + * @ndisc_type: type of ndisc packet to check + * + * Check if all IPs are valid (source, destination, target) and if + * options hw address is valid too. + * Some options might be ignored, like NS packets sent automatically + * for verification of the reachability of a neighbor. + * + * Returns true if packet is valid, otherwise false if invalid or ignored. + */ +static bool batadv_ndisc_is_valid(struct batadv_priv *bat_priv, + struct sk_buff *skb, int hdr_size, + int ndisc_type) +{ + uint8_t *hw_target = NULL; + struct in6_addr *ipv6_src, *ipv6_dst, *ipv6_target; + __u8 type = batadv_ndisc_get_type(bat_priv, skb, hdr_size); + int ndisc_hdr_len = hdr_size + ETH_HLEN + sizeof(struct ipv6hdr) + + sizeof(struct icmp6hdr) + sizeof(struct in6_addr) + + 8; /* ndisc options length */ + + if (type != ndisc_type) + return false; + if (unlikely(!pskb_may_pull(skb, ndisc_hdr_len))) + return false; + + /* Check for bad NA/NS. If the ndisc message is not sane, DAT + * will simply ignore it + */ + if (type == NDISC_NEIGHBOUR_SOLICITATION) + hw_target = batadv_ndisc_hw_opt(skb, hdr_size, + ND_OPT_SOURCE_LL_ADDR); + else if (type == NDISC_NEIGHBOUR_ADVERTISEMENT) + hw_target = batadv_ndisc_hw_opt(skb, hdr_size, + ND_OPT_TARGET_LL_ADDR); + + if (!hw_target || is_zero_ether_addr(hw_target) || + is_multicast_ether_addr(hw_target)) + return false; + + ipv6_src = batadv_ndisc_ipv6_src(skb, hdr_size); + ipv6_dst = batadv_ndisc_ipv6_dst(skb, hdr_size); + ipv6_target = batadv_ndisc_ipv6_target(skb, hdr_size); + if (ipv6_addr_loopback(ipv6_src) || ipv6_addr_loopback(ipv6_dst) || + ipv6_addr_is_multicast(ipv6_src) || + ipv6_addr_is_multicast(ipv6_target)) + return false; + + /* ignore messages with unspecified address */ + if (ipv6_addr_any(ipv6_src) || ipv6_addr_any(ipv6_dst) || + ipv6_addr_any(ipv6_target)) + return false; + + /* ignore the verification of the reachability of a neighbor */ + if (type == NDISC_NEIGHBOUR_SOLICITATION && + !ipv6_addr_is_multicast(ipv6_dst)) + return false; + + return true; +} + +static void batadv_ndisc_ip6_hdr(struct sock *sk, struct sk_buff *skb, + struct net_device *dev, + const struct in6_addr *ipv6_src, + const struct in6_addr *ipv6_dst, + int proto, int len) +{ + struct ipv6_pinfo *np = inet6_sk(sk); + struct ipv6hdr *hdr; + + skb->protocol = htons(ETH_P_IPV6); + skb->dev = dev; + + skb_reset_network_header(skb); + skb_put(skb, sizeof(struct ipv6hdr)); + hdr = ipv6_hdr(skb); + + *(__be32 *)hdr = htonl(0x60000000); + + hdr->payload_len = htons(len); + hdr->nexthdr = proto; + hdr->hop_limit = np->hop_limit; + + hdr->saddr = *ipv6_src; + hdr->daddr = *ipv6_dst; +} + +/** + * batadv_ndisc_create_na - create an NA for a solicited NS + * @net_device: the devices for which the packet is created + * @ipv6_dst: destination IPv6 + * @ipv6_target: target IPv6 (same with source IPv6) + * @hw_dst: destination HW Address + * @hw_target: target HW Address (same with source HW Address) + * @router: 1 if target is a router, else 0 + * @solicited: 1 if this is a solicited NA, else 0 + * @override: 1 if the target entry should be override, else 0 + * + * For more info see RFC2461. + * + * Returns the newly created skb, otherwise NULL. + */ +static struct +sk_buff *batadv_ndisc_create_na(struct net_device *dev, + const struct in6_addr *ipv6_dst, + const struct in6_addr *ipv6_target, + const uint8_t *hw_dst, + const uint8_t *hw_target, + int router, int solicited, int override) +{ + struct net *net = dev_net(dev); + struct sock *sk = net->ipv6.ndisc_sk; + struct sk_buff *skb; + struct icmp6hdr *icmp6hdr; + int hlen = LL_RESERVED_SPACE(dev); + int tlen = dev->needed_tailroom; + int len, err; + u8 *opt; + + /* alloc space for skb */ + len = sizeof(struct icmp6hdr) + sizeof(*ipv6_target) + 8; + skb = sock_alloc_send_skb(sk, + (MAX_HEADER + sizeof(struct ipv6hdr) + + len + hlen + tlen), + 1, &err); + if (!skb) + return NULL; + + skb_reserve(skb, hlen); + batadv_ndisc_ip6_hdr(sk, skb, dev, ipv6_target, ipv6_dst, + IPPROTO_ICMPV6, len); + + skb->transport_header = skb->tail; + skb_put(skb, len); + + /* fill the device header for the NA frame */ + if (dev_hard_header(skb, dev, ETH_P_IPV6, hw_dst, + hw_target, skb->len) < 0) { + kfree_skb(skb); + return NULL; + } + + /* set icmpv6 header */ + icmp6hdr = (struct icmp6hdr *)skb_transport_header(skb); + icmp6hdr->icmp6_type = NDISC_NEIGHBOUR_ADVERTISEMENT; + icmp6hdr->icmp6_router = router; + icmp6hdr->icmp6_solicited = solicited; + icmp6hdr->icmp6_override = override; + + /* set NA target and options */ + opt = skb_transport_header(skb) + sizeof(*icmp6hdr); + *(struct in6_addr *)opt = *ipv6_target; + opt += sizeof(*ipv6_target); + + opt[0] = ND_OPT_TARGET_LL_ADDR; + opt[1] = 1; + memcpy(opt + 2, hw_target, dev->addr_len); + + icmp6hdr->icmp6_cksum = csum_ipv6_magic(ipv6_target, ipv6_dst, len, + IPPROTO_ICMPV6, + csum_partial(icmp6hdr, len, 0)); + + return skb; +} +#endif + /** * batadv_dat_snoop_outgoing_pkt_request - snoop the ARP request and try to * answer using DAT
On Mon, Jul 08, 2013 at 03:12:43AM +0300, mihail.costea2005@gmail.com wrote:
From: Mihail Costea mihail.costea90@gmail.com
Adds functions needed for NDP snooping, like getting the IPv6 addresses or getting the target HW address from an Neighbor Advertisement (NA).
typo: an -> a
Also added functions to create NA for Neighbor Solicitations
use always the same form: added -> adds
that have already the HW address in DAT.
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 | 319 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 319 insertions(+)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index f941913..d0b9e95 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -20,7 +20,9 @@ #include <linux/if_ether.h> #include <linux/if_arp.h> #include <linux/if_vlan.h> +#include <net/addrconf.h> #include <net/arp.h> +#include <net/ipv6.h>
#include "main.h" #include "hash.h" @@ -981,6 +983,323 @@ static unsigned short batadv_dat_get_vid(struct sk_buff *skb, int *hdr_size) return vid; }
+#if IS_ENABLED(CONFIG_IPV6) +/**
- batadv_ndisc_hw_src - get source hw address from a packet
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns source hw address of the skb packet.
- */
+static uint8_t *batadv_ndisc_hw_src(struct sk_buff *skb, int hdr_size) +{
- struct ethhdr *ethhdr;
please put a blank line after variable declarations.
- ethhdr = (struct ethhdr *)(skb->data + hdr_size);
- return (uint8_t *)ethhdr->h_source;
+}
+/**
- batadv_ndisc_hw_dst - get destination hw address from a packet
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns destination hw address of the skb packet.
- */
+static uint8_t *batadv_ndisc_hw_dst(struct sk_buff *skb, int hdr_size) +{
- struct ethhdr *ethhdr;
same here
- ethhdr = (struct ethhdr *)(skb->data + hdr_size);
- return (uint8_t *)ethhdr->h_dest;
+}
+/**
- batadv_ndisc_ipv6_src - get source IPv6 address from a packet
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns source IPv6 address of the skb packet.
- */
+static struct in6_addr *batadv_ndisc_ipv6_src(struct sk_buff *skb,
int hdr_size)+{
- struct ipv6hdr *ipv6hdr;
same here
- ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);
- return &ipv6hdr->saddr;
+}
+/**
- batadv_ndisc_ipv6_dst - get destination IPv6 address from a packet
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns destination IPv6 address of the skb packet.
- */
+static struct in6_addr *batadv_ndisc_ipv6_dst(struct sk_buff *skb,
int hdr_size)+{
- struct ipv6hdr *ipv6hdr;
same here
- ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);
- return &ipv6hdr->daddr;
+}
+/**
- batadv_ndisc_ipv6_target - get target IPv6 address from a NS/NA packet
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns target IPv6 address of the skb packet.
- */
+static struct in6_addr *batadv_ndisc_ipv6_target(struct sk_buff *skb,
int hdr_size)+{
- return (struct in6_addr *)(skb->data + hdr_size + ETH_HLEN +
sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr));
please, use the needed local variables to make this statement more readable and and to align it in a proper way.
+}
+/**
- batadv_ndisc_hw_opt - get hw address from NS/NA packet options
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- @type: type of the address (ND_OPT_SOURCE_LL_ADDR or ND_OPT_TARGET_LL_ADDR)
- The address can be either the source link-layer address
- or the target link-layer address.
- For more info see RFC2461.
- Returns hw address from packet options.
- */
+static uint8_t *batadv_ndisc_hw_opt(struct sk_buff *skb, int hdr_size,
uint8_t type)+{
- unsigned char *opt_addr;
- opt_addr = skb->data + hdr_size + ETH_HLEN +
sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr) +sizeof(struct in6_addr);
align this statement properly. it should be:
like_this = this + that + and_whatever + we_need;
- /* test if option header is ok */
Please, be a bit more verbose in this comment. What are we checking here? why != 1? What does that mean? Otherwise it will be difficult for other people out of these patches business to understand
- if (*opt_addr != type || *(opt_addr + 1) != 1)
return NULL;- return (uint8_t *)(opt_addr + 2);
and why skipping the initial two bytes? Maybe you should describe what opt_addr contains? this will probably help to better understand what this statement is doing.
+}
+/**
- batadv_ndisc_get_type - get icmp6 packet type
I think you can directly call this function "batadv_icmpv6_get_type". It may be re-used in the future for the same purpose but somewhere else :)
- @bat_priv: the bat priv with all the soft interface information
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns the icmp6 type, or 0 if packet is not a valid icmp6 packet.
- */
+static __u8 batadv_ndisc_get_type(struct batadv_priv *bat_priv,
in the batman-adv code we use stdint: __u8 -> uint8_t (there are other spots where you use __8)
struct sk_buff *skb, int hdr_size)+{
- struct ethhdr *ethhdr;
- struct ipv6hdr *ipv6hdr;
- struct icmp6hdr *icmp6hdr;
- __u8 type = 0;
- /* pull headers */
- if (unlikely(!pskb_may_pull(skb, hdr_size + ETH_HLEN +
sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr))))
please properly align this statement too. I think checkpatch would have complained about this. You can use tabs + spaces to correctly align.
goto out;- /* get the ethernet header */
remove this comment :) Variables are named properly and this is obvious.
- ethhdr = (struct ethhdr *)(skb->data + hdr_size);
- if (ethhdr->h_proto != htons(ETH_P_IPV6))
goto out;- /* get the ipv6 header */
same
- ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);
- if (ipv6hdr->nexthdr != IPPROTO_ICMPV6)
goto out;- /* get the icmpv6 header */
- icmp6hdr = (struct icmp6hdr *)(skb->data + hdr_size + ETH_HLEN +
sizeof(struct ipv6hdr));
alignment..should line up to the opening parenthesis.
(blablabla *)(like this);
- /* check whether the ndisc packet carries a valid icmp6 header */
- if (ipv6hdr->hop_limit != 255)
goto out;- if (icmp6hdr->icmp6_code != 0)
goto out;- type = icmp6hdr->icmp6_type;
+out:
- return type;
+}
+/**
- batadv_ndisc_is_valid - check if a ndisc packet is valid
- @bat_priv: the bat priv with all the soft interface information
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- @ndisc_type: type of ndisc packet to check
- Check if all IPs are valid (source, destination, target) and if
- options hw address is valid too.
- Some options might be ignored, like NS packets sent automatically
- for verification of the reachability of a neighbor.
- Returns true if packet is valid, otherwise false if invalid or ignored.
- */
+static bool batadv_ndisc_is_valid(struct batadv_priv *bat_priv,
struct sk_buff *skb, int hdr_size,int ndisc_type)+{
- uint8_t *hw_target = NULL;
- struct in6_addr *ipv6_src, *ipv6_dst, *ipv6_target;
- __u8 type = batadv_ndisc_get_type(bat_priv, skb, hdr_size);
- int ndisc_hdr_len = hdr_size + ETH_HLEN + sizeof(struct ipv6hdr) +
sizeof(struct icmp6hdr) + sizeof(struct in6_addr) +8; /* ndisc options length */
when the assignment is too long, please do it after the declarations. Improves readability.
- if (type != ndisc_type)
return false;- if (unlikely(!pskb_may_pull(skb, ndisc_hdr_len)))
return false;- /* Check for bad NA/NS. If the ndisc message is not sane, DAT
* will simply ignore it*/- if (type == NDISC_NEIGHBOUR_SOLICITATION)
hw_target = batadv_ndisc_hw_opt(skb, hdr_size,ND_OPT_SOURCE_LL_ADDR);- else if (type == NDISC_NEIGHBOUR_ADVERTISEMENT)
hw_target = batadv_ndisc_hw_opt(skb, hdr_size,ND_OPT_TARGET_LL_ADDR);- if (!hw_target || is_zero_ether_addr(hw_target) ||
is_multicast_ether_addr(hw_target))return false;- ipv6_src = batadv_ndisc_ipv6_src(skb, hdr_size);
- ipv6_dst = batadv_ndisc_ipv6_dst(skb, hdr_size);
- ipv6_target = batadv_ndisc_ipv6_target(skb, hdr_size);
- if (ipv6_addr_loopback(ipv6_src) || ipv6_addr_loopback(ipv6_dst) ||
ipv6_addr_is_multicast(ipv6_src) ||ipv6_addr_is_multicast(ipv6_target))return false;- /* ignore messages with unspecified address */
- if (ipv6_addr_any(ipv6_src) || ipv6_addr_any(ipv6_dst) ||
ipv6_addr_any(ipv6_target))return false;- /* ignore the verification of the reachability of a neighbor */
- if (type == NDISC_NEIGHBOUR_SOLICITATION &&
!ipv6_addr_is_multicast(ipv6_dst))return false;- return true;
+}
+static void batadv_ndisc_ip6_hdr(struct sock *sk, struct sk_buff *skb,
struct net_device *dev,const struct in6_addr *ipv6_src,const struct in6_addr *ipv6_dst,int proto, int len)
alignment?
+{
- struct ipv6_pinfo *np = inet6_sk(sk);
- struct ipv6hdr *hdr;
- skb->protocol = htons(ETH_P_IPV6);
- skb->dev = dev;
- skb_reset_network_header(skb);
- skb_put(skb, sizeof(struct ipv6hdr));
- hdr = ipv6_hdr(skb);
- *(__be32 *)hdr = htonl(0x60000000);
What does this constant mean? you are writing on the IPv6 header? why not writing in one of its fields (I guess you wanted to write in the first one)?
- hdr->payload_len = htons(len);
I think len can be declared int16_t to avoid using wrong values? (here and where you call this function)
- hdr->nexthdr = proto;
- hdr->hop_limit = np->hop_limit;
- hdr->saddr = *ipv6_src;
- hdr->daddr = *ipv6_dst;
+}
+/**
- batadv_ndisc_create_na - create an NA for a solicited NS
- @net_device: the devices for which the packet is created
- @ipv6_dst: destination IPv6
- @ipv6_target: target IPv6 (same with source IPv6)
- @hw_dst: destination HW Address
- @hw_target: target HW Address (same with source HW Address)
- @router: 1 if target is a router, else 0
- @solicited: 1 if this is a solicited NA, else 0
- @override: 1 if the target entry should be override, else 0
- For more info see RFC2461.
If you want to cite the RFC think you should provide also a section? The "Neighbor Discovery for IP Version 6" RFC is a bit too much is I only want to understand what a NA or NS is and how the NA is created.
By the way, ok for reading the RFC, but I think you can write a bit more about what the function is doing: e.g. create a new skb containing an NA which fields are initialised with the value passed as argument. Seems obvious, but better safe than sorry :) Kernel Doc is shown without the code if you compile it.
- Returns the newly created skb, otherwise NULL.
- */
+static struct +sk_buff *batadv_ndisc_create_na(struct net_device *dev,
const struct in6_addr *ipv6_dst,const struct in6_addr *ipv6_target,const uint8_t *hw_dst,const uint8_t *hw_target,int router, int solicited, int override)
if these variables can only be 0 or 1, why not using bool?
+{
- struct net *net = dev_net(dev);
- struct sock *sk = net->ipv6.ndisc_sk;
- struct sk_buff *skb;
- struct icmp6hdr *icmp6hdr;
- int hlen = LL_RESERVED_SPACE(dev);
- int tlen = dev->needed_tailroom;
- int len, err;
- u8 *opt;
uint8_t
- /* alloc space for skb */
- len = sizeof(struct icmp6hdr) + sizeof(*ipv6_target) + 8;
write in the comment what is this 8. Otherwise, since you use it more than once, create a define with a descriptive name and it instead of the 8.
- skb = sock_alloc_send_skb(sk,
(MAX_HEADER + sizeof(struct ipv6hdr) +
why these parenthesis here?
len + hlen + tlen),
I guess hlen is ETH_HLEN? what does LL_RESERVED_SPACE exactly return? and why using needed_tailroom? what does it comport in this case? We never used that during SKB allocation...this is why I am asking.
1, &err);
and why are you using this function to allocate the skb? In the rest of the code we always use netdev_alloc_skb_ip_align() (that also takes care of properly aligning the skb).
- if (!skb)
return NULL;- skb_reserve(skb, hlen);
- batadv_ndisc_ip6_hdr(sk, skb, dev, ipv6_target, ipv6_dst,
IPPROTO_ICMPV6, len);- skb->transport_header = skb->tail;
why you care about setting the transport header? You could also use skb_set_transport_header() passing a proper offset rather than directly using skb->tail. Following the skb logic is "sometimes" not straightforward, therefore it is better to use the provided functions when possible.
- skb_put(skb, len);
- /* fill the device header for the NA frame */
- if (dev_hard_header(skb, dev, ETH_P_IPV6, hw_dst,
hw_target, skb->len) < 0) {kfree_skb(skb);return NULL;- }
mh..we never used this function as we assume that the interface below batman-adv is carrying 802.3 frame only. But looks interesting :)
- /* set icmpv6 header */
- icmp6hdr = (struct icmp6hdr *)skb_transport_header(skb);
ah now I understood why you have set the transport_header :)
- icmp6hdr->icmp6_type = NDISC_NEIGHBOUR_ADVERTISEMENT;
- icmp6hdr->icmp6_router = router;
- icmp6hdr->icmp6_solicited = solicited;
- icmp6hdr->icmp6_override = override;
- /* set NA target and options */
- opt = skb_transport_header(skb) + sizeof(*icmp6hdr);
- *(struct in6_addr *)opt = *ipv6_target;
- opt += sizeof(*ipv6_target);
- opt[0] = ND_OPT_TARGET_LL_ADDR;
- opt[1] = 1;
- memcpy(opt + 2, hw_target, dev->addr_len);
ah, these are the famous 8 bytes :) So hard to discover! :D
- icmp6hdr->icmp6_cksum = csum_ipv6_magic(ipv6_target, ipv6_dst, len,
IPPROTO_ICMPV6,csum_partial(icmp6hdr, len, 0));- return skb;
+} +#endif
/**
- batadv_dat_snoop_outgoing_pkt_request - snoop the ARP request and try to
- answer using DAT
-- 1.7.10.4
Hi,
On 10 August 2013 05:20, Antonio Quartulli ordex@autistici.org wrote:
On Mon, Jul 08, 2013 at 03:12:43AM +0300, mihail.costea2005@gmail.com wrote:
From: Mihail Costea mihail.costea90@gmail.com
Adds functions needed for NDP snooping, like getting the IPv6 addresses or getting the target HW address from an Neighbor Advertisement (NA).
typo: an -> a
Also added functions to create NA for Neighbor Solicitations
use always the same form: added -> adds
that have already the HW address in DAT.
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 | 319 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 319 insertions(+)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index f941913..d0b9e95 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -20,7 +20,9 @@ #include <linux/if_ether.h> #include <linux/if_arp.h> #include <linux/if_vlan.h> +#include <net/addrconf.h> #include <net/arp.h> +#include <net/ipv6.h>
#include "main.h" #include "hash.h" @@ -981,6 +983,323 @@ static unsigned short batadv_dat_get_vid(struct sk_buff *skb, int *hdr_size) return vid; }
+#if IS_ENABLED(CONFIG_IPV6) +/**
- batadv_ndisc_hw_src - get source hw address from a packet
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns source hw address of the skb packet.
- */
+static uint8_t *batadv_ndisc_hw_src(struct sk_buff *skb, int hdr_size) +{
struct ethhdr *ethhdr;please put a blank line after variable declarations.
ethhdr = (struct ethhdr *)(skb->data + hdr_size);return (uint8_t *)ethhdr->h_source;+}
+/**
- batadv_ndisc_hw_dst - get destination hw address from a packet
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns destination hw address of the skb packet.
- */
+static uint8_t *batadv_ndisc_hw_dst(struct sk_buff *skb, int hdr_size) +{
struct ethhdr *ethhdr;same here
ethhdr = (struct ethhdr *)(skb->data + hdr_size);return (uint8_t *)ethhdr->h_dest;+}
+/**
- batadv_ndisc_ipv6_src - get source IPv6 address from a packet
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns source IPv6 address of the skb packet.
- */
+static struct in6_addr *batadv_ndisc_ipv6_src(struct sk_buff *skb,
int hdr_size)+{
struct ipv6hdr *ipv6hdr;same here
ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);return &ipv6hdr->saddr;+}
+/**
- batadv_ndisc_ipv6_dst - get destination IPv6 address from a packet
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns destination IPv6 address of the skb packet.
- */
+static struct in6_addr *batadv_ndisc_ipv6_dst(struct sk_buff *skb,
int hdr_size)+{
struct ipv6hdr *ipv6hdr;same here
ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);return &ipv6hdr->daddr;+}
+/**
- batadv_ndisc_ipv6_target - get target IPv6 address from a NS/NA packet
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns target IPv6 address of the skb packet.
- */
+static struct in6_addr *batadv_ndisc_ipv6_target(struct sk_buff *skb,
int hdr_size)+{
return (struct in6_addr *)(skb->data + hdr_size + ETH_HLEN +sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr));please, use the needed local variables to make this statement more readable and and to align it in a proper way.
+}
+/**
- batadv_ndisc_hw_opt - get hw address from NS/NA packet options
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- @type: type of the address (ND_OPT_SOURCE_LL_ADDR or ND_OPT_TARGET_LL_ADDR)
- The address can be either the source link-layer address
- or the target link-layer address.
- For more info see RFC2461.
- Returns hw address from packet options.
- */
+static uint8_t *batadv_ndisc_hw_opt(struct sk_buff *skb, int hdr_size,
uint8_t type)+{
unsigned char *opt_addr;opt_addr = skb->data + hdr_size + ETH_HLEN +sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr) +sizeof(struct in6_addr);align this statement properly. it should be:
like_this = this + that + and_whatever + we_need;
/* test if option header is ok */Please, be a bit more verbose in this comment. What are we checking here? why != 1? What does that mean? Otherwise it will be difficult for other people out of these patches business to understand
if (*opt_addr != type || *(opt_addr + 1) != 1)return NULL;return (uint8_t *)(opt_addr + 2);and why skipping the initial two bytes? Maybe you should describe what opt_addr contains? this will probably help to better understand what this statement is doing.
+}
+/**
- batadv_ndisc_get_type - get icmp6 packet type
I think you can directly call this function "batadv_icmpv6_get_type". It may be re-used in the future for the same purpose but somewhere else :)
- @bat_priv: the bat priv with all the soft interface information
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- Returns the icmp6 type, or 0 if packet is not a valid icmp6 packet.
- */
+static __u8 batadv_ndisc_get_type(struct batadv_priv *bat_priv,
in the batman-adv code we use stdint: __u8 -> uint8_t (there are other spots where you use __8)
struct sk_buff *skb, int hdr_size)+{
struct ethhdr *ethhdr;struct ipv6hdr *ipv6hdr;struct icmp6hdr *icmp6hdr;__u8 type = 0;/* pull headers */if (unlikely(!pskb_may_pull(skb, hdr_size + ETH_HLEN +sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr))))please properly align this statement too. I think checkpatch would have complained about this. You can use tabs + spaces to correctly align.
Some of the code style problems were added by git mail it seems. In my code the alignment is correct (I did you --strict). But I didn't new comments should be :). /** * */
goto out;/* get the ethernet header */remove this comment :) Variables are named properly and this is obvious.
ethhdr = (struct ethhdr *)(skb->data + hdr_size);if (ethhdr->h_proto != htons(ETH_P_IPV6))goto out;/* get the ipv6 header */same
ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);if (ipv6hdr->nexthdr != IPPROTO_ICMPV6)goto out;/* get the icmpv6 header */icmp6hdr = (struct icmp6hdr *)(skb->data + hdr_size + ETH_HLEN +sizeof(struct ipv6hdr));alignment..should line up to the opening parenthesis.
(blablabla *)(like this);
/* check whether the ndisc packet carries a valid icmp6 header */if (ipv6hdr->hop_limit != 255)goto out;if (icmp6hdr->icmp6_code != 0)goto out;type = icmp6hdr->icmp6_type;+out:
return type;+}
+/**
- batadv_ndisc_is_valid - check if a ndisc packet is valid
- @bat_priv: the bat priv with all the soft interface information
- @skb: packet to check
- @hdr_size: size of the encapsulation header
- @ndisc_type: type of ndisc packet to check
- Check if all IPs are valid (source, destination, target) and if
- options hw address is valid too.
- Some options might be ignored, like NS packets sent automatically
- for verification of the reachability of a neighbor.
- Returns true if packet is valid, otherwise false if invalid or ignored.
- */
+static bool batadv_ndisc_is_valid(struct batadv_priv *bat_priv,
struct sk_buff *skb, int hdr_size,int ndisc_type)+{
uint8_t *hw_target = NULL;struct in6_addr *ipv6_src, *ipv6_dst, *ipv6_target;__u8 type = batadv_ndisc_get_type(bat_priv, skb, hdr_size);int ndisc_hdr_len = hdr_size + ETH_HLEN + sizeof(struct ipv6hdr) +sizeof(struct icmp6hdr) + sizeof(struct in6_addr) +8; /* ndisc options length */when the assignment is too long, please do it after the declarations. Improves readability.
if (type != ndisc_type)return false;if (unlikely(!pskb_may_pull(skb, ndisc_hdr_len)))return false;/* Check for bad NA/NS. If the ndisc message is not sane, DAT* will simply ignore it*/if (type == NDISC_NEIGHBOUR_SOLICITATION)hw_target = batadv_ndisc_hw_opt(skb, hdr_size,ND_OPT_SOURCE_LL_ADDR);else if (type == NDISC_NEIGHBOUR_ADVERTISEMENT)hw_target = batadv_ndisc_hw_opt(skb, hdr_size,ND_OPT_TARGET_LL_ADDR);if (!hw_target || is_zero_ether_addr(hw_target) ||is_multicast_ether_addr(hw_target))return false;ipv6_src = batadv_ndisc_ipv6_src(skb, hdr_size);ipv6_dst = batadv_ndisc_ipv6_dst(skb, hdr_size);ipv6_target = batadv_ndisc_ipv6_target(skb, hdr_size);if (ipv6_addr_loopback(ipv6_src) || ipv6_addr_loopback(ipv6_dst) ||ipv6_addr_is_multicast(ipv6_src) ||ipv6_addr_is_multicast(ipv6_target))return false;/* ignore messages with unspecified address */if (ipv6_addr_any(ipv6_src) || ipv6_addr_any(ipv6_dst) ||ipv6_addr_any(ipv6_target))return false;/* ignore the verification of the reachability of a neighbor */if (type == NDISC_NEIGHBOUR_SOLICITATION &&!ipv6_addr_is_multicast(ipv6_dst))return false;return true;+}
+static void batadv_ndisc_ip6_hdr(struct sock *sk, struct sk_buff *skb,
struct net_device *dev,const struct in6_addr *ipv6_src,const struct in6_addr *ipv6_dst,int proto, int len)alignment?
+{
struct ipv6_pinfo *np = inet6_sk(sk);struct ipv6hdr *hdr;skb->protocol = htons(ETH_P_IPV6);skb->dev = dev;skb_reset_network_header(skb);skb_put(skb, sizeof(struct ipv6hdr));hdr = ipv6_hdr(skb);*(__be32 *)hdr = htonl(0x60000000);What does this constant mean? you are writing on the IPv6 header? why not writing in one of its fields (I guess you wanted to write in the first one)?
I mostly took this function from another source file as it is, but it was static so I couldn't use it directly. I will make the changes.
hdr->payload_len = htons(len);I think len can be declared int16_t to avoid using wrong values? (here and where you call this function)
hdr->nexthdr = proto;hdr->hop_limit = np->hop_limit;hdr->saddr = *ipv6_src;hdr->daddr = *ipv6_dst;+}
+/**
- batadv_ndisc_create_na - create an NA for a solicited NS
- @net_device: the devices for which the packet is created
- @ipv6_dst: destination IPv6
- @ipv6_target: target IPv6 (same with source IPv6)
- @hw_dst: destination HW Address
- @hw_target: target HW Address (same with source HW Address)
- @router: 1 if target is a router, else 0
- @solicited: 1 if this is a solicited NA, else 0
- @override: 1 if the target entry should be override, else 0
- For more info see RFC2461.
If you want to cite the RFC think you should provide also a section? The "Neighbor Discovery for IP Version 6" RFC is a bit too much is I only want to understand what a NA or NS is and how the NA is created.
By the way, ok for reading the RFC, but I think you can write a bit more about what the function is doing: e.g. create a new skb containing an NA which fields are initialised with the value passed as argument. Seems obvious, but better safe than sorry :) Kernel Doc is shown without the code if you compile it.
- Returns the newly created skb, otherwise NULL.
- */
+static struct +sk_buff *batadv_ndisc_create_na(struct net_device *dev,
const struct in6_addr *ipv6_dst,const struct in6_addr *ipv6_target,const uint8_t *hw_dst,const uint8_t *hw_target,int router, int solicited, int override)if these variables can only be 0 or 1, why not using bool?
+{
struct net *net = dev_net(dev);struct sock *sk = net->ipv6.ndisc_sk;struct sk_buff *skb;struct icmp6hdr *icmp6hdr;int hlen = LL_RESERVED_SPACE(dev);int tlen = dev->needed_tailroom;int len, err;u8 *opt;uint8_t
/* alloc space for skb */len = sizeof(struct icmp6hdr) + sizeof(*ipv6_target) + 8;write in the comment what is this 8. Otherwise, since you use it more than once, create a define with a descriptive name and it instead of the 8.
skb = sock_alloc_send_skb(sk,(MAX_HEADER + sizeof(struct ipv6hdr) +why these parenthesis here?
len + hlen + tlen),I guess hlen is ETH_HLEN? what does LL_RESERVED_SPACE exactly return? and why using needed_tailroom? what does it comport in this case? We never used that during SKB allocation...this is why I am asking.
1, &err);and why are you using this function to allocate the skb? In the rest of the code we always use netdev_alloc_skb_ip_align() (that also takes care of properly aligning the skb).
if (!skb)return NULL;skb_reserve(skb, hlen);batadv_ndisc_ip6_hdr(sk, skb, dev, ipv6_target, ipv6_dst,IPPROTO_ICMPV6, len);skb->transport_header = skb->tail;why you care about setting the transport header? You could also use skb_set_transport_header() passing a proper offset rather than directly using skb->tail. Following the skb logic is "sometimes" not straightforward, therefore it is better to use the provided functions when possible.
skb_put(skb, len);/* fill the device header for the NA frame */if (dev_hard_header(skb, dev, ETH_P_IPV6, hw_dst,hw_target, skb->len) < 0) {kfree_skb(skb);return NULL;}mh..we never used this function as we assume that the interface below batman-adv is carrying 802.3 frame only. But looks interesting :)
/* set icmpv6 header */icmp6hdr = (struct icmp6hdr *)skb_transport_header(skb);ah now I understood why you have set the transport_header :)
icmp6hdr->icmp6_type = NDISC_NEIGHBOUR_ADVERTISEMENT;icmp6hdr->icmp6_router = router;icmp6hdr->icmp6_solicited = solicited;icmp6hdr->icmp6_override = override;/* set NA target and options */opt = skb_transport_header(skb) + sizeof(*icmp6hdr);*(struct in6_addr *)opt = *ipv6_target;opt += sizeof(*ipv6_target);opt[0] = ND_OPT_TARGET_LL_ADDR;opt[1] = 1;memcpy(opt + 2, hw_target, dev->addr_len);ah, these are the famous 8 bytes :) So hard to discover! :D
icmp6hdr->icmp6_cksum = csum_ipv6_magic(ipv6_target, ipv6_dst, len,IPPROTO_ICMPV6,csum_partial(icmp6hdr, len, 0));return skb;+} +#endif
/**
- batadv_dat_snoop_outgoing_pkt_request - snoop the ARP request and try to
- answer using DAT
-- 1.7.10.4
-- Antonio Quartulli
..each of us alone is worth nothing.. Ernesto "Che" Guevara
I will resolve most of the comments when I redo the patch.
Thanks, Mihail
From: Mihail Costea mihail.costea90@gmail.com
Generalize snooping functions in order to support Neighbor Advertisement and Neighbor Solicitation too. Adds snooping functions to generic struct so there won't be a need for switch-cases.
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 | 438 +++++++++++++++++++++++++++++++++++------------ types.h | 69 ++++++++ 2 files changed, 401 insertions(+), 106 deletions(-)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index d0b9e95..1a5749b 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -33,21 +33,55 @@ #include "types.h" #include "translation-table.h"
+static void batadv_dat_purge(struct work_struct *work); + +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); + +static struct sk_buff * +batadv_dat_create_arp_reply(struct batadv_priv *bat_priv, + struct batadv_dat_entry *dat_entry, + uint8_t *hw_src, void *ip_src, void *ip_dst) +{ + return arp_create(ARPOP_REPLY, ETH_P_ARP, *(__be32 *)ip_src, + bat_priv->soft_iface, *(__be32 *)ip_dst, + hw_src, dat_entry->mac_addr, hw_src); +} + +#if IS_ENABLED(CONFIG_IPV6) +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); + +static struct sk_buff * +batadv_dat_create_ndisc_na(struct batadv_priv *bat_priv, + struct batadv_dat_entry *dat_entry, + uint8_t *hw_src, void *ip_src, void *ip_dst); + +#endif + static struct batadv_dat_type_info batadv_dat_types_info[] = { { .size = sizeof(__be32), .str_fmt = "%pI4", + .snoop_pkt = batadv_dat_snoop_arp_pkt, + .create_skb = batadv_dat_create_arp_reply, }, #if IS_ENABLED(CONFIG_IPV6) { .size = sizeof(struct in6_addr), .str_fmt = "%pI6c", + .snoop_pkt = batadv_dat_snoop_ndisc_pkt, + .create_skb = batadv_dat_create_ndisc_na, }, #endif };
-static void batadv_dat_purge(struct work_struct *work); - /** * batadv_dat_data_to_str: transforms DAT data to string * @data: the DAT data @@ -61,7 +95,7 @@ static char *batadv_dat_data_to_str(void *data, uint8_t type, char *buf, size_t buf_len) { snprintf(buf, buf_len, batadv_dat_types_info[type].str_fmt, data); -return buf; + return buf; }
/** @@ -221,9 +255,9 @@ static uint8_t *batadv_arp_hw_src(struct sk_buff *skb, int hdr_size) * * Returns the value of the ip_src field in the ARP packet. */ -static __be32 batadv_arp_ip_src(struct sk_buff *skb, int hdr_size) +static __be32 *batadv_arp_ip_src(struct sk_buff *skb, int hdr_size) { - return *(__be32 *)(batadv_arp_hw_src(skb, hdr_size) + ETH_ALEN); + return (__be32 *)(batadv_arp_hw_src(skb, hdr_size) + ETH_ALEN); }
/** @@ -245,9 +279,9 @@ static uint8_t *batadv_arp_hw_dst(struct sk_buff *skb, int hdr_size) * * Returns the value of the ip_dst field in the ARP packet. */ -static __be32 batadv_arp_ip_dst(struct sk_buff *skb, int hdr_size) +static __be32 *batadv_arp_ip_dst(struct sk_buff *skb, int hdr_size) { - return *(__be32 *)(batadv_arp_hw_src(skb, hdr_size) + ETH_ALEN * 2 + 4); + return (__be32 *)(batadv_arp_hw_src(skb, hdr_size) + ETH_ALEN * 2 + 4); }
/** @@ -411,8 +445,8 @@ static void batadv_dbg_arp(struct batadv_priv *bat_priv, struct sk_buff *skb, if (msg) batadv_dbg(BATADV_DBG_DAT, bat_priv, "%s\n", msg);
- ip_src = batadv_arp_ip_src(skb, hdr_size); - ip_dst = batadv_arp_ip_dst(skb, hdr_size); + ip_src = *batadv_arp_ip_src(skb, hdr_size); + ip_dst = *batadv_arp_ip_dst(skb, hdr_size); batadv_dbg(BATADV_DBG_DAT, bat_priv, "ARP MSG = [src: %pM-%pI4 dst: %pM-%pI4]\n", batadv_arp_hw_src(skb, hdr_size), &ip_src, @@ -867,7 +901,7 @@ int batadv_dat_cache_seq_print_text(struct seq_file *seq, void *offset) last_seen_msecs = last_seen_msecs % 60000; last_seen_secs = last_seen_msecs / 1000;
- seq_printf(seq, " * %40s %15pM %6i:%02i\n", + seq_printf(seq, " * %-40s %15pM %6i:%02i\n", batadv_dat_data_to_str(dat_entry->data, dat_entry->type, dbg_data, @@ -933,8 +967,8 @@ static uint16_t batadv_arp_get_type(struct batadv_priv *bat_priv, /* Check for bad reply/request. If the ARP message is not sane, DAT * will simply ignore it */ - ip_src = batadv_arp_ip_src(skb, hdr_size); - ip_dst = batadv_arp_ip_dst(skb, hdr_size); + ip_src = *batadv_arp_ip_src(skb, hdr_size); + ip_dst = *batadv_arp_ip_dst(skb, hdr_size); if (ipv4_is_loopback(ip_src) || ipv4_is_multicast(ip_src) || ipv4_is_loopback(ip_dst) || ipv4_is_multicast(ip_dst) || ipv4_is_zeronet(ip_src) || ipv4_is_lbcast(ip_src) || @@ -958,6 +992,40 @@ out: return type; }
+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) +{ + if ((pkt_dir_type == BATADV_DAT_OUTGOING_PKT_REQUEST || + pkt_dir_type == BATADV_DAT_INCOMING_PKT_REQUEST || + pkt_dir_type == BATADV_DAT_BROADCAST_FALLBACK) && + pkt_type != ARPOP_REQUEST) + return false; + if ((pkt_dir_type == BATADV_DAT_OUTGOING_PKT_REPLY || + pkt_dir_type == BATADV_DAT_INCOMING_PKT_REPLY) && + pkt_type != ARPOP_REPLY) + return false; + + if (hw_src) + *hw_src = batadv_arp_hw_src(skb, hdr_size); + if (ip_src) + *ip_src = batadv_arp_ip_src(skb, hdr_size); + if (hw_dst) + *hw_dst = batadv_arp_hw_dst(skb, hdr_size); + if (ip_dst) + *ip_dst = batadv_arp_ip_dst(skb, hdr_size); + + if (pkt_type == ARPOP_REQUEST) + batadv_dbg_arp(bat_priv, skb, pkt_type, hdr_size, + "Parsing ARP REQUEST"); + else if (pkt_type == ARPOP_REPLY) + batadv_dbg_arp(bat_priv, skb, pkt_type, hdr_size, + "Parsing ARP REPLY"); + + return true; +} /** * batadv_dat_get_vid - extract the VLAN identifier from skb if any * @skb: the buffer containing the packet to extract the VID from @@ -1135,7 +1203,7 @@ out: * @bat_priv: the bat priv with all the soft interface information * @skb: packet to check * @hdr_size: size of the encapsulation header - * @ndisc_type: type of ndisc packet to check + * @type: type of ndisc packet to check * * Check if all IPs are valid (source, destination, target) and if * options hw address is valid too. @@ -1146,17 +1214,14 @@ out: */ static bool batadv_ndisc_is_valid(struct batadv_priv *bat_priv, struct sk_buff *skb, int hdr_size, - int ndisc_type) + uint16_t type) { uint8_t *hw_target = NULL; struct in6_addr *ipv6_src, *ipv6_dst, *ipv6_target; - __u8 type = batadv_ndisc_get_type(bat_priv, skb, hdr_size); int ndisc_hdr_len = hdr_size + ETH_HLEN + sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr) + sizeof(struct in6_addr) + 8; /* ndisc options length */
- if (type != ndisc_type) - return false; if (unlikely(!pskb_may_pull(skb, ndisc_hdr_len))) return false;
@@ -1169,6 +1234,8 @@ static bool batadv_ndisc_is_valid(struct batadv_priv *bat_priv, else if (type == NDISC_NEIGHBOUR_ADVERTISEMENT) hw_target = batadv_ndisc_hw_opt(skb, hdr_size, ND_OPT_TARGET_LL_ADDR); + else + return false;
if (!hw_target || is_zero_ether_addr(hw_target) || is_multicast_ether_addr(hw_target)) @@ -1298,11 +1365,155 @@ sk_buff *batadv_ndisc_create_na(struct net_device *dev,
return skb; } + +struct sk_buff * +batadv_dat_create_ndisc_na(struct batadv_priv *bat_priv, + struct batadv_dat_entry *dat_entry, + uint8_t *hw_src, void *ip_src, void *ip_dst) +{ + /* TODO calculate router and override parameters */ + return batadv_ndisc_create_na(bat_priv->soft_iface, + ip_src, ip_dst, + hw_src, dat_entry->mac_addr, + 0, 1, 1); +} + +/** + * batadv_dat_snoop_ndisc_addr - snoop addresses from NA / NS messages + * @skb: packet to snoop + * @hdr_size: size of the encapsulation header + * @target_address_type: type of target address + * (ND_OPT_SOURCE_LL_ADDR or ND_OPT_TARGET_LL_ADDR) + * @hw_src: source HW Address + * @ipv6_src: source IPv6 + * @hw_dst: destination HW Address + * @ipv6_dst: destination IPv6 + * @hw_target: target HW Address + * @ipv6_target: target IPv6 + * + * If an address parameter is NULL, then the correspondent field is not + * snooped. The fields might be different depending on packet type (NS / NA). + * + * Return true if snooping was successful. + */ +static bool batadv_dat_snoop_ndisc_addr(struct sk_buff *skb, int hdr_size, + uint8_t target_address_type, + uint8_t **hw_src, void **ipv6_src, + uint8_t **hw_dst, void **ipv6_dst, + uint8_t **hw_target, void **ipv6_target) +{ + if (hw_src) + *hw_src = batadv_ndisc_hw_src(skb, hdr_size); + if (ipv6_src) + *ipv6_src = batadv_ndisc_ipv6_src(skb, hdr_size); + if (hw_dst) + *hw_dst = batadv_ndisc_hw_dst(skb, hdr_size); + if (ipv6_dst) + *ipv6_dst = batadv_ndisc_ipv6_dst(skb, hdr_size); + if (hw_target) { + *hw_target = batadv_ndisc_hw_opt(skb, hdr_size, + target_address_type); + if (!*hw_target) + return false; + } + if (ipv6_target) + *ipv6_target = batadv_ndisc_ipv6_target(skb, hdr_size); + + return true; +} + +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) +{ + if (batadv_ndisc_is_valid(bat_priv, skb, hdr_size, pkt_type)) { + if ((pkt_dir_type == BATADV_DAT_OUTGOING_PKT_REQUEST || + pkt_dir_type == BATADV_DAT_INCOMING_PKT_REQUEST || + pkt_dir_type == BATADV_DAT_BROADCAST_FALLBACK) && + pkt_type == NDISC_NEIGHBOUR_SOLICITATION) { + if (!batadv_dat_snoop_ndisc_addr(skb, hdr_size, + ND_OPT_SOURCE_LL_ADDR, + NULL, ip_src, + hw_dst, NULL, + hw_src, ip_dst)) + return false; + + if (pkt_dir_type != BATADV_DAT_BROADCAST_FALLBACK) + batadv_dbg(BATADV_DBG_DAT, bat_priv, + "Parsing NS = [src: %pM / %pI6c -> " + "target: %pM / %pI6c]\n", + *hw_src, *ip_src, *hw_dst, *ip_dst); + return true; + } + + if ((pkt_dir_type == BATADV_DAT_OUTGOING_PKT_REPLY || + pkt_dir_type == BATADV_DAT_INCOMING_PKT_REPLY) && + pkt_type == NDISC_NEIGHBOUR_ADVERTISEMENT) { + if (!batadv_dat_snoop_ndisc_addr(skb, hdr_size, + ND_OPT_TARGET_LL_ADDR, + NULL, NULL, + hw_dst, ip_dst, + hw_src, ip_src)) + return false; + + batadv_dbg(BATADV_DBG_DAT, bat_priv, + "Parsing NA = [src: %pM / %pI6c -> " + "target: %pM / %pI6c]\n", + *hw_src, *ip_src, *hw_dst, *ip_dst); + return true; + + /* ignore multicast address for unsolicited NA */ + if (ipv6_addr_is_multicast(*ip_dst)) + *ip_dst = NULL; + } + } + + return false; +} +#endif + +/** + * batadv_dat_get_pair_type : gets packet and data types + * @bat_priv: the bat priv with all the soft interface information + * @skb: packet to snoop + * @hdr_size: size of the encapsulation header + * + * Returns the packet and types, or -1 for data_type if invalid. + */ +static struct batadv_dat_pair_type +batadv_dat_get_pair_type(struct batadv_priv *bat_priv, + struct sk_buff *skb, int hdr_size) +{ + struct batadv_dat_pair_type pair_type = { + .data_type = -1, + }; + + uint16_t arp_type; + __u8 ndisc_type; + + arp_type = batadv_arp_get_type(bat_priv, skb, hdr_size); + if (arp_type == ARPOP_REQUEST || arp_type == ARPOP_REPLY) { + pair_type.pkt_type = arp_type; + pair_type.data_type = BATADV_DAT_IPV4; + } + +#if IS_ENABLED(CONFIG_IPV6) + ndisc_type = batadv_ndisc_get_type(bat_priv, skb, hdr_size); + if (ndisc_type == NDISC_NEIGHBOUR_SOLICITATION || + ndisc_type == NDISC_NEIGHBOUR_ADVERTISEMENT) { + pair_type.pkt_type = ndisc_type; + pair_type.data_type = BATADV_DAT_IPV6; + } #endif
+ return pair_type; +} + /** - * batadv_dat_snoop_outgoing_pkt_request - snoop the ARP request and try to - * answer using DAT + * batadv_dat_snoop_outgoing_pkt_request - snoop the ARP request / NS + * and try to answer using DAT * @bat_priv: the bat priv with all the soft interface information * @skb: packet to check * @@ -1313,37 +1524,37 @@ sk_buff *batadv_ndisc_create_na(struct net_device *dev, bool batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, struct sk_buff *skb) { - uint16_t type = 0; - __be32 ip_dst, ip_src; - uint8_t *hw_src; + void *ip_src, *ip_dst; + uint8_t *hw_src, *hw_dst; bool ret = false; struct batadv_dat_entry *dat_entry = NULL; struct sk_buff *skb_new; int hdr_size = 0; unsigned short vid; + struct batadv_dat_pair_type dat_pair_type;
if (!atomic_read(&bat_priv->distributed_arp_table)) goto out;
vid = batadv_dat_get_vid(skb, &hdr_size);
- type = batadv_arp_get_type(bat_priv, skb, hdr_size); - /* If the node gets an ARP_REQUEST it has to send a DHT_GET unicast + /* If the node gets an ARP_REQUEST / NS it has to send a DHT_GET unicast * message to the selected DHT candidates */ - if (type != ARPOP_REQUEST) + dat_pair_type = batadv_dat_get_pair_type(bat_priv, skb, hdr_size); + if (dat_pair_type.data_type < 0) + goto out; + if (!batadv_dat_types_info[dat_pair_type.data_type].snoop_pkt( + bat_priv, skb, hdr_size, dat_pair_type.pkt_type, + BATADV_DAT_OUTGOING_PKT_REQUEST, + &hw_src, &ip_src, &hw_dst, &ip_dst)) goto out;
- batadv_dbg_arp(bat_priv, skb, type, 0, "Parsing outgoing ARP REQUEST"); - - ip_src = batadv_arp_ip_src(skb, 0); - hw_src = batadv_arp_hw_src(skb, 0); - ip_dst = batadv_arp_ip_dst(skb, 0); - - batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src, vid); + batadv_dat_entry_add(bat_priv, ip_src, dat_pair_type.data_type, + hw_src, vid);
- dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst, - BATADV_DAT_IPV4, vid); + dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst, + dat_pair_type.data_type, vid); if (dat_entry) { /* If the ARP request is destined for a local client the local * client will answer itself. DAT would only generate a @@ -1359,9 +1570,9 @@ bool batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, goto out; }
- skb_new = arp_create(ARPOP_REPLY, ETH_P_ARP, ip_src, - bat_priv->soft_iface, ip_dst, hw_src, - dat_entry->mac_addr, hw_src); + skb_new = batadv_dat_types_info[dat_pair_type.data_type]. + create_skb(bat_priv, dat_entry, + hw_src, ip_src, ip_dst); if (!skb_new) goto out;
@@ -1373,12 +1584,13 @@ bool batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, bat_priv->soft_iface->last_rx = jiffies;
netif_rx(skb_new); - batadv_dbg(BATADV_DBG_DAT, bat_priv, "ARP request replied locally\n"); + batadv_dbg(BATADV_DBG_DAT, bat_priv, + "ARP request / NS replied locally\n"); ret = true; } else { /* Send the request to the DHT */ - ret = batadv_dat_send_data(bat_priv, skb, &ip_dst, - BATADV_DAT_IPV4, + ret = batadv_dat_send_data(bat_priv, skb, ip_dst, + dat_pair_type.data_type, BATADV_P_DAT_DHT_GET); } out: @@ -1388,8 +1600,8 @@ out: }
/** - * batadv_dat_snoop_incoming_pkt_request - snoop the ARP request and try to - * answer using the local DAT storage + * batadv_dat_snoop_incoming_pkt_request - snoop the ARP request / NS and try + * to answer using the local DAT storage * @bat_priv: the bat priv with all the soft interface information * @skb: packet to check * @hdr_size: size of the encapsulation header @@ -1399,42 +1611,39 @@ out: bool batadv_dat_snoop_incoming_pkt_request(struct batadv_priv *bat_priv, struct sk_buff *skb, int hdr_size) { - uint16_t type; - __be32 ip_src, ip_dst; - uint8_t *hw_src; + void *ip_src, *ip_dst; + uint8_t *hw_src, *hw_dst; struct sk_buff *skb_new; struct batadv_dat_entry *dat_entry = NULL; bool ret = false; unsigned short vid; int err; + struct batadv_dat_pair_type dat_pair_type;
if (!atomic_read(&bat_priv->distributed_arp_table)) goto out;
vid = batadv_dat_get_vid(skb, &hdr_size);
- type = batadv_arp_get_type(bat_priv, skb, hdr_size); - if (type != ARPOP_REQUEST) + dat_pair_type = batadv_dat_get_pair_type(bat_priv, skb, hdr_size); + if (dat_pair_type.data_type < 0) + goto out; + if (!batadv_dat_types_info[dat_pair_type.data_type].snoop_pkt( + bat_priv, skb, hdr_size, dat_pair_type.pkt_type, + BATADV_DAT_INCOMING_PKT_REQUEST, + &hw_src, &ip_src, &hw_dst, &ip_dst)) goto out;
- hw_src = batadv_arp_hw_src(skb, hdr_size); - ip_src = batadv_arp_ip_src(skb, hdr_size); - ip_dst = batadv_arp_ip_dst(skb, hdr_size); - - batadv_dbg_arp(bat_priv, skb, type, hdr_size, - "Parsing incoming ARP REQUEST"); - - batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src, vid); + batadv_dat_entry_add(bat_priv, ip_src, dat_pair_type.data_type, + hw_src, vid);
- dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst, - BATADV_DAT_IPV4, vid); + dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst, + dat_pair_type.data_type, vid); if (!dat_entry) goto out;
- skb_new = arp_create(ARPOP_REPLY, ETH_P_ARP, ip_src, - bat_priv->soft_iface, ip_dst, hw_src, - dat_entry->mac_addr, hw_src); - + skb_new = batadv_dat_types_info[dat_pair_type.data_type].create_skb( + bat_priv, dat_entry, hw_src, ip_src, ip_dst); if (!skb_new) goto out;
@@ -1462,49 +1671,55 @@ out: }
/** - * batadv_dat_snoop_outgoing_pkt_reply - snoop the ARP reply and fill the DHT + * batadv_dat_snoop_outgoing_pkt_reply - snoop the ARP reply / NA and fill + * the DHT * @bat_priv: the bat priv with all the soft interface information * @skb: packet to check */ void batadv_dat_snoop_outgoing_pkt_reply(struct batadv_priv *bat_priv, struct sk_buff *skb) { - uint16_t type; - __be32 ip_src, ip_dst; + void *ip_src, *ip_dst; uint8_t *hw_src, *hw_dst; int hdr_size = 0; unsigned short vid; + struct batadv_dat_pair_type dat_pair_type;
if (!atomic_read(&bat_priv->distributed_arp_table)) return;
vid = batadv_dat_get_vid(skb, &hdr_size);
- type = batadv_arp_get_type(bat_priv, skb, hdr_size); - if (type != ARPOP_REPLY) + dat_pair_type = batadv_dat_get_pair_type(bat_priv, skb, hdr_size); + if (dat_pair_type.data_type < 0) + return; + if (!batadv_dat_types_info[dat_pair_type.data_type].snoop_pkt( + bat_priv, skb, hdr_size, dat_pair_type.pkt_type, + BATADV_DAT_OUTGOING_PKT_REPLY, + &hw_src, &ip_src, &hw_dst, &ip_dst)) return; - - batadv_dbg_arp(bat_priv, skb, type, 0, "Parsing outgoing ARP REPLY"); - - hw_src = batadv_arp_hw_src(skb, hdr_size); - ip_src = batadv_arp_ip_src(skb, hdr_size); - hw_dst = batadv_arp_hw_dst(skb, hdr_size); - ip_dst = batadv_arp_ip_dst(skb, hdr_size); - - batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src, vid); - batadv_dat_entry_add(bat_priv, &ip_dst, BATADV_DAT_IPV4, hw_dst, vid);
/* Send the ARP reply to the candidates for both the IP addresses that * the node obtained from the ARP reply */ - batadv_dat_send_data(bat_priv, skb, &ip_src, BATADV_DAT_IPV4, - BATADV_P_DAT_DHT_PUT); - batadv_dat_send_data(bat_priv, skb, &ip_dst, BATADV_DAT_IPV4, + batadv_dat_entry_add(bat_priv, ip_src, dat_pair_type.data_type, + hw_src, vid); + batadv_dat_send_data(bat_priv, skb, ip_src, dat_pair_type.data_type, BATADV_P_DAT_DHT_PUT); + + /* not a solicited advertisement (see snooping mechanism) */ + if (ip_dst) { + batadv_dat_entry_add(bat_priv, ip_dst, dat_pair_type.data_type, + hw_dst, vid); + batadv_dat_send_data(bat_priv, skb, ip_dst, + dat_pair_type.data_type, + BATADV_P_DAT_DHT_PUT); + } } + /** - * batadv_dat_snoop_incoming_pkt_reply - snoop the ARP reply and fill the local - * DAT storage only + * batadv_dat_snoop_incoming_pkt_reply - snoop the ARP reply / NA and fill + * the local DAT storage only * @bat_priv: the bat priv with all the soft interface information * @skb: packet to check * @hdr_size: size of the encapsulation header @@ -1512,34 +1727,35 @@ void batadv_dat_snoop_outgoing_pkt_reply(struct batadv_priv *bat_priv, bool batadv_dat_snoop_incoming_pkt_reply(struct batadv_priv *bat_priv, struct sk_buff *skb, int hdr_size) { - uint16_t type; - __be32 ip_src, ip_dst; + void *ip_src, *ip_dst; uint8_t *hw_src, *hw_dst; bool ret = false; unsigned short vid; + struct batadv_dat_pair_type dat_pair_type;
if (!atomic_read(&bat_priv->distributed_arp_table)) goto out;
vid = batadv_dat_get_vid(skb, &hdr_size);
- type = batadv_arp_get_type(bat_priv, skb, hdr_size); - if (type != ARPOP_REPLY) + dat_pair_type = batadv_dat_get_pair_type(bat_priv, skb, hdr_size); + if (dat_pair_type.data_type < 0) + goto out; + if (!batadv_dat_types_info[dat_pair_type.data_type].snoop_pkt( + bat_priv, skb, hdr_size, dat_pair_type.pkt_type, + BATADV_DAT_INCOMING_PKT_REPLY, + &hw_src, &ip_src, &hw_dst, &ip_dst)) goto out; - - batadv_dbg_arp(bat_priv, skb, type, hdr_size, - "Parsing incoming ARP REPLY"); - - hw_src = batadv_arp_hw_src(skb, hdr_size); - ip_src = batadv_arp_ip_src(skb, hdr_size); - hw_dst = batadv_arp_hw_dst(skb, hdr_size); - ip_dst = batadv_arp_ip_dst(skb, hdr_size);
/* Update our internal cache with both the IP addresses the node got * within the ARP reply */ - batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src, vid); - batadv_dat_entry_add(bat_priv, &ip_dst, BATADV_DAT_IPV4, hw_dst, vid); + batadv_dat_entry_add(bat_priv, ip_src, dat_pair_type.data_type, + hw_src, vid); + /* not a solicited advertisement (see snooping mechanism) */ + if (ip_dst) + batadv_dat_entry_add(bat_priv, ip_dst, + dat_pair_type.data_type, hw_dst, vid);
/* if this REPLY is directed to a client of mine, let's deliver the * packet to the interface @@ -1553,8 +1769,8 @@ out: }
/** - * batadv_dat_drop_broadcast_packet - check if an ARP request has to be dropped - * (because the node has already obtained the reply via DAT) or not + * batadv_dat_drop_broadcast_packet - check if an ARP request / NA has to be + * dropped (because the node has already obtained the reply via DAT) or not * @bat_priv: the bat priv with all the soft interface information * @forw_packet: the broadcast packet * @@ -1563,12 +1779,13 @@ out: bool batadv_dat_drop_broadcast_packet(struct batadv_priv *bat_priv, struct batadv_forw_packet *forw_packet) { - uint16_t type; - __be32 ip_dst; + void *ip_dst; struct batadv_dat_entry *dat_entry = NULL; bool ret = false; int hdr_size = sizeof(struct batadv_bcast_packet); unsigned short vid; + struct batadv_dat_pair_type dat_pair_type; + char dbg_data[BATADV_DAT_DATA_MAX_LEN];
if (!atomic_read(&bat_priv->distributed_arp_table)) goto out; @@ -1581,22 +1798,31 @@ bool batadv_dat_drop_broadcast_packet(struct batadv_priv *bat_priv,
vid = batadv_dat_get_vid(forw_packet->skb, &hdr_size);
- type = batadv_arp_get_type(bat_priv, forw_packet->skb, hdr_size); - if (type != ARPOP_REQUEST) + dat_pair_type = batadv_dat_get_pair_type(bat_priv, forw_packet->skb, + hdr_size); + if (dat_pair_type.data_type < 0) goto out; + if (!batadv_dat_types_info[dat_pair_type.data_type].snoop_pkt( + bat_priv, forw_packet->skb, hdr_size, + dat_pair_type.pkt_type, + BATADV_DAT_BROADCAST_FALLBACK, + NULL, NULL, NULL, &ip_dst)) + goto out; + + dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst, + dat_pair_type.data_type, vid);
- ip_dst = batadv_arp_ip_dst(forw_packet->skb, hdr_size); - dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst, - BATADV_DAT_IPV4, vid); /* check if the node already got this entry */ + batadv_dat_data_to_str(ip_dst, dat_pair_type.data_type, + dbg_data, sizeof(dbg_data)); if (!dat_entry) { batadv_dbg(BATADV_DBG_DAT, bat_priv, - "ARP Request for %pI4: fallback\n", &ip_dst); + "ARP Request for %s: fallback\n", dbg_data); goto out; }
batadv_dbg(BATADV_DBG_DAT, bat_priv, - "ARP Request for %pI4: fallback prevented\n", &ip_dst); + "ARP Request for %s fallback prevented\n", dbg_data); ret = true;
out: diff --git a/types.h b/types.h index 60d2d64..6ad4500 100644 --- a/types.h +++ b/types.h @@ -964,13 +964,82 @@ enum batadv_dat_types { };
/** + * batadv_dat_pkt_types - describe generic packet types + * @BATADV_DAT_OUTGOING_PKT_REQUEST: an outgoing packet request + * @BATADV_DAT_INCOMING_PKT_REQUEST: an incoming packet request + * @BATADV_DAT_OUTGOING_PKT_REPLY: an outgoing packet reply + * @BATADV_DAT_INCOMING_PKT_REPLY: an incoming packet reply + * @BATADV_DAT_BROADCAST_FALLBACK: used for broadcasts fallback; + * can any type of request packets + * + * The packets can be ARP Request / Reply, NS / NA or other packets. + */ +enum batadv_dat_pkt_types { + BATADV_DAT_OUTGOING_PKT_REQUEST, + BATADV_DAT_INCOMING_PKT_REQUEST, + BATADV_DAT_OUTGOING_PKT_REPLY, + BATADV_DAT_INCOMING_PKT_REPLY, + BATADV_DAT_BROADCAST_FALLBACK, +}; + +/** + * batadv_dat_pair_type - types needed for a dat packet + * @data_type the data type (negative values represents invalid values) + * @pkt_type: the packet type + */ +struct batadv_dat_pair_type { + int data_type; + uint16_t pkt_type; +}; + +/** * batadv_dat_type_info - info needed for a DAT type data * @size: the size of the type data * @str_fmt: string format used by the data + * @snoop_pkt: function used to snoop addresses from a packet */ struct batadv_dat_type_info { size_t size; char *str_fmt; + /** + * snoop_pkt - snooping mechanism for all packets that participate + * in DAT + * @bat_priv: the bat priv with all the soft interface information + * @skb: packet to snoop + * @hdr_size: size of the encapsulation header + * @pkt_type: the packet type + * @pkt_dir_type: outgoing / incoming message request / reply + * @hw_src: source HW Address + * @ip_src: source IP + * @hw_dst: destination HW Address + * @ip_dst: destination IP + * + * Any address pointer can be null if there is no need to memorize + * calculate that address. + * + * Returns true if snooping was successful. + */ + bool (*snoop_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); + + /** + * batadv_dat_create_skb - creates a skb as a reply to a message request + * @bat_priv: the bat priv with all the soft interface information + * @dat_entry: the DAT entry used for destination HW Address + * @hw_src: source HW Address + * @ip_src: source IP + * @ip_dst: destination IP + * + * Returns the newly created skb, or NULL if any error. + */ + struct sk_buff *(*create_skb)(struct batadv_priv *bat_priv, + struct batadv_dat_entry *dat_entry, + uint8_t *hw_src, void *ip_src, + void *ip_dst); + };
/**
Hi Mihail,
On Mon, Jul 08, 2013 at 03:12:44AM +0300, mihail.costea2005@gmail.com wrote:
From: Mihail Costea mihail.costea90@gmail.com /**
- batadv_dat_snoop_outgoing_pkt_reply - snoop the ARP reply and fill the DHT
- batadv_dat_snoop_outgoing_pkt_reply - snoop the ARP reply / NA and fill
*/
- the DHT
- @bat_priv: the bat priv with all the soft interface information
- @skb: packet to check
void batadv_dat_snoop_outgoing_pkt_reply(struct batadv_priv *bat_priv, struct sk_buff *skb) {
--- snip ---
- /* not a solicited advertisement (see snooping mechanism) */
- if (ip_dst) {
batadv_dat_entry_add(bat_priv, ip_dst, dat_pair_type.data_type,hw_dst, vid);batadv_dat_send_data(bat_priv, skb, ip_dst,dat_pair_type.data_type,BATADV_P_DAT_DHT_PUT);- }
}
I'm currently wondering about the unsolicited neighbor advertisement case. So far, your concept/patchset does not seem to apply any optimizations for those packets, does it? They are still flooded unconditionally, right?
What do you think, would it make sense to prevent unsolicited neighbor advertisements from being forwarded into the mesh network? Or would that create certain issues?
Cheers, Linus
PS: I'm very excited about your work on the IPv6 ND DAT. This is going to help us a lot in our community mesh networks. So thanks a lot for the time you've spent on this so far!
Hi Linus,
Unfortunately I won't work on this anymore because it will take another 1-2 months. I had a vacation last week and now I started school again. I will start something new and this takes too much time.
Sorry, Mihail
On 30 September 2013 23:38, Linus Lüssing linus.luessing@web.de wrote:
PS: I'm very excited about your work on the IPv6 ND DAT. This is going to help us a lot in our community mesh networks. So thanks a lot for the time you've spent on this so far!
Hi Mihail,
On Fri, Oct 04, 2013 at 09:28:54PM +0300, Mihail Costea wrote:
Hi Linus,
Unfortunately I won't work on this anymore because it will take another 1-2 months. I had a vacation last week and now I started school again. I will start something new and this takes too much time.
does this mean you will not be able to complete this work? Too bad :( It was a really nice thing..
Cheers,
Unfortunately yes. With everything it will take too many weeks and I'd like to do other things too.
Sorry, Mihail
On 5 October 2013 10:14, Antonio Quartulli antonio@meshcoding.com wrote:
Hi Mihail,
On Fri, Oct 04, 2013 at 09:28:54PM +0300, Mihail Costea wrote:
Hi Linus,
Unfortunately I won't work on this anymore because it will take another 1-2 months. I had a vacation last week and now I started school again. I will start something new and this takes too much time.
does this mean you will not be able to complete this work? Too bad :( It was a really nice thing..
Cheers,
-- Antonio Quartulli
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.
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. 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 */ + dat_entry->extra_data = NULL;
+ /* alloc data */ dat_entry->data = kmalloc(data_size, GFP_ATOMIC); if (!dat_entry->data) goto out; memcpy(dat_entry->data, data, data_size);
+ /* alloc extra data */ + if (extra_data) { + dat_entry->extra_data = kmalloc(extra_data_size, GFP_ATOMIC); + if (!dat_entry->extra_data) + goto out; + memcpy(dat_entry->extra_data, extra_data, extra_data_size); + } + dat_entry->type = data_type; dat_entry->vid = vid; memcpy(dat_entry->mac_addr, mac_addr, ETH_ALEN); @@ -996,7 +1021,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) { if ((pkt_dir_type == BATADV_DAT_OUTGOING_PKT_REQUEST || pkt_dir_type == BATADV_DAT_INCOMING_PKT_REQUEST || @@ -1371,11 +1397,13 @@ batadv_dat_create_ndisc_na(struct batadv_priv *bat_priv, struct batadv_dat_entry *dat_entry, uint8_t *hw_src, void *ip_src, void *ip_dst) { - /* TODO calculate router and override parameters */ + struct batadv_dat_ipv6_extra_data *ipv6_extra_data = + (struct batadv_dat_ipv6_extra_data *)dat_entry->extra_data; return batadv_ndisc_create_na(bat_priv->soft_iface, ip_src, ip_dst, hw_src, dat_entry->mac_addr, - 0, 1, 1); + ipv6_extra_data->router, 1, + ipv6_extra_data->override); }
/** @@ -1426,8 +1454,13 @@ 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) { + struct icmp6hdr *icmp6hdr; + struct batadv_dat_ipv6_extra_data *ipv6_extra_data; + size_t extra_data_size = sizeof(struct batadv_dat_ipv6_extra_data); + if (batadv_ndisc_is_valid(bat_priv, skb, hdr_size, pkt_type)) { if ((pkt_dir_type == BATADV_DAT_OUTGOING_PKT_REQUEST || pkt_dir_type == BATADV_DAT_INCOMING_PKT_REQUEST || @@ -1440,11 +1473,27 @@ static bool batadv_dat_snoop_ndisc_pkt(struct batadv_priv *bat_priv, hw_src, ip_dst)) return false;
- if (pkt_dir_type != BATADV_DAT_BROADCAST_FALLBACK) + if (pkt_dir_type != BATADV_DAT_BROADCAST_FALLBACK) { + /* init extra data + * TODO: this can be NULL if entry should + * be ignored until extra data is updated */ + *extra_data = kmalloc(extra_data_size, + GFP_ATOMIC); + ipv6_extra_data = + (struct batadv_dat_ipv6_extra_data *) + *extra_data; + ipv6_extra_data->router = 0; + ipv6_extra_data->override = 1; + batadv_dbg(BATADV_DBG_DAT, bat_priv, "Parsing NS = [src: %pM / %pI6c -> " - "target: %pM / %pI6c]\n", - *hw_src, *ip_src, *hw_dst, *ip_dst); + "target: %pM / %pI6c " + "(router: %i, override %i)]\n", + *hw_src, *ip_src, *hw_dst, *ip_dst, + ipv6_extra_data->router, + ipv6_extra_data->override); + } + return true; }
@@ -1458,15 +1507,27 @@ static bool batadv_dat_snoop_ndisc_pkt(struct batadv_priv *bat_priv, hw_src, ip_src)) return false;
- batadv_dbg(BATADV_DBG_DAT, bat_priv, - "Parsing NA = [src: %pM / %pI6c -> " - "target: %pM / %pI6c]\n", - *hw_src, *ip_src, *hw_dst, *ip_dst); - return true; - /* ignore multicast address for unsolicited NA */ if (ipv6_addr_is_multicast(*ip_dst)) *ip_dst = NULL; + + /* get extra data */ + *extra_data = kmalloc(extra_data_size, GFP_ATOMIC); + ipv6_extra_data = (struct batadv_dat_ipv6_extra_data *) + *extra_data; + icmp6hdr = (struct icmp6hdr *)(skb->data + hdr_size + + ETH_HLEN + sizeof(struct ipv6hdr)); + ipv6_extra_data->router = icmp6hdr->icmp6_router; + ipv6_extra_data->override = icmp6hdr->icmp6_override; + + batadv_dbg(BATADV_DBG_DAT, bat_priv, + "Parsing NA = [src: %pM / %pI6c -> " + "target: %pM / %pI6c " + "(router: %i, override %i)]\n", + *hw_src, *ip_src, *hw_dst, *ip_dst, + ipv6_extra_data->router, + ipv6_extra_data->override); + return true; } }
@@ -1532,6 +1593,7 @@ bool batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, int hdr_size = 0; unsigned short vid; struct batadv_dat_pair_type dat_pair_type; + void *extra_data = NULL;
if (!atomic_read(&bat_priv->distributed_arp_table)) goto out; @@ -1547,11 +1609,11 @@ bool batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, if (!batadv_dat_types_info[dat_pair_type.data_type].snoop_pkt( bat_priv, skb, hdr_size, dat_pair_type.pkt_type, BATADV_DAT_OUTGOING_PKT_REQUEST, - &hw_src, &ip_src, &hw_dst, &ip_dst)) + &hw_src, &ip_src, &hw_dst, &ip_dst, &extra_data)) goto out;
- batadv_dat_entry_add(bat_priv, ip_src, dat_pair_type.data_type, - hw_src, vid); + batadv_dat_entry_add(bat_priv, ip_src, extra_data, + dat_pair_type.data_type, hw_src, vid);
dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst, dat_pair_type.data_type, vid); @@ -1596,6 +1658,7 @@ bool batadv_dat_snoop_outgoing_pkt_request(struct batadv_priv *bat_priv, out: if (dat_entry) batadv_dat_entry_free_ref(dat_entry); + kfree(extra_data); return ret; }
@@ -1619,6 +1682,7 @@ bool batadv_dat_snoop_incoming_pkt_request(struct batadv_priv *bat_priv, unsigned short vid; int err; struct batadv_dat_pair_type dat_pair_type; + void *extra_data = NULL;
if (!atomic_read(&bat_priv->distributed_arp_table)) goto out; @@ -1631,11 +1695,11 @@ bool batadv_dat_snoop_incoming_pkt_request(struct batadv_priv *bat_priv, if (!batadv_dat_types_info[dat_pair_type.data_type].snoop_pkt( bat_priv, skb, hdr_size, dat_pair_type.pkt_type, BATADV_DAT_INCOMING_PKT_REQUEST, - &hw_src, &ip_src, &hw_dst, &ip_dst)) + &hw_src, &ip_src, &hw_dst, &ip_dst, &extra_data)) goto out;
- batadv_dat_entry_add(bat_priv, ip_src, dat_pair_type.data_type, - hw_src, vid); + batadv_dat_entry_add(bat_priv, ip_src, extra_data, + dat_pair_type.data_type, hw_src, vid);
dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst, dat_pair_type.data_type, vid); @@ -1667,6 +1731,7 @@ out: batadv_dat_entry_free_ref(dat_entry); if (ret) kfree_skb(skb); + kfree(extra_data); return ret; }
@@ -1684,6 +1749,7 @@ void batadv_dat_snoop_outgoing_pkt_reply(struct batadv_priv *bat_priv, int hdr_size = 0; unsigned short vid; struct batadv_dat_pair_type dat_pair_type; + void *extra_data = NULL;
if (!atomic_read(&bat_priv->distributed_arp_table)) return; @@ -1696,25 +1762,26 @@ void batadv_dat_snoop_outgoing_pkt_reply(struct batadv_priv *bat_priv, if (!batadv_dat_types_info[dat_pair_type.data_type].snoop_pkt( bat_priv, skb, hdr_size, dat_pair_type.pkt_type, BATADV_DAT_OUTGOING_PKT_REPLY, - &hw_src, &ip_src, &hw_dst, &ip_dst)) + &hw_src, &ip_src, &hw_dst, &ip_dst, &extra_data)) return;
/* Send the ARP reply to the candidates for both the IP addresses that * the node obtained from the ARP reply */ - batadv_dat_entry_add(bat_priv, ip_src, dat_pair_type.data_type, - hw_src, vid); + batadv_dat_entry_add(bat_priv, ip_src, extra_data, + dat_pair_type.data_type, hw_src, vid); batadv_dat_send_data(bat_priv, skb, ip_src, dat_pair_type.data_type, BATADV_P_DAT_DHT_PUT);
/* not a solicited advertisement (see snooping mechanism) */ if (ip_dst) { - batadv_dat_entry_add(bat_priv, ip_dst, dat_pair_type.data_type, - hw_dst, vid); + batadv_dat_entry_add(bat_priv, ip_dst, extra_data, + dat_pair_type.data_type, hw_dst, vid); batadv_dat_send_data(bat_priv, skb, ip_dst, dat_pair_type.data_type, BATADV_P_DAT_DHT_PUT); } + kfree(extra_data); }
/** @@ -1732,6 +1799,7 @@ bool batadv_dat_snoop_incoming_pkt_reply(struct batadv_priv *bat_priv, bool ret = false; unsigned short vid; struct batadv_dat_pair_type dat_pair_type; + void *extra_data = NULL;
if (!atomic_read(&bat_priv->distributed_arp_table)) goto out; @@ -1744,17 +1812,17 @@ bool batadv_dat_snoop_incoming_pkt_reply(struct batadv_priv *bat_priv, if (!batadv_dat_types_info[dat_pair_type.data_type].snoop_pkt( bat_priv, skb, hdr_size, dat_pair_type.pkt_type, BATADV_DAT_INCOMING_PKT_REPLY, - &hw_src, &ip_src, &hw_dst, &ip_dst)) + &hw_src, &ip_src, &hw_dst, &ip_dst, &extra_data)) goto out;
/* Update our internal cache with both the IP addresses the node got * within the ARP reply */ - batadv_dat_entry_add(bat_priv, ip_src, dat_pair_type.data_type, - hw_src, vid); + batadv_dat_entry_add(bat_priv, ip_src, extra_data, + dat_pair_type.data_type, hw_src, vid); /* not a solicited advertisement (see snooping mechanism) */ if (ip_dst) - batadv_dat_entry_add(bat_priv, ip_dst, + batadv_dat_entry_add(bat_priv, ip_dst, extra_data, dat_pair_type.data_type, hw_dst, vid);
/* if this REPLY is directed to a client of mine, let's deliver the @@ -1764,6 +1832,7 @@ bool batadv_dat_snoop_incoming_pkt_reply(struct batadv_priv *bat_priv, out: if (ret) kfree_skb(skb); + kfree(extra_data); /* if ret == false -> packet has to be delivered to the interface */ return ret; } @@ -1786,6 +1855,7 @@ bool batadv_dat_drop_broadcast_packet(struct batadv_priv *bat_priv, unsigned short vid; struct batadv_dat_pair_type dat_pair_type; char dbg_data[BATADV_DAT_DATA_MAX_LEN]; + void *extra_data = NULL;
if (!atomic_read(&bat_priv->distributed_arp_table)) goto out; @@ -1806,7 +1876,7 @@ bool batadv_dat_drop_broadcast_packet(struct batadv_priv *bat_priv, bat_priv, forw_packet->skb, hdr_size, dat_pair_type.pkt_type, BATADV_DAT_BROADCAST_FALLBACK, - NULL, NULL, NULL, &ip_dst)) + NULL, NULL, NULL, &ip_dst, &extra_data)) goto out;
dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst, @@ -1828,5 +1898,6 @@ bool batadv_dat_drop_broadcast_packet(struct batadv_priv *bat_priv, out: if (dat_entry) batadv_dat_entry_free_ref(dat_entry); + kfree(extra_data); return ret; } diff --git a/types.h b/types.h index 6ad4500..5301af1 100644 --- a/types.h +++ b/types.h @@ -931,7 +931,9 @@ struct batadv_algo_ops { /** * struct batadv_dat_entry - it is a single entry of batman-adv ARP backend. It * is used to stored ARP entries needed for the global DAT cache - * @data: the data corresponding to this DAT entry + * @data: the data corresponding to this DAT entry; this is used for comparison + * between DAT entries + * @extra_data: extra data for this DAT entry (NULL if none) * @type: the type corresponding to this DAT entry * @mac_addr: the MAC address associated to the stored IPv4 * @vid: the vlan ID associated to this entry @@ -942,6 +944,7 @@ struct batadv_algo_ops { */ struct batadv_dat_entry { void *data; + void *extra_data; uint8_t type; uint8_t mac_addr[ETH_ALEN]; unsigned short vid; @@ -983,6 +986,16 @@ enum batadv_dat_pkt_types { };
/** + * batadv_dat_ipv6_extra_data - defines extra data for IPv6 + * @router: entry is router + * @override: override flag (see RFC2461) + */ +struct batadv_dat_ipv6_extra_data { + __u8 router:1, + override:1; +}; + +/** * batadv_dat_pair_type - types needed for a dat packet * @data_type the data type (negative values represents invalid values) * @pkt_type: the packet type @@ -995,11 +1008,13 @@ struct batadv_dat_pair_type { /** * batadv_dat_type_info - info needed for a DAT type data * @size: the size of the type data + * @extra_size: the size of the extra data (can be 0 if no extra data) * @str_fmt: string format used by the data * @snoop_pkt: function used to snoop addresses from a packet */ struct batadv_dat_type_info { size_t size; + size_t extra_size; char *str_fmt; /** * snoop_pkt - snooping mechanism for all packets that participate @@ -1013,6 +1028,8 @@ struct batadv_dat_type_info { * @ip_src: source IP * @hw_dst: destination HW Address * @ip_dst: destination IP + * @extra_data: extra data that must be snooped; it is either allocated + * dynamically or NULL inside the function; must be freed outside * * Any address pointer can be null if there is no need to memorize * calculate that address. @@ -1023,7 +1040,7 @@ struct batadv_dat_type_info { 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);
/** * batadv_dat_create_skb - creates a skb as a reply to a message request
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,@@ -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),@@ -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,
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
On Wed, Aug 14, 2013 at 06:51:32AM -0700, Mihail Costea wrote:
On 10 August 2013 06:20, Antonio Quartulli ordex@autistici.org wrote:
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.
These are net/ specific style requirements. Try this before running checkpatch.pl:
sed -i "s#^--- a/#--- a/net/batman-adv/#;s#^+++ b/#+++ b/net/batman-adv/#" *.patch
Cheers, Linus
On Wed, Aug 14, 2013 at 05:42:19PM +0200, Linus Lüssing wrote:
On Wed, Aug 14, 2013 at 06:51:32AM -0700, Mihail Costea wrote:
On 10 August 2013 06:20, Antonio Quartulli ordex@autistici.org wrote:
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.
These are net/ specific style requirements. Try this before running checkpatch.pl:
sed -i "s#^--- a/#--- a/net/batman-adv/#;s#^+++ b/#+++ b/net/batman-adv/#" *.patch
Right, checkpatch will trigger the warning only if the patches are coming from the net/ folder inside the kernel root. Linus's suggestion will help you to make checkpatch thinks that the patches are coming from net/.
Cheers,
On Mon, Jul 08, 2013 at 03:12:40AM +0300, mihail.costea2005@gmail.com wrote:
From: Mihail Costea mihail.costea90@gmail.com
Hi Mihail,
sorry for the very long delay but I'm currently out of my office, so I'm not spending a lot of time on batman-adv.
Anyway, I'll try to review your patches as soon as possible!
Cheers,
On 23 July 2013 00:27, Antonio Quartulli ordex@autistici.org wrote:
On Mon, Jul 08, 2013 at 03:12:40AM +0300, mihail.costea2005@gmail.com wrote:
From: Mihail Costea mihail.costea90@gmail.com
Hi Mihail,
sorry for the very long delay but I'm currently out of my office, so I'm not spending a lot of time on batman-adv.
Anyway, I'll try to review your patches as soon as possible!
Hi Antonio,
No problem, there's no hurry.
Cheers, Mihail
Hi Mihail,
sorry for the very looong delay :) I'm finally sitting at my new location and I can give you some feedback on your nice work. Comments are inline.
On Mon, Jul 08, 2013 at 03:12:40AM +0300, mihail.costea2005@gmail.com wrote:
From: Mihail Costea mihail.costea90@gmail.com
Mades DAT support more types by making its data a void*, adding type field to dat_entry and adding data_type to necessary functions. This change is needed in order to make DAT support any type of data, like IPv6 too.
Adds generic function for transforming DAT data to string. The function is used in order to avoid defining different debug messages for different DAT data types. For example, if we had IPv6 as a DAT data, then "%pI4" should be "%pI6c", but all the other text of the debug message would be the same. Also everything is memorized in a struct in order to avoid further switch cases for all types.
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 | 197 +++++++++++++++++++++++++++++++++++------------ distributed-arp-table.h | 1 + types.h | 24 +++++- 3 files changed, 169 insertions(+), 53 deletions(-)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index f2543c2..90565d0 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -31,9 +31,32 @@ #include "types.h" #include "translation-table.h"
+static struct batadv_dat_type_info batadv_dat_types_info[] = {
- {
.size = sizeof(__be32),.str_fmt = "%pI4",- },
+};
static void batadv_dat_purge(struct work_struct *work);
/**
- batadv_dat_data_to_str: transforms DAT data to string
- @data: the DAT data
- @type: type of data
- @buf: the buf where the data string is stored
- @buf_len: buf length
- Returns buf.
- */
+static char *batadv_dat_data_to_str(void *data, uint8_t type,
char *buf, size_t buf_len)+{
- snprintf(buf, buf_len, batadv_dat_types_info[type].str_fmt, data);
+return buf;
alignment. Remember to check your patches with "checkpatch.pl --strict" before sending. It will tell you about these hidden mistakes.
+}
+/**
- batadv_dat_start_timer - initialise the DAT periodic worker
- @bat_priv: the bat priv with all the soft interface information
*/ @@ -45,6 +68,19 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv) }
/**
- batadv_dat_entry_free_ref_rcu - free a dat entry using its rcu
- @rcu: the dat entry rcu
- */
+static void batadv_dat_entry_free_ref_rcu(struct rcu_head *rcu) +{
- struct batadv_dat_entry *dat_entry;
- dat_entry = container_of(rcu, struct batadv_dat_entry, rcu);
- kfree(dat_entry->data);
- kfree(dat_entry);
+}
+/**
- batadv_dat_entry_free_ref - decrement the dat_entry refcounter and possibly
- free it
- @dat_entry: the entry to free
@@ -52,7 +88,7 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv) static void batadv_dat_entry_free_ref(struct batadv_dat_entry *dat_entry) { if (atomic_dec_and_test(&dat_entry->refcount))
kfree_rcu(dat_entry, rcu);
call_rcu(&dat_entry->rcu, batadv_dat_entry_free_ref_rcu);}
since you are not using the kfree_rcu() function anymore, you should also delete the compatibility function we have in compat.c (and its declaration in compat.h).
/** @@ -136,12 +172,21 @@ static void batadv_dat_purge(struct work_struct *work)
- Returns 1 if the two entries are the same, 0 otherwise.
*/ -static int batadv_compare_dat(const struct hlist_node *node, const void *data2) +static int batadv_compare_dat(const struct hlist_node *node, const void *data2) {
- const void *data1 = container_of(node, struct batadv_dat_entry,
hash_entry);
- struct batadv_dat_entry *dat_entry1 =
container_of(node, struct batadv_dat_entry,hash_entry);- struct batadv_dat_entry *dat_entry2 =
container_of(data2,struct batadv_dat_entry, data);
These assignments are really ugly :-P. Please make the assignments after the declarations. They will look much better.
- size_t data_size = batadv_dat_types_info[dat_entry1->type].size;
- return (memcmp(data1, data2, sizeof(__be32)) == 0 ? 1 : 0);
- if (dat_entry1->type != dat_entry2->type)
return 0;- return (memcmp(dat_entry1->data, dat_entry2->data,
data_size) == 0 ? 1 : 0);}
/** @@ -198,8 +243,9 @@ static __be32 batadv_arp_ip_dst(struct sk_buff *skb, int hdr_size) }
/**
- batadv_hash_dat - compute the hash value for an IP address
- batadv_hash_dat - compute the hash value for a DAT data
- @data: data to hash
- @data_type: type of data
- @size: size of the hash table
- Returns the selected index in the hash table for the given data.
@@ -209,7 +255,8 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size) uint32_t hash = 0; const struct batadv_dat_entry *dat = data;
- hash = batadv_hash_bytes(hash, &dat->ip, sizeof(dat->ip));
hash = batadv_hash_bytes(hash, dat->data,
batadv_dat_types_info[dat->type].size);hash = batadv_hash_bytes(hash, &dat->vid, sizeof(dat->vid));
hash += (hash << 3);
@@ -223,32 +270,40 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size)
- batadv_dat_entry_hash_find - look for a given dat_entry in the local hash
- table
- @bat_priv: the bat priv with all the soft interface information
- @ip: search key
- @data: search key
*/
- @data_type: type of data
- @vid: VLAN identifier
- Returns the dat_entry if found, NULL otherwise.
static struct batadv_dat_entry * -batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip,
unsigned short vid)+batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, void *data,
uint8_t data_type, unsigned short vid){ struct hlist_head *head; struct batadv_dat_entry to_find, *dat_entry, *dat_entry_tmp = NULL; struct batadv_hashtable *hash = bat_priv->dat.hash;
- uint32_t index;
uint32_t index, data_size = batadv_dat_types_info[data_type].size;
if (!hash) return NULL;
- to_find.ip = ip;
- to_find.data = kmalloc(data_size, GFP_ATOMIC);
- if (!to_find.data)
return NULL;- memcpy(to_find.data, data, data_size);
why do you create a copy of the data? It is going to be read only by the hashing function. I think you can reuse the same data passed as argument no? You can directly assign "to_find.data = data;", can't you?
to_find.type = data_type; to_find.vid = vid;
index = batadv_hash_dat(&to_find, hash->size); head = &hash->table[index];
kfree(to_find.data);
..consequently, this kfree can be deleted.
rcu_read_lock(); hlist_for_each_entry_rcu(dat_entry, head, hash_entry) {
if (dat_entry->ip != ip)
if (dat_entry->type != data_type)continue;if (memcmp(dat_entry->data, data, data_size)) continue;if (!atomic_inc_not_zero(&dat_entry->refcount))
@@ -265,25 +320,30 @@ batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip, /**
- 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
- @ip: ipv4 to add/edit
- @mac_addr: mac address to assign to the given ipv4
- @data: the data to add/edit
- @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, __be32 ip,
uint8_t *mac_addr, unsigned short vid)+static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *data,
uint8_t data_type, uint8_t *mac_addr,unsigned short vid)
here I realised we have a small conceptual problem. what we are calling "data" is not the real data, but is the "key". The data instead is supposed to be what we store in the entry: the mac address (that could possibly be generalised too..).
I think generalising the data is not very important now. We have an ARP/ND table for now, so we can assume we only want to store MAC addresses, but I would suggest to rename the member (and so the variables) from "data" to "key". It makes much more sense in terms of DHT. (Tell me if you think I am not right).
From now on I'll try to speed up the reviews so that we can get this thing done soonish ;)
Cheers,
Hi Antonio,
Thanks for the update. I will do the changes as soon as possible. I haven't sync in a while with the code base, so I think the whole changes might take a while to implement (+ I'm also working full time and trying to finish the paper for my Diploma Project). I hope that in 2 weeks I will finish everything because I can mostly work on my weekends. I don't have to finish everything by the time I present the Diploma Project, but once I have presented it, I can dedicate all my time here (at that time I would have finished my internship too).
But maybe that is done by then :).
Thanks, Mihail
On 10 August 2013 04:03, Antonio Quartulli ordex@autistici.org wrote:
Hi Mihail,
sorry for the very looong delay :) I'm finally sitting at my new location and I can give you some feedback on your nice work. Comments are inline.
On Mon, Jul 08, 2013 at 03:12:40AM +0300, mihail.costea2005@gmail.com wrote:
From: Mihail Costea mihail.costea90@gmail.com
Mades DAT support more types by making its data a void*, adding type field to dat_entry and adding data_type to necessary functions. This change is needed in order to make DAT support any type of data, like IPv6 too.
Adds generic function for transforming DAT data to string. The function is used in order to avoid defining different debug messages for different DAT data types. For example, if we had IPv6 as a DAT data, then "%pI4" should be "%pI6c", but all the other text of the debug message would be the same. Also everything is memorized in a struct in order to avoid further switch cases for all types.
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 | 197 +++++++++++++++++++++++++++++++++++------------ distributed-arp-table.h | 1 + types.h | 24 +++++- 3 files changed, 169 insertions(+), 53 deletions(-)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index f2543c2..90565d0 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -31,9 +31,32 @@ #include "types.h" #include "translation-table.h"
+static struct batadv_dat_type_info batadv_dat_types_info[] = {
{.size = sizeof(__be32),.str_fmt = "%pI4",},+};
static void batadv_dat_purge(struct work_struct *work);
/**
- batadv_dat_data_to_str: transforms DAT data to string
- @data: the DAT data
- @type: type of data
- @buf: the buf where the data string is stored
- @buf_len: buf length
- Returns buf.
- */
+static char *batadv_dat_data_to_str(void *data, uint8_t type,
char *buf, size_t buf_len)+{
snprintf(buf, buf_len, batadv_dat_types_info[type].str_fmt, data);+return buf;
alignment. Remember to check your patches with "checkpatch.pl --strict" before sending. It will tell you about these hidden mistakes.
+}
+/**
- batadv_dat_start_timer - initialise the DAT periodic worker
- @bat_priv: the bat priv with all the soft interface information
*/ @@ -45,6 +68,19 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv) }
/**
- batadv_dat_entry_free_ref_rcu - free a dat entry using its rcu
- @rcu: the dat entry rcu
- */
+static void batadv_dat_entry_free_ref_rcu(struct rcu_head *rcu) +{
struct batadv_dat_entry *dat_entry;dat_entry = container_of(rcu, struct batadv_dat_entry, rcu);kfree(dat_entry->data);kfree(dat_entry);+}
+/**
- batadv_dat_entry_free_ref - decrement the dat_entry refcounter and possibly
- free it
- @dat_entry: the entry to free
@@ -52,7 +88,7 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv) static void batadv_dat_entry_free_ref(struct batadv_dat_entry *dat_entry) { if (atomic_dec_and_test(&dat_entry->refcount))
kfree_rcu(dat_entry, rcu);
call_rcu(&dat_entry->rcu, batadv_dat_entry_free_ref_rcu);}
since you are not using the kfree_rcu() function anymore, you should also delete the compatibility function we have in compat.c (and its declaration in compat.h).
/** @@ -136,12 +172,21 @@ static void batadv_dat_purge(struct work_struct *work)
- Returns 1 if the two entries are the same, 0 otherwise.
*/ -static int batadv_compare_dat(const struct hlist_node *node, const void *data2) +static int batadv_compare_dat(const struct hlist_node *node, const void *data2) {
const void *data1 = container_of(node, struct batadv_dat_entry,hash_entry);
struct batadv_dat_entry *dat_entry1 =container_of(node, struct batadv_dat_entry,hash_entry);struct batadv_dat_entry *dat_entry2 =container_of(data2,struct batadv_dat_entry, data);These assignments are really ugly :-P. Please make the assignments after the declarations. They will look much better.
size_t data_size = batadv_dat_types_info[dat_entry1->type].size;
return (memcmp(data1, data2, sizeof(__be32)) == 0 ? 1 : 0);
if (dat_entry1->type != dat_entry2->type)return 0;return (memcmp(dat_entry1->data, dat_entry2->data,data_size) == 0 ? 1 : 0);}
/** @@ -198,8 +243,9 @@ static __be32 batadv_arp_ip_dst(struct sk_buff *skb, int hdr_size) }
/**
- batadv_hash_dat - compute the hash value for an IP address
- batadv_hash_dat - compute the hash value for a DAT data
- @data: data to hash
- @data_type: type of data
- @size: size of the hash table
- Returns the selected index in the hash table for the given data.
@@ -209,7 +255,8 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size) uint32_t hash = 0; const struct batadv_dat_entry *dat = data;
hash = batadv_hash_bytes(hash, &dat->ip, sizeof(dat->ip));
hash = batadv_hash_bytes(hash, dat->data,batadv_dat_types_info[dat->type].size); hash = batadv_hash_bytes(hash, &dat->vid, sizeof(dat->vid)); hash += (hash << 3);@@ -223,32 +270,40 @@ static uint32_t batadv_hash_dat(const void *data, uint32_t size)
- batadv_dat_entry_hash_find - look for a given dat_entry in the local hash
- table
- @bat_priv: the bat priv with all the soft interface information
- @ip: search key
- @data: search key
*/
- @data_type: type of data
- @vid: VLAN identifier
- Returns the dat_entry if found, NULL otherwise.
static struct batadv_dat_entry * -batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip,
unsigned short vid)+batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, void *data,
uint8_t data_type, unsigned short vid){ struct hlist_head *head; struct batadv_dat_entry to_find, *dat_entry, *dat_entry_tmp = NULL; struct batadv_hashtable *hash = bat_priv->dat.hash;
uint32_t index;
uint32_t index, data_size = batadv_dat_types_info[data_type].size; if (!hash) return NULL;
to_find.ip = ip;
to_find.data = kmalloc(data_size, GFP_ATOMIC);if (!to_find.data)return NULL;memcpy(to_find.data, data, data_size);why do you create a copy of the data? It is going to be read only by the hashing function. I think you can reuse the same data passed as argument no? You can directly assign "to_find.data = data;", can't you?
to_find.type = data_type; to_find.vid = vid; index = batadv_hash_dat(&to_find, hash->size); head = &hash->table[index];kfree(to_find.data);..consequently, this kfree can be deleted.
rcu_read_lock(); hlist_for_each_entry_rcu(dat_entry, head, hash_entry) {
if (dat_entry->ip != ip)
if (dat_entry->type != data_type)continue;if (memcmp(dat_entry->data, data, data_size)) continue; if (!atomic_inc_not_zero(&dat_entry->refcount))@@ -265,25 +320,30 @@ batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip, /**
- 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
- @ip: ipv4 to add/edit
- @mac_addr: mac address to assign to the given ipv4
- @data: the data to add/edit
- @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, __be32 ip,
uint8_t *mac_addr, unsigned short vid)+static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *data,
uint8_t data_type, uint8_t *mac_addr,unsigned short vid)here I realised we have a small conceptual problem. what we are calling "data" is not the real data, but is the "key". The data instead is supposed to be what we store in the entry: the mac address (that could possibly be generalised too..).
I think generalising the data is not very important now. We have an ARP/ND table for now, so we can assume we only want to store MAC addresses, but I would suggest to rename the member (and so the variables) from "data" to "key". It makes much more sense in terms of DHT. (Tell me if you think I am not right).
From now on I'll try to speed up the reviews so that we can get this thing done soonish ;)
Cheers,
-- Antonio Quartulli
..each of us alone is worth nothing.. Ernesto "Che" Guevara
On Sat, Aug 10, 2013 at 12:01:02PM -0700, Mihail Costea wrote:
Hi Antonio,
Thanks for the update. I will do the changes as soon as possible. I haven't sync in a while with the code base, so I think the whole changes might take a while to implement (+ I'm also working full time and trying to finish the paper for my Diploma Project). I hope that in 2 weeks I will finish everything because I can mostly work on my weekends. I don't have to finish everything by the time I present the Diploma Project, but once I have presented it, I can dedicate all my time here (at that time I would have finished my internship too).
But maybe that is done by then :).
Oh, ok, then good luck for your Diploma Project and your internship! Remember that you can also join the IRC channel if you want to ask/discuss whatever you may need for the implementation!
Cheers,
Hi Antonio,
Is it possible to send the new model for the generalization as a patch first (the part without IPv6), or maybe everything as a patch as once? Having 5-6 patches to rewrite every time something changes makes the development harder.
Thanks, Mihail
On 10 August 2013 23:36, Antonio Quartulli ordex@autistici.org wrote:
On Sat, Aug 10, 2013 at 12:01:02PM -0700, Mihail Costea wrote:
Hi Antonio,
Thanks for the update. I will do the changes as soon as possible. I haven't sync in a while with the code base, so I think the whole changes might take a while to implement (+ I'm also working full time and trying to finish the paper for my Diploma Project). I hope that in 2 weeks I will finish everything because I can mostly work on my weekends. I don't have to finish everything by the time I present the Diploma Project, but once I have presented it, I can dedicate all my time here (at that time I would have finished my internship too).
But maybe that is done by then :).
Oh, ok, then good luck for your Diploma Project and your internship! Remember that you can also join the IRC channel if you want to ask/discuss whatever you may need for the implementation!
Cheers,
-- Antonio Quartulli
..each of us alone is worth nothing.. Ernesto "Che" Guevara
On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
Hi Antonio,
Is it possible to send the new model for the generalization as a patch first (the part without IPv6), or maybe everything as a patch as once? Having 5-6 patches to rewrite every time something changes makes the development harder.
Which patches do you want to merge? If they are ready it is better to send them as PATCH to the ml and then base your work on top of them assuming they will be merged at some point.
Cheers,
p.s. welcome to the rebase nightmare ;)
On 9 September 2013 17:53, Antonio Quartulli antonio@meshcoding.com wrote:
On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
Hi Antonio,
Is it possible to send the new model for the generalization as a patch first (the part without IPv6), or maybe everything as a patch as once? Having 5-6 patches to rewrite every time something changes makes the development harder.
Which patches do you want to merge? If they are ready it is better to send them as PATCH to the ml and then base your work on top of them assuming they will be merged at some point.
I took a small rest last week and now I'm redoing everything. I was thinking about sending the first part for merging (the one with generalization the DAT). That is the one that needs most rewriting every time because it affects the most existing code. The rest I think I can send them together.
Cheers,
p.s. welcome to the rebase nightmare ;)
-- Antonio Quartulli
..each of us alone is worth nothing.. Ernesto "Che" Guevara
Thanls, Mihail
On Tue, Sep 10, 2013 at 07:35:34AM +0300, Mihail Costea wrote:
On 9 September 2013 17:53, Antonio Quartulli antonio@meshcoding.com wrote:
On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
Hi Antonio,
Is it possible to send the new model for the generalization as a patch first (the part without IPv6), or maybe everything as a patch as once? Having 5-6 patches to rewrite every time something changes makes the development harder.
Which patches do you want to merge? If they are ready it is better to send them as PATCH to the ml and then base your work on top of them assuming they will be merged at some point.
I took a small rest last week and now I'm redoing everything. I was thinking about sending the first part for merging (the one with generalization the DAT). That is the one that needs most rewriting every time because it affects the most existing code. The rest I think I can send them together.
I understood. Well, the problem is also that this period is a sort of "transition" because batman-adv is getting changed in some of its most important part and we would like all the "new features" that are not essential to come after these changes. We still need to merge two (or two and a bit) patchsets before we can start merging other things.
This means that before your patchset gets merged we have to wait a bit more. I think it would be better to do this: - for a while you don't care about rebasing on top of master - when you have a some code ready to be reviewed you can put in on a remote git repo that we can check (e.g. github?) - we/I review the code so that we make it ready to be sent as PATCH - when these two (and a bit) patchsets are merged you can do the final rebase and send them to the ml for merging.
What do you think? In this way we same some painful rebase cycles, but we can continue preparing the code.
Cheers,
On 10 September 2013 08:38, Antonio Quartulli antonio@meshcoding.com wrote:
On Tue, Sep 10, 2013 at 07:35:34AM +0300, Mihail Costea wrote:
On 9 September 2013 17:53, Antonio Quartulli antonio@meshcoding.com wrote:
On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
Hi Antonio,
Is it possible to send the new model for the generalization as a patch first (the part without IPv6), or maybe everything as a patch as once? Having 5-6 patches to rewrite every time something changes makes the development harder.
Which patches do you want to merge? If they are ready it is better to send them as PATCH to the ml and then base your work on top of them assuming they will be merged at some point.
I took a small rest last week and now I'm redoing everything. I was thinking about sending the first part for merging (the one with generalization the DAT). That is the one that needs most rewriting every time because it affects the most existing code. The rest I think I can send them together.
I understood. Well, the problem is also that this period is a sort of "transition" because batman-adv is getting changed in some of its most important part and we would like all the "new features" that are not essential to come after these changes. We still need to merge two (or two and a bit) patchsets before we can start merging other things.
This means that before your patchset gets merged we have to wait a bit more. I think it would be better to do this:
- for a while you don't care about rebasing on top of master
- when you have a some code ready to be reviewed you can put in on a remote git repo that we can check (e.g. github?)
- we/I review the code so that we make it ready to be sent as PATCH
- when these two (and a bit) patchsets are merged you can do the final rebase and send them to the ml for merging.
What do you think? In this way we same some painful rebase cycles, but we can continue preparing the code.
I understand, but it should be done similar? Like multiple patches? The idea is that I might add some patches and then find a bug that was in an old patch. That means to find the patch with the bug, resolve it, and re-patch everything after it.
It would be easier to do the changes directly on the existing code than restart everything from scratch. I'm not sure if this is what you meant by using github.
On Tue, Sep 10, 2013 at 03:45:44PM +0300, Mihail Costea wrote:
On 10 September 2013 08:38, Antonio Quartulli antonio@meshcoding.com wrote:
On Tue, Sep 10, 2013 at 07:35:34AM +0300, Mihail Costea wrote:
On 9 September 2013 17:53, Antonio Quartulli antonio@meshcoding.com wrote:
On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
Hi Antonio,
Is it possible to send the new model for the generalization as a patch first (the part without IPv6), or maybe everything as a patch as once? Having 5-6 patches to rewrite every time something changes makes the development harder.
Which patches do you want to merge? If they are ready it is better to send them as PATCH to the ml and then base your work on top of them assuming they will be merged at some point.
I took a small rest last week and now I'm redoing everything. I was thinking about sending the first part for merging (the one with generalization the DAT). That is the one that needs most rewriting every time because it affects the most existing code. The rest I think I can send them together.
I understood. Well, the problem is also that this period is a sort of "transition" because batman-adv is getting changed in some of its most important part and we would like all the "new features" that are not essential to come after these changes. We still need to merge two (or two and a bit) patchsets before we can start merging other things.
This means that before your patchset gets merged we have to wait a bit more. I think it would be better to do this:
- for a while you don't care about rebasing on top of master
- when you have a some code ready to be reviewed you can put in on a remote git repo that we can check (e.g. github?)
- we/I review the code so that we make it ready to be sent as PATCH
- when these two (and a bit) patchsets are merged you can do the final rebase and send them to the ml for merging.
What do you think? In this way we same some painful rebase cycles, but we can continue preparing the code.
I understand, but it should be done similar? Like multiple patches?
multiple patches is always the way to go when we have more than one change, we cannot mix them all.
The idea is that I might add some patches and then find a bug that was in an old patch. That means to find the patch with the bug, resolve it, and re-patch everything after it.
this is normal when you have multiple patches: if a fix in the very first patch of a series creates conflicts with all the following ones, you have to adjust them all (this is what the "git rebase" helps you with).
It would be easier to do the changes directly on the existing code than restart everything from scratch.
restart everything from scratch? I did not get this.
I'm not sure if this is what you meant by using github.
for using github (or whetever else remote repository) I meant that instead of rebasing on top of master every time you have to send the patches to the ml for review, you could upload your code on a remote repo and have us reviewing the code on there directly. In this way you save the pain of respinning all your patches on top of master every week..
I hope I clarified your doubts.
Cheers,
On 11 September 2013 00:01, Antonio Quartulli antonio@meshcoding.com wrote:
On Tue, Sep 10, 2013 at 03:45:44PM +0300, Mihail Costea wrote:
On 10 September 2013 08:38, Antonio Quartulli antonio@meshcoding.com wrote:
On Tue, Sep 10, 2013 at 07:35:34AM +0300, Mihail Costea wrote:
On 9 September 2013 17:53, Antonio Quartulli antonio@meshcoding.com wrote:
On Mon, Sep 09, 2013 at 05:05:47PM +0300, Mihail Costea wrote:
Hi Antonio,
Is it possible to send the new model for the generalization as a patch first (the part without IPv6), or maybe everything as a patch as once? Having 5-6 patches to rewrite every time something changes makes the development harder.
Which patches do you want to merge? If they are ready it is better to send them as PATCH to the ml and then base your work on top of them assuming they will be merged at some point.
I took a small rest last week and now I'm redoing everything. I was thinking about sending the first part for merging (the one with generalization the DAT). That is the one that needs most rewriting every time because it affects the most existing code. The rest I think I can send them together.
I understood. Well, the problem is also that this period is a sort of "transition" because batman-adv is getting changed in some of its most important part and we would like all the "new features" that are not essential to come after these changes. We still need to merge two (or two and a bit) patchsets before we can start merging other things.
This means that before your patchset gets merged we have to wait a bit more. I think it would be better to do this:
- for a while you don't care about rebasing on top of master
- when you have a some code ready to be reviewed you can put in on a remote git repo that we can check (e.g. github?)
- we/I review the code so that we make it ready to be sent as PATCH
- when these two (and a bit) patchsets are merged you can do the final rebase and send them to the ml for merging.
What do you think? In this way we same some painful rebase cycles, but we can continue preparing the code.
I understand, but it should be done similar? Like multiple patches?
multiple patches is always the way to go when we have more than one change, we cannot mix them all.
The idea is that I might add some patches and then find a bug that was in an old patch. That means to find the patch with the bug, resolve it, and re-patch everything after it.
this is normal when you have multiple patches: if a fix in the very first patch of a series creates conflicts with all the following ones, you have to adjust them all (this is what the "git rebase" helps you with).
I haven't used it before but I will try it now.
It would be easier to do the changes directly on the existing code than restart everything from scratch.
restart everything from scratch? I did not get this.
The changes I'm doing now are quite big (as they change the first patch). That will make big changes to the code base. I will send next days the first patch for review first because it changed how the generalization works (more exactly I have remove mac_addr to introduce a new void * member).
I'd like the base to be written correctly as everything depends on the structures introduces there.
I'm not sure if this is what you meant by using github.
for using github (or whetever else remote repository) I meant that instead of rebasing on top of master every time you have to send the patches to the ml for review, you could upload your code on a remote repo and have us reviewing the code on there directly. In this way you save the pain of respinning all your patches on top of master every week..
I hope I clarified your doubts.
Thanks, Mihail
On Wed, Sep 11, 2013 at 07:33:41AM +0300, Mihail Costea wrote:
this is normal when you have multiple patches: if a fix in the very first patch of a series creates conflicts with all the following ones, you have to adjust them all (this is what the "git rebase" helps you with).
I haven't used it before but I will try it now.
git rebase is essential to make changes at a patch series. It helps you making a change in an initial patch and propagate the change to the rest. This may also be why you are experiencing much difficulty in doing this operation.
Cheers,
On Saturday 10 August 2013 12:01:02 Mihail Costea wrote:
Hi Antonio,
Thanks for the update. I will do the changes as soon as possible. I haven't sync in a while with the code base, so I think the whole changes might take a while to implement (+ I'm also working full time and trying to finish the paper for my Diploma Project). I hope that in 2 weeks I will finish everything because I can mostly work on my weekends. I don't have to finish everything by the time I present the Diploma Project, but once I have presented it, I can dedicate all my time here (at that time I would have finished my internship too).
But maybe that is done by then :).
What is the state here [1, 2, 3, 4, 5, 6]?
Kind regards, Sven
[1] https://patchwork.open-mesh.org/patch/3177/ [2] https://patchwork.open-mesh.org/patch/3195/ [3] https://patchwork.open-mesh.org/patch/3194/ [4] https://patchwork.open-mesh.org/patch/3166/ [5] https://patchwork.open-mesh.org/patch/3156/ [6] https://patchwork.open-mesh.org/patch/3205/
On Thu, Mar 10, 2016 at 08:11:29PM +0100, Sven Eckelmann wrote:
On Saturday 10 August 2013 12:01:02 Mihail Costea wrote:
Hi Antonio,
Thanks for the update. I will do the changes as soon as possible. I haven't sync in a while with the code base, so I think the whole changes might take a while to implement (+ I'm also working full time and trying to finish the paper for my Diploma Project). I hope that in 2 weeks I will finish everything because I can mostly work on my weekends. I don't have to finish everything by the time I present the Diploma Project, but once I have presented it, I can dedicate all my time here (at that time I would have finished my internship too).
But maybe that is done by then :).
What is the state here [1, 2, 3, 4, 5, 6]?
There was not much interest to work on this feature and these patches are not yet complete, therefore they should not be considered for merging.
Cheers,
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Antonio Quartulli -
Antonio Quartulli -
Linus Lüssing -
Mihail Costea -
Mihail Costea -
mihail.costea2005＠gmail.com
-
Sven Eckelmann