Re: [Xen-devel] [PATCH] kexec: Add spinlock for the whole hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.04.17 at 21:44, <eric.devolder@xxxxxxxxxx> wrote:

Please don't forget Cc-ing the maintainer(s) of the code being modified.

> @@ -1187,12 +1182,22 @@ static int do_kexec_op_internal(unsigned long op,
>                                  XEN_GUEST_HANDLE_PARAM(void) uarg,
>                                  bool_t compat)
>  {
> +    static DEFINE_SPINLOCK(kexec_lock);
>      int ret = -EINVAL;
>  
>      ret = xsm_kexec(XSM_PRIV);
>      if ( ret )
>          return ret;
>  
> +    /*
> +     * Because we write directly to the reserved memory
> +     * region when loading crash kernels we need a spinlock here to
> +     * prevent multiple crash kernels from attempting to load
> +     * simultaneously, and to prevent a crash kernel from loading
> +     * over the top of a in use crash kernel.
> +     */
> +    spin_lock(&kexec_lock);
> +
>      switch ( op )
>      {
>      case KEXEC_CMD_kexec_get_range:
> @@ -1227,6 +1232,8 @@ static int do_kexec_op_internal(unsigned long op,
>          break;
>      }
>  
> +    spin_unlock(&kexec_lock);
> +
>      return ret;
>  }

How long can a kexec-op take? Are you putting systems at risk of
the watchdog firing? Even if you don't, you put all sorts of
operations inside a lock now which preferably should run outside
of atomic context (memory allocation, guest memory accesses).
If such a global locking approach is really necessary (i.e. if we
can't expect the - privileged - caller to synchronize calls properly),
wouldn't it be better to handle this with a static state variable,
which gets checked/set atomically, and which if already set simply
leads to a continuation being arranged for?

Furthermore - are there problems also with e.g. loading a
default and a crash kernel in parallel? I.e. aren't you doing more
synchronization than really necessary?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel