| 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
| ||
|
Message-ID: <CAJuCfpE2wBnekxOTNpCaHRwnMznPgBkSUJNHk5y1-togkAtkHw@mail.gmail.com>
Date: Tue, 2 May 2023 13:41:49 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Petr Tesařík <petr@...arici.cz>
Cc: akpm@...ux-foundation.org, kent.overstreet@...ux.dev,
mhocko@...e.com, vbabka@...e.cz, hannes@...xchg.org,
roman.gushchin@...ux.dev, mgorman@...e.de, dave@...olabs.net,
willy@...radead.org, liam.howlett@...cle.com, corbet@....net,
void@...ifault.com, peterz@...radead.org, juri.lelli@...hat.com,
ldufour@...ux.ibm.com, catalin.marinas@....com, will@...nel.org,
arnd@...db.de, tglx@...utronix.de, mingo@...hat.com,
dave.hansen@...ux.intel.com, x86@...nel.org, peterx@...hat.com,
david@...hat.com, axboe@...nel.dk, mcgrof@...nel.org,
masahiroy@...nel.org, nathan@...nel.org, dennis@...nel.org,
tj@...nel.org, muchun.song@...ux.dev, rppt@...nel.org,
paulmck@...nel.org, pasha.tatashin@...een.com,
yosryahmed@...gle.com, yuzhao@...gle.com, dhowells@...hat.com,
hughd@...gle.com, andreyknvl@...il.com, keescook@...omium.org,
ndesaulniers@...gle.com, gregkh@...uxfoundation.org,
ebiggers@...gle.com, ytcoode@...il.com, vincent.guittot@...aro.org,
dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
bristot@...hat.com, vschneid@...hat.com, cl@...ux.com,
penberg@...nel.org, iamjoonsoo.kim@....com, 42.hyeyoo@...il.com,
glider@...gle.com, elver@...gle.com, dvyukov@...gle.com,
shakeelb@...gle.com, songmuchun@...edance.com, jbaron@...mai.com,
rientjes@...gle.com, minchan@...gle.com, kaleshsingh@...gle.com,
kernel-team@...roid.com, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, iommu@...ts.linux.dev,
linux-arch@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-mm@...ck.org, linux-modules@...r.kernel.org,
kasan-dev@...glegroups.com, cgroups@...r.kernel.org
Subject: Re: [PATCH 19/40] change alloc_pages name in dma_map_ops to avoid
name conflicts
On Tue, May 2, 2023 at 1:39 PM Petr Tesařík <petr@...arici.cz> wrote:
>
> On Tue, 2 May 2023 13:24:37 -0700
> Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> > On Tue, May 2, 2023 at 1:09 PM Petr Tesařík <petr@...arici.cz> wrote:
> > >
> > > On Tue, 2 May 2023 11:38:49 -0700
> > > Suren Baghdasaryan <surenb@...gle.com> wrote:
> > >
> > > > On Tue, May 2, 2023 at 8:50 AM Petr Tesařík <petr@...arici.cz> wrote:
> > > > >
> > > > > On Mon, 1 May 2023 09:54:29 -0700
> > > > > Suren Baghdasaryan <surenb@...gle.com> wrote:
> > > > >
> > > > > > After redefining alloc_pages, all uses of that name are being replaced.
> > > > > > Change the conflicting names to prevent preprocessor from replacing them
> > > > > > when it's not intended.
> > > > > >
> > > > > > Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>
> > > > > > ---
> > > > > > arch/x86/kernel/amd_gart_64.c | 2 +-
> > > > > > drivers/iommu/dma-iommu.c | 2 +-
> > > > > > drivers/xen/grant-dma-ops.c | 2 +-
> > > > > > drivers/xen/swiotlb-xen.c | 2 +-
> > > > > > include/linux/dma-map-ops.h | 2 +-
> > > > > > kernel/dma/mapping.c | 4 ++--
> > > > > > 6 files changed, 7 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/x86/kernel/amd_gart_64.c b/arch/x86/kernel/amd_gart_64.c
> > > > > > index 56a917df410d..842a0ec5eaa9 100644
> > > > > > --- a/arch/x86/kernel/amd_gart_64.c
> > > > > > +++ b/arch/x86/kernel/amd_gart_64.c
> > > > > > @@ -676,7 +676,7 @@ static const struct dma_map_ops gart_dma_ops = {
> > > > > > .get_sgtable = dma_common_get_sgtable,
> > > > > > .dma_supported = dma_direct_supported,
> > > > > > .get_required_mask = dma_direct_get_required_mask,
> > > > > > - .alloc_pages = dma_direct_alloc_pages,
> > > > > > + .alloc_pages_op = dma_direct_alloc_pages,
> > > > > > .free_pages = dma_direct_free_pages,
> > > > > > };
> > > > > >
> > > > > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > > > > > index 7a9f0b0bddbd..76a9d5ca4eee 100644
> > > > > > --- a/drivers/iommu/dma-iommu.c
> > > > > > +++ b/drivers/iommu/dma-iommu.c
> > > > > > @@ -1556,7 +1556,7 @@ static const struct dma_map_ops iommu_dma_ops = {
> > > > > > .flags = DMA_F_PCI_P2PDMA_SUPPORTED,
> > > > > > .alloc = iommu_dma_alloc,
> > > > > > .free = iommu_dma_free,
> > > > > > - .alloc_pages = dma_common_alloc_pages,
> > > > > > + .alloc_pages_op = dma_common_alloc_pages,
> > > > > > .free_pages = dma_common_free_pages,
> > > > > > .alloc_noncontiguous = iommu_dma_alloc_noncontiguous,
> > > > > > .free_noncontiguous = iommu_dma_free_noncontiguous,
> > > > > > diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> > > > > > index 9784a77fa3c9..6c7d984f164d 100644
> > > > > > --- a/drivers/xen/grant-dma-ops.c
> > > > > > +++ b/drivers/xen/grant-dma-ops.c
> > > > > > @@ -282,7 +282,7 @@ static int xen_grant_dma_supported(struct device *dev, u64 mask)
> > > > > > static const struct dma_map_ops xen_grant_dma_ops = {
> > > > > > .alloc = xen_grant_dma_alloc,
> > > > > > .free = xen_grant_dma_free,
> > > > > > - .alloc_pages = xen_grant_dma_alloc_pages,
> > > > > > + .alloc_pages_op = xen_grant_dma_alloc_pages,
> > > > > > .free_pages = xen_grant_dma_free_pages,
> > > > > > .mmap = dma_common_mmap,
> > > > > > .get_sgtable = dma_common_get_sgtable,
> > > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > > > > > index 67aa74d20162..5ab2616153f0 100644
> > > > > > --- a/drivers/xen/swiotlb-xen.c
> > > > > > +++ b/drivers/xen/swiotlb-xen.c
> > > > > > @@ -403,6 +403,6 @@ const struct dma_map_ops xen_swiotlb_dma_ops = {
> > > > > > .dma_supported = xen_swiotlb_dma_supported,
> > > > > > .mmap = dma_common_mmap,
> > > > > > .get_sgtable = dma_common_get_sgtable,
> > > > > > - .alloc_pages = dma_common_alloc_pages,
> > > > > > + .alloc_pages_op = dma_common_alloc_pages,
> > > > > > .free_pages = dma_common_free_pages,
> > > > > > };
> > > > > > diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
> > > > > > index 31f114f486c4..d741940dcb3b 100644
> > > > > > --- a/include/linux/dma-map-ops.h
> > > > > > +++ b/include/linux/dma-map-ops.h
> > > > > > @@ -27,7 +27,7 @@ struct dma_map_ops {
> > > > > > unsigned long attrs);
> > > > > > void (*free)(struct device *dev, size_t size, void *vaddr,
> > > > > > dma_addr_t dma_handle, unsigned long attrs);
> > > > > > - struct page *(*alloc_pages)(struct device *dev, size_t size,
> > > > > > + struct page *(*alloc_pages_op)(struct device *dev, size_t size,
> > > > > > dma_addr_t *dma_handle, enum dma_data_direction dir,
> > > > > > gfp_t gfp);
> > > > > > void (*free_pages)(struct device *dev, size_t size, struct page *vaddr,
> > > > > > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> > > > > > index 9a4db5cce600..fc42930af14b 100644
> > > > > > --- a/kernel/dma/mapping.c
> > > > > > +++ b/kernel/dma/mapping.c
> > > > > > @@ -570,9 +570,9 @@ static struct page *__dma_alloc_pages(struct device *dev, size_t size,
> > > > > > size = PAGE_ALIGN(size);
> > > > > > if (dma_alloc_direct(dev, ops))
> > > > > > return dma_direct_alloc_pages(dev, size, dma_handle, dir, gfp);
> > > > > > - if (!ops->alloc_pages)
> > > > > > + if (!ops->alloc_pages_op)
> > > > > > return NULL;
> > > > > > - return ops->alloc_pages(dev, size, dma_handle, dir, gfp);
> > > > > > + return ops->alloc_pages_op(dev, size, dma_handle, dir, gfp);
> > > > > > }
> > > > > >
> > > > > > struct page *dma_alloc_pages(struct device *dev, size_t size,
> > > > >
> > > > > I'm not impressed. This patch increases churn for code which does not
> > > > > (directly) benefit from the change, and that for limitations in your
> > > > > tooling?
> > > > >
> > > > > Why not just rename the conflicting uses in your local tree, but then
> > > > > remove the rename from the final patch series?
> > > >
> > > > With alloc_pages function becoming a macro, the preprocessor ends up
> > > > replacing all instances of that name, even when it's not used as a
> > > > function. That what necessitates this change. If there is a way to
> > > > work around this issue without changing all alloc_pages() calls in the
> > > > source base I would love to learn it but I'm not quite clear about
> > > > your suggestion and if it solves the issue. Could you please provide
> > > > more details?
> > >
> > > Ah, right, I admit I did not quite understand why this change is
> > > needed. However, this is exactly what I don't like about preprocessor
> > > macros. Each macro effectively adds a new keyword to the language.
> > >
> > > I believe everything can be solved with inline functions. What exactly
> > > does not work if you rename alloc_pages() to e.g. alloc_pages_caller()
> > > and then add an alloc_pages() inline function which calls
> > > alloc_pages_caller() with _RET_IP_ as a parameter?
> >
> > I don't think that would work because we need to inject the codetag at
> > the file/line of the actual allocation call. If we pass _REP_IT_ then
> > we would have to lookup the codetag associated with that _RET_IP_
> > which results in additional runtime overhead.
>
> OK. If the reference to source code itself must be recorded in the
> kernel, and not resolved later (either by the debugfs read fops, or by
> a tool which reads the file), then this information can only be
> obtained with a preprocessor macro.
>
> I was hoping that a debugging feature could be less intrusive. OTOH
> it's not my call to balance the tradeoffs.
>
> Thank you for your patient explanations.
Thanks for reviewing and the suggestions! I'll address the actionable
ones in the next version.
Suren.
>
> Petr T
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@...roid.com.
>
Powered by blists - more mailing lists