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 17AEFC19F2C for ; Wed, 3 Aug 2022 20:31:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238706AbiHCUbx (ORCPT ); Wed, 3 Aug 2022 16:31:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41976 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237057AbiHCUbw (ORCPT ); Wed, 3 Aug 2022 16:31:52 -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 32D15A189 for ; Wed, 3 Aug 2022 13:31:50 -0700 (PDT) Received: by mail-ej1-x635.google.com with SMTP id kb8so18861949ejc.4 for ; Wed, 03 Aug 2022 13:31:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=n8pKImfx8Za6eHn+m1TtvWVUUWC/l25+Sr2AOODiNHU=; b=S31AfcWvWmUKKe89ihAv8h9HB4X+QuSLQ50vKDCY7bJY0i4bFO98tMAnEmfu1BNYP7 HD41b/1p9GTXLpsG5g7oypfQ5V41+OnhOO6bQeLQwFz6CQ6l3DcDIPK+BWYiH6vKDsxw pm9/F+Cln/OOqur/QEVVQ8ksN2jPXycWsM2FamD2e4i/uaQMvu1ObPykgp5+il8v3ZUn GTOFDsaX+ejctGxg3328WnxSKRDzj4NHkNFYcISP14GdXSBzaEmlfU6cXMCKCV4aQS5x /ImLT40tAOGcKH8kwDjTCvcSRJIHYByQfBV1y0oenW06d/+gzZ0xI6xPRE0VpQARD2/8 JY8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=n8pKImfx8Za6eHn+m1TtvWVUUWC/l25+Sr2AOODiNHU=; b=3CNXrBbHvDKsx0YXTAlCyQtavhMHkZRnZGjW5g+6+K0cNpYdHjptD+eYDsaJSSrDqh 5a+/aItbtc8hKCdjwPolDcT2Gdfw9odVN/QgJW3J+hQHXqKrDcKZc/xZ6sLRt0bV9B50 rWN864BGsBxXP6xRxHztoQUKSr6x7Js5m2nGfxrc3koBjyGg9r2BnG1oppApbpSXuPZH alzWt9wS6UwPIWZh1g6FegvtjurnlKMzEgKY+zcPEk+mFOzx6HF1FyaaNDWTdkAARwGj TLG4QDEk1XyruopWv/KMMYH25mouO1Ai8xLEeA9rwXikivYUFe584kx5N9yVOXn5BlFK n62w== X-Gm-Message-State: ACgBeo2K8N8w/1YWwbFapBp3kkvYImmWx1DClbUFXxbGa5rj6slLbFP8 4HAI6Gpik1MrBB/76xHsJZW50BlTADU3qnZieFheXw== X-Google-Smtp-Source: AA6agR6xltbU2DJGqBal3KX71f6EjA835Dlm+EK4mwQrI6M78ca2wmVSAssvbra/rDYa4OqgVKCEfF88QL3JIh6NQ+w= X-Received: by 2002:a17:907:9726:b0:730:9e04:f738 with SMTP id jg38-20020a170907972600b007309e04f738mr8421050ejc.631.1659558708460; Wed, 03 Aug 2022 13:31:48 -0700 (PDT) MIME-Version: 1.0 References: <20220713005221.1926290-1-davidgow@google.com> In-Reply-To: From: Brendan Higgins Date: Wed, 3 Aug 2022 16:31:36 -0400 Message-ID: Subject: Re: [PATCH] module: kunit: Load .kunit_test_suites section when CONFIG_KUNIT=m To: David Gow Cc: Luis Chamberlain , Daniel Latypov , Shuah Khan , Jeremy Kerr , linux-modules@vger.kernel.org, KUnit Development , "open list:KERNEL SELFTEST FRAMEWORK" , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: On Thu, Jul 28, 2022 at 4:42 AM David Gow wrote: > > On Thu, Jul 21, 2022 at 4:29 AM Luis Chamberlain wrote: > > > > On Wed, Jul 20, 2022 at 05:26:02PM +0800, David Gow wrote: > > > On Wed, Jul 20, 2022 at 1:58 AM Luis Chamberlain wrote: > > > > > > > > On Wed, Jul 13, 2022 at 08:24:32AM -0700, Daniel Latypov wrote: > > > > > On Tue, Jul 12, 2022 at 5:52 PM David Gow wrote: > > > > > > > > > > > > The new KUnit module handling has KUnit test suites listed in a > > > > > > .kunit_test_suites section of each module. This should be loaded when > > > > > > the module is, but at the moment this only happens if KUnit is built-in. > > > > > > > > > > > > Also load this when KUnit is enabled as a module: it'll not be usable > > > > > > unless KUnit is loaded, but such modules are likely to depend on KUnit > > > > > > anyway, so it's unlikely to ever be loaded needlessly. > > > > > > > > > > This seems reasonable to me. > > > > > > > > > > Question: what happens in this case? > > > > > 1. insmod > > > > > 2. insmod kunit > > > > > 3. rmmod > > > > > > > > > > I think on 3, we'll call the cleanup code, __kunit_test_suites_exit(), > > > > > for , I think? > > > > > But we never called __kunit_test_suites_init(). > > > > > My fear is what breaks as a result of this precondition break. > > > > > > I don't think this should be possible: any module with KUnit tests > > > will depend on the 'kunit' module (or, at least, kunit symbols), so > > > shouldn't load without kunit already present. > > > > > > If modprobe is used, kunit will automatically be loaded. If insmod is > > > used directly, loading the first module should error out with > > > something like: > > > [ 82.393629] list_test: loading test module taints kernel. > > > [ 82.409607] list_test: Unknown symbol kunit_binary_ptr_assert_format (err -2) > > > [ 82.409657] list_test: Unknown symbol kunit_do_failed_assertion (err -2) > > > [ 82.409799] list_test: Unknown symbol kunit_binary_assert_format (err -2) > > > [ 82.409820] list_test: Unknown symbol kunit_unary_assert_format (err -2) > > > insmod: ERROR: could not insert module > > > /lib/modules/5.19.0-rc1-15284-g9ec67db0c271/kernel/lib/list-test.ko: > > > Unknown symbol in module > > > > This can be fixed with a request_module() call. And since this is a > > generic requirement, you can have the wrappers do it for you. > > > > I'm not convinced that this is worth the trouble, particularly since > KUnit needs to be loaded already before any test-specific code in a > module is run. _Maybe_ we could put it in the code which looks for the > .kunit_test_suites section, but even then it seems like a bit of an > ugly hack. > > Personally, I'm not particularly concerned about test modules failing > to load if KUnit isn't already present -- if people want all of a > module's dependencies loaded, that's what modprobe is for. > > That being said, if you feel particularly strongly about it, this is > something we can look at. Let's do so in a separate patch though: this > one does fix a regression as-is. I agree. We need a fix for 3d6e44623841 to go in for 5.20 - we've gotten several complaints about it. This patch seems to accomplish that. I am not an expert on the module stuff by any means, so I am absolutely open to continuing this discussion in a follow-up patch, but I think we need this fix now. If no one objects, I am going to ask Shuah to take this patch. > > > Maybe you could get into some trouble by force-removing modules at > > > various points, but you're in undefined behaviour generally at that > > > point, so I don't think there's much point going out-of-our-way to try > > > to support that. > > > > You can prevent that by refcounting the kunit module / symbols, by each test. > > > > Again, I don't think KUnit is any more special than any other module > here. I don't think we need to do this ourselves, as it shouldn't be > possible to remove kunit without first removing any dependent modules. > > Of course, happy to look into this again if anyone can come up with an > actual crash, but I'd rather get this fix in first. At the very least, > this patch shouldn't introduce any _new_ issues. Sounds good. I will send my Reviewed-by in a separate email, as per usual. Cheers