i40e: allow i40e_update_filter_state to skip broadcast filters

Fix a bug where we modified the mac_filter_hash while outside a lock, when handling addition of broadcast filters.

Normally, we add filters to firmware by batching the additions into lists and issuing 1 update for every few filters. Broadcast filters are handled differently, by instead setting the broadcast promiscuous mode flags. In order to make sure the 1<->1 mapping of filters in our addition array lined up with filters in the hlist tmp_add_list, we had to remove the filter and move it back to the main hash. However, we didn't do this under lock, which could cause consistency problems for the list.

Fix this by updating i40e_update_filter_state logic so that it knows to avoid broadcast filters. This ensures that we don't have to remove the filter separately, and can put it back using the normal flow.

---
* v3
- Use hlist_for_each_entry_continue instead

 drivers/net/ethernet/intel/i40e/i40e_main.c | 30 +++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

index fa4a04da47b7..357ceda375f8 100644
@@ -1842,6 +1842,24 @@ static void i40e_undo_filter_entries(struct i40e_vsi *vsi,
 	}
 }
 
+/**
+ * i40e_next_entry - Get the next non-broadcast filter from a list
+ * @f: pointer to filter in list
+ *
+ * Returns the next non-broadcast filter in the list. Required so that we
+ * ignore broadcast filters within the list, since these are not handled via
+ * the normal firmware update path.
+ */
+static struct i40e_mac_filter *i40e_next_filter(struct i40e_mac_filter *f)
+{
+	hlist_for_each_entry_continue(f, hlist) {
+		if (!is_broadcast_ether_addr(f->macaddr))
+			return f;
+	}
+
+	return NULL;
+}
+
 /**
  * i40e_update_filter_state - Update filter state based on return data
  * from firmware

@@ -1874,9 +1892,9 @@ i40e_update_filter_state(int count,
 			retval++;
 		}
 
-		add_head = hlist_entry(add_head->hlist.next,
-				       typeof(struct i40e_mac_filter),
-				       hlist);
+		add_head = i40e_next_filter(add_head);
+		if (!add_head)
+			break;
 	}
 
 	return retval;

@@ -2095,7 +2113,7 @@ int i40e_sync_vsi_filters(struct i40e_vsi *vsi)
 			cmd_flags = 0;
 
 			/* handle broadcast filters by updating the broadcast
-			 * promiscuous flag instead of deleting a MAC filter.
+			 * promiscuous flag and release filter list.
 			 */
 			if (is_broadcast_ether_addr(f->macaddr)) {
 				i40e_aqc_broadcast_filter(vsi, vsi_name, f);

@@ -2164,11 +2182,7 @@ int i40e_sync_vsi_filters(struct i40e_vsi *vsi)
 			 * promiscuous flag instead of adding a MAC filter.
 			 */
 			if (is_broadcast_ether_addr(f->macaddr)) {
-				u64 key = i40e_addr_to_hkey(f->macaddr);
 				i40e_aqc_broadcast_filter(vsi, vsi_name, f);
-
-				hlist_del(&f->hlist);
-				hash_add(vsi->mac_filter_hash, &f->hlist, key);
 				continue;
 			}
 

2.12.0.rc0.177.g63172abf21d0

Refactor how we add new filters to firmware to avoid a race condition that can occur due to removing filters from the hash temporarily.

To understand the race condition, suppose that you have a number of MAC filters, but have not yet added any VLANs. Now, add two VLANs in rapid succession. A possible resulting flow would look something like the following:

(1) lock hash for add VLAN
(2) add the new MAC/VLAN combos for each current MAC filter
(3) unlock hash
(4) lock hash for filter sync
(5) notice that we have a VLAN, so prepare to update all MAC filters with VLAN=-1 to be VLAN=0.
(6) move NEW and REMOVE filters to temporary list
(7) unlock hash
(8) lock hash for add VLAN
(9) add new MAC/VLAN combos. Notice that no MAC filters are currently in the hash list, so we don't add any VLANs <--- BUG!
(10) unlock hash
(11) sync the temporary lists to firmware
(12) lock hash for post-sync
(13) move the temporary elements back to the main list
....

Because we take filters out of the main hash into temporary lists, we introduce a narrow window where it is possible that other callers to the list will not see some of the filters which were previously added but have not yet been finalized. This results in sometimes dropping VLAN additions, and could also result in failing to add a MAC address on the newly added VLAN.

One obvious way to avoid this race condition would be to lock the entire firmware process. Unfortunately this does not work because adminq firmware commands take a mutex which results in a sleep while atomic BUG(). So, we can't use the simplest approach.

An alternative approach is to simply not remove the filters from the hash list while adding. Instead, add an i40e_new_mac_filter structure which we will use to track added filters. This avoids the need to remove the filter from the hash list. We'll store a pointer to the original i40e_mac_filter, along with our own copy of the state.

We won't update the state directly, so as to avoid race with other code that may modify the state while under the lock. We are safe to read f->macaddr and f->vlan since these only change in two locations. The first is on filter creation, which must have already occurred. The second is inside i40e_correct_vlan_filters which was previously run after creation of this object and can't be run again until after. Thus, we should be safe to read the MAC address and VLAN while outside the lock.

We also aren't going to run into a use-after-free issue because the only place where we free filters is when they are marked FAILED or when we remove them inside the sync subtask. Since the subtask has its own critical flag to prevent duplicate runs, we know this won't happen. We also know that the only location to transition a filter from NEW to FAILED is inside the subtask also, so we aren't worried about that either.

Use the wrapper i40e_new_mac_filter for additions, and once we've finalized the addition to firmware, we will update the filter state inside a lock, and then free the wrapper structure.

In order to avoid a possible race condition with filter deletion, we won't update the original filter state unless it is still I40E_FILTER_NEW when we finish the firmware sync.

This approach is more complex, but avoids race conditions related to filters being temporarily removed from the list. We do not need the same behavior for deletion because we always unconditionally removed the filters from the list regardless of the firmware status.

---
This patch is necessary to replace since it would conflict with the previous patch. Everything else in the queue should be able to stay ordered as is if you just replace these inline

 drivers/net/ethernet/intel/i40e/i40e.h      |  16 ++++
 drivers/net/ethernet/intel/i40e/i40e_main.c | 141 ++++++++++++++++++----------
 2 files changed, 109 insertions(+), 48 deletions(-)

index fdd9069b6cec..7a23d3e47c6f 100644
@@ -467,6 +467,22 @@ struct i40e_mac_filter {
 	enum i40e_filter_state state;
 };
 
+/* Wrapper structure to keep track of filters while we are preparing to send
+ * firmware commands. We cannot send firmware commands while holding a
+ * spinlock, since it might sleep. To avoid this, we wrap the added filters in
+ * a separate structure, which will track the state change and update the real
+ * filter while under lock. We can't simply hold the filters in a separate
+ * list, as this opens a window for a race condition when adding new MAC
+ * addresses to all VLANs, or when adding new VLANs to all MAC addresses.
+ */
+struct i40e_new_mac_filter {
+	struct hlist_node hlist;
+	struct i40e_mac_filter *f;
+
+	/* Track future changes to state separately */
+	enum i40e_filter_state state;
+};
+
 struct i40e_veb {
 	struct i40e_pf *pf;
 	u16 idx;

2.12.0.rc0.177.g63172abf21d0

This patch does not apply cleanly, not even close...

I tried to match up this patch with something that was in my queue, and I was not able to find a single patch that this would easily replace.

Please resubmit this series, based on my current dev-queue branch. I
am dropping this 2 patch series and will await a re-submission.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20170305/02d0ae21/attachment.asc>