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;
Date:   Thu, 17 Jun 2021 12:25:27 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net, jason.wessel@windriver.com,
        dianders@chromium.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] kdb: Get rid of custom debug heap allocator
Message-ID: <20210617112527.nganuruifprwhv3h@maple.lan>
References: <20210323065519.821062-1-sumit.garg@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210323065519.821062-1-sumit.garg@linaro.org>
Precedence: bulk

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?

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.
~~~

Thanks


Daniel.