[PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[Dan Carpenter]

The dr_domain_add_vport_cap() function genereally returns NULL on error
but sometimes we want it to return ERR_PTR(-EBUSY) so the caller can
retry.  The problem here is that "ret" can be either -EBUSY or -ENOMEM
and if it's and -ENOMEM then the error pointer is propogated back and
eventually dereferenced in dr_ste_v0_build_src_gvmi_qpn_tag().

Fixes: 11a45def2e19 ("net/mlx5: DR, Add support for SF vports")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 .../net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c    | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c
index 3d74109f8230..a379e8358f82 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c
@@ -297,6 +297,8 @@ dr_domain_add_vport_cap(struct mlx5dr_domain *dmn, u16 vport)
 	if (ret) {
 		mlx5dr_dbg(dmn, "Couldn't insert new vport into xarray (%d)\n", ret);
 		kvfree(vport_caps);
+		if (ret != -EBUSY)
+			return NULL;
 		return ERR_PTR(ret);
 	}
 
-- 
2.45.2

Re: [PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[Yevgeny Kliteynik]

On 30-Nov-24 12:01, Dan Carpenter wrote:
> The dr_domain_add_vport_cap() function genereally returns NULL on error
> but sometimes we want it to return ERR_PTR(-EBUSY) so the caller can
> retry.  The problem here is that "ret" can be either -EBUSY or -ENOMEM
> and if it's and -ENOMEM then the error pointer is propogated back and
> eventually dereferenced in dr_ste_v0_build_src_gvmi_qpn_tag().
> 
> Fixes: 11a45def2e19 ("net/mlx5: DR, Add support for SF vports")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>   .../net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c    | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c
> index 3d74109f8230..a379e8358f82 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c
> @@ -297,6 +297,8 @@ dr_domain_add_vport_cap(struct mlx5dr_domain *dmn, u16 vport)
>   	if (ret) {
>   		mlx5dr_dbg(dmn, "Couldn't insert new vport into xarray (%d)\n", ret);
>   		kvfree(vport_caps);
> +		if (ret != -EBUSY)
> +			return NULL;
>   		return ERR_PTR(ret);
>   	}
>   

Thanks Dan,

Reviewed-by: Yevgeny Kliteynik <kliteyn@nvidia.com>

Re: [PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[Mateusz Polchlopek]

On 11/30/2024 11:01 AM, Dan Carpenter wrote:
> The dr_domain_add_vport_cap() function genereally returns NULL on error

Typo. Should be "generally"

> but sometimes we want it to return ERR_PTR(-EBUSY) so the caller can
> retry.  The problem here is that "ret" can be either -EBUSY or -ENOMEM

Please remove unnecessary space.

> and if it's and -ENOMEM then the error pointer is propogated back and
> eventually dereferenced in dr_ste_v0_build_src_gvmi_qpn_tag().
> 
> Fixes: 11a45def2e19 ("net/mlx5: DR, Add support for SF vports")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>   .../net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c    | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c
> index 3d74109f8230..a379e8358f82 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/sws/dr_domain.c
> @@ -297,6 +297,8 @@ dr_domain_add_vport_cap(struct mlx5dr_domain *dmn, u16 vport)
>   	if (ret) {
>   		mlx5dr_dbg(dmn, "Couldn't insert new vport into xarray (%d)\n", ret);
>   		kvfree(vport_caps);
> +		if (ret != -EBUSY)
> +			return NULL;
>   		return ERR_PTR(ret);
>   	}
>

When applied those comments please add my RB tag.

Re: [PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[Dan Carpenter]

On Tue, Dec 03, 2024 at 10:32:13AM +0100, Mateusz Polchlopek wrote:
> 
> 
> On 11/30/2024 11:01 AM, Dan Carpenter wrote:
> > The dr_domain_add_vport_cap() function genereally returns NULL on error
> 
> Typo. Should be "generally"
> 

Sure.

> > but sometimes we want it to return ERR_PTR(-EBUSY) so the caller can
> > retry.  The problem here is that "ret" can be either -EBUSY or -ENOMEM
> 
> Please remove unnecessary space.
> 

What are you talking about?

regards,
dan carpenter

Re: [PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[Yevgeny Kliteynik]

On 03-Dec-24 11:39, Dan Carpenter wrote:
> On Tue, Dec 03, 2024 at 10:32:13AM +0100, Mateusz Polchlopek wrote:
>>
>>
>> On 11/30/2024 11:01 AM, Dan Carpenter wrote:
>>> The dr_domain_add_vport_cap() function genereally returns NULL on error
>>
>> Typo. Should be "generally"
>>
> 
> Sure.
> 
>>> but sometimes we want it to return ERR_PTR(-EBUSY) so the caller can
>>> retry.  The problem here is that "ret" can be either -EBUSY or -ENOMEM
>>
>> Please remove unnecessary space.
>>
> 
> What are you talking about?

Oh, I see it :)
Double space between "retry." and "The"

-- YK

> regards,
> dan carpenter
> 
> 

Re: [PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[Dan Carpenter]

On Tue, Dec 03, 2024 at 11:44:12AM +0200, Yevgeny Kliteynik wrote:
> On 03-Dec-24 11:39, Dan Carpenter wrote:
> > On Tue, Dec 03, 2024 at 10:32:13AM +0100, Mateusz Polchlopek wrote:
> > > 
> > > 
> > > On 11/30/2024 11:01 AM, Dan Carpenter wrote:
> > > > The dr_domain_add_vport_cap() function genereally returns NULL on error
> > > 
> > > Typo. Should be "generally"
> > > 
> > 
> > Sure.
> > 
> > > > but sometimes we want it to return ERR_PTR(-EBUSY) so the caller can
> > > > retry.  The problem here is that "ret" can be either -EBUSY or -ENOMEM
> > > 
> > > Please remove unnecessary space.
> > > 
> > 
> > What are you talking about?
> 
> Oh, I see it :)
> Double space between "retry." and "The"

I'm old.  I'm still using fixed width fonts in my editor so I still use
the old school rules.

regards,
dan carpenter

Re: [PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[Mateusz Polchlopek]

On 12/3/2024 10:44 AM, Yevgeny Kliteynik wrote:
> On 03-Dec-24 11:39, Dan Carpenter wrote:
>> On Tue, Dec 03, 2024 at 10:32:13AM +0100, Mateusz Polchlopek wrote:
>>>
>>>
>>> On 11/30/2024 11:01 AM, Dan Carpenter wrote:
>>>> The dr_domain_add_vport_cap() function genereally returns NULL on error
>>>
>>> Typo. Should be "generally"
>>>
>>
>> Sure.
>>
>>>> but sometimes we want it to return ERR_PTR(-EBUSY) so the caller can
>>>> retry.  The problem here is that "ret" can be either -EBUSY or -ENOMEM
>>>
>>> Please remove unnecessary space.
>>>
>>
>> What are you talking about?
> 
> Oh, I see it :)
> Double space between "retry." and "The"
> 
> -- YK
> 

Yup, exactly. Sorry, I could point it.


>> regards,
>> dan carpenter
>>
>>
> 

Re: [PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[Leon Romanovsky]

On Tue, Dec 03, 2024 at 11:02:15AM +0100, Mateusz Polchlopek wrote:
> 
> 
> On 12/3/2024 10:44 AM, Yevgeny Kliteynik wrote:
> > On 03-Dec-24 11:39, Dan Carpenter wrote:
> > > On Tue, Dec 03, 2024 at 10:32:13AM +0100, Mateusz Polchlopek wrote:
> > > > 
> > > > 
> > > > On 11/30/2024 11:01 AM, Dan Carpenter wrote:
> > > > > The dr_domain_add_vport_cap() function genereally returns NULL on error
> > > > 
> > > > Typo. Should be "generally"
> > > > 
> > > 
> > > Sure.
> > > 
> > > > > but sometimes we want it to return ERR_PTR(-EBUSY) so the caller can
> > > > > retry.  The problem here is that "ret" can be either -EBUSY or -ENOMEM
> > > > 
> > > > Please remove unnecessary space.
> > > > 
> > > 
> > > What are you talking about?
> > 
> > Oh, I see it :)
> > Double space between "retry." and "The"
> > 
> > -- YK
> > 
> 
> Yup, exactly. Sorry, I could point it.

Double space after period is perfectly valid typographic style. It is
with us for last 250 years.

Thanks

> 
> 
> > > regards,
> > > dan carpenter
> > > 
> > > 
> > 
> 

Re: [PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[Mateusz Polchlopek]

On 12/5/2024 9:32 AM, Leon Romanovsky wrote:
> On Tue, Dec 03, 2024 at 11:02:15AM +0100, Mateusz Polchlopek wrote:
>>
>>
>> On 12/3/2024 10:44 AM, Yevgeny Kliteynik wrote:
>>> On 03-Dec-24 11:39, Dan Carpenter wrote:
>>>> On Tue, Dec 03, 2024 at 10:32:13AM +0100, Mateusz Polchlopek wrote:
>>>>>
>>>>>
>>>>> On 11/30/2024 11:01 AM, Dan Carpenter wrote:
>>>>>> The dr_domain_add_vport_cap() function genereally returns NULL on error
>>>>>
>>>>> Typo. Should be "generally"
>>>>>
>>>>
>>>> Sure.
>>>>
>>>>>> but sometimes we want it to return ERR_PTR(-EBUSY) so the caller can
>>>>>> retry.  The problem here is that "ret" can be either -EBUSY or -ENOMEM
>>>>>
>>>>> Please remove unnecessary space.
>>>>>
>>>>
>>>> What are you talking about?
>>>
>>> Oh, I see it :)
>>> Double space between "retry." and "The"
>>>
>>> -- YK
>>>
>>
>> Yup, exactly. Sorry, I could point it.
> 
> Double space after period is perfectly valid typographic style. It is
> with us for last 250 years.
> 
> Thanks
> 

Cool, thanks for explanation and please forget about this unnecessary
nitpick :)

Thanks

>>
>>
>>>> regards,
>>>> dan carpenter
>>>>
>>>>
>>>
>>

Re: [PATCH net] net/mlx5: DR, prevent potential error pointer dereference

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

On Sat, Nov 30, 2024 at 3:31 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> The dr_domain_add_vport_cap() function genereally returns NULL on error
> but sometimes we want it to return ERR_PTR(-EBUSY) so the caller can
> retry.  The problem here is that "ret" can be either -EBUSY or -ENOMEM
> and if it's and -ENOMEM then the error pointer is propogated back and
> eventually dereferenced in dr_ste_v0_build_src_gvmi_qpn_tag().
>
> Fixes: 11a45def2e19 ("net/mlx5: DR, Add support for SF vports")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

couple of typos in commit log?
s/propogated/propagated
s/genereally/generally

LGTM otherwise
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>


-- 
Regards,
Kalesh A P

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]