| Message ID | 20221017142652.13906-1-n.petrova@fintech.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rdmavt: avoid NULL pointer dereference in rvt_qp_exit() | expand |
On Mon, Oct 17, 2022 at 05:26:52PM +0300, Natalia Petrova wrote: > rvt_qp_exit() checks 'rdi->qp_dev' for NULL, but the pointer is > dereferenced before that in rvt_free_all_qps(). > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: f92e48718889 ("IB/rdmavt: Reset all QPs when the device is shut > down") Please never break fixes line. > Signed-off-by: Natalia Petrova <n.petrova@fintech.ru> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > --- > drivers/infiniband/sw/rdmavt/qp.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c > index 3acab569fbb9..06e755975f61 100644 > --- a/drivers/infiniband/sw/rdmavt/qp.c > +++ b/drivers/infiniband/sw/rdmavt/qp.c > @@ -459,13 +459,16 @@ static unsigned rvt_free_all_qps(struct rvt_dev_info *rdi) > */ > void rvt_qp_exit(struct rvt_dev_info *rdi) > { > - u32 qps_inuse = rvt_free_all_qps(rdi); > + u32 qps_inuse = 0; > + > + if (!rdi->qp_dev) > + return; > + > + qps_inuse = rvt_free_all_qps(rdi); These lines are not needed. > > if (qps_inuse) > rvt_pr_err(rdi, "QP memory leak! %u still in use\n", > qps_inuse); > - if (!rdi->qp_dev) > - return; It is enough to delete these two lines. At this stage, rdi->qp_dev always exists as it was created in rvt_register_device(). Thanks
On 10/18/22 4:41 AM, Leon Romanovsky wrote: > On Mon, Oct 17, 2022 at 05:26:52PM +0300, Natalia Petrova wrote: >> rvt_qp_exit() checks 'rdi->qp_dev' for NULL, but the pointer is >> dereferenced before that in rvt_free_all_qps(). >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Fixes: f92e48718889 ("IB/rdmavt: Reset all QPs when the device is shut >> down") > > Please never break fixes line. > >> Signed-off-by: Natalia Petrova <n.petrova@fintech.ru> >> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> >> --- >> drivers/infiniband/sw/rdmavt/qp.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c >> index 3acab569fbb9..06e755975f61 100644 >> --- a/drivers/infiniband/sw/rdmavt/qp.c >> +++ b/drivers/infiniband/sw/rdmavt/qp.c >> @@ -459,13 +459,16 @@ static unsigned rvt_free_all_qps(struct rvt_dev_info *rdi) >> */ >> void rvt_qp_exit(struct rvt_dev_info *rdi) >> { >> - u32 qps_inuse = rvt_free_all_qps(rdi); >> + u32 qps_inuse = 0; >> + >> + if (!rdi->qp_dev) >> + return; >> + >> + qps_inuse = rvt_free_all_qps(rdi); > > These lines are not needed. > >> >> if (qps_inuse) >> rvt_pr_err(rdi, "QP memory leak! %u still in use\n", >> qps_inuse); >> - if (!rdi->qp_dev) >> - return; > > It is enough to delete these two lines. At this stage, rdi->qp_dev always > exists as it was created in rvt_register_device(). > Agree with Leon here. qp_dev is created in rvt_register_device which will fail if the qp dev allocation fails in rvt_driver_qp_init(). -Denny
diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c index 3acab569fbb9..06e755975f61 100644 --- a/drivers/infiniband/sw/rdmavt/qp.c +++ b/drivers/infiniband/sw/rdmavt/qp.c @@ -459,13 +459,16 @@ static unsigned rvt_free_all_qps(struct rvt_dev_info *rdi) */ void rvt_qp_exit(struct rvt_dev_info *rdi) { - u32 qps_inuse = rvt_free_all_qps(rdi); + u32 qps_inuse = 0; + + if (!rdi->qp_dev) + return; + + qps_inuse = rvt_free_all_qps(rdi); if (qps_inuse) rvt_pr_err(rdi, "QP memory leak! %u still in use\n", qps_inuse); - if (!rdi->qp_dev) - return; kfree(rdi->qp_dev->qp_table); free_qpn_table(&rdi->qp_dev->qpn_table);
rvt_qp_exit() checks 'rdi->qp_dev' for NULL, but the pointer is dereferenced before that in rvt_free_all_qps(). Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: f92e48718889 ("IB/rdmavt: Reset all QPs when the device is shut down") Signed-off-by: Natalia Petrova <n.petrova@fintech.ru> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- drivers/infiniband/sw/rdmavt/qp.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)