On Mon, May 15, 2017 at 04:24:04PM +0530, Selvin Xavier wrote: > On Sun, May 14, 2017 at 11:40 AM, Leon Romanovsky 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 > >> Signed-off-by: Selvin Xavier > >> --- > >> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html