| Message ID | 20210625233118.2814915-2-kw@linux.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Allow deferred execution of iomem_get_mapping() | expand |
On Fri, Jun 25, 2021 at 4:31 PM Krzysztof Wilczyński <kw@linux.com> wrote: > > Defer invocation of the iomem_get_mapping() to the sysfs open callback > so that it can be executed as needed when the binary sysfs object has > been accessed. > > To do that, convert the "mapping" member of the struct bin_attribute > from a pointer to the struct address_space into a function pointer with > a signature that requires the same return type, and then updates the > sysfs_kf_bin_open() to invoke provided function should the function > pointer be valid. > > Thus, this change removes the need for the fs_initcalls to complete > before any other sub-system that uses the iomem_get_mapping() would be > able to invoke it safely without leading to a failure and an Oops > related to an invalid iomem_get_mapping() access. > > Co-authored-by: Dan Williams <dan.j.williams@intel.com> Go ahead and replace this with: Signed-off-by: Dan Williams <dan.j.williams@intel.com>
On Fri, Jun 25, 2021 at 11:31:17PM +0000, Krzysztof Wilczy??ski wrote: > if (battr->mapping) > - of->file->f_mapping = battr->mapping; > + of->file->f_mapping = battr->mapping(); I think get_mapping() is a better name now. That being said this whole programming model looks a little weird. Also, does this patch imply the mapping field of the sysfs bin attributes wasn't used before at all?
Hi Christoph, > > if (battr->mapping) > > - of->file->f_mapping = battr->mapping; > > + of->file->f_mapping = battr->mapping(); > > I think get_mapping() is a better name now. That being said this > whole programming model looks a little weird. I would have to lean on Daniel and Dan here as they might have more context than I do, especially since in the PCI world we are only consumers of this new API. The related patches are: commit 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region") commit 636b21b50152 ("PCI: Revoke mappings like devmem") > Also, does this patch imply the mapping field of the sysfs bin > attributes wasn't used before at all? No, everything worked as intended, thankfully. Changes here are meant to help us transition to use static sysfs objects when we add various PCI-related attributes a particular device. This in turn will allow us to remove the need for late_initcall() in the drivers/pci/pci-sysfs.c, and thus fix the race condition people noticed on some platforms when sysfs objects are being added while PCI sub-system and devices are initialised. More details are captured in the following conversations: https://lore.kernel.org/linux-pci/20200716110423.xtfyb3n6tn5ixedh@pali/ https://lore.kernel.org/linux-pci/20210527205845.GA1421476@bjorn-Precision-5520/ https://lore.kernel.org/linux-pci/20210313215747.GA2394467@bjorn-Precision-5520/ Dan's original patch: https://lore.kernel.org/linux-pci/CAPcyv4i0y_4cMGEpNVShLUyUk3nyWH203Ry3S87BqnDJE0Rmxg@mail.gmail.com/ Krzysztof
On Mon, Jun 28, 2021 at 3:12 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Fri, Jun 25, 2021 at 11:31:17PM +0000, Krzysztof Wilczy??ski wrote: > > if (battr->mapping) > > - of->file->f_mapping = battr->mapping; > > + of->file->f_mapping = battr->mapping(); > > I think get_mapping() is a better name now. That being said this > whole programming model looks a little weird. I think both those points are fair. > Also, does this patch imply the mapping field of the sysfs bin > attributes wasn't used before at all? It defaulted to an address_space per file rather than a shared address space across all files that map physical addresses as file offsets.
Hi Dan, On 21-06-28 10:36:13, Dan Williams wrote: > On Mon, Jun 28, 2021 at 3:12 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > On Fri, Jun 25, 2021 at 11:31:17PM +0000, Krzysztof Wilczy??ski wrote: > > > if (battr->mapping) > > > - of->file->f_mapping = battr->mapping; > > > + of->file->f_mapping = battr->mapping(); > > > > I think get_mapping() is a better name now. That being said this > > whole programming model looks a little weird. > > I think both those points are fair. Anything for us to do? > > Also, does this patch imply the mapping field of the sysfs bin > > attributes wasn't used before at all? > > It defaulted to an address_space per file rather than a shared address > space across all files that map physical addresses as file offsets. I will include this in the commit message for v3 of the patch series, if you don't mind. Also, would this shared address space be a potential issue? Security, functional, etc. Krzysztof
On Mon, Jun 28, 2021 at 11:07 AM Krzysztof Wilczyński <kw@linux.com> wrote: > > Hi Dan, > > On 21-06-28 10:36:13, Dan Williams wrote: > > On Mon, Jun 28, 2021 at 3:12 AM Christoph Hellwig <hch@infradead.org> wrote: > > > > > > On Fri, Jun 25, 2021 at 11:31:17PM +0000, Krzysztof Wilczy??ski wrote: > > > > if (battr->mapping) > > > > - of->file->f_mapping = battr->mapping; > > > > + of->file->f_mapping = battr->mapping(); > > > > > > I think get_mapping() is a better name now. That being said this > > > whole programming model looks a little weird. > > > > I think both those points are fair. > > Anything for us to do? > > > > Also, does this patch imply the mapping field of the sysfs bin > > > attributes wasn't used before at all? > > > > It defaulted to an address_space per file rather than a shared address > > space across all files that map physical addresses as file offsets. > > I will include this in the commit message for v3 of the patch series, if > you don't mind. Also, would this shared address space be a potential > issue? Security, functional, etc. The shared address_space arrangement is what allows for a "revoke" mechanism for /dev/mem and pci-resource-sysfs mappings. Without a shared address space there would need to be tracking for each 'inode' instance to run revoke_iomem(). Note that this shared address_space scheme is also deployed for block-device special files, see blkdev_open(). It's done for similar reasons of allowing all address_space operations to act on a common representation of the single block-device.
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index 9aefa7779b29..a3ee4c32a264 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -175,7 +175,7 @@ static int sysfs_kf_bin_open(struct kernfs_open_file *of) struct bin_attribute *battr = of->kn->priv; if (battr->mapping) - of->file->f_mapping = battr->mapping; + of->file->f_mapping = battr->mapping(); return 0; } diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h index d76a1ddf83a3..fbb7c7df545c 100644 --- a/include/linux/sysfs.h +++ b/include/linux/sysfs.h @@ -170,7 +170,7 @@ struct bin_attribute { struct attribute attr; size_t size; void *private; - struct address_space *mapping; + struct address_space *(*mapping)(void); ssize_t (*read)(struct file *, struct kobject *, struct bin_attribute *, char *, loff_t, size_t); ssize_t (*write)(struct file *, struct kobject *, struct bin_attribute *,
Defer invocation of the iomem_get_mapping() to the sysfs open callback so that it can be executed as needed when the binary sysfs object has been accessed. To do that, convert the "mapping" member of the struct bin_attribute from a pointer to the struct address_space into a function pointer with a signature that requires the same return type, and then updates the sysfs_kf_bin_open() to invoke provided function should the function pointer be valid. Thus, this change removes the need for the fs_initcalls to complete before any other sub-system that uses the iomem_get_mapping() would be able to invoke it safely without leading to a failure and an Oops related to an invalid iomem_get_mapping() access. Co-authored-by: Dan Williams <dan.j.williams@intel.com> Suggested-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Krzysztof Wilczyński <kw@linux.com> --- fs/sysfs/file.c | 2 +- include/linux/sysfs.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)