[PATCH 0/3] net/mlx5e: Fix some static analysis warnings

[PATCH 0/3] net/mlx5e: Fix some static analysis warnings

[Alex Dewar]

Hi,

Coverity has flagged up some warnings for this driver, which I address
in this patch series. All the fixes are fairly trivial and have been
build-tested.

Best,
Alex

[PATCH 2/3] net/mlx5e: Clean up error handling in mlx5e_alloc_flow()

[Alex Dewar]

The variable flow is used after being allocated but before being
null-checked, which will cause a null pointer dereference if the
allocation failed. Fix this and tidy up the error-checking logic in this
function.

Addresses-Coverity: CID 1497154: Null pointer dereferences (REVERSE_INULL)
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 20 ++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index b3c57b984a2a..ed308407be6f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4536,20 +4536,22 @@ mlx5e_alloc_flow(struct mlx5e_priv *priv, int attr_size,
 	struct mlx5e_tc_flow_parse_attr *parse_attr;
 	struct mlx5_flow_attr *attr;
 	struct mlx5e_tc_flow *flow;
-	int out_index, err;
+	int out_index;
 
 	flow = kzalloc(sizeof(*flow), GFP_KERNEL);
+	if (!flow)
+		return -ENOMEM;
 	parse_attr = kvzalloc(sizeof(*parse_attr), GFP_KERNEL);
+	if (!parse_attr)
+		goto err_free_flow;
 
 	flow->flags = flow_flags;
 	flow->cookie = f->cookie;
 	flow->priv = priv;
 
 	attr = mlx5_alloc_flow_attr(get_flow_name_space(flow));
-	if (!parse_attr || !flow || !attr) {
-		err = -ENOMEM;
-		goto err_free;
-	}
+	if (!attr)
+		goto err_free_parse_attr;
 	flow->attr = attr;
 
 	for (out_index = 0; out_index < MLX5_MAX_FLOW_FWD_VPORTS; out_index++)
@@ -4564,11 +4566,11 @@ mlx5e_alloc_flow(struct mlx5e_priv *priv, int attr_size,
 
 	return 0;
 
-err_free:
-	kfree(flow);
+err_free_parse_attr:
 	kvfree(parse_attr);
-	kfree(attr);
-	return err;
+err_free_flow:
+	kfree(flow);
+	return -ENOMEM;
 }
 
 static void
-- 
2.28.0

[PATCH 3/3] net/mlx5e: Fix use of freed pointer

[Alex Dewar]

If the call to mlx5_fc_create() fails, then shared_counter will be freed
before its member, shared_counter->counter, is accessed to retrieve the
error code. Fix by using an intermediate variable.

Addresses-Coverity: CID 1497153: Memory - illegal accesses (USE_AFTER_FREE)
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
index b5f8ed30047b..5851a1dfe6e4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
@@ -738,6 +738,7 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
 	struct mlx5_ct_shared_counter *shared_counter;
 	struct mlx5_core_dev *dev = ct_priv->dev;
 	struct mlx5_ct_entry *rev_entry;
+	struct mlx5_fc *counter;
 	__be16 tmp_port;
 
 	/* get the reversed tuple */
@@ -775,12 +776,13 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
 	if (!shared_counter)
 		return ERR_PTR(-ENOMEM);
 
-	shared_counter->counter = mlx5_fc_create(dev, true);
-	if (IS_ERR(shared_counter->counter)) {
+	counter = mlx5_fc_create(dev, true);
+	if (IS_ERR(counter)) {
 		ct_dbg("Failed to create counter for ct entry");
 		kfree(shared_counter);
-		return ERR_PTR(PTR_ERR(shared_counter->counter));
+		return (struct mlx5_ct_shared_counter *)counter;
 	}
+	shared_counter->counter = counter;
 
 	refcount_set(&shared_counter->refcount, 1);
 	return shared_counter;
-- 
2.28.0

[PATCH 1/3] net/mlx5e: Fix possible null pointer dereference

[Alex Dewar]

In mlx5e_tc_unoffload_from_slow_path() a null check is performed for the
variable slow_attr and a warning is issued if it is null. However,
slow_attr is used later on in the function regardless. Fix this by
returning if slow_attr is null.

Addresses-Coverity: CID 1497163: Null pointer dereferences (FORWARD_NULL)
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index f815b0c60a6c..b3c57b984a2a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -1238,8 +1238,10 @@ mlx5e_tc_unoffload_from_slow_path(struct mlx5_eswitch *esw,
 	struct mlx5_flow_attr *slow_attr;
 
 	slow_attr = mlx5_alloc_flow_attr(MLX5_FLOW_NAMESPACE_FDB);
-	if (!slow_attr)
+	if (!slow_attr) {
 		mlx5_core_warn(flow->priv->mdev, "Unable to unoffload slow path rule\n");
+		return;
+	}
 
 	memcpy(slow_attr, flow->attr, ESW_FLOW_ATTR_SZ);
 	slow_attr->action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
-- 
2.28.0

Re: [PATCH 2/3] net/mlx5e: Clean up error handling in mlx5e_alloc_flow()

[Leon Romanovsky]

On Sun, Sep 27, 2020 at 12:32:52PM +0100, Alex Dewar wrote:
> The variable flow is used after being allocated but before being
> null-checked, which will cause a null pointer dereference if the
> allocation failed. Fix this and tidy up the error-checking logic in this
> function.
>
> Addresses-Coverity: CID 1497154: Null pointer dereferences (REVERSE_INULL)
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
>  .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 20 ++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>

Thanks, but Gustavo already sent a fix.
https://lore.kernel.org/lkml/20200925164913.GA18472@embeddedor

Re: [PATCH 3/3] net/mlx5e: Fix use of freed pointer

[Leon Romanovsky]

On Sun, Sep 27, 2020 at 12:32:53PM +0100, Alex Dewar wrote:
> If the call to mlx5_fc_create() fails, then shared_counter will be freed
> before its member, shared_counter->counter, is accessed to retrieve the
> error code. Fix by using an intermediate variable.
>
> Addresses-Coverity: CID 1497153: Memory - illegal accesses (USE_AFTER_FREE)
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---

Please add Fixes line.

>  drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> index b5f8ed30047b..5851a1dfe6e4 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
> @@ -738,6 +738,7 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
>  	struct mlx5_ct_shared_counter *shared_counter;
>  	struct mlx5_core_dev *dev = ct_priv->dev;
>  	struct mlx5_ct_entry *rev_entry;
> +	struct mlx5_fc *counter;
>  	__be16 tmp_port;
>
>  	/* get the reversed tuple */
> @@ -775,12 +776,13 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
>  	if (!shared_counter)
>  		return ERR_PTR(-ENOMEM);
>
> -	shared_counter->counter = mlx5_fc_create(dev, true);
> -	if (IS_ERR(shared_counter->counter)) {
> +	counter = mlx5_fc_create(dev, true);
> +	if (IS_ERR(counter)) {
>  		ct_dbg("Failed to create counter for ct entry");
>  		kfree(shared_counter);
> -		return ERR_PTR(PTR_ERR(shared_counter->counter));
> +		return (struct mlx5_ct_shared_counter *)counter;

return ERR_CAST(counter);


>  	}
> +	shared_counter->counter = counter;
>
>  	refcount_set(&shared_counter->refcount, 1);
>  	return shared_counter;
> --
> 2.28.0
>

[PATCH v2] net/mlx5e: Fix use of freed pointer

[Alex Dewar]

If the call to mlx5_fc_create() fails, then shared_counter will be freed
before its member, shared_counter->counter, is accessed to retrieve the
error code. Fix by using an intermediate variable.

Addresses-Coverity: CID 1497153: Memory - illegal accesses (USE_AFTER_FREE)
Fixes: 1edae2335adf ("net/mlx5e: CT: Use the same counter for both directions")
Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
---
v2:
	- Add Fixes tag (Leon)
	- Use ERR_CAST (Leon)

Hi Leon,

I've made the suggested changes. Let me know if there's anything else
you need :)

There is also this patch in the series which doesn't seem to have been
reviewed yet: https://lore.kernel.org/lkml/20200927113254.362480-4-alex.dewar90@gmail.com/

Best,
Alex


 drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
index b5f8ed30047b..1e80e7669995 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_ct.c
@@ -738,6 +738,7 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
 	struct mlx5_ct_shared_counter *shared_counter;
 	struct mlx5_core_dev *dev = ct_priv->dev;
 	struct mlx5_ct_entry *rev_entry;
+	struct mlx5_fc *counter;
 	__be16 tmp_port;
 
 	/* get the reversed tuple */
@@ -775,12 +776,13 @@ mlx5_tc_ct_shared_counter_get(struct mlx5_tc_ct_priv *ct_priv,
 	if (!shared_counter)
 		return ERR_PTR(-ENOMEM);
 
-	shared_counter->counter = mlx5_fc_create(dev, true);
-	if (IS_ERR(shared_counter->counter)) {
+	counter = mlx5_fc_create(dev, true);
+	if (IS_ERR(counter)) {
 		ct_dbg("Failed to create counter for ct entry");
 		kfree(shared_counter);
-		return ERR_PTR(PTR_ERR(shared_counter->counter));
+		return ERR_CAST(counter);
 	}
+	shared_counter->counter = counter;
 
 	refcount_set(&shared_counter->refcount, 1);
 	return shared_counter;
-- 
2.28.0

Re: [PATCH v2] net/mlx5e: Fix use of freed pointer

[Leon Romanovsky]

On Tue, Sep 29, 2020 at 11:15:49AM +0100, Alex Dewar wrote:
> If the call to mlx5_fc_create() fails, then shared_counter will be freed
> before its member, shared_counter->counter, is accessed to retrieve the
> error code. Fix by using an intermediate variable.
>
> Addresses-Coverity: CID 1497153: Memory - illegal accesses (USE_AFTER_FREE)
> Fixes: 1edae2335adf ("net/mlx5e: CT: Use the same counter for both directions")
> Signed-off-by: Alex Dewar <alex.dewar90@gmail.com>
> ---
> v2:
> 	- Add Fixes tag (Leon)
> 	- Use ERR_CAST (Leon)
>
> Hi Leon,
>
> I've made the suggested changes. Let me know if there's anything else
> you need :)

Hi Alex,

Saeed already picked Dan's patch.
https://lore.kernel.org/linux-rdma/1017ab3724b83818c03dfa7661b3f31827a7f62f.camel@kernel.org/T/#t

>
> There is also this patch in the series which doesn't seem to have been
> reviewed yet: https://lore.kernel.org/lkml/20200927113254.362480-4-alex.dewar90@gmail.com/

Ariel is handling this internally.
https://lore.kernel.org/linux-rdma/64f6a3eaaac505c341f996df0b0877ee9af56c00.camel@kernel.org/T/#t

Thanks