[PATCH net 0/2] sfc: aRFS fixes

[PATCH net 0/2] sfc: aRFS fixes

[Edward Cree]

The issue fixed in patch #1 was found two years ago and might not still
 happen on current kernels, but (a) we didn't figure out what caused it,
 and (b) the fix should be harmless even if it's unnecessary.

Edward Cree (1):
  sfc: handle nonlinear SKBs in efx_filter_rfs()

Jon Cooper (1):
  sfc: Track RPS flow IDs per channel instead of per function

 drivers/net/ethernet/sfc/efx.c        | 32 ++++++++++++++---
 drivers/net/ethernet/sfc/net_driver.h | 12 ++++---
 drivers/net/ethernet/sfc/rx.c         | 68 ++++++++++++++++++++++-------------
 3 files changed, 79 insertions(+), 33 deletions(-)

[PATCH net 1/2] sfc: handle nonlinear SKBs in efx_filter_rfs()

[Edward Cree]

Previously efx_filter_rfs() assumed that the headers it needed (802.1Q, IP)
 would be present in the linear data area of the SKB.
When running with debugging I found that this is not always the case and
 that in fact the data may all be paged.
So now use skb_header_pointer() to extract the data.

Also replace EFX_BUG_ON_PARANOID checks for insufficient data with checks
 that return -EINVAL, as this case is possible if the received packet was
 too short.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/rx.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index 8956995..52790f0 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -842,25 +842,32 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 	struct efx_nic *efx = netdev_priv(net_dev);
 	struct efx_channel *channel;
 	struct efx_filter_spec spec;
+	/* 60 octets is the maximum length of an IPv4 header (all IPv6 headers
+	 * are 40 octets), and we pull 4 more to get the port numbers
+	 */
+	#define EFX_RFS_HEADER_LENGTH	(sizeof(struct vlan_hdr) + 60 + 4)
+	unsigned char header[EFX_RFS_HEADER_LENGTH];
+	int headlen = min_t(int, EFX_RFS_HEADER_LENGTH, skb->len);
+	#undef EFX_RFS_HEADER_LENGTH
+	void *hptr;
 	const __be16 *ports;
 	__be16 ether_type;
 	int nhoff;
 	int rc;
 
