RDMA/iw_cxgb4: Avoid touch after free error in ARP failure handlers

Hello Raju Rangoju,

The patch 1dad0ebeea1c: "RDMA/iw_cxgb4: Avoid touch after free error in ARP failure handlers" from May 15, 2017, leads to the following static checker warning:

	drivers/infiniband/hw/cxgb4/cm.c:4310 process_work()
	warn: 'skb' was already freed.

drivers/infiniband/hw/cxgb4/cm.c
4289 static void process_work(struct work_struct *work) 4290 {

  4291          struct sk_buff *skb = NULL;
  4292          struct c4iw_dev *dev;
  4293          struct cpl_act_establish *rpl;
  4294          unsigned int opcode;
  4295          int ret;
  4296  
  4297          process_timedout_eps();
  4298          while ((skb = skb_dequeue(&rxq))) {
  4299                  rpl = cplhdr(skb);
  4300                  dev = *((struct c4iw_dev **) (skb->cb + sizeof(void *)));
  4301                  opcode = rpl->ot.opcode;

4302
4303 if (opcode >= ARRAY_SIZE(work_handlers) ||
4304 !work_handlers[opcode]) {
4305 pr_err("No handler for opcode 0x%x.\n", opcode);
4306 kfree_skb(skb);
4307 } else {
4308 ret = work_handlers[opcode](dev, skb);
4309 if (!ret)
4310 kfree_skb(skb);

I'm not sure why this warning didn't show up before... :(

We added some kfree_skb() calls to _put_ep_safe() and _put_pass_ep_safe(). The thing about kfree_skb() is that it's refcounted so it might not free anything so this could be a false positive. I've looked at the code and it looks like it could be a bug?

  4311                  }
  4312                  process_timedout_eps();
  4313          }
  4314  }

regards,
dan carpenter

Hi Dan,
Thanks for the check.
I have observed this and looks like the kfree_skb() in _put_ep_safe() and _put_pass_ep_safe() are not needed as the skb is anyway freed by process_work() I have checked the refcounts before kfree_skb() for _put_ep_safe() and _put_pass_ep_safe() and they are '1' by simulating the error, so I believe kfree_skb() in process_work() should just be enough.
I have tested the patch to remove the kfree_skb() from _put_ep_safe() and
_put_pass_ep_safe(). It runs fine.

Thanks,
Bharat.