Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752349AbdFNPAc (ORCPT <rfc822;w@1wt.eu>);
        Wed, 14 Jun 2017 11:00:32 -0400
Received: from aserp1040.oracle.com ([141.146.126.69]:19061 "EHLO
        aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751836AbdFNPAa (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 14 Jun 2017 11:00:30 -0400
Subject: Re: [PATCH v3 4/4] xen: add sysfs node for hypervisor build id
To: Juergen Gross <jgross@suse.com>, linux-kernel@vger.kernel.org,
        xen-devel@lists.xenproject.org
References: <20170612142150.15283-1-jgross@suse.com>
 <20170612142150.15283-5-jgross@suse.com>
Cc: gregkh@linuxfoundation.org
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Message-ID: <7dda9b9a-2335-3484-6170-2bb621c82a0a@oracle.com>
Date: Wed, 14 Jun 2017 11:00:13 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170612142150.15283-5-jgross@suse.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
X-Source-IP: aserv0021.oracle.com [141.146.126.233]
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3078
Lines: 97

On 06/12/2017 10:21 AM, Juergen Gross wrote:
> For support of Xen hypervisor live patching the hypervisor build id is
> needed. Add a node /sys/hypervisor/properties/buildid containing the
> information.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  Documentation/ABI/testing/sysfs-hypervisor-xen | 11 +++++++++-
>  drivers/xen/sys-hypervisor.c                   | 29 ++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-hypervisor-xen b/Documentation/ABI/testing/sysfs-hypervisor-xen
> index c0edb3fdd6eb..53b7b2ea7515 100644
> --- a/Documentation/ABI/testing/sysfs-hypervisor-xen
> +++ b/Documentation/ABI/testing/sysfs-hypervisor-xen
> @@ -1,5 +1,5 @@
>  What:		/sys/hypervisor/guest_type
> -Date:		May 2017
> +Date:		June 2017
>  KernelVersion:	4.13
>  Contact:	xen-devel@lists.xenproject.org
>  Description:	If running under Xen:
> @@ -32,3 +32,12 @@ Description:	If running under Xen:
>  		Describes Xen PMU features (as an integer). A set bit indicates
>  		that the corresponding feature is enabled. See
>  		include/xen/interface/xenpmu.h for available features
> +
> +What:		/sys/hypervisor/properties/buildid
> +Date:		June 2017
> +KernelVersion:	4.13
> +Contact:	xen-devel@lists.xenproject.org
> +Description:	If running under Xen:
> +		Build id of the hypervisor, needed for hypervisor live patching.
> +		Might return "<denied>" in case of special security settings
> +		in the hypervisor.

It might? I don't see xen_deny() calls in XENVER_build_id (as I said
below, assuming that's the command you are using).

> diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c
> index d641e9970d5d..92307636ed54 100644
> --- a/drivers/xen/sys-hypervisor.c
> +++ b/drivers/xen/sys-hypervisor.c
> @@ -339,12 +339,41 @@ static ssize_t features_show(struct hyp_sysfs_attr *attr, char *buffer)
>  
>  HYPERVISOR_ATTR_RO(features);
>  
> +static ssize_t buildid_show(struct hyp_sysfs_attr *attr, char *buffer)
> +{
> +	ssize_t ret;
> +	struct xen_build_id dummy;
> +	struct xen_build_id *buildid;
> +
> +	dummy.len = 0;
> +	ret = HYPERVISOR_xen_version(XENVER_get_features, &dummy);

Why XENVER_get_features and not XENVER_build_id?

> +	if (ret < 0) {
> +		if (ret == -EPERM)
> +			ret = sprintf(buffer, "<denied>");
> +		return ret;
> +	}

Assuming you meant XENVER_build_id, how is this supposed to work?
Hypervisor code specifically has

    if ( build_id.len == 0 )
                return -EINVAL;

-boris

> +
> +	buildid = kmalloc(sizeof(*buildid) + dummy.len, GFP_KERNEL);
> +	if (!buildid)
> +		return -ENOMEM;
> +
> +	ret = HYPERVISOR_xen_version(XENVER_get_features, buildid);
> +	if (ret > 0)
> +		ret = sprintf(buffer, "%s", buildid->buf);
> +	kfree(buildid);
> +
> +	return ret;
> +}
> +
> +HYPERVISOR_ATTR_RO(buildid);
> +
>  static struct attribute *xen_properties_attrs[] = {
>  	&capabilities_attr.attr,
>  	&changeset_attr.attr,
>  	&virtual_start_attr.attr,
>  	&pagesize_attr.attr,
>  	&features_attr.attr,
> +	&buildid_attr.attr,
>  	NULL
>  };
>