| Message ID | 580de136ad9b85b0d70709e912cfddd21b7e3f6f.1729930153.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] RDMA/bnxt_re: Fix some error handling paths in bnxt_re_probe() | expand |
Hi Christophe, Thank you for the patch. Please see my response below. On Sat, Oct 26, 2024 at 1:39 PM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > If bnxt_re_add_device() fails, 'en_info' still needs to be freed. This is > done in bnxt_re_remove() with the needed locking. > > The commit in Fixes: in-correctly removed this call, certainly because it > was expecting the .remove() function was called anyway. But if the probe > fails, the remove function is not called. > > To fix this memory leak, partly revert this patch and restore the explicit > call to the remove function in the error handling path of the probe. > > Fixes: a5e099e0c464 ("RDMA/bnxt_re: Fix an error path in bnxt_re_add_device") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested only > > > Another solution, maybe more elegant, would be only call kfree() in the > error handling path. In fact locking and the other stuff in the remove > look useless in this specific case. > --- > drivers/infiniband/hw/bnxt_re/main.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c > index 6715c96a3eee..d183e293ec96 100644 > --- a/drivers/infiniband/hw/bnxt_re/main.c > +++ b/drivers/infiniband/hw/bnxt_re/main.c > @@ -2025,7 +2025,15 @@ static int bnxt_re_probe(struct auxiliary_device *adev, > auxiliary_set_drvdata(adev, en_info); > > rc = bnxt_re_add_device(adev, BNXT_RE_COMPLETE_INIT); > + if (rc) [Kalesh] As you suggested, how about a small change to free en_info here? You do not have to invoke bnxt_re_remove() in the error path here. > + goto err; > mutex_unlock(&bnxt_re_mutex); > + return 0; > + > +err: > + mutex_unlock(&bnxt_re_mutex); > + bnxt_re_remove(adev); > + > return rc; > } > > -- > 2.47.0 >
diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c index 6715c96a3eee..d183e293ec96 100644 --- a/drivers/infiniband/hw/bnxt_re/main.c +++ b/drivers/infiniband/hw/bnxt_re/main.c @@ -2025,7 +2025,15 @@ static int bnxt_re_probe(struct auxiliary_device *adev, auxiliary_set_drvdata(adev, en_info); rc = bnxt_re_add_device(adev, BNXT_RE_COMPLETE_INIT); + if (rc) + goto err; mutex_unlock(&bnxt_re_mutex); + return 0; + +err: + mutex_unlock(&bnxt_re_mutex); + bnxt_re_remove(adev); + return rc; }
If bnxt_re_add_device() fails, 'en_info' still needs to be freed. This is done in bnxt_re_remove() with the needed locking. The commit in Fixes: in-correctly removed this call, certainly because it was expecting the .remove() function was called anyway. But if the probe fails, the remove function is not called. To fix this memory leak, partly revert this patch and restore the explicit call to the remove function in the error handling path of the probe. Fixes: a5e099e0c464 ("RDMA/bnxt_re: Fix an error path in bnxt_re_add_device") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Compile tested only Another solution, maybe more elegant, would be only call kfree() in the error handling path. In fact locking and the other stuff in the remove look useless in this specific case. --- drivers/infiniband/hw/bnxt_re/main.c | 8 ++++++++ 1 file changed, 8 insertions(+)