| Message ID | f6662b7b-bdb7-2706-1e12-47c61d3474b6@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [1/1] RDMA/cma: Fix rdma_resolve_route memory leak | expand |
On 6/25/2021 2:55 AM, Gerd Rausch wrote: > Fix a memory leak when "rmda_resolve_route" is called > more than once on the same "rdma_cm_id". > > Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com> > --- > drivers/infiniband/core/cma.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index ab148a696c0c..4a76d5b4163e 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -2819,7 +2819,8 @@ static int cma_resolve_ib_route(struct rdma_id_private *id_priv, > > cma_init_resolve_route_work(work, id_priv); > > - route->path_rec = kmalloc(sizeof *route->path_rec, GFP_KERNEL); > + if (!route->path_rec) > + route->path_rec = kmalloc(sizeof *route->path_rec, GFP_KERNEL); > if (!route->path_rec) { > ret = -ENOMEM; > goto err1; If route->path_rec does exist (meaning this is not the first time called), then it would be freed if cma_query_ib_route() below is failed, is it good?
> On 25 Jun 2021, at 07:49, Mark Zhang <markzhang@nvidia.com> wrote: > > On 6/25/2021 2:55 AM, Gerd Rausch wrote: >> Fix a memory leak when "rmda_resolve_route" is called >> more than once on the same "rdma_cm_id". >> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com> >> --- >> drivers/infiniband/core/cma.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c >> index ab148a696c0c..4a76d5b4163e 100644 >> --- a/drivers/infiniband/core/cma.c >> +++ b/drivers/infiniband/core/cma.c >> @@ -2819,7 +2819,8 @@ static int cma_resolve_ib_route(struct rdma_id_private *id_priv, >> cma_init_resolve_route_work(work, id_priv); >> - route->path_rec = kmalloc(sizeof *route->path_rec, GFP_KERNEL); >> + if (!route->path_rec) >> + route->path_rec = kmalloc(sizeof *route->path_rec, GFP_KERNEL); >> if (!route->path_rec) { >> ret = -ENOMEM; >> goto err1; > > If route->path_rec does exist (meaning this is not the first time called), then it would be freed if cma_query_ib_route() below is failed, is it good? This may happen if rdma_resolve_route() is called after a route error event has been received. In that case, the state is set back to RDMA_CM_ADDR_RESOLVED, so it seems the API allows rdma_resolve_route() to be called again without renewing the cm_id in this case. Another thing, if this fix is eligible, it should probably also go into cma_resolve_iboe_route() as well. Thxs, Håkon
On Thu, Jun 24, 2021 at 11:55:31AM -0700, Gerd Rausch wrote: > Fix a memory leak when "rmda_resolve_route" is called > more than once on the same "rdma_cm_id". > > Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com> > --- > drivers/infiniband/core/cma.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) I wondered for a while if it would be better to clear this in cma_query_handler(), but it seems this is OK Applied to for-next Thanks, Jason
Hi Mark, On 24/06/2021 22.49, Mark Zhang wrote: > On 6/25/2021 2:55 AM, Gerd Rausch wrote: >> Fix a memory leak when "rmda_resolve_route" is called >> more than once on the same "rdma_cm_id". >> >> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com> >> --- >> drivers/infiniband/core/cma.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c >> index ab148a696c0c..4a76d5b4163e 100644 >> --- a/drivers/infiniband/core/cma.c >> +++ b/drivers/infiniband/core/cma.c >> @@ -2819,7 +2819,8 @@ static int cma_resolve_ib_route(struct rdma_id_private *id_priv, >> cma_init_resolve_route_work(work, id_priv); >> - route->path_rec = kmalloc(sizeof *route->path_rec, GFP_KERNEL); >> + if (!route->path_rec) >> + route->path_rec = kmalloc(sizeof *route->path_rec, GFP_KERNEL); >> if (!route->path_rec) { >> ret = -ENOMEM; >> goto err1; > > If route->path_rec does exist (meaning this is not the first time called), then it would be freed if cma_query_ib_route() below is failed, is it good? So the caller performs "rdma_resolve_route" which returns an immediate error, but the expectation would be that the cm_id still points to a valid (!= NULL) path record (even though this error indicateed route lookup failed). Which code-part and call-sequence would have such expectation? I can't say I'm in love with this approach either, since the code makes the assumption that "path_rec" (if != NULL) points to a space that was allocated by "kmalloc" before and can be overwritten. It's not permitted to point to something else. But "__rdma_free" makes that assumption anyway, since it unconditionally calls "kfree" on this pointer. So there is precedence. Just my 2ç, Gerd
Hi Jason, On 25/06/2021 08.03, Jason Gunthorpe wrote: > On Thu, Jun 24, 2021 at 11:55:31AM -0700, Gerd Rausch wrote: >> Fix a memory leak when "rmda_resolve_route" is called >> more than once on the same "rdma_cm_id". >> >> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com> >> --- >> drivers/infiniband/core/cma.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > I wondered for a while if it would be better to clear this in > cma_query_handler(), but it seems this is OK > > Applied to for-next > Thank you, I appreciate it. Gerd
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index ab148a696c0c..4a76d5b4163e 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -2819,7 +2819,8 @@ static int cma_resolve_ib_route(struct rdma_id_private *id_priv, cma_init_resolve_route_work(work, id_priv); - route->path_rec = kmalloc(sizeof *route->path_rec, GFP_KERNEL); + if (!route->path_rec) + route->path_rec = kmalloc(sizeof *route->path_rec, GFP_KERNEL); if (!route->path_rec) { ret = -ENOMEM; goto err1;
Fix a memory leak when "rmda_resolve_route" is called more than once on the same "rdma_cm_id". Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com> --- drivers/infiniband/core/cma.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)