Re: [bug report] net/sched: use the hardware intermediate representation for matchall

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, May 08, 2019 at 08:41:13AM +0100, Pieter Jansen van Vuuren wrote:
> On 08/05/2019 08:17, Dan Carpenter wrote:
> > Hello Pieter Jansen van Vuuren,
> > 
> > The patch f00cbf196814: "net/sched: use the hardware intermediate
> > representation for matchall" from May 4, 2019, leads to the following
> > static checker warning:
> > 
> > 	net/sched/cls_matchall.c:317 mall_reoffload()
> > 	error: double free of 'cls_mall.rule'
> > 
> > net/sched/cls_matchall.c
> >    286  static int mall_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb,
> >    287                            void *cb_priv, struct netlink_ext_ack *extack)
> >    288  {
> >    289          struct cls_mall_head *head = rtnl_dereference(tp->root);
> >    290          struct tc_cls_matchall_offload cls_mall = {};
> >    291          struct tcf_block *block = tp->chain->block;
> >    292          int err;
> >    293  
> >    294          if (tc_skip_hw(head->flags))
> >    295                  return 0;
> >    296  
> >    297          cls_mall.rule = flow_rule_alloc(tcf_exts_num_actions(&head->exts));
> >    298          if (!cls_mall.rule)
> >    299                  return -ENOMEM;
> >    300  
> >    301          tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, block,
> >    302                                     extack);
> >    303          cls_mall.command = add ?
> >    304                  TC_CLSMATCHALL_REPLACE : TC_CLSMATCHALL_DESTROY;
> >    305          cls_mall.cookie = (unsigned long)head;
> >    306  
> >    307          err = tc_setup_flow_action(&cls_mall.rule->action, &head->exts);
> >    308          if (err) {
> >    309                  kfree(cls_mall.rule);
> >                         ^^^^^^^^^^^^^^^^^^^
> >    310                  if (add && tc_skip_sw(head->flags)) {
> >    311                          NL_SET_ERR_MSG_MOD(extack, "Failed to setup flow action");
> >    312                          return err;
> >    313                  }
> > 
> > My guess is that there should be a "return err;" here?
> 
> Thank you. Yes, I think this should be "return 0;" instead of "return err;"

You would know the code better, than I.  :)

Could you send a patch to Dave?  The merge window is open but he still
accepts bug fixes.

regards,
dan carpenter