mbox series

[v2,0/2] Fix get a wrong pci host bridge in cxl_setup_parent_dport()

Message ID 20240809082750.3015641-1-ming4.li@intel.com
Headers show
Series Fix get a wrong pci host bridge in cxl_setup_parent_dport() | expand

Message

Li, Ming4 Aug. 9, 2024, 8:27 a.m. UTC
The cxl_test unit test environment on qemu always hit below call trace
with KASAN enabled:

 BUG: KASAN: slab-out-of-bounds in cxl_setup_parent_dport+0x480/0x530 [cxl_core]
 Read of size 1 at addr ff110000676014f8 by task (udev-worker)/676[   24.424403] CPU: 2 PID: 676 Comm: (udev-worker) Tainted: G           O     N 6.10.0-qemucxl #1
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20240214-2.el9 02/14/2024
 Call Trace:
  <TASK>
  dump_stack_lvl+0xea/0x150
  print_report+0xce/0x610
  ? kasan_complete_mode_report_info+0x40/0x200
  kasan_report+0xcc/0x110
  __asan_report_load1_noabort+0x18/0x20
  cxl_setup_parent_dport+0x480/0x530 [cxl_core]
  cxl_mem_probe+0x49b/0xaa0 [cxl_mem]

The root cause is that a wrong host bridge was gotten from
dport->dport_dev in cxl_setup_parent_dport(). In
cxl_setup_parent_dport(), it always calls
to_pci_host_bridge(dport->dport_dev) to get a pci_host_bridge structure.
There are two issues in the implementation:

* to_pci_host_bridge(dport->dport_dev) should be used only for RCH
  cases, dport->dport_dev points to a pci device rather than a pci host
  bridge in VH cases. The solution is checking if dport is in RCH mode
  then calling to_pci_host_bridge().
  (Patch 1)

* In cxl_test unit test environment, cxl_test will create a emulated CXL
  topology with emulated dports, the dport_dev of a emulated dport
  points to a platform device. to_pci_host_bridge(dport->dport_dev) also
  gets a wrong pci host bridge in the case. The solution is implementing
  a new wrap function called __wrap_cxl_setup_parent_dport() on cxl_test
  side, the function will filter all emulated dports, make sure only
  real dports can be handled by cxl_setup_parent_dport().
  (Patch 2)

v1 link: https://lore.kernel.org/linux-cxl/ZrHR+0w3bwM1Ik8h@xpf.sh.intel.com/T/#med6200e54ec12f09fdcc04571516adda261c9561

v2:
- Add call trace log into changelog
- Remove 'dev_is_platform(dport->dport_dev)' checking out of cxl driver
  scope. Check if dport is emulated in cxl_test.
  

Li Ming (2):
  cxl/pci: Get AER capability address from RCRB only for RCH dport
  cxl/test: Skip cxl_setup_parent_dport() for emulated dports

 drivers/cxl/core/pci.c        | 10 ++++++----
 tools/testing/cxl/Kbuild      |  1 +
 tools/testing/cxl/test/mock.c | 12 ++++++++++++
 3 files changed, 19 insertions(+), 4 deletions(-)

Comments

