diff mbox series

[v2,net] net: mscc: ocelot: return error if VCAP filter is not found

Message ID 20200922004657.181282-1-vladimir.oltean@nxp.com
State Superseded
Delegated to: David Miller
Headers show
Series [v2,net] net: mscc: ocelot: return error if VCAP filter is not found | expand

Commit Message

Vladimir Oltean Sept. 22, 2020, 12:46 a.m. UTC 
From: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>

There are 2 separate, but related, issues.

First, the ocelot_vcap_block_get_filter_index function, née
ocelot_ace_rule_get_index_id prior to the aae4e500e106 ("net: mscc:
ocelot: generalize the "ACE/ACL" names") rename, does not do what the
author probably intended. If the desired filter entry is not present in
the ACL block, this function returns an index equal to the total number
of filters, instead of -1, which is maybe what was intended, judging
from the curious initialization with -1, and the "++index" idioms.
Either way, none of the callers seems to expect this behavior.

Second issue, the callers don't actually check the return value at all.
So in case the filter is not found in the rule list, propagate the
return code to avoid kernel panics.

So update the callers and also take the opportunity to get rid of the
odd coding idioms that appear to work but don't.

Fixes: b596229448dd ("net: mscc: ocelot: Add support for tcam")
Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
Add Fixes tag.

 drivers/net/ethernet/mscc/ocelot_vcap.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Vladimir Oltean Sept. 23, 2020, 6:45 p.m. UTC | #1 
On Tue, Sep 22, 2020 at 03:46:57AM +0300, Vladimir Oltean wrote:
> From: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
> 
> There are 2 separate, but related, issues.
> 
> First, the ocelot_vcap_block_get_filter_index function, née
> ocelot_ace_rule_get_index_id prior to the aae4e500e106 ("net: mscc:
> ocelot: generalize the "ACE/ACL" names") rename, does not do what the
> author probably intended. If the desired filter entry is not present in
> the ACL block, this function returns an index equal to the total number
> of filters, instead of -1, which is maybe what was intended, judging
> from the curious initialization with -1, and the "++index" idioms.
> Either way, none of the callers seems to expect this behavior.
> 
> Second issue, the callers don't actually check the return value at all.
> So in case the filter is not found in the rule list, propagate the
> return code to avoid kernel panics.
> 
> So update the callers and also take the opportunity to get rid of the
> odd coding idioms that appear to work but don't.
> 
> Fixes: b596229448dd ("net: mscc: ocelot: Add support for tcam")
> Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> Changes in v2:
> Add Fixes tag.
> 
>  drivers/net/ethernet/mscc/ocelot_vcap.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot_vcap.c b/drivers/net/ethernet/mscc/ocelot_vcap.c
> index 3ef620faf995..39edaaca836e 100644
> --- a/drivers/net/ethernet/mscc/ocelot_vcap.c
> +++ b/drivers/net/ethernet/mscc/ocelot_vcap.c
> @@ -726,14 +726,15 @@ static int ocelot_vcap_block_get_filter_index(struct ocelot_vcap_block *block,
>  					      struct ocelot_vcap_filter *filter)
>  {
>  	struct ocelot_vcap_filter *tmp;
> -	int index = -1;
> +	int index = 0;
>  
>  	list_for_each_entry(tmp, &block->rules, list) {
> -		++index;
>  		if (filter->id == tmp->id)
>  			break;

Please don't apply this. I meant to "return index" here instead of
leaving the "break". I'm not really sure what happened.

> +		index++;
>  	}
> -	return index;
> +
> +	return -ENOENT;
>  }
>  
>  static struct ocelot_vcap_filter*
> @@ -877,6 +878,8 @@ int ocelot_vcap_filter_add(struct ocelot *ocelot,
>  
>  	/* Get the index of the inserted filter */
>  	index = ocelot_vcap_block_get_filter_index(block, filter);
> +	if (index < 0)
> +		return index;
>  
>  	/* Move down the rules to make place for the new filter */
>  	for (i = block->count - 1; i > index; i--) {
> @@ -924,6 +927,8 @@ int ocelot_vcap_filter_del(struct ocelot *ocelot,
>  
>  	/* Gets index of the filter */
>  	index = ocelot_vcap_block_get_filter_index(block, filter);
> +	if (index < 0)
> +		return index;
>  
>  	/* Delete filter */
>  	ocelot_vcap_block_remove_filter(ocelot, block, filter);
> @@ -950,6 +955,9 @@ int ocelot_vcap_filter_stats_update(struct ocelot *ocelot,
>  	int index;
>  
>  	index = ocelot_vcap_block_get_filter_index(block, filter);
> +	if (index < 0)
> +		return index;
> +
>  	is2_entry_get(ocelot, filter, index);
>  
>  	/* After we get the result we need to clear the counters */
> -- 
> 2.25.1
> 

Thanks,
-Vladimir
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mscc/ocelot_vcap.c b/drivers/net/ethernet/mscc/ocelot_vcap.c
index 3ef620faf995..39edaaca836e 100644
--- a/drivers/net/ethernet/mscc/ocelot_vcap.c
+++ b/drivers/net/ethernet/mscc/ocelot_vcap.c
@@ -726,14 +726,15 @@  static int ocelot_vcap_block_get_filter_index(struct ocelot_vcap_block *block,
 					      struct ocelot_vcap_filter *filter)
 {
 	struct ocelot_vcap_filter *tmp;
-	int index = -1;
+	int index = 0;
 
 	list_for_each_entry(tmp, &block->rules, list) {
-		++index;
 		if (filter->id == tmp->id)
 			break;
+		index++;
 	}
-	return index;
+
+	return -ENOENT;
 }
 
 static struct ocelot_vcap_filter*
@@ -877,6 +878,8 @@  int ocelot_vcap_filter_add(struct ocelot *ocelot,
 
 	/* Get the index of the inserted filter */
 	index = ocelot_vcap_block_get_filter_index(block, filter);
+	if (index < 0)
+		return index;
 
 	/* Move down the rules to make place for the new filter */
 	for (i = block->count - 1; i > index; i--) {
@@ -924,6 +927,8 @@  int ocelot_vcap_filter_del(struct ocelot *ocelot,
 
 	/* Gets index of the filter */
 	index = ocelot_vcap_block_get_filter_index(block, filter);
+	if (index < 0)
+		return index;
 
 	/* Delete filter */
 	ocelot_vcap_block_remove_filter(ocelot, block, filter);
@@ -950,6 +955,9 @@  int ocelot_vcap_filter_stats_update(struct ocelot *ocelot,
 	int index;
 
 	index = ocelot_vcap_block_get_filter_index(block, filter);
+	if (index < 0)
+		return index;
+
 	is2_entry_get(ocelot, filter, index);
 
 	/* After we get the result we need to clear the counters */