On Wed, 2018-11-28 at 12:28 +0300, Dan Carpenter wrote: > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > master > head: ef78e5ec9214376c5cb989f5da70b02d0c117b66 > commit: f2cbd485282014132851bf37cb2ca624a456275d net/sched: act_police: fix > race condition on state variables
hello, > smatch warnings: > net/sched/act_police.c:232 tcf_police_init() warn: possible memory leak of > 'new' > > # > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f2cbd485282014132851bf37cb2ca624a456275d > git remote add linus > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > git remote update linus > git checkout f2cbd485282014132851bf37cb2ca624a456275d > vim +/new +232 net/sched/act_police.c <...> > a883bf564 net/sched/act_police.c Jarek Poplawski 2009-03-04 155 } else > if (tb[TCA_POLICE_AVRATE] && > a883bf564 net/sched/act_police.c Jarek Poplawski 2009-03-04 156 > (ret == ACT_P_CREATED || > 1c0d32fde net/sched/act_police.c Eric Dumazet 2016-12-04 157 > !gen_estimator_active(&police->tcf_rate_est))) { > a883bf564 net/sched/act_police.c Jarek Poplawski 2009-03-04 158 > err = -EINVAL; > 74030603d net/sched/act_police.c WANG Cong 2017-06-13 159 > goto failure; > ^1da177e4 net/sched/police.c Linus Torvalds 2005-04-16 160 } > 71bcb09a5 net/sched/act_police.c Stephen Hemminger 2008-11-25 161 > 2d550dbad net/sched/act_police.c Davide Caratti 2018-09-13 162 new = > kzalloc(sizeof(*new), GFP_KERNEL); > 2d550dbad net/sched/act_police.c Davide Caratti 2018-09-13 163 if > (unlikely(!new)) { > 2d550dbad net/sched/act_police.c Davide Caratti 2018-09-13 164 > err = -ENOMEM; > 2d550dbad net/sched/act_police.c Davide Caratti 2018-09-13 165 > goto failure; > 2d550dbad net/sched/act_police.c Davide Caratti 2018-09-13 166 } > 2d550dbad net/sched/act_police.c Davide Caratti 2018-09-13 167 > ^1da177e4 net/sched/police.c Linus Torvalds 2005-04-16 168 /* No > failure allowed after this point */ in commit c08f5ed5d ("net/sched: act_police: disallow 'goto chain' on fallback control action"), I didn't notice the above comment, <...> > ^1da177e4 net/sched/police.c Linus Torvalds 2005-04-16 197 > c08f5ed5d net/sched/act_police.c Davide Caratti 2018-10-20 198 if > (tb[TCA_POLICE_RESULT]) { > c08f5ed5d net/sched/act_police.c Davide Caratti 2018-10-20 199 > new->tcfp_result = nla_get_u32(tb[TCA_POLICE_RESULT]); > c08f5ed5d net/sched/act_police.c Davide Caratti 2018-10-20 200 > if (TC_ACT_EXT_CMP(new->tcfp_result, TC_ACT_GOTO_CHAIN)) { > c08f5ed5d net/sched/act_police.c Davide Caratti 2018-10-20 201 > NL_SET_ERR_MSG(extack, > c08f5ed5d net/sched/act_police.c Davide Caratti 2018-10-20 202 > "goto chain not allowed on fallback"); > c08f5ed5d net/sched/act_police.c Davide Caratti 2018-10-20 203 > err = -EINVAL; > c08f5ed5d net/sched/act_police.c Davide Caratti 2018-10-20 204 > goto failure; > > ^^^^^^^^^^^^ > kfree_rcu(new, rcu); ? ... and I introduced a 'goto failure', to avoid the NULL pointer dereference in the traffic path. I think it's correct to call kfree_rcu(new, rcu), or (maybe better) reject invalid values of TCA_POLICE_RESULT before allocating 'new', so we avoid kzalloc() + kfree_rcu() if we already know that the control action is not valid. I will send a patch in the next hours. thanks! -- davide
