| Message ID | 154275557457.76910.16923571232582744134.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: Merge hmm into devm_memremap_pages, mark GPL-only | expand |
On Tue 20-11-18 15:12:54, Dan Williams wrote: > devm_memremap_pages() is a facility that can create struct page entries > for any arbitrary range and give drivers the ability to subvert core > aspects of page management. > > Specifically the facility is tightly integrated with the kernel's memory > hotplug functionality. It injects an altmap argument deep into the > architecture specific vmemmap implementation to allow allocating from > specific reserved pages, and it has Linux specific assumptions about > page structure reference counting relative to get_user_pages() and > get_user_pages_fast(). It was an oversight and a mistake that this was > not marked EXPORT_SYMBOL_GPL from the outset. > > Again, devm_memremap_pagex() exposes and relies upon core kernel > internal assumptions and will continue to evolve along with 'struct > page', memory hotplug, and support for new memory types / topologies. > Only an in-kernel GPL-only driver is expected to keep up with this > ongoing evolution. This interface, and functionality derived from this > interface, is not suitable for kernel-external drivers. As I've said earlier I do not buy this justification because there is simply no stable API for modules by definition (Documentation/process/stable-api-nonsense.rst). I do understand your reasoning that you as an author never intended to export the symbol this way. That is fair and justified reason for this patch. Whoever needs a wrapper around arch_add_memory can do so because this symbol has no restriction for the usage. It will be still the same fiddling with struct page and deep mm internals. Do we care? I am not convinced because once we grow any in tree user we have to cope with any potential abuse like we have in other areas in the past. And out-of-tree modules? Who cares. Those are on their own completely and have their ways to go around. > Cc: Michal Hocko <mhocko@suse.com> > Cc: "Jérôme Glisse" <jglisse@redhat.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> That being said Acked-by: Michal Hocko <mhocko@suse.com> > --- > kernel/memremap.c | 2 +- > tools/testing/nvdimm/test/iomap.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/memremap.c b/kernel/memremap.c > index 9eced2cc9f94..61dbcaa95530 100644 > --- a/kernel/memremap.c > +++ b/kernel/memremap.c > @@ -233,7 +233,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) > err_array: > return ERR_PTR(error); > } > -EXPORT_SYMBOL(devm_memremap_pages); > +EXPORT_SYMBOL_GPL(devm_memremap_pages); > > unsigned long vmem_altmap_offset(struct vmem_altmap *altmap) > { > diff --git a/tools/testing/nvdimm/test/iomap.c b/tools/testing/nvdimm/test/iomap.c > index ff9d3a5825e1..ed18a0cbc0c8 100644 > --- a/tools/testing/nvdimm/test/iomap.c > +++ b/tools/testing/nvdimm/test/iomap.c > @@ -113,7 +113,7 @@ void *__wrap_devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) > return nfit_res->buf + offset - nfit_res->res.start; > return devm_memremap_pages(dev, pgmap); > } > -EXPORT_SYMBOL(__wrap_devm_memremap_pages); > +EXPORT_SYMBOL_GPL(__wrap_devm_memremap_pages); > > pfn_t __wrap_phys_to_pfn_t(phys_addr_t addr, unsigned long flags) > {
On Thu, Nov 22, 2018 at 02:30:13PM +0100, Michal Hocko wrote: > Whoever needs a wrapper around arch_add_memory can do so because this > symbol has no restriction for the usage. arch_add_memory is not exported, and it really should not be.
On Thu, Nov 22, 2018 at 05:38:58PM +0100, Christoph Hellwig wrote: > On Thu, Nov 22, 2018 at 02:30:13PM +0100, Michal Hocko wrote: > > Whoever needs a wrapper around arch_add_memory can do so because this > > symbol has no restriction for the usage. > > arch_add_memory is not exported, and it really should not be. And in some older trees it oddly has an EXPORY_SYMBOL_GPL on x86 and sh, but no actual modular users..
On Thu 22-11-18 17:38:58, Christoph Hellwig wrote: > On Thu, Nov 22, 2018 at 02:30:13PM +0100, Michal Hocko wrote: > > Whoever needs a wrapper around arch_add_memory can do so because this > > symbol has no restriction for the usage. > > arch_add_memory is not exported, and it really should not be. It is not, but nobody really prevents from wrapping it and exporting. I am definitely not arguing for that and I would even agree with you that it shouldn't be exported at all unless there is a _very_ good reason for that. Because usecases is what we care about here.
diff --git a/kernel/memremap.c b/kernel/memremap.c index 9eced2cc9f94..61dbcaa95530 100644 --- a/kernel/memremap.c +++ b/kernel/memremap.c @@ -233,7 +233,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) err_array: return ERR_PTR(error); } -EXPORT_SYMBOL(devm_memremap_pages); +EXPORT_SYMBOL_GPL(devm_memremap_pages); unsigned long vmem_altmap_offset(struct vmem_altmap *altmap) { diff --git a/tools/testing/nvdimm/test/iomap.c b/tools/testing/nvdimm/test/iomap.c index ff9d3a5825e1..ed18a0cbc0c8 100644 --- a/tools/testing/nvdimm/test/iomap.c +++ b/tools/testing/nvdimm/test/iomap.c @@ -113,7 +113,7 @@ void *__wrap_devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap) return nfit_res->buf + offset - nfit_res->res.start; return devm_memremap_pages(dev, pgmap); } -EXPORT_SYMBOL(__wrap_devm_memremap_pages); +EXPORT_SYMBOL_GPL(__wrap_devm_memremap_pages); pfn_t __wrap_phys_to_pfn_t(phys_addr_t addr, unsigned long flags) {