Dan Williams Aug. 9, 2024, 9:36 p.m. UTC | #1
Li Ming wrote:
> The cxl_test unit test environment on qemu always hit below call trace
> with KASAN enabled:
> 
>  BUG: KASAN: slab-out-of-bounds in cxl_setup_parent_dport+0x480/0x530 [cxl_core]
>  Read of size 1 at addr ff110000676014f8 by task (udev-worker)/676[   24.424403] CPU: 2 PID: 676 Comm: (udev-worker) Tainted: G           O     N 6.10.0-qemucxl #1
>  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20240214-2.el9 02/14/2024
>  Call Trace:
>   <TASK>
>   dump_stack_lvl+0xea/0x150
>   print_report+0xce/0x610
>   ? kasan_complete_mode_report_info+0x40/0x200
>   kasan_report+0xcc/0x110
>   __asan_report_load1_noabort+0x18/0x20
>   cxl_setup_parent_dport+0x480/0x530 [cxl_core]
>   cxl_mem_probe+0x49b/0xaa0 [cxl_mem]
> 
> The root cause is that a wrong host bridge was gotten from
> dport->dport_dev in cxl_setup_parent_dport(). In
> cxl_setup_parent_dport(), it always calls
> to_pci_host_bridge(dport->dport_dev) to get a pci_host_bridge structure.
> There are two issues in the implementation:
> 
> * to_pci_host_bridge(dport->dport_dev) should be used only for RCH
>   cases, dport->dport_dev points to a pci device rather than a pci host
>   bridge in VH cases. The solution is checking if dport is in RCH mode
>   then calling to_pci_host_bridge().
>   (Patch 1)
> 
> * In cxl_test unit test environment, cxl_test will create a emulated CXL
>   topology with emulated dports, the dport_dev of a emulated dport
>   points to a platform device. to_pci_host_bridge(dport->dport_dev) also
>   gets a wrong pci host bridge in the case. The solution is implementing
>   a new wrap function called __wrap_cxl_setup_parent_dport() on cxl_test
>   side, the function will filter all emulated dports, make sure only
>   real dports can be handled by cxl_setup_parent_dport().
>   (Patch 2)
> 
> v1 link: https://lore.kernel.org/linux-cxl/ZrHR+0w3bwM1Ik8h@xpf.sh.intel.com/T/#med6200e54ec12f09fdcc04571516adda261c9561
> 
> v2:
> - Add call trace log into changelog
> - Remove 'dev_is_platform(dport->dport_dev)' checking out of cxl driver
>   scope. Check if dport is emulated in cxl_test.

I am suprised we got this far without this being a problem earlier.

For the series you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

...can you also follow along with a rename, documentation, and cleanup
patch?  cxl_setup_parent_dport() tells the reader absolutely nothing
about what the function does, and that it is specifically initializing
PCI AER operation.

It should be called something like cxl_dport_init_aer(), and it should
have kernel-doc associated with it. The cleanup opportunity I see is to
consolidate the ->native_aer check for the cxl_rcrb_to_aer() and
cxl_disable_rch_root_ints() into one location. It is a bit silly that
cxl_disable_rch_root_ints() needs to check if dport->regs.dport_aer was
initialized when it was just setup a few lines above. I.e. there are
just too many helper functions and it all could be consolidated in a
bigger cxl_dport_init_aer() function.

> 
> Li Ming (2):
>   cxl/pci: Get AER capability address from RCRB only for RCH dport
>   cxl/test: Skip cxl_setup_parent_dport() for emulated dports

This is the right thing to do whenever possible. The usage of
dev_is_platform() only makes sense for scenarios where a function is
called by the cxl_core that needs to avoid getting confused by cxl_test.

