Re: [bug report] RDMA/iw_cxgb4: Avoid touch after free error in ARP failure handlers

[Potnuri Bharat Teja <bharat@chelsio.com>]

On Friday, October 10/25/19, 2019 at 16:11:11 +0530, Dan Carpenter wrote:
> 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?
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.

> 
>   4311                  }
>   4312                  process_timedout_eps();
>   4313          }
>   4314  }
> 
> regards,
> dan carpenter