| Message ID | aadb7736-c497-43db-a93a-4461d1426de4@stanley.mountain (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net] net/mlx5: DR, prevent potential error pointer dereference | expand |
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>
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.
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
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 > >
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
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 >> >> >
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 > > > > > > > > >
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 >>>> >>>> >>> >>
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>
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); }
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(+)