Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210323065519.821062-1-sumit.garg@linaro.org> <20210617112527.nganuruifprwhv3h@maple.lan>
In-Reply-To: <20210617112527.nganuruifprwhv3h@maple.lan>
From:   Sumit Garg <sumit.garg@linaro.org>
Date:   Thu, 17 Jun 2021 17:21:12 +0530
Message-ID: <CAFA6WYMwRr5iO3uv_8sSocPYhBBaxEN-M+J3GCze5UvtkeUyzw@mail.gmail.com>
Subject: Re: [PATCH v2] kdb: Get rid of custom debug heap allocator
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net,
        Jason Wessel <jason.wessel@windriver.com>,
        Douglas Anderson <dianders@chromium.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 17 Jun 2021 at 16:55, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Tue, Mar 23, 2021 at 12:25:19PM +0530, Sumit Garg wrote:
> > Currently the only user for debug heap is kdbnearsym() which can be
> > modified to rather use statically allocated buffer for symbol name as
> > per it's current usage. So do that and hence remove custom debug heap
> > allocator.
> >
> > Note that this change puts a restriction on kdbnearsym() callers to
> > carefully use shared namebuf such that a caller should consume the symbol
> > returned immediately prior to another call to fetch a different symbol.
> >
> > This change has been tested using kgdbtest on arm64 which doesn't show
> > any regressions.
> >
> > Suggested-by: Daniel Thompson <daniel.thompson@linaro.org>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>
> I'm afraid the passage of time (mostly due to my dropping the ball)
> means this no longer applies cleanly to latest kernel and `git am
> -3way` tells me that "sha1 information is lacking or useless".
> Please can you rebase this on v5.13-rc4 and repost?
>

Sure.

> Also...
>
>
> > diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
> > index b857a84de3b5..ec91d7e02334 100644
> > diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
> > index b59aad1f0b55..e131d74abb8d 100644
> > --- a/kernel/debug/kdb/kdb_support.c
> > +++ b/kernel/debug/kdb/kdb_support.c
> > @@ -57,35 +57,26 @@ int kdbgetsymval(const char *symname, kdb_symtab_t *symtab)
> >  }
> >  EXPORT_SYMBOL(kdbgetsymval);
> >
> > -static char *kdb_name_table[100];    /* arbitrary size */
> > -
> >  /*
> > - * kdbnearsym -      Return the name of the symbol with the nearest address
> > - *   less than 'addr'.
> > + * kdbnearsym() - Return the name of the symbol with the nearest address
> > + *                less than @addr.
> > + * @addr: Address to check for near symbol
> > + * @symtab: Structure to receive results
> >   *
> > - * Parameters:
> > - *   addr    Address to check for symbol near
> > - *   symtab  Structure to receive results
> > - * Returns:
> > - *   0       No sections contain this address, symtab zero filled
> > - *   1       Address mapped to module/symbol/section, data in symtab
> > - * Remarks:
> > - *   2.6 kallsyms has a "feature" where it unpacks the name into a
> > - *   string.  If that string is reused before the caller expects it
> > - *   then the caller sees its string change without warning.  To
> > - *   avoid cluttering up the main kdb code with lots of kdb_strdup,
> > - *   tests and kfree calls, kdbnearsym maintains an LRU list of the
> > - *   last few unique strings.  The list is sized large enough to
> > - *   hold active strings, no kdb caller of kdbnearsym makes more
> > - *   than ~20 later calls before using a saved value.
> > + * Note here that only single statically allocated namebuf is used for every
> > + * symbol, so the caller should consume it immediately prior to another call
> > + * to fetch a different symbol.
>
> This still looks like it will confused experienced kernel devs who
> aren't aware of kdb's calling context. Nor does it help future kdb
> developers understand some of the subtlty of interactions here.
>
> Can you enlarge this to the following (editing anything below that you
> don't want git to blame you for ;-) ):
>
> ~~~
> WARNING: This function may return a pointer to a single statically
> allocated buffer (namebuf). kdb's unusual calling context (single
> threaded, all other CPUs halted) provides us sufficient locking for
> this to be safe. The only constraint imposed by the static buffer is
> that the caller must consume any previous reply prior to another call
> to lookup a new symbol.
>
> Note that, strictly speaking, some architectures may re-enter the kdb
> trap if the system turns out to be very badly damaged and this breaks
> the single-threaded assumption above. In these circumstances successful
> continuation and exit from the inner trap is unlikely to work and any
> user attempting this receives a prominent warning before being allowed
> to progress. In these circumstances we remain memory safe because
> namebuf[KSYM_NAME_LEN-1] will never change from '\0' although we do
> tolerate the possibility of garbled symbol display from the outer kdb
> trap.
> ~~~
>

Okay I will use this comment instead.

-Sumit

> Thanks
>
>
> Daniel.