[{"id":1763864,"web_url":"http://patchwork.ozlabs.org/comment/1763864/","msgid":"<20170906073839.GD2523@nanopsycho>","list_archive_url":null,"date":"2017-09-06T07:38:39","subject":"Re: [Patch net] net_sched: fix a memory leak of filter chain","submitter":{"id":15321,"url":"http://patchwork.ozlabs.org/api/people/15321/","name":"Jiri Pirko","email":"jiri@resnulli.us"},"content":"Wed, Sep 06, 2017 at 07:03:10AM CEST, xiyou.wangcong@gmail.com wrote:\n>tcf_chain_destroy() is called by tcf_block_put() and tcf_chain_put().\n>tcf_chain_put() is refcn'ed and paired with tcf_chain_get(),\n>but tcf_block_put() is not, it should be paired with tcf_block_get()\n>and we still need to decrease the refcnt. However, tcf_block_put()\n>is special, it stores the chains too, we have to detach them if\n>it is not the last user.\n\nYou don't describe the original issue, or I am missing that from your\ndescription.\n\n\n>\n>What's more, index 0 is not special at all, it should be treated\n>like other chains. This also makes the code more readable.\n\n[...]\n\n\n>@@ -246,10 +246,7 @@ EXPORT_SYMBOL(tcf_chain_get);\n> \n> void tcf_chain_put(struct tcf_chain *chain)\n> {\n>-\t/* Destroy unused chain, with exception of chain 0, which is the\n>-\t * default one and has to be always present.\n>-\t */\n>-\tif (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)\n>+\tif (--chain->refcnt == 0)\n\nThe refcounting is only done for actions holding reference to the chain.\nYou still need to check is the filter chain is not empty.\nSee tc_ctl_tfilter.\n\nAlso, chain 0 is created by default on a block creation. It has to be\npresent always for a reason. Please see tcf_block_get. The pointer to\nchain 0 is assigned to the qdisc filter list pointer.\n\n\n\n> \t\ttcf_chain_destroy(chain);\n> }\n> EXPORT_SYMBOL(tcf_chain_put);\n>@@ -296,8 +293,11 @@ void tcf_block_put(struct tcf_block *block)\n> \n> \tlist_for_each_entry_safe(chain, tmp, &block->chain_list, list) {\n> \t\ttcf_chain_flush(chain);\n>-\t\ttcf_chain_destroy(chain);\n>+\t\ttcf_chain_put(chain);\n> \t}\n>+\t/* If tc actions still hold the chain, just detach it. */\n>+\tlist_for_each_entry_safe(chain, tmp, &block->chain_list, list)\n>+\t\ttcf_chain_detach(chain);\n> \tkfree(block);\n> }\n> EXPORT_SYMBOL(tcf_block_put);\n>-- \n>2.13.0\n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=resnulli-us.20150623.gappssmtp.com\n\theader.i=@resnulli-us.20150623.gappssmtp.com\n\theader.b=\"StE/WiuW\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnFnk5fWrz9sCZ\n\tfor ;\n\tWed, 6 Sep 2017 17:38:50 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751872AbdIFHio (ORCPT );\n\tWed, 6 Sep 2017 03:38:44 -0400","from mail-wm0-f66.google.com ([74.125.82.66]:34752 \"EHLO\n\tmail-wm0-f66.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751737AbdIFHim (ORCPT\n\t); Wed, 6 Sep 2017 03:38:42 -0400","by mail-wm0-f66.google.com with SMTP id l19so1691793wmi.1\n\tfor ; Wed, 06 Sep 2017 00:38:41 -0700 (PDT)","from localhost (ip-89-177-125-82.net.upcbroadband.cz.\n\t[89.177.125.82]) by smtp.gmail.com with ESMTPSA id\n\tk18sm481003wmd.22.2017.09.06.00.38.40\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tWed, 06 Sep 2017 00:38:40 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=resnulli-us.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=53rPAbs1OcYiQzvjbNwtxR0jQh/R6OCj7SeMIkZciYA=;\n\tb=StE/WiuWflgPRgobbvHQzvEc0zCcZpZTV4F7F3pnJKvtN5eJNsz2zg7k9QxYTLIgu7\n\tUklvxX/nsNIpq9PNgli5tRslV9DZSziKp+4ONGezuEgKFFA9XGiy+S34GDUk9tbJ6tyg\n\tAClmO08Mh4gql9iN8LUYAshHobuNFbEzB/yyJ3cVQ1kPE29IhJQfG+c63YN7TVPvAYcw\n\tKybtIvFWy4FvR2S4zPvKgu94e6DqMUT73uKx21ci2uaSX5j3CQmAOc5B3SEN9famwpb5\n\ta8U5WxhuJz0y+5EqssqlYVtw9eviacCiVljZcuaGY4SqnFkkW0juILXavTr00TiaNP3o\n\tTxYA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=53rPAbs1OcYiQzvjbNwtxR0jQh/R6OCj7SeMIkZciYA=;\n\tb=ZB6IOkGkmx/Y1kLrpnKjmcGvmetDJUhdLr+gUBQZEb1lUC7KWsJ/goSlxQ1QllzvLA\n\tj6aqYUqZUsSSdHUafjG80U2VQeoTNPlR2sNMcsTh5GSbweCcaJCV0DanQLLZN7kJNqsT\n\tI5QOdz0X2SJmJSPclQRHZXg8svXpg0r93Eq6hds2WhQhwvcUtsVgTZdh61eeyHBgVIXQ\n\twkMJVTGc3E8KTG2bdmOicjq4O9aVSf6UE+QbEK2a1tiByrJ4BYKM65KxQ0qpY7JGElk2\n\tnwRyMnTs7WxwyHSDStY5j7tj3ew9hYDbbf5DVl3KacbquZgDqUgbLxYul+0jqupMDaRS\n\twE3g==","X-Gm-Message-State":"AHPjjUhvsgFS4XRDrF7LAFOKVv6VVHHq52zkNKAf3d3lqi8azk9LZOuR\n\tgaR+6NqevsJaaQkg","X-Google-Smtp-Source":"ADKCNb5N1Fu3saTWh5Wvj/cVaNLiiEAz+zOXC2UiAQP65hFq314dw085wSiGPOUW7snUWFVVQi802g==","X-Received":"by 10.28.229.129 with SMTP id c123mr949265wmh.153.1504683521274; \n\tWed, 06 Sep 2017 00:38:41 -0700 (PDT)","Date":"Wed, 6 Sep 2017 09:38:39 +0200","From":"Jiri Pirko ","To":"Cong Wang ","Cc":"netdev@vger.kernel.org, jakub.kicinski@netronome.com,\n\tJiri Pirko ","Subject":"Re: [Patch net] net_sched: fix a memory leak of filter chain","Message-ID":"<20170906073839.GD2523@nanopsycho>","References":"<20170906050310.26474-1-xiyou.wangcong@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170906050310.26474-1-xiyou.wangcong@gmail.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1764272,"web_url":"http://patchwork.ozlabs.org/comment/1764272/","msgid":"","list_archive_url":null,"date":"2017-09-06T17:25:51","subject":"Re: [Patch net] net_sched: fix a memory leak of filter chain","submitter":{"id":211,"url":"http://patchwork.ozlabs.org/api/people/211/","name":"Cong Wang","email":"xiyou.wangcong@gmail.com"},"content":"On Wed, Sep 6, 2017 at 12:38 AM, Jiri Pirko wrote:\n> Wed, Sep 06, 2017 at 07:03:10AM CEST, xiyou.wangcong@gmail.com wrote:\n>>tcf_chain_destroy() is called by tcf_block_put() and tcf_chain_put().\n>>tcf_chain_put() is refcn'ed and paired with tcf_chain_get(),\n>>but tcf_block_put() is not, it should be paired with tcf_block_get()\n>>and we still need to decrease the refcnt. However, tcf_block_put()\n>>is special, it stores the chains too, we have to detach them if\n>>it is not the last user.\n>\n> You don't describe the original issue, or I am missing that from your\n> description.\n\nThe original issue is the mismatch of tcf_block_put() and tcf_block_get()\nw.r.t. refcnt. Think it in this way: if you call tcf_bock_put() immediately\nafter tcf_block_get(), would you get effectively a nop?\n\n\n>\n>\n>>\n>>What's more, index 0 is not special at all, it should be treated\n>>like other chains. This also makes the code more readable.\n>\n> [...]\n>\n>\n>>@@ -246,10 +246,7 @@ EXPORT_SYMBOL(tcf_chain_get);\n>>\n>> void tcf_chain_put(struct tcf_chain *chain)\n>> {\n>>- /* Destroy unused chain, with exception of chain 0, which is the\n>>- * default one and has to be always present.\n>>- */\n>>- if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)\n>>+ if (--chain->refcnt == 0)\n>\n> The refcounting is only done for actions holding reference to the chain.\n> You still need to check is the filter chain is not empty.\n> See tc_ctl_tfilter.\n\nWith my patch refcnt is done for block too, if you notice the\ntcf_chain_put() in tcf_block_put().\n\n\n>\n> Also, chain 0 is created by default on a block creation. It has to be\n> present always for a reason. Please see tcf_block_get. The pointer to\n> chain 0 is assigned to the qdisc filter list pointer.\n\nSure, this is why block holds a refcnt to chain (not just chain 0) with\nmy patch, aka why the initial refcnt is 1 rather than 0.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"YRNdg+Ey\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnVqY6sVdz9t3Z\n\tfor ;\n\tThu, 7 Sep 2017 03:26:17 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932565AbdIFR0O (ORCPT );\n\tWed, 6 Sep 2017 13:26:14 -0400","from mail-pf0-f195.google.com ([209.85.192.195]:38671 \"EHLO\n\tmail-pf0-f195.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S932130AbdIFR0M (ORCPT\n\t); Wed, 6 Sep 2017 13:26:12 -0400","by mail-pf0-f195.google.com with SMTP id q76so3329860pfq.5\n\tfor ; Wed, 06 Sep 2017 10:26:12 -0700 (PDT)","by 10.100.140.134 with HTTP; Wed, 6 Sep 2017 10:25:51 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=k6CA9p9C49tLYb/OleGte3RWcHUsQk82VCqqw0nLfgI=;\n\tb=YRNdg+EyaSl2JT/eq324qkBYhgVgThQLU4zU2Na3ctg/OZJyYgDlpac+4Z9dLy57dM\n\tqdeHLulo9rdGjDeHSJT6BwrTMah9djzPjD6XScyloEbQM7vOVclZuGyAy02Yl986P5of\n\tpo+xLIxaVOA6RSrmxuvlbHcqdt280MSb8F+M59YhsTPFBWGkbWWvsUF+sD/nE1g8Smhu\n\tQDGKjaxsEFbBt0Q5ezZ7r446W4TGk1GKfA7fDZM7vF4MArQq49l09snuxBP4ChzAI74j\n\tzoBvnUS7Wngxnhk4UE9jz+YLxXxEH0J3kokOjBBVTs/zlt96cfZG1e6X1tdxttS3Oa8Q\n\txs1g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=k6CA9p9C49tLYb/OleGte3RWcHUsQk82VCqqw0nLfgI=;\n\tb=sUh+6TXrQHFNEzrhrZ1Fk4TjZyD29wb7qzCcUEq+ma5nVD5P30bORBhh8rDZJm5CdE\n\tYWHRgjIcD7nSlnyQlFTtfEiDAfLSrHqUJqAyVnxMwhs5Ewuh8+CESzRXnJza5xtP3dL3\n\tOkoDdAqDuE9BxU0ojdoRnsLosbw4vxu5yg5q0Uay3WWnanlZ+G10YgfZ+0245eU/R0yd\n\tE1IZPS+x3h5vGxSI8JKz5zI033VnWwAj1Y3iWazz6+F6OX40HBTFq6CuG29/BIFl1tPm\n\tWBJBAdd9caoDHVb1D5YcCgUUghA+0Y604t0sQXbriIXN8ExqRZwBPvUNtSjX8IVX/Iv/\n\tCflQ==","X-Gm-Message-State":"AHPjjUjf912E/oZY9zGyfTj9pzJiyflSmCGwIGbp1k+s3k0X2g1tJsQm\n\tf1rcJBOxlw/goLr+Hr7cFfh5BM4Y/w==","X-Google-Smtp-Source":"ADKCNb6PLQ01ULUiM+MUPhI1PX2/ZnaMJtDjxRdw72miS3h+7CyUtV37Wt3GMaiUIt54kUxp68/u2FLORvOzwRr3hMY=","X-Received":"by 10.84.129.193 with SMTP id b59mr9274791plb.33.1504718772326; \n\tWed, 06 Sep 2017 10:26:12 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170906073839.GD2523@nanopsycho>","References":"<20170906050310.26474-1-xiyou.wangcong@gmail.com>\n\t<20170906073839.GD2523@nanopsycho>","From":"Cong Wang ","Date":"Wed, 6 Sep 2017 10:25:51 -0700","Message-ID":"","Subject":"Re: [Patch net] net_sched: fix a memory leak of filter chain","To":"Jiri Pirko ","Cc":"Linux Kernel Network Developers ,\n\tJakub Kicinski ,\n\tJiri Pirko ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]