From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BD037C54F70 for ; Sun, 19 Apr 2020 21:55:48 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 88764218AC for ; Sun, 19 Apr 2020 21:55:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 88764218AC Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=acm.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 193BC8E0005; Sun, 19 Apr 2020 17:55:48 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 144A18E0003; Sun, 19 Apr 2020 17:55:48 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 058EE8E0005; Sun, 19 Apr 2020 17:55:47 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0181.hostedemail.com [216.40.44.181]) by kanga.kvack.org (Postfix) with ESMTP id E1E4A8E0003 for ; Sun, 19 Apr 2020 17:55:47 -0400 (EDT) Received: from smtpin02.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id A48C9181AEF23 for ; Sun, 19 Apr 2020 21:55:47 +0000 (UTC) X-FDA: 76725962334.02.milk21_3288aa0a5022d X-HE-Tag: milk21_3288aa0a5022d X-Filterd-Recvd-Size: 5141 Received: from mail-pg1-f196.google.com (mail-pg1-f196.google.com [209.85.215.196]) by imf09.hostedemail.com (Postfix) with ESMTP for ; Sun, 19 Apr 2020 21:55:47 +0000 (UTC) Received: by mail-pg1-f196.google.com with SMTP id x26so4053952pgc.10 for ; Sun, 19 Apr 2020 14:55:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Br4qVD6vNcOTeGAPUTvXdaMBJRDE1FMGq+EEBulVyKA=; b=NNEdcO1eDV2opIbQz0FudkQHCI2goYGoCvxarP/D5ewljuklmCrwxWqWsBkmD+x/DP 7fPGcWRZrK+F3qCRgYj6s4qi5erLMvBHbOPBNicw6Ci8sVSisl6Ggrr4ahrVa1vLtKij tpkx2SbZzM+la9A1Z2ihK/y+AUN1F41TdrW5BLfAsKddcB/p1uYhSsGF5NH/VXi/4+Cp +Cx4qBOkgB/J6EClSHhyN/CwrjN5JQjSKqoAHqJyNp9jivQoqEFYMLtzoDNXqkKnKiEg VnHljsCWOnc3iHnzfgLKlfGCSSd8sKSa0S2YFEI98eYZQL/ubOxtk56qmtzCbSzMZAeC Ff8g== X-Gm-Message-State: AGi0PuYtmzmbI03nkV6/VMQ3VAPnBqXSRzAFhef2dBvCeBR9xbBFABhI llB8OrAr5TQnSlIkjZ5rVP8= X-Google-Smtp-Source: APiQypILUJeV066/FeccQlF/yOmA6aYxTTF0zi1MK6KK770wqgS/jEy9+PKiMhVDohfqUAoAFP9Tjw== X-Received: by 2002:aa7:8429:: with SMTP id q9mr13601282pfn.205.1587333345858; Sun, 19 Apr 2020 14:55:45 -0700 (PDT) Received: from [100.124.11.78] ([104.129.198.64]) by smtp.gmail.com with ESMTPSA id 80sm24479420pgb.45.2020.04.19.14.55.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 19 Apr 2020 14:55:44 -0700 (PDT) Subject: Re: [PATCH v2 03/10] blktrace: fix debugfs use after free To: Luis Chamberlain , axboe@kernel.dk, viro@zeniv.linux.org.uk, gregkh@linuxfoundation.org, rostedt@goodmis.org, mingo@redhat.com, jack@suse.cz, ming.lei@redhat.com, nstange@suse.de, akpm@linux-foundation.org Cc: mhocko@suse.com, yukuai3@huawei.com, linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Omar Sandoval , Hannes Reinecke , Michal Hocko , syzbot+603294af2d01acfdd6da@syzkaller.appspotmail.com References: <20200419194529.4872-1-mcgrof@kernel.org> <20200419194529.4872-4-mcgrof@kernel.org> From: Bart Van Assche Message-ID: <91c82e6a-24ce-0b7d-e6e4-e8aa89f3fb79@acm.org> Date: Sun, 19 Apr 2020 14:55:42 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <20200419194529.4872-4-mcgrof@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 4/19/20 12:45 PM, Luis Chamberlain wrote: > +int __must_check blk_queue_debugfs_register(struct request_queue *q) > +{ > + struct dentry *dir = NULL; > + > + /* This can happen if we have a bug in the lower layers */ What does "this" refer to? Which layers does "lower layers" refer to? Most software developers consider a module that calls directly into another module as a higher layer (callbacks through function pointers do not count; see also https://en.wikipedia.org/wiki/Modular_programming). According to that definition block drivers are a software layer immediately above the block layer core. How about changing that comment into the following to make it unambiguous (if this is what you meant)? /* * Check whether the debugfs directory already exists. This can * only happen as the result of a bug in a block driver. */ > + dir = debugfs_lookup(kobject_name(q->kobj.parent), blk_debugfs_root); > + if (dir) { > + pr_warn("%s: registering request_queue debugfs directory twice is not allowed\n", > + kobject_name(q->kobj.parent)); > + dput(dir); > + return -EALREADY; > + } > + > + q->debugfs_dir = debugfs_create_dir(kobject_name(q->kobj.parent), > + blk_debugfs_root); > + if (!q->debugfs_dir) > + return -ENOMEM; > + > + return 0; > +} kobject_name(q->kobj.parent) is used three times in the above function. How about introducing a local variable that holds the result of that expression? > +static bool blk_trace_target_disk(const char *target, const char *diskname) > +{ > + if (strlen(target) != strlen(diskname)) > + return false; > + > + if (!strncmp(target, diskname, > + min_t(size_t, strlen(target), strlen(diskname)))) > + return true; > + > + return false; > +} The above code looks weird to me. When the second if-statement is reached, it is guaranteed that 'target' and 'diskname' have the same length. So why to calculate the minimum length in the second if-statement of two strings that have the same length? Independent of what the purpose of the above code is, can that code be rewritten such that it does not depend on the details of how names are assigned to disks and partitions? Would disk_get_part() be useful here? Thanks, Bart.