* [bug report] rdma/siw: connection management
@ 2019-08-19 12:09 Dan Carpenter
2019-08-19 14:02 ` Bernard Metzler
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2019-08-19 12:09 UTC (permalink / raw)
To: bmt; +Cc: linux-rdma
Hello Bernard Metzler,
This is a semi-automatic email about new static checker warnings.
The patch 6c52fdc244b5: "rdma/siw: connection management" from Jun
20, 2019, leads to the following Smatch complaint:
drivers/infiniband/sw/siw/siw_cm.c:1518 siw_connect()
error: we previously assumed 'qp' could be null (see line 1372)
drivers/infiniband/sw/siw/siw_cm.c
1371 qp = siw_qp_id2obj(sdev, params->qpn);
1372 if (!qp) {
^^^
NULL
1373 WARN(1, "[QP %u] does not exist\n", params->qpn);
1374 rv = -EINVAL;
1375 goto error;
^^^^^^^^^^
1376 }
1377 if (v4)
1378 siw_dbg_qp(qp,
[ snip ]
1508 if (rv >= 0) {
1509 rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
1510 if (!rv) {
1511 siw_dbg_cep(cep, "id 0x%p, [QP %u]: exit\n", id,
1512 qp_id(qp));
1513 siw_cep_set_free(cep);
1514 return 0;
1515 }
1516 }
1517 error:
1518 siw_dbg_qp(qp, "failed: %d\n", rv);
^^
NULL dereference.
1519
1520 if (cep) {
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] rdma/siw: connection management
2019-08-19 12:09 [bug report] rdma/siw: connection management Dan Carpenter
@ 2019-08-19 14:02 ` Bernard Metzler
0 siblings, 0 replies; 5+ messages in thread
From: Bernard Metzler @ 2019-08-19 14:02 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-rdma
-----"Dan Carpenter" <dan.carpenter@oracle.com> wrote: -----
>To: bmt@zurich.ibm.com
>From: "Dan Carpenter" <dan.carpenter@oracle.com>
>Date: 08/19/2019 02:12PM
>Cc: linux-rdma@vger.kernel.org
>Subject: [EXTERNAL] [bug report] rdma/siw: connection management
>
>Hello Bernard Metzler,
>
>This is a semi-automatic email about new static checker warnings.
>
>The patch 6c52fdc244b5: "rdma/siw: connection management" from Jun
>20, 2019, leads to the following Smatch complaint:
>
> drivers/infiniband/sw/siw/siw_cm.c:1518 siw_connect()
> error: we previously assumed 'qp' could be null (see line 1372)
>
>drivers/infiniband/sw/siw/siw_cm.c
> 1371 qp = siw_qp_id2obj(sdev, params->qpn);
> 1372 if (!qp) {
> ^^^
>NULL
>
> 1373 WARN(1, "[QP %u] does not exist\n", params->qpn);
> 1374 rv = -EINVAL;
> 1375 goto error;
> ^^^^^^^^^^
>
> 1376 }
> 1377 if (v4)
> 1378 siw_dbg_qp(qp,
>
>[ snip ]
>
> 1508 if (rv >= 0) {
> 1509 rv = siw_cm_queue_work(cep, SIW_CM_WORK_MPATIMEOUT);
> 1510 if (!rv) {
> 1511 siw_dbg_cep(cep, "id 0x%p, [QP %u]: exit\n", id,
> 1512 qp_id(qp));
> 1513 siw_cep_set_free(cep);
> 1514 return 0;
> 1515 }
> 1516 }
> 1517 error:
> 1518 siw_dbg_qp(qp, "failed: %d\n", rv);
> ^^
>NULL dereference.
>
> 1519
> 1520 if (cep) {
>
Thanks Dan!
A patch is on its way...
Bernard.
>regards,
>dan carpenter
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [bug report] rdma/siw: connection management
@ 2023-10-25 11:56 Dan Carpenter
2023-10-25 14:31 ` Bernard Metzler
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2023-10-25 11:56 UTC (permalink / raw)
To: bmt; +Cc: linux-rdma
Hello Bernard Metzler,
The patch 6c52fdc244b5: "rdma/siw: connection management" from Jun
20, 2019 (linux-next), leads to the following Smatch static checker
warning:
drivers/infiniband/sw/siw/siw_cm.c:1560 siw_accept()
error: double free of 'cep->mpa.pdata'
drivers/infiniband/sw/siw/siw_cm.c
1545 int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param *params)
1546 {
1547 struct siw_device *sdev = to_siw_dev(id->device);
1548 struct siw_cep *cep = (struct siw_cep *)id->provider_data;
1549 struct siw_qp *qp;
1550 struct siw_qp_attrs qp_attrs;
1551 int rv, max_priv_data = MPA_MAX_PRIVDATA;
1552 bool wait_for_peer_rts = false;
1553
1554 siw_cep_set_inuse(cep);
1555 siw_cep_put(cep);
^^^^^^^^^^^^^^^^^
This potentially calls __siw_cep_dealloc() which frees cep->mpa.pdata.
1556
1557 /* Free lingering inbound private data */
1558 if (cep->mpa.hdr.params.pd_len) {
1559 cep->mpa.hdr.params.pd_len = 0;
--> 1560 kfree(cep->mpa.pdata);
^^^^^^^^^^^^^^
Double free?
1561 cep->mpa.pdata = NULL;
1562 }
1563 siw_cancel_mpatimer(cep);
See also:
drivers/infiniband/hw/erdma/erdma_cm.c:1141 erdma_accept() error: double free of 'cep->mpa.pdata'
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [bug report] rdma/siw: connection management
2023-10-25 11:56 Dan Carpenter
@ 2023-10-25 14:31 ` Bernard Metzler
2023-10-25 15:02 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Bernard Metzler @ 2023-10-25 14:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-rdma@vger.kernel.org
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Sent: Wednesday, October 25, 2023 1:57 PM
> To: Bernard Metzler <BMT@zurich.ibm.com>
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [bug report] rdma/siw: connection management
>
> Hello Bernard Metzler,
>
> The patch 6c52fdc244b5: "rdma/siw: connection management" from Jun
> 20, 2019 (linux-next), leads to the following Smatch static checker
> warning:
>
> drivers/infiniband/sw/siw/siw_cm.c:1560 siw_accept()
> error: double free of 'cep->mpa.pdata'
>
> drivers/infiniband/sw/siw/siw_cm.c
> 1545 int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param
> *params)
> 1546 {
> 1547 struct siw_device *sdev = to_siw_dev(id->device);
> 1548 struct siw_cep *cep = (struct siw_cep *)id->provider_data;
> 1549 struct siw_qp *qp;
> 1550 struct siw_qp_attrs qp_attrs;
> 1551 int rv, max_priv_data = MPA_MAX_PRIVDATA;
> 1552 bool wait_for_peer_rts = false;
> 1553
> 1554 siw_cep_set_inuse(cep);
> 1555 siw_cep_put(cep);
> ^^^^^^^^^^^^^^^^^
>
> This potentially calls __siw_cep_dealloc() which frees cep->mpa.pdata.
>
> 1556
> 1557 /* Free lingering inbound private data */
> 1558 if (cep->mpa.hdr.params.pd_len) {
> 1559 cep->mpa.hdr.params.pd_len = 0;
> --> 1560 kfree(cep->mpa.pdata);
> ^^^^^^^^^^^^^^
> Double free?
>
> 1561 cep->mpa.pdata = NULL;
> 1562 }
> 1563 siw_cancel_mpatimer(cep);
>
> See also:
> drivers/infiniband/hw/erdma/erdma_cm.c:1141 erdma_accept() error: double
> free of 'cep->mpa.pdata'
>
> regards,
> dan carpenter
Thanks Dan.
siw_cep_put() only calls kfree() on cep->mpa.pdata
if cep was on its last reference. It then frees cep as well and
no further reference to cep is allowed. This cannot be the case here.
To satisfy Smatch, without changing functionality we can
reorder and first explicitly free any mpa.pdata and put it NULL
before calling siw_cep_put(). I don't like it though, because
it just satisfies Smatch but sacrifies readability.
Bernard.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] rdma/siw: connection management
2023-10-25 14:31 ` Bernard Metzler
@ 2023-10-25 15:02 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2023-10-25 15:02 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma@vger.kernel.org
On Wed, Oct 25, 2023 at 02:31:08PM +0000, Bernard Metzler wrote:
>
>
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@linaro.org>
> > Sent: Wednesday, October 25, 2023 1:57 PM
> > To: Bernard Metzler <BMT@zurich.ibm.com>
> > Cc: linux-rdma@vger.kernel.org
> > Subject: [EXTERNAL] [bug report] rdma/siw: connection management
> >
> > Hello Bernard Metzler,
> >
> > The patch 6c52fdc244b5: "rdma/siw: connection management" from Jun
> > 20, 2019 (linux-next), leads to the following Smatch static checker
> > warning:
> >
> > drivers/infiniband/sw/siw/siw_cm.c:1560 siw_accept()
> > error: double free of 'cep->mpa.pdata'
> >
> > drivers/infiniband/sw/siw/siw_cm.c
> > 1545 int siw_accept(struct iw_cm_id *id, struct iw_cm_conn_param
> > *params)
> > 1546 {
> > 1547 struct siw_device *sdev = to_siw_dev(id->device);
> > 1548 struct siw_cep *cep = (struct siw_cep *)id->provider_data;
> > 1549 struct siw_qp *qp;
> > 1550 struct siw_qp_attrs qp_attrs;
> > 1551 int rv, max_priv_data = MPA_MAX_PRIVDATA;
> > 1552 bool wait_for_peer_rts = false;
> > 1553
> > 1554 siw_cep_set_inuse(cep);
> > 1555 siw_cep_put(cep);
> > ^^^^^^^^^^^^^^^^^
> >
> > This potentially calls __siw_cep_dealloc() which frees cep->mpa.pdata.
> >
> > 1556
> > 1557 /* Free lingering inbound private data */
> > 1558 if (cep->mpa.hdr.params.pd_len) {
> > 1559 cep->mpa.hdr.params.pd_len = 0;
> > --> 1560 kfree(cep->mpa.pdata);
> > ^^^^^^^^^^^^^^
> > Double free?
> >
> > 1561 cep->mpa.pdata = NULL;
> > 1562 }
> > 1563 siw_cancel_mpatimer(cep);
> >
> > See also:
> > drivers/infiniband/hw/erdma/erdma_cm.c:1141 erdma_accept() error: double
> > free of 'cep->mpa.pdata'
> >
> > regards,
> > dan carpenter
>
> Thanks Dan.
> siw_cep_put() only calls kfree() on cep->mpa.pdata
> if cep was on its last reference. It then frees cep as well and
> no further reference to cep is allowed. This cannot be the case here.
>
Thanks!
> To satisfy Smatch, without changing functionality we can
> reorder and first explicitly free any mpa.pdata and put it NULL
> before calling siw_cep_put(). I don't like it though, because
> it just satisfies Smatch but sacrifies readability.
Yeah, don't do that. This Smatch warning is not finished or published
yet. The rule is never write code just to make the checker happy.
I started to write a kref_put() dummy function for when you know that
you are not dropping the last reference. I need to dust that off an get
it merged. That might not be a bad option.
But either way, when this check is published people can just search lore
for the warning and find this conversation if they have any questions.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-25 15:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-19 12:09 [bug report] rdma/siw: connection management Dan Carpenter
2019-08-19 14:02 ` Bernard Metzler
-- strict thread matches above, loose matches on Subject: below --
2023-10-25 11:56 Dan Carpenter
2023-10-25 14:31 ` Bernard Metzler
2023-10-25 15:02 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox