linux-kernel - RE: [PATCH v2 3/4] hv_utils: Support host-initiated hibernation 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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Jan 2020 17:39:00 +0000
From:   Michael Kelley <mikelley@...rosoft.com>
To:     Dexuan Cui <decui@...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 3/4] hv_utils: Support host-initiated hibernation
 request

From: Dexuan Cui <decui@...rosoft.com> Sent: Sunday, January 12, 2020 10:32 PM
> 
> Update the Shutdown IC version to 3.2, which is required for the host to
> send the hibernation request.
> 
> The user is expected to create the below udev rule file, which is applied
> upon the host-initiated hibernation request:
> 
> root@...alhost:~# cat /usr/lib/udev/rules.d/40-vm-hibernation.rules
> SUBSYSTEM=="vmbus", ACTION=="change", DRIVER=="hv_utils",
> ENV{EVENT}=="hibernate", RUN+="/usr/bin/systemctl hibernate"
> 
> Signed-off-by: Dexuan Cui <decui@...rosoft.com>
> ---
>  drivers/hv/hv_util.c | 52 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index fe3a316380c2..d5216af62788 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -25,7 +25,9 @@
>  #define SD_MAJOR	3
>  #define SD_MINOR	0
>  #define SD_MINOR_1	1
> +#define SD_MINOR_2	2
>  #define SD_VERSION_3_1	(SD_MAJOR << 16 | SD_MINOR_1)
> +#define SD_VERSION_3_2	(SD_MAJOR << 16 | SD_MINOR_2)
>  #define SD_VERSION	(SD_MAJOR << 16 | SD_MINOR)
> 
>  #define SD_MAJOR_1	1
> @@ -52,9 +54,10 @@ static int sd_srv_version;
>  static int ts_srv_version;
>  static int hb_srv_version;
> 
> -#define SD_VER_COUNT 3
> +#define SD_VER_COUNT 4
>  static const int sd_versions[] = {
>  	SD_VERSION_3_1,
> +	SD_VERSION_3_2,

I think these versions need to listed in descending order, so the new
SD_VERSION_3_2 should be listed first.  Otherwise a Hyper-V host that
supports both 3.1 and 3.2 might match on 3.1 first without ever checking
for a match with 3.2.

>  	SD_VERSION,
>  	SD_VERSION_1
>  };
> @@ -78,9 +81,45 @@ static const int fw_versions[] = {
>  	UTIL_WS2K8_FW_VERSION
>  };
> 
> +/*
> + * Send the "hibernate" udev event in a thread context.
> + */
> +struct hibernate_work_context {
> +	struct work_struct work;
> +	struct hv_device *dev;
> +};
> +
> +static struct hibernate_work_context hibernate_context;
> +static bool execute_hibernate;
> +
> +static void send_hibernate_uevent(struct work_struct *work)
> +{
> +	char *uevent_env[2] = { "EVENT=hibernate", NULL };
> +	struct hibernate_work_context *ctx;
> +
> +	ctx = container_of(work, struct hibernate_work_context, work);
> +
> +	kobject_uevent_env(&ctx->dev->device.kobj, KOBJ_CHANGE, uevent_env);
> +
> +	pr_info("Sent hibernation uevent\n");
> +}
> +
> +static int hv_shutdown_init(struct hv_util_service *srv)
> +{
> +	struct vmbus_channel *channel = srv->channel;
> +
> +	INIT_WORK(&hibernate_context.work, send_hibernate_uevent);
> +	hibernate_context.dev = channel->device_obj;
> +
> +	execute_hibernate = hv_is_hibernation_supported();
> +
> +	return 0;
> +}
> +
>  static void shutdown_onchannelcallback(void *context);
>  static struct hv_util_service util_shutdown = {
>  	.util_cb = shutdown_onchannelcallback,
> +	.util_init = hv_shutdown_init,
>  };
> 
>  static int hv_timesync_init(struct hv_util_service *srv);
> @@ -187,6 +226,17 @@ static void shutdown_onchannelcallback(void *context)
> 
>  				schedule_work(&restart_work);
>  				break;
> +			case 4:
> +			case 5:

As before, I'm wondering about the interpretation of these numbers.

> +				pr_info("Hibernation request received\n");
> +
> +				if (execute_hibernate) {
> +					icmsghdrp->status = HV_S_OK;
> +					schedule_work(&hibernate_context.work);

Same comment here about the ordering of the schedule_work() call and the
sending of the response message.  Seems like the code should be consistent
for all three cases -- shutdown, restart, and hibernate.

> +				} else {
> +					icmsghdrp->status = HV_E_FAIL;
> +				}
> +				break;
>  			default:
>  				icmsghdrp->status = HV_E_FAIL;
>  				execute_shutdown = false;
> --
> 2.19.1