On Wed, Oct 25, 2023 at 02:31:08PM +0000, Bernard Metzler wrote:
>
>
> > -----Original Message-----
> > From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > Sent: Wednesday, October 25, 2023 1:57 PM
> > To: Bernard Metzler <BMT@xxxxxxxxxxxxxx>
> > Cc: linux-rdma@xxxxxxxxxxxxxxx
> > 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
