Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <35b45d85f2c5072d921b555df43bcc4e5a9feb64.camel@redhat.com>
Subject: Re: net/sched/act_police.c:232 tcf_police_init() warn: possible
 memory leak of 'new'
From:   Davide Caratti <dcaratti@redhat.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>, kbuild@01.org
Cc:     kbuild-all@01.org, linux-kernel@vger.kernel.org,
        Eric Dumazet <edumazet@google.com>
In-Reply-To: <20181128092822.GK3073@unbuntlaptop>
References: <20181128092822.GK3073@unbuntlaptop>
Organization: red hat
Content-Type: text/plain; charset="UTF-8"
Date:   Wed, 28 Nov 2018 11:07:10 +0100
Mime-Version: 1.0
User-Agent: Evolution 3.30.2 (3.30.2-2.fc29) 
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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