From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-f196.google.com ([209.85.223.196]:41666 "EHLO mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752495AbeCOO21 (ORCPT ); Thu, 15 Mar 2018 10:28:27 -0400 Received: by mail-io0-f196.google.com with SMTP id m83so8778686ioi.8 for ; Thu, 15 Mar 2018 07:28:27 -0700 (PDT) From: Roman Mashak To: Davide Caratti Cc: Cong Wang , Manish Kurup , Jiri Pirko , "David S. Miller" , netdev@vger.kernel.org Subject: Re: [PATCH net] net/sched: fix NULL dereference in the error path of tcf_vlan_init() References: <1d301fb86becf863283ee1be107f17e837ef4255.1521122595.git.dcaratti@redhat.com> Date: Thu, 15 Mar 2018 10:28:24 -0400 In-Reply-To: <1d301fb86becf863283ee1be107f17e837ef4255.1521122595.git.dcaratti@redhat.com> (Davide Caratti's message of "Thu, 15 Mar 2018 15:06:30 +0100") Message-ID: <851sglifav.fsf@mojatatu.com> MIME-Version: 1.0 Content-Type: text/plain Sender: netdev-owner@vger.kernel.org List-ID: Davide Caratti writes: > when the following command > > # tc actions replace action vlan pop index 100 > > is run for the first time, and tcf_vlan_init() fails allocating struct > tcf_vlan_params, tcf_vlan_cleanup() calls kfree_rcu(NULL, ...). This causes > the following error: > [...] > fix this in tcf_vlan_cleanup(), ensuring that kfree_rcu(p, ...) is called > only when p is not NULL. > > Fixes: 4c5b9d9642c8 ("act_vlan: VLAN action rewrite to use RCU lock/unlock and update") > Signed-off-by: Davide Caratti > --- > net/sched/act_vlan.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c > index e1a1b3f3983a..c2914e9a4a6f 100644 > --- a/net/sched/act_vlan.c > +++ b/net/sched/act_vlan.c > @@ -225,7 +225,8 @@ static void tcf_vlan_cleanup(struct tc_action *a) > struct tcf_vlan_params *p; > > p = rcu_dereference_protected(v->vlan_p, 1); > - kfree_rcu(p, rcu); > + if (p) > + kfree_rcu(p, rcu); > } > > static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a, Good catch. I think you can propagate the fix on the other actions ->cleanup(), where private parameters structure may not be present at cleanup time, e.g. csum, ife.