[{"id":1760367,"web_url":"http://patchwork.ozlabs.org/comment/1760367/","msgid":"","list_archive_url":null,"date":"2017-08-30T17:36:25","subject":"Re: [PATCH net 6/9] sch_fq_codel: avoid double free on init failure","submitter":{"id":211,"url":"http://patchwork.ozlabs.org/api/people/211/","name":"Cong Wang","email":"xiyou.wangcong@gmail.com"},"content":"On Wed, Aug 30, 2017 at 2:49 AM, Nikolay Aleksandrov\n wrote:\n> It is very unlikely to happen but the backlogs memory allocation\n> could fail and will free q->flows, but then ->destroy() will free\n> q->flows too. For correctness remove the first free and let ->destroy\n> clean up.\n>\n> Fixes: 87b60cfacf9f (\"net_sched: fix error recovery at qdisc creation\")\n> Signed-off-by: Nikolay Aleksandrov \n> ---\n> net/sched/sch_fq_codel.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c\n> index 337f2d6d81e4..2c0c05f2cc34 100644\n> --- a/net/sched/sch_fq_codel.c\n> +++ b/net/sched/sch_fq_codel.c\n> @@ -491,10 +491,8 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt)\n> if (!q->flows)\n> return -ENOMEM;\n> q->backlogs = kvzalloc(q->flows_cnt * sizeof(u32), GFP_KERNEL);\n> - if (!q->backlogs) {\n> - kvfree(q->flows);\n> + if (!q->backlogs)\n> return -ENOMEM;\n> - }\n\nThis is fine. Or we can NULL it after kvfree().\n\nI have no preference here. The only difference here is if we still\nexpect ->init() to cleanup its own failure.","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=\"b6x1uTOL\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjCNx3lTYz9sN7\n\tfor ;\n\tThu, 31 Aug 2017 03:36:49 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752076AbdH3Rgr (ORCPT );\n\tWed, 30 Aug 2017 13:36:47 -0400","from mail-pg0-f68.google.com ([74.125.83.68]:36784 \"EHLO\n\tmail-pg0-f68.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751771AbdH3Rgq (ORCPT\n\t); Wed, 30 Aug 2017 13:36:46 -0400","by mail-pg0-f68.google.com with SMTP id 83so5538706pgb.3\n\tfor ; Wed, 30 Aug 2017 10:36:46 -0700 (PDT)","by 10.100.166.199 with HTTP; Wed, 30 Aug 2017 10:36:25 -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=2DT5HFzQ/yTNw7pFeYRJk9558nm5NRPfekuiwhKNoj4=;\n\tb=b6x1uTOL1CNHnyS7LjXky2V/Q/NIBLTzHxxEIaHSjDk9dbgLNLLpFDK6BjmlmaUk5X\n\ttZCE2CjpLk4JPv7ek0TWwUIsagHCzy+SwhBVP5KBBWaZGhRBfw0EauR+YZ47/13DqATa\n\t2YlzgLvfS9S4+B6Yut1WKQ+4G5FoXOj3g782mcJK4FKKG4P4ppwyOhetZUxz0Oa1tCg3\n\tdBZY0u4MRstU6Wk0Coksh8orHJlszIZgjplujKTgFzUtvLYtX5GHUsavVF+s5ikRdwfN\n\tXG5ezfkb7tWhQzpZd1AkWxEPdecM35urseiksBr5qkp9C9cAIUnhiTXKpM6laLe0YfS+\n\tyT5w==","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=2DT5HFzQ/yTNw7pFeYRJk9558nm5NRPfekuiwhKNoj4=;\n\tb=kZO5ljVvjyZHZ25GE46IihkkUkTEyLIcPAXETvaIk2OROYiqZlB8jgPC6Y8HYZ2hQj\n\tnjr5mJtfNCiN4eOxhd+Y2VfvEzJ65U/osYbgSQDlR4t5CjqqLXhoASdDz+pT++GUK2og\n\tM10r4/1oaF7HTW6/dvDeUxb6TwQYCF1hjN2ddDkddVQ3JE4yqSif0dhcot1u89Oe85K1\n\tqYUZtR78wF5TwD9TfWWBLHz0zXSoj2ZmnK93dyJ73/wOUnryrYcdZjgUfnl2VJG2E/r0\n\t15UkZw6YX6JljbfoLzlM6nEWjqNJmht3gZDOSDsyiYdQgd3GYPbor3Uxkdy80qJa2G2v\n\tRazw==","X-Gm-Message-State":"AHYfb5gE4UF4lIVKZKuS96rv0KSbNN8vZVbfyEa9++m0PTEkWTxESD7r\n\twnDVbL15ERLsqKjf8XiTIW22zfKYcQ==","X-Received":"by 10.98.32.81 with SMTP id g78mr2381403pfg.24.1504114605811;\n\tWed, 30 Aug 2017 10:36:45 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<1504086545-7777-7-git-send-email-nikolay@cumulusnetworks.com>","References":"<1504086545-7777-1-git-send-email-nikolay@cumulusnetworks.com>\n\t<1504086545-7777-7-git-send-email-nikolay@cumulusnetworks.com>","From":"Cong Wang ","Date":"Wed, 30 Aug 2017 10:36:25 -0700","Message-ID":"","Subject":"Re: [PATCH net 6/9] sch_fq_codel: avoid double free on init failure","To":"Nikolay Aleksandrov ","Cc":"Linux Kernel Network Developers ,\n\tEric Dumazet , Jamal Hadi Salim ,\n\tJiri Pirko , Roopa Prabhu ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1760511,"web_url":"http://patchwork.ozlabs.org/comment/1760511/","msgid":"<152fa91b-f2a6-1fa1-f6d6-3a5a8f5db590@cumulusnetworks.com>","list_archive_url":null,"date":"2017-08-30T21:37:21","subject":"Re: [PATCH net 6/9] sch_fq_codel: avoid double free on init failure","submitter":{"id":66448,"url":"http://patchwork.ozlabs.org/api/people/66448/","name":"Nikolay Aleksandrov","email":"nikolay@cumulusnetworks.com"},"content":"On 30/08/17 20:36, Cong Wang wrote:\n> On Wed, Aug 30, 2017 at 2:49 AM, Nikolay Aleksandrov\n> wrote:\n>> It is very unlikely to happen but the backlogs memory allocation\n>> could fail and will free q->flows, but then ->destroy() will free\n>> q->flows too. For correctness remove the first free and let ->destroy\n>> clean up.\n>>\n>> Fixes: 87b60cfacf9f (\"net_sched: fix error recovery at qdisc creation\")\n>> Signed-off-by: Nikolay Aleksandrov \n>> ---\n>> net/sched/sch_fq_codel.c | 4 +---\n>> 1 file changed, 1 insertion(+), 3 deletions(-)\n>>\n>> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c\n>> index 337f2d6d81e4..2c0c05f2cc34 100644\n>> --- a/net/sched/sch_fq_codel.c\n>> +++ b/net/sched/sch_fq_codel.c\n>> @@ -491,10 +491,8 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt)\n>> if (!q->flows)\n>> return -ENOMEM;\n>> q->backlogs = kvzalloc(q->flows_cnt * sizeof(u32), GFP_KERNEL);\n>> - if (!q->backlogs) {\n>> - kvfree(q->flows);\n>> + if (!q->backlogs)\n>> return -ENOMEM;\n>> - }\n> \n> This is fine. Or we can NULL it after kvfree().\n> \n> I have no preference here. The only difference here is if we still\n> expect ->init() to cleanup its own failure.\n> \n\nWe don't, that's the point of the changes that lead to these fixes,\nthe way ->destroy() is used by both the default qdisc infra and the\nnormal qdisc add suggest that it should clean up after ->init failure,\nthus the change.","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 (1024-bit key;\n\tunprotected) header.d=cumulusnetworks.com\n\theader.i=@cumulusnetworks.com header.b=\"CFsf1xb6\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjJkb1gGFz9s8P\n\tfor ;\n\tThu, 31 Aug 2017 07:37:27 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751351AbdH3VhZ (ORCPT );\n\tWed, 30 Aug 2017 17:37:25 -0400","from mail-wm0-f46.google.com ([74.125.82.46]:38440 \"EHLO\n\tmail-wm0-f46.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751330AbdH3VhY (ORCPT\n\t); Wed, 30 Aug 2017 17:37:24 -0400","by mail-wm0-f46.google.com with SMTP id 187so9097627wmn.1\n\tfor ; Wed, 30 Aug 2017 14:37:23 -0700 (PDT)","from [192.168.0.103] (46-10-142-144.ip.btc-net.bg. [46.10.142.144])\n\tby smtp.googlemail.com with ESMTPSA id\n\tn6sm2952635wmg.11.2017.08.30.14.37.21\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tWed, 30 Aug 2017 14:37:21 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=cumulusnetworks.com; s=google;\n\th=subject:to:references:cc:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=7yu3YTIRJpW0qiT2ozAYECZFLyAp3s32QicYq+8BHBg=;\n\tb=CFsf1xb6MWOkLBz7lyAsa7gwWiAZIsFqsbsLSbw8PzaV4UMBkQBcWG8Fh0+i+ZaAlE\n\tXo/kHaUTi7j2+znJx/uDA7lyuS2OK87PhSSzDMvQsyj7fTWuKIohF5ewNLnfx6gST+ZL\n\toBXElnBiMGms0M+NHO+w96ZnqlqX+ioo1jlkg=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:cc:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=7yu3YTIRJpW0qiT2ozAYECZFLyAp3s32QicYq+8BHBg=;\n\tb=f8Mrtzw8bSU5xlqAjVrTSfrfV2HE9dTiT1WAp0/iPIkVreKC0D8xuO5XI22Zp4rbEi\n\t1MsqU5V8WSBwX+qUU7U56uexVkDVwd6Ri1b761GjghjybXzFQYn2r9EwKYlXbBsxw8bL\n\tr1QW5gvOtmA2eELLjMvCGO6HTNTx5Yj3b7oZ+iqrloNsPbnfIJW8U6HEDWaMNVYio0b1\n\tc21+s8ncPVHPWq8aHnmTDBY58A17hRCpWdEEN7iyyryZ9c4aZQqr0VMEy5vt3b57mm05\n\tO8QPS8GB/Hlzcg4vmcAzmzBfZEwW4n1iY78B7qM2LN0T05eP5xLIqOrCrtcytL0cFGtu\n\tKmvA==","X-Gm-Message-State":"AHYfb5g/eLuPN7mB9nDRxC7uq2K9crKDUGjyznALD/e6MoSxd0AFBpSq\n\twdie5JQP3/OV3LO2","X-Received":"by 10.28.174.75 with SMTP id x72mr1896895wme.107.1504129042817; \n\tWed, 30 Aug 2017 14:37:22 -0700 (PDT)","Subject":"Re: [PATCH net 6/9] sch_fq_codel: avoid double free on init failure","To":"Cong Wang ","References":"<1504086545-7777-1-git-send-email-nikolay@cumulusnetworks.com>\n\t<1504086545-7777-7-git-send-email-nikolay@cumulusnetworks.com>\n\t","Cc":"Linux Kernel Network Developers ,\n\tEric Dumazet , Jamal Hadi Salim ,\n\tJiri Pirko , Roopa Prabhu ","From":"Nikolay Aleksandrov ","Message-ID":"<152fa91b-f2a6-1fa1-f6d6-3a5a8f5db590@cumulusnetworks.com>","Date":"Thu, 31 Aug 2017 00:37:21 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101\n\tIcedove/45.6.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]