[PATCH net v2 0/3] net: dsa: bcm_sf2: CFP fixes

[PATCH net v2 0/3] net: dsa: bcm_sf2: CFP fixes

[Florian Fainelli]

Hi David,

This patch series fixes a number of usability issues with the SF2 Compact Field
Processor code:

- we would not be properly bound checking the location when we let the kernel
  automatically place rules with RX_CLS_LOC_ANY

- when using IPv6 rules and user space specifies a location identifier we
  would be off by one in what the chain ID (within the Broadcom tag) indicates

- it would be possible to delete one of the two slices of an IPv6 while leaving
  the other one programming leading to various problems

Florian Fainelli (3):
  net: dsa: bcm_sf2: Fix RX_CLS_LOC_ANY overwrite for last rule
  net: dsa: bcm_sf2: Fix IPv6 rules and chain ID
  net: dsa: bcm_sf2: Fix IPv6 rule half deletion

 drivers/net/dsa/bcm_sf2_cfp.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

-- 
2.14.1

[PATCH net v2 1/3] net: dsa: bcm_sf2: Fix RX_CLS_LOC_ANY overwrite for last rule

[Florian Fainelli]

When we let the kernel pick up a rule location with RX_CLS_LOC_ANY, we
would be able to overwrite the last rules because of a number of issues.

The IPv4 code path would not be checking that rule_index is within
bounds, and it would also only be allowed to pick up rules from range
0..126 instead of the full 0..127 range. This would lead us to allow
overwriting the last rule when we let the kernel pick-up the location.

Fixes: 3306145866b6 ("net: dsa: bcm_sf2: Move IPv4 CFP processing to specific functions")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2_cfp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
index 23b45da784cb..9e04786e3139 100644
--- a/drivers/net/dsa/bcm_sf2_cfp.c
+++ b/drivers/net/dsa/bcm_sf2_cfp.c
@@ -354,10 +354,13 @@ static int bcm_sf2_cfp_ipv4_rule_set(struct bcm_sf2_priv *priv, int port,
 	/* Locate the first rule available */
 	if (fs->location == RX_CLS_LOC_ANY)
 		rule_index = find_first_zero_bit(priv->cfp.used,
-						 bcm_sf2_cfp_rule_size(priv));
+						 priv->num_cfp_rules);
 	else
 		rule_index = fs->location;
 
+	if (rule_index > bcm_sf2_cfp_rule_size(priv))
+		return -ENOSPC;
+
 	layout = &udf_tcpip4_layout;
 	/* We only use one UDF slice for now */
 	slice_num = bcm_sf2_get_slice_number(layout, 0);
-- 
2.14.1

[PATCH net v2 2/3] net: dsa: bcm_sf2: Fix IPv6 rules and chain ID

[Florian Fainelli]

We had several issues that would make the programming of IPv6 rules both
inconsistent and error prone:

- the chain ID that we would be asking the hardware to put in the
  packet's Broadcom tag would be off by one, it would return one of the
  two indexes, but not the one user-space specified

- when an user specified a particular location to insert a CFP rule at,
  we would not be returning the same index, which would be confusing if
  nothing else

- finally, like IPv4, it would be possible to overflow the last entry by
  re-programming it

Fix this by swapping the usage of rule_index[0] and rule_index[1] where
relevant in order to return a consistent and correct user-space
experience.

