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 7BC9FC433EF for ; Wed, 4 May 2022 19:43:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1377625AbiEDTrS (ORCPT ); Wed, 4 May 2022 15:47:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48802 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1377597AbiEDTrR (ORCPT ); Wed, 4 May 2022 15:47:17 -0400 Received: from mail-pg1-x52b.google.com (mail-pg1-x52b.google.com [IPv6:2607:f8b0:4864:20::52b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D04CB4DF54 for ; Wed, 4 May 2022 12:43:39 -0700 (PDT) Received: by mail-pg1-x52b.google.com with SMTP id a191so1963646pge.2 for ; Wed, 04 May 2022 12:43:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=RQ5+Z93YtWEK5xXREhrhRleXnskqFAblQsxgrgprxiY=; b=PWTj7QFNwdFF2OE0C4xFUgoIvDGn2+dYizGm/ZxeTyJ+Iz/tyIN+jbKS/JUP74dMIB W7u3atVP0GNPWpIF5TIzABeYYT/Et9n+I/Ppab3jaBr0jw2QDopw+U1E91lEzpAOCQFs xQr1OFa2NKNeyWLt6H4cI0w0eWAyTF94chl/w= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=RQ5+Z93YtWEK5xXREhrhRleXnskqFAblQsxgrgprxiY=; b=k2u6/qmzdrPL/4TUxvF2cQ2YF/uL7kADaRWcjUA7tRG4KHgaPH1EGxJtQ+yXXCFEwk 5567ZIrI9KU50a5polZxtSVoeZ5Jrb2TlL9R5dG0Kfhx8RC7HF36jUyE9MwOyT03H/iK 5gKN8iAmQWo6/oCK4UMoD80vydMFJGwLFTQ6sMW+rvlBFbsglhSHBjjr1BvNrFs0MrhU F6YH1VQ+Cgy9sP3Sk0bSMdoD96XUKjbyJQMxz3qpjbMSwh5zklhutieWI1G/EpvbSQ3O XObCoqwMTQ2OWGZ9R0i1oO6fSY9QAF+hpaNlMdbEanMIRmcayOpuoarwDSVqUBX4Qm8n tOzg== X-Gm-Message-State: AOAM532Sw5AXEg/2L8Zb5Ld3h2EgZIRy5t4PrL2dA+E3zPjbnvheKCC5 dg5M4mbCoVTAmZl+zYiNd6r5wg== X-Google-Smtp-Source: ABdhPJxMOUaGN1HZweue295reBywbQJPuhTeE52Ky8jS8qudgy4bxd3bAxIpi3n4Zjg11NpOxjxBqg== X-Received: by 2002:a63:91c9:0:b0:3ab:11e6:4ff9 with SMTP id l192-20020a6391c9000000b003ab11e64ff9mr17952537pge.121.1651693419276; Wed, 04 May 2022 12:43:39 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id d12-20020a170902654c00b0015e8d4eb24dsm8677848pln.151.2022.05.04.12.43.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 May 2022 12:43:38 -0700 (PDT) Date: Wed, 4 May 2022 12:43:37 -0700 From: Kees Cook To: David Gow Cc: "Gustavo A . R . Silva" , KUnit Development , Alexei Starovoitov , alsa-devel@alsa-project.org, Al Viro , Andrew Gabbasov , Andrew Morton , Andy Gross , Andy Lavr , Arend van Spriel , Baowen Zheng , Bjorn Andersson , Boris Ostrovsky , Bradley Grove , brcm80211-dev-list.pdl@broadcom.com, Christian Brauner , Christian =?iso-8859-1?Q?G=F6ttsche?= , Christian Lamparter , Chris Zankel , Cong Wang , Daniel Axtens , Daniel Vetter , Dan Williams , David Howells , "David S. Miller" , Dennis Dalessandro , devicetree@vger.kernel.org, Dexuan Cui , Dmitry Kasatkin , Eli Cohen , Eric Dumazet , Eric Paris , Eugeniu Rosca , Felipe Balbi , Francis Laniel , Frank Rowand , Franky Lin , Greg Kroah-Hartman , Gregory Greenman , Guenter Roeck , Haiyang Zhang , Hante Meuleman , Herbert Xu , Hulk Robot , Jakub Kicinski , "James E.J. Bottomley" , James Morris , Jarkko Sakkinen , Jaroslav Kysela , Jason Gunthorpe , Jens Axboe , Johan Hedberg , Johannes Berg , Johannes Berg , John Keeping , Juergen Gross , Kalle Valo , Keith Packard , keyrings@vger.kernel.org, Kuniyuki Iwashima , "K. Y. Srinivasan" , Lars-Peter Clausen , Lee Jones , Leon Romanovsky , Liam Girdwood , linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, Linux ARM , linux-arm-msm@vger.kernel.org, linux-bluetooth@vger.kernel.org, linux-hardening@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-integrity@vger.kernel.org, linux-rdma@vger.kernel.org, linux-scsi@vger.kernel.org, linux-security-module , linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux-xtensa@linux-xtensa.org, llvm@lists.linux.dev, Loic Poulain , Louis Peens , Luca Coelho , Luiz Augusto von Dentz , Marcel Holtmann , Mark Brown , "Martin K. Petersen" , Max Filippov , Mimi Zohar , Muchun Song , Nathan Chancellor , Networking , Nick Desaulniers , Nuno =?iso-8859-1?Q?S=E1?= , Paolo Abeni , Paul Moore , Rich Felker , Rob Herring , Russell King , selinux@vger.kernel.org, "Serge E. Hallyn" , SHA-cyfmac-dev-list@infineon.com, Simon Horman , Stefano Stabellini , Stefan Richter , Steffen Klassert , Stephen Hemminger , Stephen Smalley , Tadeusz Struk , Takashi Iwai , Tom Rix , Udipto Goswami , Vincenzo Frascino , wcn36xx@lists.infradead.org, Wei Liu , xen-devel@lists.xenproject.org, Xiu Jianfeng , Yang Yingliang Subject: Re: [PATCH 03/32] flex_array: Add Kunit tests Message-ID: <202205041220.4BAF15F6B4@keescook> References: <20220504014440.3697851-1-keescook@chromium.org> <20220504014440.3697851-4-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-hardening@vger.kernel.org On Wed, May 04, 2022 at 11:00:38AM +0800, David Gow wrote: > On Wed, May 4, 2022 at 9:47 AM Kees Cook wrote: > > > > Add tests for the new flexible array structure helpers. These can be run > > with: > > > > make ARCH=um mrproper > > ./tools/testing/kunit/kunit.py config > > Nit: it shouldn't be necessary to run kunit.py config separately: > kunit.py run will configure the kernel if necessary. Ah yes, I think you mentioned this before. I'll adjust the commit log. > > > ./tools/testing/kunit/kunit.py run flex_array > > > > Cc: David Gow > > Cc: kunit-dev@googlegroups.com > > Signed-off-by: Kees Cook > > --- > > This looks pretty good to me: it certainly worked on the different > setups I tried (um, x86_64, x86_64+KASAN). > > A few minor nitpicks inline, mostly around minor config-y things, or > things which weren't totally clear on my first read-through. > > Hopefully one day, with the various stubbing features or something > similar, we'll be able to check against allocation failures in > flex_dup(), too, but otherwise nothing seems too obviously missing. > > Reviewed-by: David Gow Great; thanks for the review and testing! > > -- David > > > lib/Kconfig.debug | 12 +- > > lib/Makefile | 1 + > > lib/flex_array_kunit.c | 523 +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 531 insertions(+), 5 deletions(-) > > create mode 100644 lib/flex_array_kunit.c > > > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > index 9077bb38bc93..8bae6b169c50 100644 > > --- a/lib/Kconfig.debug > > +++ b/lib/Kconfig.debug > > @@ -2551,11 +2551,6 @@ config OVERFLOW_KUNIT_TEST > > Builds unit tests for the check_*_overflow(), size_*(), allocation, and > > related functions. > > > > - For more information on KUnit and unit tests in general please refer > > - to the KUnit documentation in Documentation/dev-tools/kunit/. > > - > > - If unsure, say N. > > - > > Nit: while I'm not against removing some of this boilerplate, is it > better suited for a separate commit? Make sense, yes. I'll drop this for now. > > > config STACKINIT_KUNIT_TEST > > tristate "Test level of stack variable initialization" if !KUNIT_ALL_TESTS > > depends on KUNIT > > @@ -2567,6 +2562,13 @@ config STACKINIT_KUNIT_TEST > > CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF, > > or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. > > > > +config FLEX_ARRAY_KUNIT_TEST > > + tristate "Test flex_*() family of helper functions at runtime" if !KUNIT_ALL_TESTS > > + depends on KUNIT > > + default KUNIT_ALL_TESTS > > + help > > + Builds unit tests for flexible array copy helper functions. > > + > > Nit: checkpatch warns that the description here may be insufficient: > WARNING: please write a help paragraph that fully describes the config symbol Yeah, I don't know anything to put here that isn't just more boilerplate, so I'm choosing to ignore this for now. :) > > [...] > > +struct normal { > > + size_t datalen; > > + u32 data[]; > > +}; > > + > > +struct decl_normal { > > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, datalen); > > + DECLARE_FLEX_ARRAY_ELEMENTS(u32, data); > > +}; > > + > > +struct aligned { > > + unsigned short datalen; > > + char data[] __aligned(__alignof__(u64)); > > +}; > > + > > +struct decl_aligned { > > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(unsigned short, datalen); > > + DECLARE_FLEX_ARRAY_ELEMENTS(char, data) __aligned(__alignof__(u64)); > > +}; > > + > > +static void struct_test(struct kunit *test) > > +{ > > + COMPARE_STRUCTS(struct normal, struct decl_normal); > > + COMPARE_STRUCTS(struct aligned, struct decl_aligned); > > +} > > If I understand it, the purpose of this is to ensure that structs both > with and without the flexible array declaration have the same memory > layout? > > If so, any chance of a comment briefly stating that's the purpose (or > renaming this test struct_layout_test())? Yeah, good idea; I'll improve the naming. > > Also, would it make sense to do the same with the struct with internal > padding below? Heh, yes, good point! :) > [...] > > +#define CHECK_COPY(ptr) do { \ > > + typeof(*(ptr)) *_cc_dst = (ptr); \ > > + KUNIT_EXPECT_EQ(test, _cc_dst->induce_padding, 0); \ > > + memcpy(&padding, &_cc_dst->induce_padding + sizeof(_cc_dst->induce_padding), \ > > + sizeof(padding)); \ > > + /* Padding should be zero too. */ \ > > + KUNIT_EXPECT_EQ(test, padding, 0); \ > > + KUNIT_EXPECT_EQ(test, src->count, _cc_dst->count); \ > > + KUNIT_EXPECT_EQ(test, _cc_dst->count, TEST_TARGET); \ > > + for (i = 0; i < _cc_dst->count - 1; i++) { \ > > + /* 'A' is 0x41, and here repeated in a u32. */ \ > > Would it be simpler to just note that the magic value is 0x41, rather > than have it be the character 'A'? Yeah, now fixed. > [...] > > + CHECK_COPY(&encap->fas); > > + /* Check that items external to "fas" are zero. */ > > + KUNIT_EXPECT_EQ(test, encap->flags, 0); > > + KUNIT_EXPECT_EQ(test, encap->junk, 0); > > + kfree(encap); > > +#undef MAGIC_WORD > > MAGIC_WORD isn't defined (or used) for flux_dup_test? Is it worth > using it (or something similar) for the 'A' / 0x14141414 and the > CHECK_COPY() macro? Oops, yes. Fixed. Thanks again! -Kees -- Kees Cook