[bug report] rdma/siw: connection management

[bug report] rdma/siw: connection management

[Dan Carpenter]

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

Re: [bug report] rdma/siw: connection management

[Bernard Metzler]

-----"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
>
>

Re: [bug report] rdma/siw: connection management

[Dan Carpenter]

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

RE: [bug report] rdma/siw: connection management

[Bernard Metzler]

> -----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.

[bug report] rdma/siw: connection management

[Dan Carpenter]

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