Message-ID: <639a6e9c6da4e_b41e3294d8@dwillia2-xfh.jf.intel.com.notmuch>
Date:   Wed, 14 Dec 2022 16:47:24 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     Luis Chamberlain <mcgrof@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Lucas De Marchi <lucas.de.marchi@...il.com>
CC:     <alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
        <ira.weiny@...el.com>, <bwidawsk@...nel.org>, <dave@...olabs.net>,
        <a.manzanares@...sung.com>, <linux-cxl@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cxl/acpi: fix null dereference on probe for missing
 ACPI_COMPANION()

Luis Chamberlain wrote:
> On Tue, Dec 13, 2022 at 04:54:32PM -0800, Dan Williams wrote:
> > Luis Chamberlain wrote:
> > > On Fri, Dec 09, 2022 at 01:07:53PM -0800, Dan Williams wrote:
> > > > Dan Williams wrote:
> > > > > Luis Chamberlain wrote:
> > > > > > Simply loading cxl_test ends up triggering a null pointer dereference
> > > > > > on next-20221207, 
> > > >
> > > > Ok, my 6.1.0-rc8-next-20221208 build passed.
> > > > 
> > > > # meson test -C build --suite cxl
> > > > ninja: Entering directory `/root/git/ndctl/build'
> > > > [109/109] Linking target ndctl/ndctl
> > > > 1/5 ndctl:cxl / cxl-topology.sh             OK              11.84s
> > > > 2/5 ndctl:cxl / cxl-region-sysfs.sh         OK               6.82s
> > > > 3/5 ndctl:cxl / cxl-labels.sh               OK              10.14s
> > > > 4/5 ndctl:cxl / cxl-create-region.sh        OK              18.32s
> > > > 5/5 ndctl:cxl / security-cxl.sh             OK               3.35s
> > > 
> > > What branch of ndctl do you use?
> > 
> > Yeah, somewhat unfair of me to say "works for me" with patches that had
> > not yet been posted to the list. That's fixed now with this posting:
> > 
> > http://lore.kernel.org/r/167097752151.1189953.3189708700022130101.stgit@dwillia2-xfh.jf.intel.com
> 
> I b4 am'd this after my branch reset to the latest pending today.
> 
> > ...and this one that is now on the pending branch:
> > 
> > http://lore.kernel.org/r/167053487710.582963.17616889985000817682.stgit@dwillia2-xfh.jf.intel.com
> 
> Groovy thanks. Progress, but still fails now on the cxl-xor-region.sh line 29
> 
> https://gist.github.com/mcgrof/aeac639365a651bd77f143cf38eb7493

This is also good news in that we are finding integration issues early
as I did not have this in my local tree previously. Will take a look,
although I think Alison and Vishal might beat me to it.

> 
> > > > So, what I suspect is happening is
> > > 
> > > <-- snip -->
> > > 
> > > Yes you're right.
> > > 
> > > > From 93bf2c04cd3a708c73c0e4ad7a4121505a0698da Mon Sep 17 00:00:00 2001
> > > > From: Dan Williams <dan.j.williams@...el.com>
> > > > Date: Fri, 9 Dec 2022 13:04:26 -0800
> > > > Subject: [PATCH] tools/testing/cxl: Prevent cxl_test from confusing production
> > > >  modules
> > > > 
> > > > The cxl_test machinery builds modified versions of the modules in
> > > > drivers/cxl/ and intercepts some of their calls to allow cxl_test to
> > > > inject mock CXL topologies for test.
> > > > 
> > > > However, if cxl_test attempts the same with production modules,
> > > > fireworks ensue as Luis discovered [1]. Prevent that scenario by
> > > > arranging for cxl_test to check for a "watermark" symbol in each of the
> > > > modules it expects to be modified before the test can run. This turns
> > > > undefined runtime behavior or crashes into a safer failure to load the
> > > > cxl_test module.
> > > > 
> > > > Link: http://lore.kernel.org/r/20221209062919.1096779-1-mcgrof@kernel.org [1]
> > > > Reported-by: Luis Chamberlain <mcgrof@...nel.org>
> > > > Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> > > 
> > > Indeed that fixes the same crash. However that highlights a few other
> > > issues.
> > > 
> > > 1) ndcl unit tests still fail once you do get the right driver loaded:
> > >    a) pending branch fails on the first test and stops there
> > >    b) main branch fails at the first test and continues and passes on
> > >    the rest of the tests. What is with the discrepancy?
> > > 
> > > 2) The instructions on ndctl to use an external module are easily
> > > misguiding folks on how to use external replacement modules, I'd like
> > > to suggest a fix below.
> > > 
> > > Details below.
> > [..]
> > > 
> > > Anyway I think we should simplify the ndctl README.md to just use
> > > INSTALL_MOD_DIR follows, thoughts?
> > 
> > Hmm, but scripts/Makefile.modinst already has this:
> > 
> > INSTALL_MOD_DIR ?= extra
> > 
> > ...are you not getting these modules deployed in the "$modules/extra"
> > directory, or is your distro not priortizing modules in that directory
> > over the others? Fedora seems to have this policy by default, but it
> > seems at least Ubuntu does not. That's what led to the discussion of a
> > modprobe "override" policy in the Troubleshooting section of the README. 
> 
> I figured this was the assumption behind made. I'll very well *aware* of
> this situation, and no, developers *should not* assume everyone pegs
> "extra" with for example a /etc/depmod.d/dist.conf with:
> 
> search updates extra built-in weak-updates
> 
> That is a distribution specific policy. The *updates* however *is*
> picked up by upstream kmod *by default*, and so stuffing things into
> extra just ensures ensures folks don't break things now by using extra...
> unless they are on these distros that do use that...
> 
> But it was *only* redhat derivatives that used it. Fedora's cloud
> vagrant image *does* not use it. And no developer should assume this
> generally for all Linux distributions.
> 
> The *real* sensible thing to do would be to be very explicit about this
> and have say:
> 
> /etc/depmod.d/cxl-mock.conf
> search mock built-in
> 
> *But* then that would mean ndctl would have to carry installing this
> *if* CXL tests are enabled. That also leave folks *not* using ndctl
> without any clue about this issue and so if we wanted to just be super
> lazy, we just use updates as that is built-in to the C code for depmod.c
> on kmod.git *ifff* the distribution lacked *any* depmod.d file:
> 
> https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/tree/tools/depmod.c#n867
> 
> This has been there since the start on kmods since 2012. It's good
> enough for me, as old tools likely would not be used for CXL capable
> systems. Except maybe Android.
> 
> And so using "INSTALL_MOD_DIR=updates" seems better than having us get
> ndctl into the business of installing a /etc/depmod.d/cxl-mock.conf.

Makes sense, and acked your change.

> 
> This would also allow experimentations to see if the cxl_test mock
> driver strategy could be replaced by a proper upstream kernel sefltests
> driver.

Yes, I learned about this approach from this article:

https://lwn.net/Articles/558106/

...but that pre-dated KUnit. However, KUnit does more traditional per
function unit testing whereas nfit_test and cxl_test are quick and dirty
emulation. They are more for testing kernel-user-ABI than kernel
internals, but kernel-internal testing comes along for that ride as a
side effect.

> 
> > My concern with changing the INSTALL_MOD_PATH recommendation is whether
> > having modules in "$modules/updates" is guaranteed to work in all
> > distros. Otherwise we're just shuffling which distros need a custom
> > override workaround.
> 
> All Linux distributions without *any* depmod.d file will work that use kmod.
> Most Linux distributions has phased old module-utils for kmod a long time ago,
> 11 ago, the code for the old crap module-init-toolls hasn't been updated
> for 11 years and specifically mentioned being replaced with libkmod
> usage which is how kmod got started.
> 
> https://git.kernel.org/pub/scm/utils/kernel/module-init-tools/module-init-tools.git/
> 
> This is enough justification for me to simply replace the upstream
> scripts/Makefile.modinst to default to updates instead of extra as well 
> so I'll go ahead and just do that now and CC the list on the patch.

Thanks for that.

> 
> > > But this also raises the question of *if* using ndctl and linux-next
> > > shoudl one use the main branch or the pending branch? Can there be
> > > issues with synchronizing ? Or should the main branch always work,
> > > and the pending should just have the latest and greatest and *can*
> > > fail?
> > 
> > This falls into the category of a "good problem to have" in the sense
> > that I never had to worry before about others wanting to reproduce unit
> > test results this early in the dev cycle, so I appreciate the nudge
> > here.
> 
> Oh groovy, glad this is a step forward then :)
> 
> I know folks have been using run-qemu for a while and have mentioned how
> it is still a bit difficult to ramp up with CXL, and so the goal here was
> reduce barrier to entry even further by automating the hell out of the
> entire process with modern automation tooling. Since kdevops did that
> well for complex testing such as filesystem testing and block layer
> testing I figured it'd be a good match to bring parity with CXL.
> 
> So this also should now enable folks who want to say, run tests with
> qemu specific topologies too. It'd just be another guest on when
> spawing, and so tests can be run in parallel for each different
> topology.

cxl_test is good for testing ABI, but not hardware interfaces. That's
where QEMU shines and so they will continue to compliment each other.
Any automation that helps people get from zero to testing their CXL
patches is appreciated.

> 
> > As to what to do about it, I am open to suggestions. The typical flow
> > has some lag between upstream tools/testing/cxl/ changes and when the
> > corresponding ndctl/cxl changes land. This is because the tool enabling
> > does not start until after it is clear that the kernel changes are going
> > to land. After that it's another round of review to settle on the tool
> > changes. The ndctl/pending branch should usually be up to date by the
> > time -rc1 arrives. Is that sufficient?
> 
> I think it would make sense to always assume that using the latest
> tooling will not break anything and we should then be able to always
> rely on it for all automationf or bleeding edge linux-next based
> testing.

Latest tools should never break, but that's different that saying that
tests should not break since tests are hard coded to the topology. Maybe
the kernel could publish a version number that increments whenever it
makes a change that breaks existing tests. Unless then you can notice
that the failure is due to out-of-date tests.

At the same time I like the flexibility of new tests brewing alongside
kernel patches and then taking the time to review the new test code
while the kernel patches move ahead.

> Tests however should then always look to see if a feature is available
> and gracefully skip if not available / supported by the target kernel
> being tested. This would then allow the *latest* ndctl and pending
> branch to be relied on to test also even older kernels. That way, new
> tests, but which can be useful for testing old kernels, can *also*
> be enabled to test things on those old kernels.

The tests are backwards compatible for older kernels. It's the older
tests getting confused on newer kernels that is the problem.

> And so the latest testing userspace scripts should *always* work with
> *any* kernel. It should not assume you have the latest and greatest
> respective cxl_test driver. Is that something we can count on today?

There will always be the tension that the user tooling not move ahead
until it is clear that the kernel is moving ahead. So cxl_test changes that
cause kernel regressions will always hit -next before the next tool
release.

> Then as a developer, one can assume / expect the latest and greatest
> both tooling / testing will work regardless of the kernel.
> 
> That would mimic the experience with using tools/testing/selftests ,
> even linux-next selftests are supposed to work on older kernels. The
> respective old selftest modules would just be used if they were enabled
> for the older kernel.

The difference there is that you can check the test in with the kernel
change in the same commit.

> Then, a few questions, but while we're at it:
> 
> Do we have enough experience at this point to say what things should be
> tested with the pair cxl_test / ndctl test suite and say what things
> instead are probably better tested using qemu?

There are some things that QEMU may just never support like RCH
topologies. In general cxl_test is amenable to small hacks to test a
driver corner case that are a much heavier lift on QEMU. On the other
side, there are things that cxl_test will never support like register
emulation. So, I think both are needed and complimentary going forward.

> Are there tests, part of ndctl test suite which likely would be useful
> for non cxl_test.ko usages, but rather run on spawned qemu / bare metal
> CXL enabled systems, so one can just hammer on them?

Yes, and nfit_test has an example of such things in its "destructive"
suite where those try to mutate resources outside of the mock
environment. To date there are no "destructive" CXL tests, but I can
imagine, for example, cxl-create-region.sh being modified to run against
the ACPI.CXL topology instead of cxl_test.

Powered by blists - more mailing lists