From patchwork Fri Jan 20 06:20:55 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cong Wang X-Patchwork-Id: 717483 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3v4Vw31VTLz9sDG for ; Fri, 20 Jan 2017 17:21:23 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="pRqN5YjQ"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751299AbdATGVS (ORCPT ); Fri, 20 Jan 2017 01:21:18 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:33432 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751216AbdATGVR (ORCPT ); Fri, 20 Jan 2017 01:21:17 -0500 Received: by mail-wm0-f67.google.com with SMTP id r144so4255298wme.0 for ; Thu, 19 Jan 2017 22:21:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=JRE1g8DD3hHKlGObI4h5Iuflnf+yUmeFEWXR5j6kxBE=; b=pRqN5YjQtcl8wk6F0eLj6CamumBn025zNAYbeFym92NpWbt0eZKP2CL6WyqCZu2+xd GejPlmc4fMUbWMGrZr2b19StaSVnn4PJk7r+a+DClRgDEtyvp9J24qAJOqQIdCtgvCmM fM9zFquUlogT3yqvVCbR0VRG+ttMH5304/9WdHROMA2yLVT44z1O9SOEqLXjQA9yXMmq 7duGc92CM0LoKuM5s/H4aGkQS3wPlhEvz1nJC57hPqjeIpNnWO4eAzt21pytmJHKbjoD ZeAukkk3O0656FcIfe7vhEgq156itqJ8tdlT8d9U9YiHcL/lLzwQsroZBGnxVWmGxJg6 uoDA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=JRE1g8DD3hHKlGObI4h5Iuflnf+yUmeFEWXR5j6kxBE=; b=ar8kb0RNgiHEDP1oyi+4fYkwNTHKLFzIvciv4u955/5fCGqGxSHZ7YlNLNj8Cs47W0 IwXMr9LrbOmbtdQFHXDGIWCr5/urm1uuYc7K7LUA3cVVkjXi10eW4t6T5EynrM5m9pqo BvdHeoxaMn9ek+L7yJhvbYDX0Vr8tMuLF/D/z2SkpU9EyU/UXMZ8PfxLFPHrQ0jV3jTO /U/Rbh+d/PTTnb/xHDWvK5H0mDTPRA2soZzMXBfo0IthMrvqDL7O6QKJR2BF0DRKGKRT CWrwmGNpjfZmSTdqmnwB9DLzhMM3SN+Pudoakm4s+/9/8O4yg3l9aeqCd4H8IZqQsj/f kQmA== X-Gm-Message-State: AIkVDXLaVw8U4nl5KZ8tz8dE9w+03wYWbPxypBENgrB7rSqiV3iP6naiFqkCeniGQJuqP4FQpdyNuqk99nhADQ== X-Received: by 10.223.128.77 with SMTP id 71mr10074058wrk.48.1484893275876; Thu, 19 Jan 2017 22:21:15 -0800 (PST) MIME-Version: 1.0 Received: by 10.80.164.146 with HTTP; Thu, 19 Jan 2017 22:20:55 -0800 (PST) In-Reply-To: <7a55a129-1e96-80f7-373d-1ff6a95b5269@mojatatu.com> References: <1484493246-10911-1-git-send-email-jhs@emojatatu.com> <7a55a129-1e96-80f7-373d-1ff6a95b5269@mojatatu.com> From: Cong Wang Date: Thu, 19 Jan 2017 22:20:55 -0800 Message-ID: Subject: Re: [PATCH net 1/1] net sched actions: fix refcnt when GETing of action after bind To: Jamal Hadi Salim Cc: David Miller , Linux Kernel Network Developers Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, Jan 18, 2017 at 3:33 AM, Jamal Hadi Salim wrote: > On 17-01-17 01:17 PM, Cong Wang wrote: >> >> Why this check for RTM_GETACTION? It does not make sense >> at least for the error case, that is, when tcf_action_get_1() fails >> in the middle of the loop, all the previous ones should be destroyed >> no matter it's GETACTION or DELACTION... >> >> Also, for the non-error case of GETACTION, they should be >> destroyed too after dumping to user-space, otherwise there is no >> way to use them since 'actions' is local to that function. >> >> I thought in commit aecc5cefc389 you grab that refcnt on purpose. >> > > I did. > The issue there (after your original patch) was destroy() would > decrement the refcount to zero and a GET was essentially translated > to a DEL. Incrementing the refcount earlier protected against that > assuming destroy was going to decrement it. > However, when an action is bound the destroy() doesnt decrement > the refcnt. So the refcnt keeps going up forever (and therefore > deleting fails in the future). So we cant use destroy() as is. Hmm, tcf_action_destroy() should not touch the refcnt at all in this case, right? Since the refcnt here is not for readers in kernel code but for user-space. We mix the use of this refcnt, which leads to problems. Your patch is not correct either for DEL, tcf_action_destroy() is not needed to call again after tcf_del_notify() fails, right? Probably it is not needed at all: diff --git a/net/sched/act_api.c b/net/sched/act_api.c index e10456ef6f..82eed18 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -907,13 +907,8 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n, ret = act_get_notify(net, portid, n, &actions, event); else { /* delete */ ret = tcf_del_notify(net, n, &actions, portid); - if (ret) - goto err; - return ret; } err: - if (event != RTM_GETACTION) - tcf_action_destroy(&actions, 0); return ret; }