-	/* The core RPS/RFS code has already parsed and validated
-	 * VLAN, IP and transport headers.  We assume they are in the
-	 * header area.
-	 */
+	hptr = skb_header_pointer(skb, 0, headlen, header);
+	if (!hptr)
+		return -EINVAL;
 
 	if (skb->protocol == htons(ETH_P_8021Q)) {
-		const struct vlan_hdr *vh =
-			(const struct vlan_hdr *)skb->data;
+		const struct vlan_hdr *vh = hptr;
 
 		/* We can't filter on the IP 5-tuple and the vlan
 		 * together, so just strip the vlan header and filter
 		 * on the IP part.
 		 */
-		EFX_BUG_ON_PARANOID(skb_headlen(skb) < sizeof(*vh));
+		if (headlen < sizeof(*vh))
+			return -EINVAL;
 		ether_type = vh->h_vlan_encapsulated_proto;
 		nhoff = sizeof(struct vlan_hdr);
 	} else {
@@ -881,23 +888,23 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 	spec.ether_type = ether_type;
 
 	if (ether_type == htons(ETH_P_IP)) {
-		const struct iphdr *ip =
-			(const struct iphdr *)(skb->data + nhoff);
+		const struct iphdr *ip = hptr + nhoff;
 
-		EFX_BUG_ON_PARANOID(skb_headlen(skb) < nhoff + sizeof(*ip));
+		if (headlen < nhoff + sizeof(*ip))
+			return -EINVAL;
 		if (ip_is_fragment(ip))
 			return -EPROTONOSUPPORT;
 		spec.ip_proto = ip->protocol;
 		spec.rem_host[0] = ip->saddr;
 		spec.loc_host[0] = ip->daddr;
-		EFX_BUG_ON_PARANOID(skb_headlen(skb) < nhoff + 4 * ip->ihl + 4);
-		ports = (const __be16 *)(skb->data + nhoff + 4 * ip->ihl);
+		if (headlen < nhoff + 4 * ip->ihl + 4)
+			return -EINVAL;
+		ports = (const __be16 *)(hptr + nhoff + 4 * ip->ihl);
 	} else {
-		const struct ipv6hdr *ip6 =
-			(const struct ipv6hdr *)(skb->data + nhoff);
+		const struct ipv6hdr *ip6 = (hptr + nhoff);
 
-		EFX_BUG_ON_PARANOID(skb_headlen(skb) <
-				    nhoff + sizeof(*ip6) + 4);
+		if (headlen < nhoff + sizeof(*ip6) + 4)
+			return -EINVAL;
 		spec.ip_proto = ip6->nexthdr;
 		memcpy(spec.rem_host, &ip6->saddr, sizeof(ip6->saddr));
 		memcpy(spec.loc_host, &ip6->daddr, sizeof(ip6->daddr));

Re: [PATCH net 1/2] sfc: handle nonlinear SKBs in efx_filter_rfs()

[Eric Dumazet]

On Thu, 2016-05-26 at 17:01 +0100, Edward Cree wrote:
> Previously efx_filter_rfs() assumed that the headers it needed (802.1Q, IP)
>  would be present in the linear data area of the SKB.
> When running with debugging I found that this is not always the case and
>  that in fact the data may all be paged.
> So now use skb_header_pointer() to extract the data.
> 
> Also replace EFX_BUG_ON_PARANOID checks for insufficient data with checks
>  that return -EINVAL, as this case is possible if the received packet was
>  too short.
> 
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
>  drivers/net/ethernet/sfc/rx.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
> index 8956995..52790f0 100644
> --- a/drivers/net/ethernet/sfc/rx.c
> +++ b/drivers/net/ethernet/sfc/rx.c
> @@ -842,25 +842,32 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
>  	struct efx_nic *efx = netdev_priv(net_dev);
>  	struct efx_channel *channel;
>  	struct efx_filter_spec spec;
> +	/* 60 octets is the maximum length of an IPv4 header (all IPv6 headers
> +	 * are 40 octets), and we pull 4 more to get the port numbers
> +	 */
> +	#define EFX_RFS_HEADER_LENGTH	(sizeof(struct vlan_hdr) + 60 + 4)

Lot of magic here. Yet another flow dissection.

Seems to be a good place to use net/core/flow_dissector.c helpers.

I truly believe that every time a driver has a private flow dissector,
it always have at least one bug.

Re: [PATCH net 1/2] sfc: handle nonlinear SKBs in efx_filter_rfs()

[Edward Cree]

On 26/05/16 17:56, Eric Dumazet wrote:
> Lot of magic here. Yet another flow dissection.
>
> Seems to be a good place to use net/core/flow_dissector.c helpers.
Fair point, but that doesn't really feel like 'net' material.
I'll look into flow_dissector and see if I can get something ready for when net-next opens back up.  But in the meantime I think this fix is still needed.

Re: [PATCH net 1/2] sfc: handle nonlinear SKBs in efx_filter_rfs()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 26 May 2016 09:56:36 -0700

> I truly believe that every time a driver has a private flow dissector,
> it always have at least one bug.

+1

[PATCH net] sfc: use flow dissector helpers for aRFS

[Edward Cree]

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
This seems to work in my testing, but I first looked at the flow dissector
API less than four hours ago, so I might be doing it wrong.

I still think that this is too big a change for 'net' and that it's better
to take the original fix now and then I'll respin this patch for net-next
when it opens up.  But I'm also happy for you to take this now in which
case I'll respin Jon's patch 2/2 on top of the result.  Your choice.

 drivers/net/ethernet/sfc/rx.c | 76 +++++++++++++------------------------------
 1 file changed, 23 insertions(+), 53 deletions(-)

diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index 8956995..adbce33 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -842,33 +842,15 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 	struct efx_nic *efx = netdev_priv(net_dev);
 	struct efx_channel *channel;
 	struct efx_filter_spec spec;
-	const __be16 *ports;
-	__be16 ether_type;
-	int nhoff;
+	struct flow_keys fk;
 	int rc;
 
-	/* The core RPS/RFS code has already parsed and validated
-	 * VLAN, IP and transport headers.  We assume they are in the
-	 * header area.
-	 */
-
-	if (skb->protocol == htons(ETH_P_8021Q)) {
-		const struct vlan_hdr *vh =
-			(const struct vlan_hdr *)skb->data;
-
-		/* We can't filter on the IP 5-tuple and the vlan
-		 * together, so just strip the vlan header and filter
-		 * on the IP part.
-		 */
-		EFX_BUG_ON_PARANOID(skb_headlen(skb) < sizeof(*vh));
-		ether_type = vh->h_vlan_encapsulated_proto;
-		nhoff = sizeof(struct vlan_hdr);
-	} else {
-		ether_type = skb->protocol;
-		nhoff = 0;
-	}
+	if (!skb_flow_dissect_flow_keys(skb, &fk, 0))
+		return -EPROTONOSUPPORT;
 
-	if (ether_type != htons(ETH_P_IP) && ether_type != htons(ETH_P_IPV6))
+	if (fk.basic.n_proto != htons(ETH_P_IP) && fk.basic.n_proto != htons(ETH_P_IPV6))
+		return -EPROTONOSUPPORT;
+	if (fk.control.flags & FLOW_DIS_IS_FRAGMENT)
 		return -EPROTONOSUPPORT;
 
 	efx_filter_init_rx(&spec, EFX_FILTER_PRI_HINT,
@@ -878,34 +863,19 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 		EFX_FILTER_MATCH_ETHER_TYPE | EFX_FILTER_MATCH_IP_PROTO |
 		EFX_FILTER_MATCH_LOC_HOST | EFX_FILTER_MATCH_LOC_PORT |
 		EFX_FILTER_MATCH_REM_HOST | EFX_FILTER_MATCH_REM_PORT;
-	spec.ether_type = ether_type;
-
-	if (ether_type == htons(ETH_P_IP)) {
-		const struct iphdr *ip =
-			(const struct iphdr *)(skb->data + nhoff);
-
-		EFX_BUG_ON_PARANOID(skb_headlen(skb) < nhoff + sizeof(*ip));
-		if (ip_is_fragment(ip))
-			return -EPROTONOSUPPORT;
-		spec.ip_proto = ip->protocol;
-		spec.rem_host[0] = ip->saddr;
-		spec.loc_host[0] = ip->daddr;
-		EFX_BUG_ON_PARANOID(skb_headlen(skb) < nhoff + 4 * ip->ihl + 4);
-		ports = (const __be16 *)(skb->data + nhoff + 4 * ip->ihl);
+	spec.ether_type = fk.basic.n_proto;
+	spec.ip_proto = fk.basic.ip_proto;
+
+	if (fk.basic.n_proto == htons(ETH_P_IP)) {
+		spec.rem_host[0] = fk.addrs.v4addrs.src;
+		spec.loc_host[0] = fk.addrs.v4addrs.dst;
 	} else {
-		const struct ipv6hdr *ip6 =
-			(const struct ipv6hdr *)(skb->data + nhoff);
-
-		EFX_BUG_ON_PARANOID(skb_headlen(skb) <
-				    nhoff + sizeof(*ip6) + 4);
-		spec.ip_proto = ip6->nexthdr;
-		memcpy(spec.rem_host, &ip6->saddr, sizeof(ip6->saddr));
-		memcpy(spec.loc_host, &ip6->daddr, sizeof(ip6->daddr));
-		ports = (const __be16 *)(ip6 + 1);
+		memcpy(spec.rem_host, &fk.addrs.v6addrs.src, sizeof(struct in6_addr));
+		memcpy(spec.loc_host, &fk.addrs.v6addrs.dst, sizeof(struct in6_addr));
 	}
 
-	spec.rem_port = ports[0];
-	spec.loc_port = ports[1];
+	spec.rem_port = fk.ports.src;
+	spec.loc_port = fk.ports.dst;
 
 	rc = efx->type->filter_rfs_insert(efx, &spec);
 	if (rc < 0)
@@ -916,18 +886,18 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 	channel = efx_get_channel(efx, skb_get_rx_queue(skb));
 	++channel->rfs_filters_added;
 
-	if (ether_type == htons(ETH_P_IP))
+	if (spec.ether_type == htons(ETH_P_IP))
 		netif_info(efx, rx_status, efx->net_dev,
 			   "steering %s %pI4:%u:%pI4:%u to queue %u [flow %u filter %d]\n",
 			   (spec.ip_proto == IPPROTO_TCP) ? "TCP" : "UDP",
-			   spec.rem_host, ntohs(ports[0]), spec.loc_host,
-			   ntohs(ports[1]), rxq_index, flow_id, rc);
+			   spec.rem_host, ntohs(spec.rem_port), spec.loc_host,
+			   ntohs(spec.loc_port), rxq_index, flow_id, rc);
 	else
 		netif_info(efx, rx_status, efx->net_dev,
 			   "steering %s [%pI6]:%u:[%pI6]:%u to queue %u [flow %u filter %d]\n",
 			   (spec.ip_proto == IPPROTO_TCP) ? "TCP" : "UDP",
-			   spec.rem_host, ntohs(ports[0]), spec.loc_host,
-			   ntohs(ports[1]), rxq_index, flow_id, rc);
+			   spec.rem_host, ntohs(spec.rem_port), spec.loc_host,
+			   ntohs(spec.loc_port), rxq_index, flow_id, rc);
 
 	return rc;
 }

Re: [PATCH net] sfc: use flow dissector helpers for aRFS

[David Miller]

From: Edward Cree <ecree@solarflare.com>
Date: Thu, 26 May 2016 21:46:05 +0100

> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
> This seems to work in my testing, but I first looked at the flow dissector
> API less than four hours ago, so I might be doing it wrong.
> 
> I still think that this is too big a change for 'net' and that it's better
> to take the original fix now and then I'll respin this patch for net-next
> when it opens up.  But I'm also happy for you to take this now in which
> case I'll respin Jon's patch 2/2 on top of the result.  Your choice.

I definitely prefer this and have applied it, please respin Jon's patch.

Thanks!

[PATCH net 2/2] sfc: Track RPS flow IDs per channel instead of per function

[Edward Cree]

From: Jon Cooper <jcooper@solarflare.com>

Otherwise we get confused when two flows on different channels get the
 same flow ID.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 drivers/net/ethernet/sfc/efx.c        | 32 +++++++++++++++++++++++++++-----
 drivers/net/ethernet/sfc/net_driver.h | 12 ++++++++----
 drivers/net/ethernet/sfc/rx.c         | 29 +++++++++++++++++++++--------
 3 files changed, 56 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 0705ec86..097f363 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1726,14 +1726,33 @@ static int efx_probe_filters(struct efx_nic *efx)
 
 #ifdef CONFIG_RFS_ACCEL
 	if (efx->type->offload_features & NETIF_F_NTUPLE) {
-		efx->rps_flow_id = kcalloc(efx->type->max_rx_ip_filters,
-					   sizeof(*efx->rps_flow_id),
-					   GFP_KERNEL);
-		if (!efx->rps_flow_id) {
+		struct efx_channel *channel;
+		int i, success = 1;
+
+		efx_for_each_channel(channel, efx) {
+			channel->rps_flow_id =
+				kcalloc(efx->type->max_rx_ip_filters,
+					sizeof(*channel->rps_flow_id),
+					GFP_KERNEL);
+			if (!channel->rps_flow_id)
+				success = 0;
+			else
+				for (i = 0;
+				     i < efx->type->max_rx_ip_filters;
+				     ++i)
+					channel->rps_flow_id[i] =
+						RPS_FLOW_ID_INVALID;
+		}
+
+		if (!success) {
+			efx_for_each_channel(channel, efx)
+				kfree(channel->rps_flow_id);
 			efx->type->filter_table_remove(efx);
 			rc = -ENOMEM;
 			goto out_unlock;
 		}
+
+		efx->rps_expire_index = efx->rps_expire_channel = 0;
 	}
 #endif
 out_unlock:
@@ -1744,7 +1763,10 @@ out_unlock:
 static void efx_remove_filters(struct efx_nic *efx)
 {
 #ifdef CONFIG_RFS_ACCEL
-	kfree(efx->rps_flow_id);
+	struct efx_channel *channel;
+
+	efx_for_each_channel(channel, efx)
+		kfree(channel->rps_flow_id);
 #endif
 	down_write(&efx->filter_sem);
 	efx->type->filter_table_remove(efx);
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 38c4223..d13ddf9 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -403,6 +403,8 @@ enum efx_sync_events_state {
  * @event_test_cpu: Last CPU to handle interrupt or test event for this channel
  * @irq_count: Number of IRQs since last adaptive moderation decision
  * @irq_mod_score: IRQ moderation score
+ * @rps_flow_id: Flow IDs of filters allocated for accelerated RFS,
+ *      indexed by filter ID
  * @n_rx_tobe_disc: Count of RX_TOBE_DISC errors
  * @n_rx_ip_hdr_chksum_err: Count of RX IP header checksum errors
  * @n_rx_tcp_udp_chksum_err: Count of RX TCP and UDP checksum errors
@@ -446,6 +448,8 @@ struct efx_channel {
 	unsigned int irq_mod_score;
 #ifdef CONFIG_RFS_ACCEL
 	unsigned int rfs_filters_added;
+#define RPS_FLOW_ID_INVALID 0xFFFFFFFF
+	u32 *rps_flow_id;
 #endif
 
 	unsigned n_rx_tobe_disc;
@@ -889,9 +893,9 @@ struct vfdi_status;
  * @filter_sem: Filter table rw_semaphore, for freeing the table
  * @filter_lock: Filter table lock, for mere content changes
  * @filter_state: Architecture-dependent filter table state
- * @rps_flow_id: Flow IDs of filters allocated for accelerated RFS,
- *	indexed by filter ID
- * @rps_expire_index: Next index to check for expiry in @rps_flow_id
+ * @rps_expire_channel: Next channel to check for expiry
+ * @rps_expire_index: Next index to check for expiry in
+ *	@rps_expire_channel's @rps_flow_id
  * @active_queues: Count of RX and TX queues that haven't been flushed and drained.
  * @rxq_flush_pending: Count of number of receive queues that need to be flushed.
  *	Decremented when the efx_flush_rx_queue() is called.
@@ -1035,7 +1039,7 @@ struct efx_nic {
 	spinlock_t filter_lock;
 	void *filter_state;
 #ifdef CONFIG_RFS_ACCEL
-	u32 *rps_flow_id;
+	unsigned int rps_expire_channel;
 	unsigned int rps_expire_index;
 #endif
 
diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index 52790f0..1533c08 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -855,6 +855,9 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 	int nhoff;
 	int rc;
 
+	if (flow_id == RPS_FLOW_ID_INVALID)
+		return -EINVAL;
+
 	hptr = skb_header_pointer(skb, 0, headlen, header);
 	if (!hptr)
 		return -EINVAL;
@@ -919,8 +922,8 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 		return rc;
 
 	/* Remember this so we can check whether to expire the filter later */
-	efx->rps_flow_id[rc] = flow_id;
-	channel = efx_get_channel(efx, skb_get_rx_queue(skb));
+	channel = efx_get_channel(efx, rxq_index);
+	channel->rps_flow_id[rc] = flow_id;
 	++channel->rfs_filters_added;
 
 	if (ether_type == htons(ETH_P_IP))
@@ -942,24 +945,34 @@ int efx_filter_rfs(struct net_device *net_dev, const struct sk_buff *skb,
 bool __efx_filter_rfs_expire(struct efx_nic *efx, unsigned int quota)
 {
 	bool (*expire_one)(struct efx_nic *efx, u32 flow_id, unsigned int index);
-	unsigned int index, size;
+	unsigned int channel_idx, index, size;
 	u32 flow_id;
 
 	if (!spin_trylock_bh(&efx->filter_lock))
 		return false;
 
 	expire_one = efx->type->filter_rfs_expire_one;
+	channel_idx = efx->rps_expire_channel;
 	index = efx->rps_expire_index;
 	size = efx->type->max_rx_ip_filters;
 	while (quota--) {
-		flow_id = efx->rps_flow_id[index];
-		if (expire_one(efx, flow_id, index))
+		struct efx_channel *channel = efx_get_channel(efx, channel_idx);
+		flow_id = channel->rps_flow_id[index];
+
+		if (flow_id != RPS_FLOW_ID_INVALID &&
+		    expire_one(efx, flow_id, index)) {
 			netif_info(efx, rx_status, efx->net_dev,
-				   "expired filter %d [flow %u]\n",
-				   index, flow_id);
-		if (++index == size)
+				   "expired filter %d [queue %u flow %u]\n",
+				   index, channel_idx, flow_id);
+			channel->rps_flow_id[index] = RPS_FLOW_ID_INVALID;
+		}
+		if (++index == size) {
+			if (++channel_idx == efx->n_channels)
+				channel_idx = 0;
 			index = 0;
+		}
 	}
+	efx->rps_expire_channel = channel_idx;
 	efx->rps_expire_index = index;
 
 	spin_unlock_bh(&efx->filter_lock);