Message ID | 20231030104732.241339-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:d641:0:b0:403:3b70:6f57 with SMTP id cy1csp2114284vqb; Mon, 30 Oct 2023 03:48:33 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHNnKBQPYHnXI5ebltBmOURGmmX8h3dvhFMo/Ka/Mq2dSXbyRvzSI3vRzPwS4x0CRGxki1R X-Received: by 2002:a05:6a20:c19a:b0:16b:ff2c:c42c with SMTP id bg26-20020a056a20c19a00b0016bff2cc42cmr7074459pzb.62.1698662913069; Mon, 30 Oct 2023 03:48:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698662913; cv=none; d=google.com; s=arc-20160816; b=MQE6eaMQRbIbl3fcqlkEJRXLr2Dex3rDA0Dzjz7EhZnUDLRa2JABBn8IMR4LBuy4Nb X/dFrkYYkWxS2qSYKcbX+HIi8OndI7mvKMlhOA3WjoflvFAAseYKW0dp0Ym5lnXvJ0XM 0Y9EDb8kd4y6iEZGni2VtBQU6pl2y1MlZtcAp+Uy7GK3VspgIZMRj3t3z6VDhOavzDRt tHp4vz1EnajOjguePOXtspSO07ZtsDUN79T9JO4jRkgUhDC3g0seq8mPDYuLapbXzvK/ jVrtbL0mn+roV8e9ES3MyOeTMcpI7KR8Em1GliP9JdNcCxeub4JFOKfX05qVlb3jGiP2 +3wQ== 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=NkjnRUFCqAWqmRd8TZFPGqSu7yQpHb4x+ERWcyrzClg=; fh=zFy42mhpE2u4xFwte26xQa/ZLoka/uZdP9ObmF0xxgQ=; b=fd22hwKrxJqcjIRgkbMXbZCZLICWt+05YgRD7t92wf8/MGuj2pY+0ImU4Zs5NQr4Nn zh72uta5v7WpiLLuHsLp6mgXqPYkTUpyVB6Kh2fLotdFphI/FTcxUygxE998K57ZNsrN dpFkH7UhIziJhUXZ42FulT6+U2cmVJvPurenjWWZYh2UVjnzaF0niCSggHkixdEfOg8H bgofv22vK/jPCIMbH+0oIoD3Knj6kixkiqRfIFAPhcixM8pvpBCvL6Tigj7E9LV3ePBf y4uYBZVOrMF4ifG7LJzUdBxvL2nW77uksRZI85I1reD3xpk9BzYGzaLChxO0U17ei0qa Ya4w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@cirrus.com header.s=PODMain02222019 header.b=nRXJPiP7; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 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. [23.128.96.35]) by mx.google.com with ESMTPS id u11-20020a170903308b00b001cc33933e45si3054376plc.42.2023.10.30.03.48.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Oct 2023 03:48:33 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; Authentication-Results: mx.google.com; dkim=pass header.i=@cirrus.com header.s=PODMain02222019 header.b=nRXJPiP7; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 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 F27E1805EB08; Mon, 30 Oct 2023 03:48:25 -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 S232513AbjJ3Kr4 (ORCPT <rfc822;zxc52fgh@gmail.com> + 31 others); Mon, 30 Oct 2023 06:47:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57788 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232535AbjJ3Krx (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 30 Oct 2023 06:47:53 -0400 Received: from mx0b-001ae601.pphosted.com (mx0a-001ae601.pphosted.com [67.231.149.25]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B9569C9; Mon, 30 Oct 2023 03:47:50 -0700 (PDT) Received: from pps.filterd (m0077473.ppops.net [127.0.0.1]) by mx0a-001ae601.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id 39U612R1032696; Mon, 30 Oct 2023 05:47:34 -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=N kjnRUFCqAWqmRd8TZFPGqSu7yQpHb4x+ERWcyrzClg=; b=nRXJPiP7SObKLRdYB eAcQTLpPoQLAvXApUAmg9Ij3hAlFXMveCmLforiuwNJIu1i8FyU7GzMzHnG9fWnE y5LEkSe558YpQhgdHc5vLvDhXwaK4D6g+nxyKoCiP6HAAbme2/ZJ9v/6XL7TDVr/ KABrGu0X6xYKWynNr4wwO7hNahpXN6VlWY7eW/Vq+bXT8yuQxLrlborbcjRWJAF7 ++RE91twPt0j+6YZgdGf4S5AuodbqA3UHvwEeznXw6OJq3IQmLj4fZ2L2nZRQdof c2KJN2mb0Ur/+WL006NVSI1LrNme8LD+iDPSwTP41mEVBw6u2Ej2KPzclJ9B8vFg UseIw== Received: from ediex02.ad.cirrus.com ([84.19.233.68]) by mx0a-001ae601.pphosted.com (PPS) with ESMTPS id 3u0ypxay6j-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 30 Oct 2023 05:47:34 -0500 (CDT) Received: from ediex01.ad.cirrus.com (198.61.84.80) by ediex02.ad.cirrus.com (198.61.84.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.39; Mon, 30 Oct 2023 10:47:32 +0000 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.39 via Frontend Transport; Mon, 30 Oct 2023 10:47:32 +0000 Received: from edi-sw-dsktp-006.ad.cirrus.com (edi-sw-dsktp-006.ad.cirrus.com [198.90.251.82]) by ediswmail.ad.cirrus.com (Postfix) with ESMTP id 3621811AB; Mon, 30 Oct 2023 10:47:32 +0000 (UTC) From: Richard Fitzgerald <rf@opensource.cirrus.com> To: <brendan.higgins@linux.dev>, <davidgow@google.com>, <rmoar@google.com> CC: <linux-kselftest@vger.kernel.org>, <kunit-dev@googlegroups.com>, <linux-kernel@vger.kernel.org>, <patches@opensource.cirrus.com>, "Richard Fitzgerald" <rf@opensource.cirrus.com>, Dan Carpenter <dan.carpenter@linaro.org> Subject: [PATCH v2 RESEND] kunit: debugfs: Handle errors from alloc_string_stream() Date: Mon, 30 Oct 2023 10:47:32 +0000 Message-ID: <20231030104732.241339-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: 549nZg4elloeUtvQIK1wKsk8q2C8c3if X-Proofpoint-ORIG-GUID: 549nZg4elloeUtvQIK1wKsk8q2C8c3if 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]); Mon, 30 Oct 2023 03:48:26 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781177162463803592 X-GMAIL-MSGID: 1781177162463803592 |
Series |
[v2,RESEND] kunit: debugfs: Handle errors from alloc_string_stream()
|
|
Commit Message
Richard Fitzgerald
Oct. 30, 2023, 10:47 a.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 | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
Comments
On Mon, Oct 30, 2023 at 6:47 AM Richard Fitzgerald <rf@opensource.cirrus.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() Hello! Thanks for sending the re-sends of these patches! This patch also looks good to me! I have one comment below but I would still be happy with the patch as is. Reviewed-by: Rae Moar <rmoar@google.com> Thanks! -Rae > > 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 | 30 +++++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c > index 270d185737e6..9d167adfa746 100644 > --- a/lib/kunit/debugfs.c > +++ b/lib/kunit/debugfs.c > @@ -109,14 +109,28 @@ 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 suite->log and test_case->log pointer are expected to be NULL > + * if there isn't a log, so only set it if the log stream was created > + * successfully. > + */ I like this new comment. Thanks! > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) In response to Dan Carpenter's comment from the last version, I see the benefits of changing IS_ERR_OR_NULL() to IS_ERR() instead because "stream" will not be NULL. This would then also be the same as the check in kunit_alloc_string_stream. However, I also see the benefit of checking for NULL just in case anyways. I'm overall happy with either solution but just wanted to bring this up. > + return; > + > + 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 +138,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 >
On Mon, 30 Oct 2023 at 18:47, Richard Fitzgerald <rf@opensource.cirrus.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") > --- Thanks for fixing all the nasty C error handling. Closes: https://groups.google.com/g/kunit-dev/c/sf6MsFzeEV4 Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David
On Thu, Nov 30, 2023 at 04:11:18PM -0500, Rae Moar wrote: > > + stream = alloc_string_stream(GFP_KERNEL); > > + if (IS_ERR_OR_NULL(stream)) > > In response to Dan Carpenter's comment from the last version, I see > the benefits of changing IS_ERR_OR_NULL() to IS_ERR() instead because > "stream" will not be NULL. This would then also be the same as the > check in kunit_alloc_string_stream. > > However, I also see the benefit of checking for NULL just in case anyways. > Returning NULL in alloc_string_stream() is a bug. Checking for NULL is a work around for bugs. There are basically two times where it can be valid to work around bugs like this instead of fixing them. 1) When the function is implemented by over 10 different driver authors. In that case you can guarantee that at least one of them is going to do the wrong thing. There are between 2-5 places which do this in the kernel. 2) If it's a API that used to return NULL and it's changed to returning error pointers. I've never seen anyone do this, but I've proposed it as a solution to make backporting easier. regards, dan carpenter
diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c index 270d185737e6..9d167adfa746 100644 --- a/lib/kunit/debugfs.c +++ b/lib/kunit/debugfs.c @@ -109,14 +109,28 @@ 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 suite->log and test_case->log pointer are expected to 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)) + return; + + 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 +138,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)