lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6716b7b18a1c4_b8f1f294aa@iweiny-mobl.notmuch>
Date: Mon, 21 Oct 2024 15:21:05 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: "Zhijian Li (Fujitsu)" <lizhijian@...itsu.com>, Dan Williams
	<dan.j.williams@...el.com>, Alison Schofield <alison.schofield@...el.com>
CC: "linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>, Davidlohr Bueso
	<dave@...olabs.net>, Jonathan Cameron <jonathan.cameron@...wei.com>, "Dave
 Jiang" <dave.jiang@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, "Ira
 Weiny" <ira.weiny@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Huang, Ying" <ying.huang@...el.com>
Subject: Re: [PATCH] testing/cxl: Fix abused pci_bus_read_config_word() on
 platform device

Zhijian Li (Fujitsu) wrote:
> Hey Dan,
> 
> Thanks for your review...
> 
> I want to confirm with you
> 
> 
> On 18/10/2024 09:20, Dan Williams wrote:
> > Alison Schofield wrote:
> >> On Wed, Oct 16, 2024 at 09:52:13AM +0800, Li Zhijian wrote:
> >>> The cxl_region_shared_upstream_bandwidth_update() in clx_core works on
> >>> PCI/PCIe CXL device only while cxl_test was implemeneted by platform
> >>> device.
> >>>
> >>> Mock a cxl_region_shared_upstream_bandwidth_update() which does nothing
> >>> for cxl_core so that the cxl_test goes well.
> >>>
> >>> Abuse cxl_region_shared_upstream_bandwidth_update() on platform device
> >>> will cause a kernel panic with calltrace:
> >>
> >> snip
> >>
> >>> ---
> >>>   tools/testing/cxl/Kbuild      | 2 ++
> >>>   tools/testing/cxl/mock_cdat.c | 8 ++++++++
> >>>   2 files changed, 10 insertions(+)
> >>>   create mode 100644 tools/testing/cxl/mock_cdat.c
> >>>
> >>> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> >>> index b1256fee3567..ed9f50dee3f5 100644
> >>> --- a/tools/testing/cxl/Kbuild
> >>> +++ b/tools/testing/cxl/Kbuild
> >>> @@ -15,6 +15,7 @@ ldflags-y += --wrap=devm_cxl_add_rch_dport
> >>>   ldflags-y += --wrap=cxl_rcd_component_reg_phys
> >>>   ldflags-y += --wrap=cxl_endpoint_parse_cdat
> >>>   ldflags-y += --wrap=cxl_dport_init_ras_reporting
> >>> +ldflags-y += --wrap=cxl_region_shared_upstream_bandwidth_update
> >>>   
> >>>   DRIVERS := ../../../drivers
> >>>   CXL_SRC := $(DRIVERS)/cxl
> >>> @@ -61,6 +62,7 @@ cxl_core-y += $(CXL_CORE_SRC)/pci.o
> >>>   cxl_core-y += $(CXL_CORE_SRC)/hdm.o
> >>>   cxl_core-y += $(CXL_CORE_SRC)/pmu.o
> >>>   cxl_core-y += $(CXL_CORE_SRC)/cdat.o
> >>> +cxl_core-y += mock_cdat.o
> >>>   cxl_core-$(CONFIG_TRACING) += $(CXL_CORE_SRC)/trace.o
> >>>   cxl_core-$(CONFIG_CXL_REGION) += $(CXL_CORE_SRC)/region.o
> >>>   cxl_core-y += config_check.o
> >>> diff --git a/tools/testing/cxl/mock_cdat.c b/tools/testing/cxl/mock_cdat.c
> >>> new file mode 100644
> >>> index 000000000000..99974153b3f6
> >>> --- /dev/null
> >>> +++ b/tools/testing/cxl/mock_cdat.c
> >>> @@ -0,0 +1,8 @@
> >>> +// SPDX-License-Identifier: GPL-2.0-only
> >>> +/* Copyright (c) 2024 FUJITSU LIMITED. All rights reserved. */
> >>> +
> >>> +#include <cxl.h>
> >>> +
> >>> +void __wrap_cxl_region_shared_upstream_bandwidth_update(struct cxl_region *cxlr)
> >>> +{
> >>> +}
> >>
> >> The addition of file mock_cdat.c made me wonder why this wrapper couldn't join
> >> all the other __wrap's defined in test/mock.c. I tried, as you probably did,
> >> and see the circular dependency. I mention it here in case anyone else has a
> >> way to simplify this.
> > 
> > Yeah, unfortunately symbols can only be mocked across EXPORT_SYMBOL()
> > boundaries, but since the caller of this is internal to the CXL core it
> > is not amenable to the wrap approach.
> > 
> > So, unfortunately what this patch does is break the expectation that
> > cxl_test can live alongside and not regress any production flows. I.e.
> > what this patch does is replace
> > cxl_region_shared_upstream_bandwidth_update() for all use cases, not
> > just platform devices.
> 
> Yes, that's true.
> 
> 
> > 
> > Compare that to tools/testing/cxl/test/mock.c which arranges for all the
> > mocked use cases to call back into the real routines in the real device
> > case.
> > 
> > Given that I think this puts the device type check back in play.
> 
> Just to confirm, do you mean add device type check to drivers/cxl/core/cdat.c
> 
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -5,6 +5,7 @@
>    #include <linux/fw_table.h>
>    #include <linux/node.h>
>    #include <linux/overflow.h>
> +#include <linux/platform_device.h>
>    #include "cxlpci.h"
>    #include "cxlmem.h"
>    #include "core.h"
> @@ -641,9 +642,13 @@ static int cxl_endpoint_gather_bandwidth(struct cxl_region *cxlr,
>           void *ptr;
>           int rc;
> 
> +       if (dev_is_pci(cxlds->dev))

Did you mean...

	if (!dev_is_pci(cxlds->dev))
???

> +               return -ENODEV;
> +
>           if (cxlds->rcd)
>                   return -ENODEV;
>    
> 
> or
> 
> tools/testing/cxl/mock_cdat.c
> 
> #include <cxl.h>
>                                                                                  
> void __wrap_cxl_region_shared_upstream_bandwidth_update(struct cxl_region *cxlr)
> {
>          for (int i = 0; i < cxlr->params.nr_targets; i++) {
>                  struct cxl_endpoint_decoder *cxled = cxlr->params.targets[i];
>                  struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>                  struct cxl_dev_state *cxlds = cxlmd->cxlds;
>                                                                                  
>                  if (!dev_is_pci(cxlds->dev))
>                          return;
>          }
>                                                                                  
>          cxl_region_shared_upstream_bandwidth_update(cxlr);
> }

Couldn't this be done on the endpoint call?  (Making it non-static?)

int __wrap_cxl_endpoint_gather_bandwidth(...
                                         struct cxl_endpoint_decoder *cxled,
					...)
{
        struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
        struct cxl_dev_state *cxlds = cxlmd->cxlds;

	if (!dev_is_pci(cxlds->dev))
		return -ENODEV;

	return cxl_endpoint_gather_bandwidth(...);
}

Ira

> 
> 
> Thanks
> Zhijian
> 
> > 
> > However, instead of checking "dev_is_platform()" check "dev_is_pci()" to
> > be consistent with all the other CXL core internal functions that exit
> > early when passed cxl_test devices.



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