* [bug report] net/sched: use the hardware intermediate representation for matchall
@ 2019-05-08 7:17 Dan Carpenter
2019-05-08 7:41 ` Pieter Jansen van Vuuren
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Dan Carpenter @ 2019-05-08 7:17 UTC (permalink / raw)
To: kernel-janitors
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?
314 }
315
316 err = cb(TC_SETUP_CLSMATCHALL, &cls_mall, cb_priv);
^^
I did check whether the cb() function pointers re-allocate ->rule and
it doesn't look like it.
317 kfree(cls_mall.rule);
^^^^^^^^^^^^^^^^^^^^
318
319 if (err) {
320 if (add && tc_skip_sw(head->flags))
321 return err;
322 return 0;
323 }
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 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
end of thread, other threads:[~2019-05-08 8:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.