In this case, since cxl_mem is making the call to the exported
cxl_setup_parent_dport() symbol, then mocking it for cxl_test is
appropriate.
Ira Weiny Aug. 9, 2024, 9:44 p.m. UTC | #2
Dan Williams wrote:
> Li Ming wrote:
> > The cxl_test unit test environment on qemu always hit below call trace
> > with KASAN enabled:
> > 
> >  BUG: KASAN: slab-out-of-bounds in cxl_setup_parent_dport+0x480/0x530 [cxl_core]
> >  Read of size 1 at addr ff110000676014f8 by task (udev-worker)/676[   24.424403] CPU: 2 PID: 676 Comm: (udev-worker) Tainted: G           O     N 6.10.0-qemucxl #1
> >  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20240214-2.el9 02/14/2024
> >  Call Trace:
> >   <TASK>
> >   dump_stack_lvl+0xea/0x150
> >   print_report+0xce/0x610
> >   ? kasan_complete_mode_report_info+0x40/0x200
> >   kasan_report+0xcc/0x110
> >   __asan_report_load1_noabort+0x18/0x20
> >   cxl_setup_parent_dport+0x480/0x530 [cxl_core]
> >   cxl_mem_probe+0x49b/0xaa0 [cxl_mem]
> > 
> > The root cause is that a wrong host bridge was gotten from
> > dport->dport_dev in cxl_setup_parent_dport(). In
> > cxl_setup_parent_dport(), it always calls
> > to_pci_host_bridge(dport->dport_dev) to get a pci_host_bridge structure.
> > There are two issues in the implementation:
> > 
> > * to_pci_host_bridge(dport->dport_dev) should be used only for RCH
> >   cases, dport->dport_dev points to a pci device rather than a pci host
> >   bridge in VH cases. The solution is checking if dport is in RCH mode
> >   then calling to_pci_host_bridge().
> >   (Patch 1)
> > 
> > * In cxl_test unit test environment, cxl_test will create a emulated CXL
> >   topology with emulated dports, the dport_dev of a emulated dport
> >   points to a platform device. to_pci_host_bridge(dport->dport_dev) also
> >   gets a wrong pci host bridge in the case. The solution is implementing
> >   a new wrap function called __wrap_cxl_setup_parent_dport() on cxl_test
> >   side, the function will filter all emulated dports, make sure only
> >   real dports can be handled by cxl_setup_parent_dport().
> >   (Patch 2)
> > 
> > v1 link: https://lore.kernel.org/linux-cxl/ZrHR+0w3bwM1Ik8h@xpf.sh.intel.com/T/#med6200e54ec12f09fdcc04571516adda261c9561
> > 
> > v2:
> > - Add call trace log into changelog
> > - Remove 'dev_is_platform(dport->dport_dev)' checking out of cxl driver
> >   scope. Check if dport is emulated in cxl_test.
> 
> I am suprised we got this far without this being a problem earlier.
> 
> For the series you can add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

This fixes some of the issues I was seeing in the cxl-test failures.  So
I'm going to tag this too.

That said, on patch 1 I would like to see further clean up.  Diging into
the details of the dport->rcrb.aer_cap usage this check and code should be
moved to cxl_dport_map_rch_aer() and make aer_cap a local and remove
struct cxl_rcrb_info completely.

For now, for the series.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> 
> ...can you also follow along with a rename, documentation, and cleanup
> patch?  cxl_setup_parent_dport() tells the reader absolutely nothing
> about what the function does, and that it is specifically initializing
> PCI AER operation.
> 
> It should be called something like cxl_dport_init_aer(), and it should
> have kernel-doc associated with it. The cleanup opportunity I see is to
> consolidate the ->native_aer check for the cxl_rcrb_to_aer() and
> cxl_disable_rch_root_ints() into one location. It is a bit silly that
> cxl_disable_rch_root_ints() needs to check if dport->regs.dport_aer was
> initialized when it was just setup a few lines above. I.e. there are
> just too many helper functions and it all could be consolidated in a
> bigger cxl_dport_init_aer() function.
> 
> > 
> > Li Ming (2):
> >   cxl/pci: Get AER capability address from RCRB only for RCH dport
> >   cxl/test: Skip cxl_setup_parent_dport() for emulated dports
> 
> This is the right thing to do whenever possible. The usage of
> dev_is_platform() only makes sense for scenarios where a function is
> called by the cxl_core that needs to avoid getting confused by cxl_test.
> 
> In this case, since cxl_mem is making the call to the exported
> cxl_setup_parent_dport() symbol, then mocking it for cxl_test is
> appropriate.
>
Li, Ming4 Aug. 12, 2024, 6:49 a.m. UTC | #3
On 8/10/2024 5:36 AM, Dan Williams wrote:
> Li Ming wrote:
>> The cxl_test unit test environment on qemu always hit below call trace
>> with KASAN enabled:
>>
>>  BUG: KASAN: slab-out-of-bounds in cxl_setup_parent_dport+0x480/0x530 [cxl_core]
>>  Read of size 1 at addr ff110000676014f8 by task (udev-worker)/676[   24.424403] CPU: 2 PID: 676 Comm: (udev-worker) Tainted: G           O     N 6.10.0-qemucxl #1
>>  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20240214-2.el9 02/14/2024
>>  Call Trace:
>>   <TASK>
>>   dump_stack_lvl+0xea/0x150
>>   print_report+0xce/0x610
>>   ? kasan_complete_mode_report_info+0x40/0x200
>>   kasan_report+0xcc/0x110
>>   __asan_report_load1_noabort+0x18/0x20
>>   cxl_setup_parent_dport+0x480/0x530 [cxl_core]
>>   cxl_mem_probe+0x49b/0xaa0 [cxl_mem]
>>
>> The root cause is that a wrong host bridge was gotten from
>> dport->dport_dev in cxl_setup_parent_dport(). In
>> cxl_setup_parent_dport(), it always calls
>> to_pci_host_bridge(dport->dport_dev) to get a pci_host_bridge structure.
>> There are two issues in the implementation:
>>
>> * to_pci_host_bridge(dport->dport_dev) should be used only for RCH
>>   cases, dport->dport_dev points to a pci device rather than a pci host
>>   bridge in VH cases. The solution is checking if dport is in RCH mode
>>   then calling to_pci_host_bridge().
>>   (Patch 1)
>>
>> * In cxl_test unit test environment, cxl_test will create a emulated CXL
>>   topology with emulated dports, the dport_dev of a emulated dport
>>   points to a platform device. to_pci_host_bridge(dport->dport_dev) also
>>   gets a wrong pci host bridge in the case. The solution is implementing
>>   a new wrap function called __wrap_cxl_setup_parent_dport() on cxl_test
>>   side, the function will filter all emulated dports, make sure only
>>   real dports can be handled by cxl_setup_parent_dport().
>>   (Patch 2)
>>
>> v1 link: https://lore.kernel.org/linux-cxl/ZrHR+0w3bwM1Ik8h@xpf.sh.intel.com/T/#med6200e54ec12f09fdcc04571516adda261c9561
>>
>> v2:
>> - Add call trace log into changelog
>> - Remove 'dev_is_platform(dport->dport_dev)' checking out of cxl driver
>>   scope. Check if dport is emulated in cxl_test.
> I am suprised we got this far without this being a problem earlier.
>
> For the series you can add:
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>
> ...can you also follow along with a rename, documentation, and cleanup
> patch?  cxl_setup_parent_dport() tells the reader absolutely nothing
> about what the function does, and that it is specifically initializing
> PCI AER operation.
>
> It should be called something like cxl_dport_init_aer(), and it should
> have kernel-doc associated with it. The cleanup opportunity I see is to
> consolidate the ->native_aer check for the cxl_rcrb_to_aer() and
> cxl_disable_rch_root_ints() into one location. It is a bit silly that
> cxl_disable_rch_root_ints() needs to check if dport->regs.dport_aer was
> initialized when it was just setup a few lines above. I.e. there are
> just too many helper functions and it all could be consolidated in a
> bigger cxl_dport_init_aer() function.

Sure, I will do that, thanks for review.


>
>> Li Ming (2):
>>   cxl/pci: Get AER capability address from RCRB only for RCH dport
>>   cxl/test: Skip cxl_setup_parent_dport() for emulated dports
> This is the right thing to do whenever possible. The usage of
> dev_is_platform() only makes sense for scenarios where a function is
> called by the cxl_core that needs to avoid getting confused by cxl_test.
>
> In this case, since cxl_mem is making the call to the exported
> cxl_setup_parent_dport() symbol, then mocking it for cxl_test is
> appropriate.