Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 7 Oct 2019 14:54:59 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Douglas Anderson <dianders@chromium.org>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Jason Wessel <jason.wessel@windriver.com>,
        kgdb-bugreport@lists.sourceforge.net,
        Christophe Leroy <christophe.leroy@c-s.fr>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/4] kdb: Fix "btc <cpu>" crash if the CPU didn't
 round up
Message-ID: <20191007135459.lj3qc2tqzcv3xcia@holly.lan>
References: <20190925200220.157670-1-dianders@chromium.org>
 <20190925125811.v3.3.Id33c06cbd1516b49820faccd80da01c7c4bf15c7@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190925125811.v3.3.Id33c06cbd1516b49820faccd80da01c7c4bf15c7@changeid>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Sep 25, 2019 at 01:02:19PM -0700, Douglas Anderson wrote:
> 
> I noticed that when I did "btc <cpu>" and the CPU I passed in hadn't
> rounded up that I'd crash.  I was going to copy the same fix from
> commit 162bc7f5afd7 ("kdb: Don't back trace on a cpu that didn't round
> up") into the "not all the CPUs" case, but decided it'd be better to
> clean things up a little bit.
> 
> This consolidates the two code paths.  It is _slightly_ wasteful in in
> that the checks for "cpu" being too small or being offline isn't
> really needed when we're iterating over all online CPUs, but that
> really shouldn't hurt.  Better to have the same code path.
> 
> While at it, eliminate at least one slightly ugly (and totally
> needless) recursive use of kdb_parse().
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v3:
> - Patch ("kdb: Fix "btc <cpu>" crash if the CPU...") new for v3.
> 
> Changes in v2: None
> 
>  kernel/debug/kdb/kdb_bt.c | 61 ++++++++++++++++++++++-----------------
>  1 file changed, 34 insertions(+), 27 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_bt.c b/kernel/debug/kdb/kdb_bt.c
> index 120fc686c919..d9af139f9a31 100644
> --- a/kernel/debug/kdb/kdb_bt.c
> +++ b/kernel/debug/kdb/kdb_bt.c
> @@ -101,6 +101,27 @@ kdb_bt1(struct task_struct *p, unsigned long mask, bool btaprompt)
>  	return 0;
>  }
>  
> +static void
> +kdb_bt_cpu(unsigned long cpu)
> +{
> +	struct task_struct *kdb_tsk;
> +
> +	if (cpu >= num_possible_cpus() || !cpu_online(cpu)) {
> +		kdb_printf("WARNING: no process for cpu %ld\n", cpu);
> +		return;
> +	}
> +
> +	/* If a CPU failed to round up we could be here */
> +	kdb_tsk = KDB_TSK(cpu);
> +	if (!kdb_tsk) {
> +		kdb_printf("WARNING: no task for cpu %ld\n", cpu);
> +		return;
> +	}
> +
> +	kdb_set_current_task(kdb_tsk);
> +	kdb_bt1(kdb_tsk, ~0UL, false);
> +}
> +
>  int
>  kdb_bt(int argc, const char **argv)
>  {
> @@ -161,7 +182,6 @@ kdb_bt(int argc, const char **argv)
>  	} else if (strcmp(argv[0], "btc") == 0) {
>  		unsigned long cpu = ~0;
>  		struct task_struct *save_current_task = kdb_current_task;
> -		char buf[80];
>  		if (argc > 1)
>  			return KDB_ARGCOUNT;
>  		if (argc == 1) {
> @@ -169,35 +189,22 @@ kdb_bt(int argc, const char **argv)
>  			if (diag)
>  				return diag;
>  		}
> -		/* Recursive use of kdb_parse, do not use argv after
> -		 * this point */
> -		argv = NULL;
>  		if (cpu != ~0) {
> -			if (cpu >= num_possible_cpus() || !cpu_online(cpu)) {
> -				kdb_printf("no process for cpu %ld\n", cpu);
> -				return 0;
> -			}
> -			sprintf(buf, "btt 0x%px\n", KDB_TSK(cpu));
> -			kdb_parse(buf);
> -			return 0;
> -		}
> -		kdb_printf("btc: cpu status: ");
> -		kdb_parse("cpu\n");
> -		for_each_online_cpu(cpu) {
> -			void *kdb_tsk = KDB_TSK(cpu);
> -
> -			/* If a CPU failed to round up we could be here */
> -			if (!kdb_tsk) {
> -				kdb_printf("WARNING: no task for cpu %ld\n",
> -					   cpu);
> -				continue;
> +			kdb_bt_cpu(cpu);
> +		} else {
> +			/*
> +			 * Recursive use of kdb_parse, do not use argv after
> +			 * this point.
> +			 */
> +			argv = NULL;
> +			kdb_printf("btc: cpu status: ");
> +			kdb_parse("cpu\n");
> +			for_each_online_cpu(cpu) {
> +				kdb_bt_cpu(cpu);
> +				touch_nmi_watchdog();
>  			}
> -
> -			sprintf(buf, "btt 0x%px\n", kdb_tsk);
> -			kdb_parse(buf);
> -			touch_nmi_watchdog();
> +			kdb_set_current_task(save_current_task);
>  		}
> -		kdb_set_current_task(save_current_task);

Why does this move out into only one of the conditional branches?
Don't both of the above paths modify the current task?


Daniel.


>  		return 0;
>  	} else {
>  		if (argc) {
> -- 
> 2.23.0.351.gc4317032e6-goog
>