[Crash-utility] [PATCH v4 4/4] Add check if an syment element is installed one more time

[Philipp Rudo]

Hi Tao,

On Sat, 18 Sep 2021 15:59:32 +0800
Tao Liu <ltao at redhat.com> wrote:

> symname_hash_install won't check if spn has been installed before. If
> it does, the second install will corrupt the hash table as well as
> spn->cnt counting. This patch adds the check to avoid such risks.
> 
> Signed-off-by: Tao Liu <ltao at redhat.com>
> ---
>  symbols.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/symbols.c b/symbols.c
> index f7157b1..6d12c55 100644
> --- a/symbols.c
> +++ b/symbols.c
> @@ -1147,6 +1147,20 @@ mod_symtable_hash_remove_range(struct syment *from, struct syment *to)
>  		symname_hash_remove(st->mod_symname_hash, sp);
>  }
>  
> +static inline int
> +syment_is_installed(struct syment *table[], struct syment *spn)
> +{
> +	struct syment *sp;
> +	int index;
> +
> +	index = SYMNAME_HASH_INDEX(spn->name);
> +	for (sp = table[index]; sp; sp = sp->name_hash_next) {
> +		if (sp == spn)
> +			return TRUE;
> +	}
> +	return FALSE;
> +}
> +
>  /*
>   *  Install a single static kernel symbol into the symname_hash.
>   */
> @@ -1156,7 +1170,7 @@ symname_hash_install(struct syment *table[], struct syment *spn)
>  	struct syment *sp;
>          int index;
>  
> -	if (!spn)
> +	if (!spn || syment_is_installed(table, spn))
>  		return;
>  
>          index = SYMNAME_HASH_INDEX(spn->name);

hmm... not sure if this is a little bit over the top. The idea I had
was in your v3 simply replace

assert(sp != spn);

by

if (sp == spn) {
	error(WARNING, "Symbol %s already installed in symname_hash\n",
	      sp->name);
	continue;
}

That's less code plus the warning makes it easier to detect that there
is a problem (for me the case sp == spn is a sign for a bug in crash).
What do you think?

Thanks
Philipp