Fixes: ba0696c22e7c ("net: dsa: bcm_sf2: Add support for IPv6 CFP rules")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2_cfp.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
index 9e04786e3139..6fd0f8a12cc2 100644
--- a/drivers/net/dsa/bcm_sf2_cfp.c
+++ b/drivers/net/dsa/bcm_sf2_cfp.c
@@ -565,19 +565,21 @@ static int bcm_sf2_cfp_ipv6_rule_set(struct bcm_sf2_priv *priv, int port,
 	 * first half because the HW search is by incrementing addresses.
 	 */
 	if (fs->location == RX_CLS_LOC_ANY)
-		rule_index[0] = find_first_zero_bit(priv->cfp.used,
-						    bcm_sf2_cfp_rule_size(priv));
+		rule_index[1] = find_first_zero_bit(priv->cfp.used,
+						    priv->num_cfp_rules);
 	else
-		rule_index[0] = fs->location;
+		rule_index[1] = fs->location;
+	if (rule_index[1] > bcm_sf2_cfp_rule_size(priv))
+		return -ENOSPC;
 
 	/* Flag it as used (cleared on error path) such that we can immediately
 	 * obtain a second one to chain from.
 	 */
-	set_bit(rule_index[0], priv->cfp.used);
+	set_bit(rule_index[1], priv->cfp.used);
 
-	rule_index[1] = find_first_zero_bit(priv->cfp.used,
-					    bcm_sf2_cfp_rule_size(priv));
-	if (rule_index[1] > bcm_sf2_cfp_rule_size(priv)) {
+	rule_index[0] = find_first_zero_bit(priv->cfp.used,
+					    priv->num_cfp_rules);
+	if (rule_index[0] > bcm_sf2_cfp_rule_size(priv)) {
 		ret = -ENOSPC;
 		goto out_err;
 	}
@@ -715,14 +717,14 @@ static int bcm_sf2_cfp_ipv6_rule_set(struct bcm_sf2_priv *priv, int port,
 	/* Flag the second half rule as being used now, return it as the
 	 * location, and flag it as unique while dumping rules
 	 */
-	set_bit(rule_index[1], priv->cfp.used);
+	set_bit(rule_index[0], priv->cfp.used);
 	set_bit(rule_index[1], priv->cfp.unique);
 	fs->location = rule_index[1];
 
 	return ret;
 
 out_err:
-	clear_bit(rule_index[0], priv->cfp.used);
+	clear_bit(rule_index[1], priv->cfp.used);
 	return ret;
 }
 
-- 
2.14.1

[PATCH net v2 3/3] net: dsa: bcm_sf2: Fix IPv6 rule half deletion

[Florian Fainelli]

It was possible to delete only one half of an IPv6, which would leave
the second half still programmed and possibly in use. Instead of
checking for the unused bitmap, we need to check the unique bitmap, and
refuse any deletion that does not match that criteria. We also need to
move that check from bcm_sf2_cfp_rule_del_one() into its caller:
bcm_sf2_cfp_rule_del() otherwise we would not be able to delete second
halves anymore that would not pass the first test.

Fixes: ba0696c22e7c ("net: dsa: bcm_sf2: Add support for IPv6 CFP rules")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2_cfp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
index 6fd0f8a12cc2..b89acaee12d4 100644
--- a/drivers/net/dsa/bcm_sf2_cfp.c
+++ b/drivers/net/dsa/bcm_sf2_cfp.c
@@ -790,10 +790,6 @@ static int bcm_sf2_cfp_rule_del_one(struct bcm_sf2_priv *priv, int port,
 	int ret;
 	u32 reg;
 
-	/* Refuse deletion of unused rules, and the default reserved rule */
-	if (!test_bit(loc, priv->cfp.used) || loc == 0)
-		return -EINVAL;
-
 	/* Indicate which rule we want to read */
 	bcm_sf2_cfp_rule_addr_set(priv, loc);
 
@@ -831,6 +827,13 @@ static int bcm_sf2_cfp_rule_del(struct bcm_sf2_priv *priv, int port,
 	u32 next_loc = 0;
 	int ret;
 
+	/* Refuse deleting unused rules, and those that are not unique since
+	 * that could leave IPv6 rules with one of the chained rule in the
+	 * table.
+	 */
+	if (!test_bit(loc, priv->cfp.unique) || loc == 0)
+		return -EINVAL;
+
 	ret = bcm_sf2_cfp_rule_del_one(priv, port, loc, &next_loc);
 	if (ret)
 		return ret;
-- 
2.14.1

Re: [PATCH net v2 0/3] net: dsa: bcm_sf2: CFP fixes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 15 May 2018 16:01:22 -0700

> This patch series fixes a number of usability issues with the SF2 Compact Field
> Processor code:
> 
> - we would not be properly bound checking the location when we let the kernel
>   automatically place rules with RX_CLS_LOC_ANY
> 
> - when using IPv6 rules and user space specifies a location identifier we
>   would be off by one in what the chain ID (within the Broadcom tag) indicates
> 
> - it would be possible to delete one of the two slices of an IPv6 while leaving
>   the other one programming leading to various problems

Series applied, thanks.

Queue up for -stable?

Re: [PATCH net v2 0/3] net: dsa: bcm_sf2: CFP fixes

[Florian Fainelli]

On 05/16/2018 11:12 AM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Tue, 15 May 2018 16:01:22 -0700
> 
>> This patch series fixes a number of usability issues with the SF2 Compact Field
>> Processor code:
>>
>> - we would not be properly bound checking the location when we let the kernel
>>   automatically place rules with RX_CLS_LOC_ANY
>>
>> - when using IPv6 rules and user space specifies a location identifier we
>>   would be off by one in what the chain ID (within the Broadcom tag) indicates
>>
>> - it would be possible to delete one of the two slices of an IPv6 while leaving
>>   the other one programming leading to various problems
> 
> Series applied, thanks.
> 
> Queue up for -stable?

Yes please!
-- 
Florian

Re: [PATCH net v2 0/3] net: dsa: bcm_sf2: CFP fixes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 16 May 2018 11:15:37 -0700

> On 05/16/2018 11:12 AM, David Miller wrote:
>> Queue up for -stable?
> 
> Yes please!

Done.