[bug report] RDMA/iw_cxgb4: Low resource fixes for Completion queue

[bug report] RDMA/iw_cxgb4: Low resource fixes for Completion queue

[Dan Carpenter]

Hello,

The patch dd6b0241260d: "RDMA/iw_cxgb4: Low resource fixes for
Completion queue" from Jun 10, 2016 (linux-next), leads to the
following Smatch static checker warning:

	drivers/infiniband/hw/cxgb4/cq.c:1153 c4iw_create_cq()
	error: double free of 'chp->destroy_skb'

drivers/infiniband/hw/cxgb4/cq.c
    1138         pr_debug("cqid 0x%0x chp %p size %u memsize %zu, dma_addr %pad\n",
    1139                  chp->cq.cqid, chp, chp->cq.size, chp->cq.memsize,
    1140                  &chp->cq.dma_addr);
    1141         return 0;
    1142 err_free_mm2:
    1143         kfree(mm2);
    1144 err_free_mm:
    1145         kfree(mm);
    1146 err_remove_handle:
    1147         xa_erase_irq(&rhp->cqs, chp->cq.cqid);
    1148 err_destroy_cq:
    1149         destroy_cq(&chp->rhp->rdev, &chp->cq,
    1150                    ucontext ? &ucontext->uctx : &rhp->rdev.uctx,
    1151                    chp->destroy_skb, chp->wr_waitp);

destroy_cq() calls kfree_skb(chp->destroy_skb).  The call tree is:

destroy_cq()
-> c4iw_ref_send_wait()
   -> c4iw_ofld_send()
      -> kfree_skb()

    1152 err_free_skb:
--> 1153         kfree_skb(chp->destroy_skb);

Probably we can just delete this kfree_skb()?

    1154 err_free_wr_wait:
    1155         c4iw_put_wr_wait(chp->wr_waitp);
    1156 err_free_chp:
    1157         return ret;
    1158 }

regards,
dan carpenter

Re: [bug report] RDMA/iw_cxgb4: Low resource fixes for Completion queue

[Leon Romanovsky]

On Fri, Oct 20, 2023 at 06:45:19PM +0300, Dan Carpenter wrote:
> Hello,
> 
> The patch dd6b0241260d: "RDMA/iw_cxgb4: Low resource fixes for
> Completion queue" from Jun 10, 2016 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	drivers/infiniband/hw/cxgb4/cq.c:1153 c4iw_create_cq()
> 	error: double free of 'chp->destroy_skb'
> 
> drivers/infiniband/hw/cxgb4/cq.c
>     1138         pr_debug("cqid 0x%0x chp %p size %u memsize %zu, dma_addr %pad\n",
>     1139                  chp->cq.cqid, chp, chp->cq.size, chp->cq.memsize,
>     1140                  &chp->cq.dma_addr);
>     1141         return 0;
>     1142 err_free_mm2:
>     1143         kfree(mm2);
>     1144 err_free_mm:
>     1145         kfree(mm);
>     1146 err_remove_handle:
>     1147         xa_erase_irq(&rhp->cqs, chp->cq.cqid);
>     1148 err_destroy_cq:
>     1149         destroy_cq(&chp->rhp->rdev, &chp->cq,
>     1150                    ucontext ? &ucontext->uctx : &rhp->rdev.uctx,
>     1151                    chp->destroy_skb, chp->wr_waitp);
> 
> destroy_cq() calls kfree_skb(chp->destroy_skb).  The call tree is:
> 
> destroy_cq()
> -> c4iw_ref_send_wait()
>    -> c4iw_ofld_send()
>       -> kfree_skb()

It is not always the case, kfree_skb() is called in c4iw_ofld_send() error flows only.

> 
>     1152 err_free_skb:
> --> 1153         kfree_skb(chp->destroy_skb);
> 
> Probably we can just delete this kfree_skb()?

I don't think so, in case create_cq() failed, there is a need to clean
preallocated SKB.

Thanks

> 
>     1154 err_free_wr_wait:
>     1155         c4iw_put_wr_wait(chp->wr_waitp);
>     1156 err_free_chp:
>     1157         return ret;
>     1158 }
> 
> regards,
> dan carpenter