Message ID | 20230927165058.29484-1-rf@opensource.cirrus.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp3015342vqu; Wed, 27 Sep 2023 18:52:26 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFlyJZjhiiY202y4hYzJ4oBYtcPuRLcl9pZp4N4s1Cg/K3QvA1CmF5nzzWNMaZJuWAMW4gO X-Received: by 2002:a05:6808:a93:b0:3ab:8295:f2f1 with SMTP id q19-20020a0568080a9300b003ab8295f2f1mr3716131oij.45.1695865946192; Wed, 27 Sep 2023 18:52:26 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695865946; cv=none; d=google.com; s=arc-20160816; b=pY6lelECspT2NItApy5rzVKO/25T158+CmcimXiQC5CHqlhfOspVQL85cgeUM00If1 TZVE1LTsqV5xifVo/t8Ujycm35Qcbu5pptaXK39/llw+T8aohvTjc3M0IAlomvjZ1VaE iLOx6Lkn/fv31dnsxvPs23IvVGj/s822ySv7dgqUM6LSO5LIgfIYSSE7xoeLSDYhGbir waibtqB8SN1b3IR0KWLfsFO1opjdGFSgVM9llP9Nuk/kFMnlfyROjY8XJIR51Xceb8P7 gVoGvwM1lcXmQBJeCwvuAucNVys/rcX17HsxDgNUEr9SeC9KYdlDOQu2MRefH9yKgKWv tbzg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=66YwtrVPB63m73rzfrkXNgTtu4wVWf9hd6q5sRDERfc=; fh=rfWZ5MhfUZw8dgrvo+Mn0E8mx5tu5AITXK4Ouig35hs=; b=oywq4JpMfobcQYXvPrU0GRG7kA5qNsICk6KFlR9gZdg6oinWtLKNPc1wcdTnx74SMp btp91DUmofmNh5NQNCBKlF/kZ6Wv/DsWZirpP9ceq7jTNadfDG5BJbDPf6Ocn4GcVRqY Lf0dMOEuufoNVOXJtTJTUjBimVvn1K9O+VMOxPA8AxQiaFLNhg+5EZcWQMAHCcFNlBpu nyaiVqDQ3HOq1aMQTZziIARVGIwGcLJ3qN0xwgt27t02KxZeBlj7goS9HAzw6z5ItK1s 6x+FOZMfbK7GKf/6qEMKsgGpmybS4GPMet7hRcs43InQvizYwYM4nuj1XKVJUmBS7Tcn lYsg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@cirrus.com header.s=PODMain02222019 header.b="qXvYGkq/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=cirrus.com Received: from groat.vger.email (groat.vger.email. [2620:137:e000::3:5]) by mx.google.com with ESMTPS id h9-20020a056a00230900b0068fba70d25dsi18877574pfh.33.2023.09.27.18.52.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 Sep 2023 18:52:26 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5; Authentication-Results: mx.google.com; dkim=pass header.i=@cirrus.com header.s=PODMain02222019 header.b="qXvYGkq/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=cirrus.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id C2825807C850; Wed, 27 Sep 2023 09:51:26 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229487AbjI0QvR (ORCPT <rfc822;ruipengqi7@gmail.com> + 19 others); Wed, 27 Sep 2023 12:51:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50544 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229437AbjI0QvP (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 27 Sep 2023 12:51:15 -0400 Received: from mx0b-001ae601.pphosted.com (mx0b-001ae601.pphosted.com [67.231.152.168]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D28EF9C; Wed, 27 Sep 2023 09:51:13 -0700 (PDT) Received: from pps.filterd (m0077474.ppops.net [127.0.0.1]) by mx0b-001ae601.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id 38RGf4a4009144; Wed, 27 Sep 2023 11:51:03 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cirrus.com; h= from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding:content-type; s=PODMain02222019; bh=6 6YwtrVPB63m73rzfrkXNgTtu4wVWf9hd6q5sRDERfc=; b=qXvYGkq/xQtDPr0ut UyCW1PJVEAA5IoA7TwFPqD80tzMyREgOIAMDqm5qL7EKlvbIE9UzD3hBQypP59+Y cnTPids4PNVEbe4QxCXM0KDPMHIuGYYNDJAOjVce3TFLqfBKRhstOgipajcPzJhs Iuqi0QBHmqxvfmkVNK8hS/XSJpH14GJyP4BfC0EokLKyK0XZ9TtxXMgTllemtJ+X MZ+yw9c3+wEcwxIrxOVqMFRuo6jrGXqOrGxMe3rhs6AJG0DXnY16LQA2nQyqb499 WHatROUrqOJS3r3cy9I+Si4PRuCRxa1smBII/gUKSO3gWpM92ZQ/VJu+sgWnrXRn GnQFw== Received: from ediex01.ad.cirrus.com ([84.19.233.68]) by mx0b-001ae601.pphosted.com (PPS) with ESMTPS id 3t9vejd1ag-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 27 Sep 2023 11:51:03 -0500 (CDT) Received: from ediex01.ad.cirrus.com (198.61.84.80) by ediex01.ad.cirrus.com (198.61.84.80) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.37; Wed, 27 Sep 2023 17:51:01 +0100 Received: from ediswmail.ad.cirrus.com (198.61.86.93) by ediex01.ad.cirrus.com (198.61.84.80) with Microsoft SMTP Server id 15.2.1118.37 via Frontend Transport; Wed, 27 Sep 2023 17:51:01 +0100 Received: from EDIN4L06LR3.ad.cirrus.com (EDIN4L06LR3.ad.cirrus.com [198.61.65.112]) by ediswmail.ad.cirrus.com (Postfix) with ESMTP id 5998E15B9; Wed, 27 Sep 2023 16:51:01 +0000 (UTC) From: Richard Fitzgerald <rf@opensource.cirrus.com> To: <brendan.higgins@linux.dev>, <davidgow@google.com> CC: <linux-kselftest@vger.kernel.org>, <kunit-dev@googlegroups.com>, <linux-kernel@vger.kernel.org>, Richard Fitzgerald <rf@opensource.cirrus.com>, Dan Carpenter <dan.carpenter@linaro.org> Subject: [PATCH] kunit: debugfs: Handle errors from alloc_string_stream() Date: Wed, 27 Sep 2023 17:50:58 +0100 Message-ID: <20230927165058.29484-1-rf@opensource.cirrus.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Proofpoint-GUID: RA6cTYiBvohA6WeMlQ8Tn2N_krYxb87g X-Proofpoint-ORIG-GUID: RA6cTYiBvohA6WeMlQ8Tn2N_krYxb87g X-Proofpoint-Spam-Reason: safe X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Wed, 27 Sep 2023 09:51:26 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778244330708003753 X-GMAIL-MSGID: 1778244330708003753 |
Series |
kunit: debugfs: Handle errors from alloc_string_stream()
|
|
Commit Message
Richard Fitzgerald
Sept. 27, 2023, 4:50 p.m. UTC
In kunit_debugfs_create_suite() give up and skip creating the debugfs
file if any of the alloc_string_stream() calls return an error or NULL.
Only put a value in the log pointer of kunit_suite and kunit_test if it
is a valid pointer to a log.
This prevents the potential invalid dereference reported by smatch:
lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log'
dereferencing possible ERR_PTR()
lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log'
dereferencing possible ERR_PTR()
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 05e2006ce493 ("kunit: Use string_stream for test log")
---
lib/kunit/debugfs.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
Comments
On Wed, Sep 27, 2023 at 12:51 PM 'Richard Fitzgerald' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > In kunit_debugfs_create_suite() give up and skip creating the debugfs > file if any of the alloc_string_stream() calls return an error or NULL. > Only put a value in the log pointer of kunit_suite and kunit_test if it > is a valid pointer to a log. > > This prevents the potential invalid dereference reported by smatch: > > lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log' > dereferencing possible ERR_PTR() > lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log' > dereferencing possible ERR_PTR() > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Fixes: 05e2006ce493 ("kunit: Use string_stream for test log") Hi! I've tested this and this all looks good to me. Reviewed-by: Rae Moar <rmoar@google.com> Thanks! Rae > --- > lib/kunit/debugfs.c | 29 ++++++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c > index 270d185737e6..73075ca6e88c 100644 > --- a/lib/kunit/debugfs.c > +++ b/lib/kunit/debugfs.c > @@ -109,14 +109,27 @@ static const struct file_operations debugfs_results_fops = { > void kunit_debugfs_create_suite(struct kunit_suite *suite) > { > struct kunit_case *test_case; > + struct string_stream *stream; > > - /* Allocate logs before creating debugfs representation. */ > - suite->log = alloc_string_stream(GFP_KERNEL); > - string_stream_set_append_newlines(suite->log, true); > + /* > + * Allocate logs before creating debugfs representation. > + * The log pointer must be NULL if there isn't a log so only > + * set it if the log stream was created successfully. > + */ > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) > + goto err; > + > + string_stream_set_append_newlines(stream, true); > + suite->log = stream; > > kunit_suite_for_each_test_case(suite, test_case) { > - test_case->log = alloc_string_stream(GFP_KERNEL); > - string_stream_set_append_newlines(test_case->log, true); > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) > + goto err; > + > + string_stream_set_append_newlines(stream, true); > + test_case->log = stream; > } > > suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir); > @@ -124,6 +137,12 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite) > debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444, > suite->debugfs, > suite, &debugfs_results_fops); > + return; > + > +err: > + string_stream_destroy(suite->log); > + kunit_suite_for_each_test_case(suite, test_case) > + string_stream_destroy(test_case->log); > } > > void kunit_debugfs_destroy_suite(struct kunit_suite *suite) > -- > 2.30.2 > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230927165058.29484-1-rf%40opensource.cirrus.com.
On Wed, Sep 27, 2023 at 05:50:58PM +0100, Richard Fitzgerald wrote: > In kunit_debugfs_create_suite() give up and skip creating the debugfs > file if any of the alloc_string_stream() calls return an error or NULL. > Only put a value in the log pointer of kunit_suite and kunit_test if it > is a valid pointer to a log. > > This prevents the potential invalid dereference reported by smatch: > > lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log' > dereferencing possible ERR_PTR() > lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log' > dereferencing possible ERR_PTR() > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Fixes: 05e2006ce493 ("kunit: Use string_stream for test log") > --- > lib/kunit/debugfs.c | 29 ++++++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c > index 270d185737e6..73075ca6e88c 100644 > --- a/lib/kunit/debugfs.c > +++ b/lib/kunit/debugfs.c > @@ -109,14 +109,27 @@ static const struct file_operations debugfs_results_fops = { > void kunit_debugfs_create_suite(struct kunit_suite *suite) > { > struct kunit_case *test_case; > + struct string_stream *stream; > > - /* Allocate logs before creating debugfs representation. */ > - suite->log = alloc_string_stream(GFP_KERNEL); > - string_stream_set_append_newlines(suite->log, true); > + /* > + * Allocate logs before creating debugfs representation. > + * The log pointer must be NULL if there isn't a log so only > + * set it if the log stream was created successfully. > + */ > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) > + goto err; So when you're programming, there is an aspect where you are telling the computer what to do, but there is also an aspect where you are communicating to other programmers. Checking for IS_ERR_OR_NULL() works in terms of computers, but in terms of communicating to humans it's misleading. And to be honest the comments are even more confusing because they suggest something complicated is happening so I had to review the code extra carefully. When a function returns both error pointers and NULL then it means it is an optional feature which can be disabled. Error pointers means errors. NULL means disabled. So typically the IS_ERR_OR_NULL() or NULL check looks like this: blinky_lights = get_blinky(); if (IS_ERR_OR_NULL(blinky_lights)) return PTR_ERR(blinky_lights); blinky_lights->blink(); return 0; This means that the function requires the blinky feature to continue. If blinky_lights is an error pointer then it returns an error. If blinky_lights is NULL then PTR_ERR(NULL) is zero which is success. We didn't blink the lights but only because the admin told us not to. It's a no-op but it's not an error. In this case, alloc_string_stream() is not optional. It only returns error pointers. It can never return NULL. I have written a blog about this in more detail but already this email is probably longer than my blog was. https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ regards, dan carpenter
On 27/09/2023 17:50, Richard Fitzgerald wrote: > In kunit_debugfs_create_suite() give up and skip creating the debugfs > file if any of the alloc_string_stream() calls return an error or NULL. > Only put a value in the log pointer of kunit_suite and kunit_test if it > is a valid pointer to a log. > > This prevents the potential invalid dereference reported by smatch: > > lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log' > dereferencing possible ERR_PTR() > lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log' > dereferencing possible ERR_PTR() > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Fixes: 05e2006ce493 ("kunit: Use string_stream for test log") > --- > lib/kunit/debugfs.c | 29 ++++++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c > index 270d185737e6..73075ca6e88c 100644 > --- a/lib/kunit/debugfs.c > +++ b/lib/kunit/debugfs.c > @@ -109,14 +109,27 @@ static const struct file_operations debugfs_results_fops = { > void kunit_debugfs_create_suite(struct kunit_suite *suite) > { > struct kunit_case *test_case; > + struct string_stream *stream; > > - /* Allocate logs before creating debugfs representation. */ > - suite->log = alloc_string_stream(GFP_KERNEL); > - string_stream_set_append_newlines(suite->log, true); > + /* > + * Allocate logs before creating debugfs representation. > + * The log pointer must be NULL if there isn't a log so only > + * set it if the log stream was created successfully. > + */ > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) > + goto err; This can be a return. Nothing has been created at this point so there is nothing to clean up. I'll send a V2. > + > + string_stream_set_append_newlines(stream, true); > + suite->log = stream; > > kunit_suite_for_each_test_case(suite, test_case) { > - test_case->log = alloc_string_stream(GFP_KERNEL); > - string_stream_set_append_newlines(test_case->log, true); > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) > + goto err; > + > + string_stream_set_append_newlines(stream, true); > + test_case->log = stream; > } > > suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir); > @@ -124,6 +137,12 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite) > debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444, > suite->debugfs, > suite, &debugfs_results_fops); > + return; > + > +err: > + string_stream_destroy(suite->log); > + kunit_suite_for_each_test_case(suite, test_case) > + string_stream_destroy(test_case->log); > } > > void kunit_debugfs_destroy_suite(struct kunit_suite *suite)
diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c index 270d185737e6..73075ca6e88c 100644 --- a/lib/kunit/debugfs.c +++ b/lib/kunit/debugfs.c @@ -109,14 +109,27 @@ static const struct file_operations debugfs_results_fops = { void kunit_debugfs_create_suite(struct kunit_suite *suite) { struct kunit_case *test_case; + struct string_stream *stream; - /* Allocate logs before creating debugfs representation. */ - suite->log = alloc_string_stream(GFP_KERNEL); - string_stream_set_append_newlines(suite->log, true); + /* + * Allocate logs before creating debugfs representation. + * The log pointer must be NULL if there isn't a log so only + * set it if the log stream was created successfully. + */ + stream = alloc_string_stream(GFP_KERNEL); + if (IS_ERR_OR_NULL(stream)) + goto err; + + string_stream_set_append_newlines(stream, true); + suite->log = stream; kunit_suite_for_each_test_case(suite, test_case) { - test_case->log = alloc_string_stream(GFP_KERNEL); - string_stream_set_append_newlines(test_case->log, true); + stream = alloc_string_stream(GFP_KERNEL); + if (IS_ERR_OR_NULL(stream)) + goto err; + + string_stream_set_append_newlines(stream, true); + test_case->log = stream; } suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir); @@ -124,6 +137,12 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite) debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444, suite->debugfs, suite, &debugfs_results_fops); + return; + +err: + string_stream_destroy(suite->log); + kunit_suite_for_each_test_case(suite, test_case) + string_stream_destroy(test_case->log); } void kunit_debugfs_destroy_suite(struct kunit_suite *suite)