* Re: [bug report] net/sched: use the hardware intermediate representation for matchall
2019-05-08 7:17 [bug report] net/sched: use the hardware intermediate representation for matchall Dan Carpenter
@ 2019-05-08 7:41 ` Pieter Jansen van Vuuren
2019-05-08 8:19 ` Dan Carpenter
2019-05-08 8:53 ` Pieter Jansen van Vuuren
2 siblings, 0 replies; 4+ messages in thread
From: Pieter Jansen van Vuuren @ 2019-05-08 7:41 UTC (permalink / raw)
To: kernel-janitors
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;"
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] net/sched: use the hardware intermediate representation for matchall
2019-05-08 7:17 [bug report] net/sched: use the hardware intermediate representation for matchall Dan Carpenter
2019-05-08 7:41 ` Pieter Jansen van Vuuren
@ 2019-05-08 8:19 ` Dan Carpenter
2019-05-08 8:53 ` Pieter Jansen van Vuuren
2 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2019-05-08 8:19 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] net/sched: use the hardware intermediate representation for matchall
2019-05-08 7:17 [bug report] net/sched: use the hardware intermediate representation for matchall Dan Carpenter
2019-05-08 7:41 ` Pieter Jansen van Vuuren
2019-05-08 8:19 ` Dan Carpenter
@ 2019-05-08 8:53 ` Pieter Jansen van Vuuren
2 siblings, 0 replies; 4+ messages in thread
From: Pieter Jansen van Vuuren @ 2019-05-08 8:53 UTC (permalink / raw)
To: kernel-janitors
On 08/05/2019 09:19, Dan Carpenter wrote:
> 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
>
Will do. And thank you again.
^ permalink raw reply [flat|nested] 4+ messages in thread