Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230720160729.39324-1-l3b2w1@gmail.com> <20230721162114.GA1321524-robh@kernel.org>
In-Reply-To: <20230721162114.GA1321524-robh@kernel.org>
From:   binglei wang <l3b2w1@gmail.com>
Date:   Sat, 22 Jul 2023 05:34:02 +0800
Message-ID: <CAJ3C4KwEgGaHaWv+W89fXNUQiHd+iu=JHe3v6V85mU7oGYCX1w@mail.gmail.com>
Subject: Re: [PATCH] cma: check for memory region overlapping
To:     Rob Herring <robh@kernel.org>
Cc:     frowand.list@gmail.com, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, iommu@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Rob Herring <robh@kernel.org> =E4=BA=8E2023=E5=B9=B47=E6=9C=8822=E6=97=A5=
=E5=91=A8=E5=85=AD 00:21=E5=86=99=E9=81=93=EF=BC=9A
>
> On Fri, Jul 21, 2023 at 12:07:29AM +0800, Binglei Wang wrote:
> > From: Binglei Wang <l3b2w1@gmail.com>
> >
> > Cma memory region editted carelessly in dts may overlap
> > with kernel code/data memory region which is reserved by memblock
> > during the early phase of system memory initialization.
>
> Is your goal for the kernel to function with this careless editing or
> warn enough to fix the DT?
>
> What if other regions overlap with the kernel? Wouldn't we have the same
> problem?
>
> >
> > Without checking overlap and cma area setup done,
> > this region will be released to buddy system later.
> >
> > When memory usage under pressure, memory allocated from
> > this region will collide with kernel code which is read-only.
> > And the following writing to this region will trigger the panic
> > of writing to read-only memory.
> >

Dear Rob:
Thanks for your reply.

In the production environment, I encountered this issue in the 5.4 kernel.
The goal is just to warn enough to fix the DT.

> > So when rmem_cma_setup returns EBUSY, do not phys-free this region
> > to memblock or else we end up with free the kernel code memory
> > to buddy system.
> >
> > Signed-off-by: Binglei Wang <l3b2w1@gmail.com>
> > ---
> >  drivers/of/of_reserved_mem.c | 3 ---
> >  kernel/dma/contiguous.c      | 5 +++++
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.=
c
> > index 7ec94cfcb..d62cc76ef 100644
> > --- a/drivers/of/of_reserved_mem.c
> > +++ b/drivers/of/of_reserved_mem.c
> > @@ -338,9 +338,6 @@ void __init fdt_init_reserved_mem(void)
> >                                       rmem->name);
> >                               if (nomap)
> >                                       memblock_clear_nomap(rmem->base, =
rmem->size);
> > -                             else
> > -                                     memblock_phys_free(rmem->base,
> > -                                                        rmem->size);
>
> I don't understand this change. Seems like perhaps someone would want
> the free here?
>
> >                       } else {
> >                               phys_addr_t end =3D rmem->base + rmem->si=
ze - 1;
> >                               bool reusable =3D
> > diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> > index 6ea80ae42..a349f3e97 100644
> > --- a/kernel/dma/contiguous.c
> > +++ b/kernel/dma/contiguous.c
> > @@ -410,6 +410,11 @@ static int __init rmem_cma_setup(struct reserved_m=
em *rmem)
> >               return -EBUSY;
>
> Here we return EBUSY if we are using cmdline params. In that case, isn't
> freeing the DT CMA area the right thing to do?
>
>

yes, in some cases, freeing the DT CMA area is the right thing to do.
in another case, like the read-only panic,  freeing will cause problems.

Actually,  giving a warning message is already sufficient.
I should not overthink it, after all, it was caused by human factors.

When using command line,
I find cma_declare_contiguous_nid will check the overlap in the latest kern=
el.
But in the program flow of parsing dts, both the 5.4 kernel and the
latest version of the kernel lack this check.
That's the reason for this patch.

> >       }
> >
> > +     if (memblock_is_region_reserved(rmem->base, rmem->size)) {
> > +             pr_info("Reserved memory: overlap with exsiting one\n");
> > +             return -EBUSY;
> > +     }

Do you think a warning message is sufficient?

Best wishes
Binglei Wang.

> > +
> >       if (!of_get_flat_dt_prop(node, "reusable", NULL) ||
> >           of_get_flat_dt_prop(node, "no-map", NULL))
> >               return -EINVAL;
> > --
> > 2.34.1
> >