From: Stefano Stabellini stefano.stabellini@xilinx.com
In case of errors in xenbus_init (e.g. missing xen_store_gfn parameter), we goto out_error but we forget to reset xen_store_domain_type to XS_UNKNOWN. As a consequence xenbus_probe_initcall and other initcalls will still try to initialize xenstore resulting into a crash at boot.
[ 2.479830] Call trace: [ 2.482314] xb_init_comms+0x18/0x150 [ 2.486354] xs_init+0x34/0x138 [ 2.489786] xenbus_probe+0x4c/0x70 [ 2.498432] xenbus_probe_initcall+0x2c/0x7c [ 2.503944] do_one_initcall+0x54/0x1b8 [ 2.507358] kernel_init_freeable+0x1ac/0x210 [ 2.511617] kernel_init+0x28/0x130 [ 2.516112] ret_from_fork+0x10/0x20
Cc: Stable@vger.kernel.org Cc: jbeulich@suse.com Signed-off-by: Stefano Stabellini stefano.stabellini@xilinx.com --- Changes in v2: - use return 0 for the success case - remove err initializer as it is not useful any longer --- drivers/xen/xenbus/xenbus_probe.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index bd003ca8acbe..5967aa937255 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -909,7 +909,7 @@ static struct notifier_block xenbus_resume_nb = {
static int __init xenbus_init(void) { - int err = 0; + int err; uint64_t v = 0; xen_store_domain_type = XS_UNKNOWN;
@@ -983,8 +983,10 @@ static int __init xenbus_init(void) */ proc_create_mount_point("xen"); #endif + return 0;
out_error: + xen_store_domain_type = XS_UNKNOWN; return err; }
On 15.11.2021 23:27, Stefano Stabellini wrote:
From: Stefano Stabellini stefano.stabellini@xilinx.com
In case of errors in xenbus_init (e.g. missing xen_store_gfn parameter), we goto out_error but we forget to reset xen_store_domain_type to XS_UNKNOWN. As a consequence xenbus_probe_initcall and other initcalls will still try to initialize xenstore resulting into a crash at boot.
[ 2.479830] Call trace: [ 2.482314] xb_init_comms+0x18/0x150 [ 2.486354] xs_init+0x34/0x138 [ 2.489786] xenbus_probe+0x4c/0x70 [ 2.498432] xenbus_probe_initcall+0x2c/0x7c [ 2.503944] do_one_initcall+0x54/0x1b8 [ 2.507358] kernel_init_freeable+0x1ac/0x210 [ 2.511617] kernel_init+0x28/0x130 [ 2.516112] ret_from_fork+0x10/0x20
Cc: Stable@vger.kernel.org Cc: jbeulich@suse.com Signed-off-by: Stefano Stabellini stefano.stabellini@xilinx.com
For the immediate purpose as described this looks okay, so Reviewed-by: Jan Beulich jbeulich@suse.com
However, aren't there further pieces missing on this error patch: - clearing of xenstored_ready in case it got set, - rolling back xenstored_local_init() (XS_LOCAL) and xen_remap() (XS_HVM). And shouldn't xs_init() failure when called from xenbus_probe() also result in the driver not giving the appearance of being usable?
--- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -909,7 +909,7 @@ static struct notifier_block xenbus_resume_nb = { static int __init xenbus_init(void) {
- int err = 0;
- int err; uint64_t v = 0; xen_store_domain_type = XS_UNKNOWN;
Minor remark: You may want to take the opportunity and add the missing blank line here to visually separate the assignment from the declarations.
Jan
On Tue, 16 Nov 2021, Jan Beulich wrote:
On 15.11.2021 23:27, Stefano Stabellini wrote:
From: Stefano Stabellini stefano.stabellini@xilinx.com
In case of errors in xenbus_init (e.g. missing xen_store_gfn parameter), we goto out_error but we forget to reset xen_store_domain_type to XS_UNKNOWN. As a consequence xenbus_probe_initcall and other initcalls will still try to initialize xenstore resulting into a crash at boot.
[ 2.479830] Call trace: [ 2.482314] xb_init_comms+0x18/0x150 [ 2.486354] xs_init+0x34/0x138 [ 2.489786] xenbus_probe+0x4c/0x70 [ 2.498432] xenbus_probe_initcall+0x2c/0x7c [ 2.503944] do_one_initcall+0x54/0x1b8 [ 2.507358] kernel_init_freeable+0x1ac/0x210 [ 2.511617] kernel_init+0x28/0x130 [ 2.516112] ret_from_fork+0x10/0x20
Cc: Stable@vger.kernel.org Cc: jbeulich@suse.com Signed-off-by: Stefano Stabellini stefano.stabellini@xilinx.com
For the immediate purpose as described this looks okay, so Reviewed-by: Jan Beulich jbeulich@suse.com
Thank you!
However, aren't there further pieces missing on this error patch:
- clearing of xenstored_ready in case it got set,
- rolling back xenstored_local_init() (XS_LOCAL) and xen_remap() (XS_HVM).
And shouldn't xs_init() failure when called from xenbus_probe() also result in the driver not giving the appearance of being usable?
I prioritized this patch because I have a direct test case for this issue and I can see that this patch solves it. But what you wrote is true, and I can have a look in the following weeks.
--- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -909,7 +909,7 @@ static struct notifier_block xenbus_resume_nb = { static int __init xenbus_init(void) {
- int err = 0;
- int err; uint64_t v = 0; xen_store_domain_type = XS_UNKNOWN;
Minor remark: You may want to take the opportunity and add the missing blank line here to visually separate the assignment from the declarations.
Jan
On 11/15/21 5:27 PM, Stefano Stabellini wrote:
From: Stefano Stabellini stefano.stabellini@xilinx.com
In case of errors in xenbus_init (e.g. missing xen_store_gfn parameter), we goto out_error but we forget to reset xen_store_domain_type to XS_UNKNOWN. As a consequence xenbus_probe_initcall and other initcalls will still try to initialize xenstore resulting into a crash at boot.
[ 2.479830] Call trace: [ 2.482314] xb_init_comms+0x18/0x150 [ 2.486354] xs_init+0x34/0x138 [ 2.489786] xenbus_probe+0x4c/0x70 [ 2.498432] xenbus_probe_initcall+0x2c/0x7c [ 2.503944] do_one_initcall+0x54/0x1b8 [ 2.507358] kernel_init_freeable+0x1ac/0x210 [ 2.511617] kernel_init+0x28/0x130 [ 2.516112] ret_from_fork+0x10/0x20
Cc: Stable@vger.kernel.org Cc: jbeulich@suse.com Signed-off-by: Stefano Stabellini stefano.stabellini@xilinx.com
Applied to for-linus-5.16c
-boris
linux-stable-mirror@lists.linaro.org
-
Boris Ostrovsky -
Jan Beulich -
Stefano Stabellini