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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 87070C433F5 for ; Tue, 29 Mar 2022 19:29:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240970AbiC2TbK (ORCPT ); Tue, 29 Mar 2022 15:31:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43484 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240940AbiC2TbI (ORCPT ); Tue, 29 Mar 2022 15:31:08 -0400 Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 85479517CC for ; Tue, 29 Mar 2022 12:29:25 -0700 (PDT) Received: by mail-ej1-x635.google.com with SMTP id j15so37131745eje.9 for ; Tue, 29 Mar 2022 12:29:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=B5UYIP8SGQjxl6HHM2puamfh3AUwmyljeoli+qwWwAY=; b=D7GisGTzI8xLen8EAeQSTteYE6hALAZt9nrQMQJpPWjRkXDFmI32APn0NszipPzuMz i8ekYTV8xc3iD/Qz/qvrRynW7B0WGm9RGQIn/vLk93ZqRkR7pckkhiZUL6M/axKxHaiy fQUhTAre9qC57KkbxvGc2mzFY9ibz2WNgaGOw+1mW3NCPkS2lFprfeQoX9X0pn+4OHe7 C4HBZtUnhLbZGJdHvtKMzGQxyT8qtwStFp28hU64HH5r25Bh2ewsP/p1elUQo6Q+29Gh 5F6WJl17WhoSFhkJzIfUAUUAfMyJfgjtPq1yNS+ij+1Wx54ERBVEIg+DJSSeYztob8eF +LEA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=B5UYIP8SGQjxl6HHM2puamfh3AUwmyljeoli+qwWwAY=; b=v9osEoWm3DXqRZTAESYz0losB5r9zpt43zuaNrnjG0D6SnEssh1GHxmVCIuqyi98xs u3gza9LXdTiIiqmqMzbI03fB0R8mEEcM6Vl0icOxHsBg+NIkFNB1kXd+xunCOlvjQfp5 cCsl15Gr9dexkwKN6xJAiMuGKRddg6oTu132CuhIldNLl4tjh5eCZH7ijDJJ/Bzf6nzJ OIwNQTt5hpHXQ/Aaen64AiKQSirk2Xq7/BWD6UUPuRpOey5PaB0aD5AugSXIA3OPwRKc Ha+/biGAl2Rt0+S1g/yckpVBZEk4G37xxTMYa7k2G8zChGz9wbQpf0lcpwlYzazNarac 0urg== X-Gm-Message-State: AOAM5307TxJxbrvqb0kbkWPhI8tZlPWzUc9U6yIb5ofmqEI1H3q7cKjc aZgWHg2Am4SFMpfgWb1LFrPEyeaEH+EPWUdP5wldKQ== X-Google-Smtp-Source: ABdhPJxME9rqCqtD+YyjhoLF8AMHjGE8/rDwNWZoTr9qqHV5Nd6s4I/10V3Ga1QgKWeZq0HW7cy8w6FfTFeMjT1Bkok= X-Received: by 2002:a17:907:a0c9:b0:6df:eaef:d93d with SMTP id hw9-20020a170907a0c900b006dfeaefd93dmr35558344ejc.369.1648582163761; Tue, 29 Mar 2022 12:29:23 -0700 (PDT) MIME-Version: 1.0 References: <20220329103919.2376818-1-lv.ruyi@zte.com.cn> In-Reply-To: <20220329103919.2376818-1-lv.ruyi@zte.com.cn> From: Daniel Latypov Date: Tue, 29 Mar 2022 14:29:12 -0500 Message-ID: Subject: Re: [PATCH] kunit: tool: add null pointer check To: cgel.zte@gmail.com Cc: brendanhiggins@google.com, linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com, linux-kernel@vger.kernel.org, Lv Ruyi , Zeal Robot Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Mar 29, 2022 at 5:39 AM wrote: > > From: Lv Ruyi > > kmalloc and kcalloc is a memory allocation function which can return NULL > when some internal memory errors happen. Add null pointer check to avoid > dereferencing null pointer. > > Reported-by: Zeal Robot > Signed-off-by: Lv Ruyi > --- > lib/kunit/executor.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c > index 22640c9ee819..be21d0451367 100644 > --- a/lib/kunit/executor.c > +++ b/lib/kunit/executor.c > @@ -71,9 +71,13 @@ kunit_filter_tests(struct kunit_suite *const suite, const char *test_glob) > > /* Use memcpy to workaround copy->name being const. */ > copy = kmalloc(sizeof(*copy), GFP_KERNEL); > + if (!copy) > + return NULL; While this is technically correct to check, in this context it's less clear. If we can't allocate this memory, we likely can't run any subsequent tests, either because the test cases will want to allocate some memory and/or KUnit will need to allocate some for internal bookkeeping. The existing code (and by extension this patch) "handles" OOM situations by silently dropping test suites/cases. So I sort of intentionally figured we should let it crash early in this case since that's probably more debuggable. This code does check for NULL returns earlier on in the call chain, i.e. first in kunit_filter_suites() 158 copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL); 159 filtered.start = copy; 160 if (!copy) { /* won't be able to run anything, return an empty set */ 161 filtered.end = copy; 162 return filtered; 163 } and second in kunit_filter_subsuite() 107 filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL); 108 if (!filtered) 109 return NULL; The first kmalloc_array() is our first allocation in this file. If we can't handle that, then things are really going wrong, and I assumed there'd be plenty of debug messages in dmesg, so silently returning is probably fine. The second one also felt similar. So I think that * it's highly unlikely that we pass those checks and fail on these new ones (we're not allocating much) * if we do fail, this is now harder to debug since it's partially running tests, partially not Should we instead rework the code to more clearly signal allocation errors instead of overloading NULL to mean "no matches or error?" Or maybe just adding some pr_err() calls is sufficient. > memcpy(copy, suite, sizeof(*copy)); > > filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL); > + if (!filtered) > + return NULL; > > n = 0; > kunit_suite_for_each_test_case(suite, test_case) { > -- > 2.25.1 >