| Message ID | 1494413139-11883-10-git-send-email-selvin.xavier@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, May 10, 2017 at 03:45:34AM -0700, Selvin Xavier wrote: > Do not free context table entry if delete GID fails. Stack may call > back the add_gid for the same context again which could result in > panic "may call" ??? Will you leave memory leak if this "may call" won't happen? > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > --- > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > index 61703f3..525f4b0 100644 > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > @@ -375,15 +375,17 @@ int bnxt_re_del_gid(struct ib_device *ibdev, u8 port_num, > return -EINVAL; > ctx->refcnt--; > if (!ctx->refcnt) { > - rc = bnxt_qplib_del_sgid > - (sgid_tbl, > - &sgid_tbl->tbl[ctx->idx], true); > - if (rc) > + rc = bnxt_qplib_del_sgid(sgid_tbl, > + &sgid_tbl->tbl[ctx->idx], > + true); > + if (rc) { > dev_err(rdev_to_dev(rdev), > "Failed to remove GID: %#x", rc); > - ctx_tbl = sgid_tbl->ctx; > - ctx_tbl[ctx->idx] = NULL; > - kfree(ctx); > + } else { > + ctx_tbl = sgid_tbl->ctx; > + ctx_tbl[ctx->idx] = NULL; > + kfree(ctx); > + } > } > } else { > return -EINVAL; > -- > 2.5.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 14, 2017 at 11:40 AM, Leon Romanovsky <leon@kernel.org> wrote: > On Wed, May 10, 2017 at 03:45:34AM -0700, Selvin Xavier wrote: >> Do not free context table entry if delete GID fails. Stack may call >> back the add_gid for the same context again which could result in >> panic > > "may call" ??? Will you leave memory leak if this "may call" won't happen? > This fix was added only to avoid system crash in some a specific scenario. When bnxt_re driver is loaded and if user tries to change interface mac address, our delete GID fails because QP1 is still associated with existing MAC (default GID). If the above command fails GID tables are not modified in the h/w or driver, but the GID context memory is freed. Now, if the user changes the mac back to the original value, another add_gid comes to the driver where the driver reports that the GID is already present in its table and tries to access the context which was already freed. So, in this case, in order to avoid NULL pointer de-reference, this patch removes the context memory free if delete_gid fails and the same context memory is re-used in new add_gid. Memory cleanup will be taken care during driver unload, while deleting the GID table. I understood that the "may call" comment is not detailing this problem. I will update the commit log and send a v2. Thanks for your comments. Selvin Xavier > >> >> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> >> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> >> --- >> drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c >> index 61703f3..525f4b0 100644 >> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c >> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c >> @@ -375,15 +375,17 @@ int bnxt_re_del_gid(struct ib_device *ibdev, u8 port_num, >> return -EINVAL; >> ctx->refcnt--; >> if (!ctx->refcnt) { >> - rc = bnxt_qplib_del_sgid >> - (sgid_tbl, >> - &sgid_tbl->tbl[ctx->idx], true); >> - if (rc) >> + rc = bnxt_qplib_del_sgid(sgid_tbl, >> + &sgid_tbl->tbl[ctx->idx], >> + true); >> + if (rc) { >> dev_err(rdev_to_dev(rdev), >> "Failed to remove GID: %#x", rc); >> - ctx_tbl = sgid_tbl->ctx; >> - ctx_tbl[ctx->idx] = NULL; >> - kfree(ctx); >> + } else { >> + ctx_tbl = sgid_tbl->ctx; >> + ctx_tbl[ctx->idx] = NULL; >> + kfree(ctx); >> + } >> } >> } else { >> return -EINVAL; >> -- >> 2.5.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 15, 2017 at 04:24:04PM +0530, Selvin Xavier wrote: > On Sun, May 14, 2017 at 11:40 AM, Leon Romanovsky <leon@kernel.org> wrote: > > On Wed, May 10, 2017 at 03:45:34AM -0700, Selvin Xavier wrote: > >> Do not free context table entry if delete GID fails. Stack may call > >> back the add_gid for the same context again which could result in > >> panic > > > > "may call" ??? Will you leave memory leak if this "may call" won't happen? > > > This fix was added only to avoid system crash in some a specific scenario. > When bnxt_re driver is loaded and if user tries to change interface mac address, > our delete GID fails because QP1 is still associated with existing MAC > (default GID). > If the above command fails GID tables are not modified in the h/w or > driver, but the GID context > memory is freed. Now, if the user changes the mac back to the original > value, another add_gid > comes to the driver where the driver reports that the GID is already > present in its table > and tries to access the context which was already freed. > > So, in this case, in order to avoid NULL pointer de-reference, this > patch removes the context memory > free if delete_gid fails and the same context memory is re-used in new add_gid. > Memory cleanup will be taken care during driver unload, while deleting > the GID table. > > > I understood that the "may call" comment is not detailing this problem. > I will update the commit log and send a v2. Thanks > > Thanks for your comments. > Selvin Xavier > > > > >> > >> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > >> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > >> --- > >> drivers/infiniband/hw/bnxt_re/ib_verbs.c | 16 +++++++++------- > >> 1 file changed, 9 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > >> index 61703f3..525f4b0 100644 > >> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > >> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > >> @@ -375,15 +375,17 @@ int bnxt_re_del_gid(struct ib_device *ibdev, u8 port_num, > >> return -EINVAL; > >> ctx->refcnt--; > >> if (!ctx->refcnt) { > >> - rc = bnxt_qplib_del_sgid > >> - (sgid_tbl, > >> - &sgid_tbl->tbl[ctx->idx], true); > >> - if (rc) > >> + rc = bnxt_qplib_del_sgid(sgid_tbl, > >> + &sgid_tbl->tbl[ctx->idx], > >> + true); > >> + if (rc) { > >> dev_err(rdev_to_dev(rdev), > >> "Failed to remove GID: %#x", rc); > >> - ctx_tbl = sgid_tbl->ctx; > >> - ctx_tbl[ctx->idx] = NULL; > >> - kfree(ctx); > >> + } else { > >> + ctx_tbl = sgid_tbl->ctx; > >> + ctx_tbl[ctx->idx] = NULL; > >> + kfree(ctx); > >> + } > >> } > >> } else { > >> return -EINVAL; > >> -- > >> 2.5.5 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c index 61703f3..525f4b0 100644 --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c @@ -375,15 +375,17 @@ int bnxt_re_del_gid(struct ib_device *ibdev, u8 port_num, return -EINVAL; ctx->refcnt--; if (!ctx->refcnt) { - rc = bnxt_qplib_del_sgid - (sgid_tbl, - &sgid_tbl->tbl[ctx->idx], true); - if (rc) + rc = bnxt_qplib_del_sgid(sgid_tbl, + &sgid_tbl->tbl[ctx->idx], + true); + if (rc) { dev_err(rdev_to_dev(rdev), "Failed to remove GID: %#x", rc); - ctx_tbl = sgid_tbl->ctx; - ctx_tbl[ctx->idx] = NULL; - kfree(ctx); + } else { + ctx_tbl = sgid_tbl->ctx; + ctx_tbl[ctx->idx] = NULL; + kfree(ctx); + } } } else { return -EINVAL;