On Thursday, July 04, 2013 00:33:36 Antonio Quartulli wrote:
--- a/originator.c +++ b/originator.c @@ -44,6 +44,95 @@ static int batadv_compare_orig(const struct hlist_node *node, const void *data2) return (memcmp(data1, data2, ETH_ALEN) == 0 ? 1 : 0); }
+/**
- batadv_orig_node_vlan_get - get an orig_node_vlan object
- @orig_node: the originator servin the VLAN
- @vid: the VLAN identifier
- Returns the vlan object identified by vid and belonging to orig_node or
NULL + * if it does not exist
- */
+struct batadv_orig_node_vlan * +batadv_orig_node_vlan_get(struct batadv_orig_node *orig_node,
unsigned short vid)+{
- struct batadv_orig_node_vlan *vlan;
- rcu_read_lock();
- list_for_each_entry_rcu(vlan, &orig_node->vlan_list, list) {
if (!atomic_inc_not_zero(&vlan->refcount))continue;if (vlan->vid != vid)continue;rcu_read_unlock();return vlan;- }
- rcu_read_unlock();
- return NULL;
+}
Big memory leak here. First check the validity of the list item before increasing its refcounter. Please also keep the return variable style similar to other functions in the batman-adv code.
+/**
- batadv_orig_node_vlan_new - search and possibly create an
orig_node_vlan + * object
- @orig_node: the originator serving the VLAN
- @vid: the VLAN identifier
- Returns NULL in case of failure or the vlan object identified by vid
and + * belonging to orig_node otherwise. The object is created and added to the list + * if it does not exist.
- The object is returned with refcounter increased by 1
- */
+struct batadv_orig_node_vlan * +batadv_orig_node_vlan_new(struct batadv_orig_node *orig_node,
unsigned short vid)+{
- struct batadv_orig_node_vlan *vlan;
- spin_lock_bh(&orig_node->vlan_list_lock);
- /* first look if an object for this vid already exists */
- list_for_each_entry(vlan, &orig_node->vlan_list, list) {
if (!atomic_inc_not_zero(&vlan->refcount))continue;if (vlan->vid != vid)continue;/* there is already an entry for this vid: return it */goto out;- }
Same bug as above. Instead of duplicating the code, simply call batadv_orig_node_vlan_get().
+/**
- struct batadv_tvlv_tt_vlan_data - vlan specific tt data propagated
through + * the tt vlv container
- @crc: crc32 checksum of the entries belonging to this vlan
- @vid: vlan identifier
- @reserved: unused, useful for alignment purposes
- */
+struct batadv_tvlv_tt_vlan_data {
- __be32 crc;
- __be16 vid; uint16_t reserved;
- __be32 crc;
};
How about turning the reserved field into "__be16" as well ?
/**
- batadv_tt_prepare_tvlv_global_data - prepare the TVLV TT header to send
- within a TT Response directed to another node
- @orig_node: originator for which the TT data has to be prepared
- @tt_data: uninitialised pointer to the address of the TVLV buffer
- @tt_len: space to reserve for the TT entries. If negative, this
function will + * allocate as much space is needed for the full table
- Allocate the needed amount of memory for the entire TT TVLV and write
its + * header made up by one tvlv_tt_data object and a series of tvlv_tt_vlan_data + * objects, one per active VLAN served by the originator node.
- Return the size of the allocated buffer or 0 in case of failure.
- */
+static uint16_t +batadv_tt_prepare_tvlv_global_data(struct batadv_orig_node *orig_node,
struct batadv_tvlv_tt_data **tt_data,int32_t tt_len)+{
- struct batadv_tvlv_tt_vlan_data *tt_vlan;
- struct batadv_orig_node_vlan *vlan;
- uint16_t num_vlan = 0, num_entries = 0;
- rcu_read_lock();
- list_for_each_entry_rcu(vlan, &orig_node->vlan_list, list) {
num_vlan++;num_entries += atomic_read(&vlan->tt.num_entries);- }
- /* if tt_len is negative, allocate the space needed by the full table */
- if (tt_len < 0)
tt_len = batadv_tt_len(num_entries);- tt_len += sizeof(**tt_data);
- tt_len += sizeof(*tt_vlan) * num_vlan;
- *tt_data = kmalloc(tt_len, GFP_ATOMIC);
- if (!*tt_data) {
tt_len = 0;goto out;- }
- (*tt_data)->flags = BATADV_NO_FLAGS;
- (*tt_data)->ttvn = atomic_read(&orig_node->last_ttvn);
- (*tt_data)->num_vlan = htons(num_vlan);
- tt_vlan = (struct batadv_tvlv_tt_vlan_data *)(*tt_data + 1);
- list_for_each_entry_rcu(vlan, &orig_node->vlan_list, list) {
tt_vlan->vid = htons(vlan->vid);tt_vlan->crc = htonl(vlan->tt.crc);tt_vlan++;- }
+out:
- rcu_read_unlock();
- return tt_len;
+}
Are you certain this is race condition free ? What if local or global node is added while this function is running ?
+/**
- batadv_tt_tvlv_container_update - update the translation table tvlv
container * after local tt changes have been committed
- @bat_priv: the bat priv with all the soft interface information
@@ -473,10 +679,11 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) struct batadv_tt_change_node *entry, *safe; struct batadv_tvlv_tt_data *tt_data; struct batadv_tvlv_tt_change *tt_change;
- int tt_diff_len = 0, tt_change_len = 0;
- int tt_diff_len, tt_change_len = 0; int tt_diff_entries_num = 0, tt_diff_entries_count = 0;
- uint16_t tt_len, change_offset;
- tt_diff_len += batadv_tt_len(atomic_read(&bat_priv->tt.local_changes));
tt_diff_len = batadv_tt_len(atomic_read(&bat_priv->tt.local_changes));
/* if we have too many changes for one packet don't send any
- and wait for the tt table request which will be fragmented
@@ -484,13 +691,16 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv) if (tt_diff_len > bat_priv->soft_iface->mtu) tt_diff_len = 0;
- tt_data = kzalloc(sizeof(*tt_data) + tt_diff_len, GFP_ATOMIC);
- if (!tt_data)
tt_len = batadv_tt_prepare_tvlv_local_data(bat_priv, &tt_data,
tt_diff_len);if (!tt_len) return;
tt_data->flags = BATADV_TT_OGM_DIFF;
- tt_data->ttvn = atomic_read(&bat_priv->tt.vn);
- tt_data->crc = htonl(bat_priv->tt.local_crc);
If you avoided this removal you could have one function instead of batadv_tt_prepare_tvlv_local_data() and batadv_tt_prepare_tvlv_global_data() doing basically the same thing.
- tt_change = (struct batadv_tvlv_tt_change *)(tt_data + 1);
- tt_change = (struct batadv_tvlv_tt_change *)((uint8_t *)tt_data +
change_offset);
David won't like that.
seq_printf(seq, " * %pM %4i [%c%c%c%c%c] %3u.%03u\n",
vlan = batadv_softif_vlan_get(bat_priv, vid);if (!vlan) {seq_printf(seq, "Cannot retrieve VLAN %d\n",BATADV_PRINT_VID(vid));continue;}seq_printf(seq," * %pM %4i [%c%c%c%c%c] %3u.%03u (%#.8x)\n", tt_common_entry->addr, BATADV_PRINT_VID(tt_common_entry->vid), (tt_common_entry->flags &@@ -597,7 +818,8 @@ int batadv_tt_local_seq_print_text(struct seq_file *seq, void *offset) (tt_common_entry->flags & BATADV_TT_CLIENT_WIFI ? 'W' : '.'), no_purge ? 0 : last_seen_secs,
no_purge ? 0 : last_seen_msecs);
no_purge ? 0 : last_seen_msecs,vlan->tt.crc);
My feeling tells me we are missing batadv_softif_vlan_free_ref() call ?
@@ -1070,35 +1292,49 @@ static void batadv_tt_global_print_entry(struct batadv_tt_global_entry *tt_global_entry, struct seq_file *seq) {
- struct hlist_head *head; struct batadv_tt_orig_list_entry *orig_entry, *best_entry; struct batadv_tt_common_entry *tt_common_entry;
- uint16_t flags;
struct batadv_orig_node_vlan *vlan;
struct hlist_head *head; uint8_t last_ttvn;
uint16_t flags;
tt_common_entry = &tt_global_entry->common; flags = tt_common_entry->flags;
Now you try to remove empty lines ? :)
best_entry = batadv_transtable_best_orig(tt_global_entry); if (best_entry) {
vlan = batadv_orig_node_vlan_get(best_entry->orig_node,tt_common_entry->vid);if (!vlan) {seq_printf(seq," * Cannot retrieve VLAN %d for originator %pM\n",BATADV_PRINT_VID(tt_common_entry->vid),best_entry->orig_node->orig);goto print_list;}- last_ttvn = atomic_read(&best_entry->orig_node->last_ttvn); seq_printf(seq, " %c %pM %4i (%3u) via %pM (%3u) (%#.8x) [%c%c%c]\n", '*', tt_global_entry->common.addr, BATADV_PRINT_VID(tt_global_entry->common.vid), best_entry->ttvn, best_entry->orig_node->orig,
last_ttvn, best_entry->orig_node->tt_crc,
last_ttvn, vlan->tt.crc, (flags & BATADV_TT_CLIENT_ROAM ? 'R' : '.'), (flags & BATADV_TT_CLIENT_WIFI ? 'W' : '.'), (flags & BATADV_TT_CLIENT_TEMP ? 'T' : '.'));
How about a batadv_orig_node_vlan_free_ref() call ?
+print_list: head = &tt_global_entry->orig_list;
hlist_for_each_entry_rcu(orig_entry, head, list) { if (best_entry == orig_entry) continue;
vlan = batadv_orig_node_vlan_get(orig_entry->orig_node,tt_common_entry->vid);- last_ttvn = atomic_read(&orig_entry->orig_node->last_ttvn); seq_printf(seq, " %c %pM %4d (%3u) via %pM (%3u) [%c%c%c]\n",
Another one ?
Cheers, Marek