From: Stefano Stabellini <[email protected]>
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: <[email protected]>
Cc: [email protected]
Signed-off-by: Stefano Stabellini <[email protected]>
---
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;
}
--
2.25.1
On 15.11.2021 23:27, Stefano Stabellini wrote:
> From: Stefano Stabellini <[email protected]>
>
> 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: <[email protected]>
> Cc: [email protected]
> Signed-off-by: Stefano Stabellini <[email protected]>
For the immediate purpose as described this looks okay, so
Reviewed-by: Jan Beulich <[email protected]>
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 <[email protected]>
> >
> > 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: <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Stefano Stabellini <[email protected]>
>
> For the immediate purpose as described this looks okay, so
> Reviewed-by: Jan Beulich <[email protected]>
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 <[email protected]>
>
> 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: <[email protected]>
> Cc: [email protected]
> Signed-off-by: Stefano Stabellini <[email protected]>
Applied to for-linus-5.16c
-boris