From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:40050 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751131AbeCNWnu (ORCPT ); Wed, 14 Mar 2018 18:43:50 -0400 Message-ID: <1521067427.2750.40.camel@redhat.com> Subject: Re: [PATCH net] net/sched: act_simple: don't leak 'index' in the error path From: Davide Caratti To: Cong Wang Cc: Jamal Hadi Salim , Jiri Pirko , "David S. Miller" , Linux Kernel Network Developers In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Date: Wed, 14 Mar 2018 23:43:47 +0100 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: hello Cong, thank you for reviewing this. On Wed, 2018-03-14 at 11:41 -0700, Cong Wang wrote: > On Tue, Mar 13, 2018 at 7:13 PM, Davide Caratti wrote: > > Looks like we just need to replace the tcf_idr_cleanup() with > tcf_idr_release()? Which is also simpler. I just tried it on act_simple, and I can confirm: 'index' does not leak anymore if alloc_defdata() fails to kzalloc(), and then tcf_idr_release() is called in place of of tcf_idr_cleanup(). > Looks like all other callers of tcf_idr_cleanup() need to be replaced too, > but I don't audit all of them... no problem, I can try to do that, it's not going to be a big series anyway. while at it, I will also fix other spots where the same bug can be reproduced, even if tcf_idr_cleanup() is not there: for example, when tcf_vlan_init() fails allocating struct tcf_vlan_params *p, ASSERT_RTNL(); p = kzalloc(sizeof(*p), GFP_KERNEL); if (!p) { if (ovr) tcf_idr_release(*a, bind); return -ENOMEM; } the followinng behavior can be observed: # tc actions flush action vlan # tc actions add action vlan pop index 5 RTNETLINK answers: Cannot allocate memory We have an error talking to the kernel # tc actions add action vlan pop index 5 RTNETLINK answers: No space left on device We have an error talking to the kernel # tc actions add action vlan pop index 5 RTNETLINK answers: No space left on device We have an error talking to the kernel Probably testing the value of 'ovr' here is wrong, or maybe it's not enough: I will also verify what happens using 'replace' keyword instead of 'add'. > > [...] > > > if (!exists) { > > + defdata = nla_strdup(tb[TCA_DEF_DATA], GFP_KERNEL); > > + if (unlikely(!defdata)) > > + return -ENOMEM; > > + > > ret = tcf_idr_create(tn, parm->index, est, a, > > &act_simp_ops, bind, false); > > if (ret) > > return ret; > > You leak memory here on failure, BTW. Ouch, you are right. I was wrongly convinced that act_simp_ops.cleanup() was called also in case of failure of tcf_idr_create(), but it's not true. Indeed, a call to act_simp_ops.cleanup() happens if we call tcf_idr_release() after tcf_idr_create() returned 0. regards, -- davide