linux-kernel - RE: [PATCH v2 2/4] hv_utils: Support host-initiated restart request

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 23 Jan 2020 08:11:09 +0000
From:   Dexuan Cui <decui@...rosoft.com>
To:     Michael Kelley <mikelley@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "sashal@...nel.org" <sashal@...nel.org>,
        Sasha Levin <Alexander.Levin@...rosoft.com>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        vkuznets <vkuznets@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 2/4] hv_utils: Support host-initiated restart request

> From: Michael Kelley <mikelley@...rosoft.com>
> Sent: Wednesday, January 22, 2020 9:16 AM
> >
> > To test the code, run this command on the host:
> >
> > Restart-VM $vm -Type Reboot
> 
> Need a better commit message here.  How about:
> 
> The hv_util driver currently supports a "shutdown" operation initiated from the
> Hyper-V host.  Newer versions of Hyper-V also support a "restart" operation.
> So add support for the updated protocol version that has "restart" support, and
> perform a clean reboot when such a message is received from Hyper-V.
> 
> To test the restart functionality, run this PowerShell command on the Hyper-V
> host:
> 
> Restart-VM  <vmname>  -Type Reboot

Thanks a lot! I'll use this version. 

> > @@ -166,6 +179,14 @@ static void shutdown_onchannelcallback(void
> *context)
> >  				pr_info("Shutdown request received -"
> >  					    " graceful shutdown initiated\n");
> >  				break;
> > +			case 2:
> > +			case 3:
> 
> How are the flags values 0, 1, 2, and 3 interpreted?  Perhaps a short comment
> would be helpful.

If bit 0 is 1, it means a flag of "doing the operation by force".  IMO we'd like to
always perform the operation by force, even if the host doesn't set the flag -- this
is what the existing shutdown handling code does.

I'll add a comment.

> 
> > +				pr_info("Restart request received -"
> > +					    " graceful restart initiated\n");
> > +				icmsghdrp->status = HV_S_OK;
> > +
> > +				schedule_work(&restart_work);
> > +				break;
> 
> For case 0 and 1 (shutdown), the schedule_work() call is performed only
> after the response packet has been sent to the host.  Is there a reason the
> new code for case 2 and 3 (restart) is doing it in the opposite order?

The channel callback runs in a tasklet context, and an active tasklet handler can
not be cancelled, so even if the "work" starts to run on another CPU immediately,
I'm sure the channel callback will send the response packet and finish normally.

This way we can save a local bool variable "execute_reboot". :-)

But, let me change the patch to follow the shutdown handling code for
consistency.

Thanks,
-- Dexuan