linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Krzysztof Wilczyński" <kw@linux.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Slivka, Danijel" <Danijel.Slivka@amd.com>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: Re: [PATCH] PCI: Fix accessing freed memory in pci_remove_resource_files
Date: Wed, 26 May 2021 00:54:45 +0200	[thread overview]
Message-ID: <20210525225445.GA108348@rocinante.localdomain> (raw)
In-Reply-To: <20210510225733.GA2307664@bjorn-Precision-5520>

Hello Bjorn and Danijel,

Sorry for late reply!

[...]
> Krzysztof did some really nice work in v5.13-rc1 that removes a lot of
> the explicit sysfs file management.  I think he's planning similar
> work for pdev->res_attr[], and I suspect that will solve this problem
> nicely.  But there are some wrinkles to work out before that's ready.

Yes, the idea is to completely retire the late_initcall() and in the
process the pci_create_resource_files() and pci_create_legacy_files()
that are currently called from the pci_sysfs_init() function.

This will in turn allow for the removal of the "sysfs_initialized"
global variable that was, and most likely still is, responsible for the
race condition we have seen on some platforms, see:

  https://lore.kernel.org/linux-pci/20200716110423.xtfyb3n6tn5ixedh@pali/

> Any ideas here, Krzysztof?  IIRC some of the wrinkles have to do with
> Alpha, which has its own pci_create_resource_files() implementation.

Correct, Alpha support is something we have to address as one of the
outstanding problems to resolve.

To be more precise, there are two problems left to solve before we can
finally retire the late_initcall, and these would be:

  - pci_create_resource_files()
  - pci_create_legacy_files() 

The pci_create_resource_files() can be overridden on certain platforms
through a weak linkage which then allows for alternative implementations
to be provided - and this is true for Alpha as it provides a complete
implementation of the original function with all the platform-specific
changes it requires.

Similarly, an auxiliary function called pci_adjust_legacy_attr() can
also be overridden on some platforms again through a weak linkage
offering a way to adjust properties of some of the sysfs objects to be
added, and Alpha is also the platform that uses this for some custom
adjustments.

Having said that, aside of some dependency Alpha has on the functions we
are trying to retire, both of them have a larger challenge to overcome,
and which is related to the following commits:

- 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region")
- 636b21b50152 ("PCI: Revoke mappings like devmem")

Following these two changes, functions pci_create_attr() (called internally
from the pci_create_resource_files()) and pci_create_legacy_files() have
since a dependency on the iomem_get_mapping() function - which in turn also
creates an implicit dependency on the VFS and fs_initcall.

This dependency on iomem_get_mapping() stops us from retiring the
late_initcall at the moment as when we convert dynamically added sysfs
objects (that are primarily added in the pci_create_resource_files() and
pci_create_legacy_files() functions), these attributes are added before
the VFS completes its initialisation, and since most of the PCI devices
are typically enumerated in subsys_initcall this leads to a failure and
an Oops related to iomem_get_mapping() access.

See relevant conversations:

  https://lore.kernel.org/linux-pci/20210204165831.2703772-1-daniel.vetter@ffwll.ch/
  https://lore.kernel.org/linux-pci/20210313215747.GA2394467@bjorn-Precision-5520/

After some deliberation on a potential solution, we have settled on
changing the order when PCI sub-system and relevant device drivers are
initialised so that PCI will come after the VFS but before ACPI, thus
allowing for the iomem_get_mapping() to work without issues.

And this initialisation order change is what I am currently working on
towards.

Hopefully once this is done, and nothing breaks, we would be able to
retire the late_initcall and pci_sysfs_init(), and move everything to
static sysfs object.  This in turn would definitely solve the problem
Danijel stumbled upon in regards to module unloading.

> If you have any work-in-progress that works on x86, I'd be curious to
> see if it would solve Danijel's problem.
[...]

Nothing as of yet.  But I will focus on this now, so that we can finish
this work a lot sooner.

	Krzysztof

  reply	other threads:[~2021-05-25 22:54 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-07 10:27 [PATCH] PCI: Fix accessing freed memory in pci_remove_resource_files Danijel Slivka
2021-05-07 22:07 ` Bjorn Helgaas
2021-05-10 15:02   ` Slivka, Danijel
2021-05-10 22:57     ` Bjorn Helgaas
2021-05-25 22:54       ` Krzysztof Wilczyński [this message]
2021-06-03 18:37         ` Krzysztof Wilczyński
2021-10-26 22:12       ` Bjorn Helgaas
2021-05-10 15:08   ` Slivka, Danijel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210525225445.GA108348@rocinante.localdomain \
    --to=kw@linux.com \
    --cc=Danijel.Slivka@amd.com \
    --cc=bhelgaas@google.com \
    --cc=helgaas@kernel.org \
    --cc=linux-pci@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).