linux-kernel - Re: [PATCH v06 27/36] uapi xen/gntdev.h: include xen/privcmd.h and define grant_ref

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]

Message-ID: <1e7f62c1-d1d8-a45f-d0e0-a88f618e4f2a@oracle.com>
Date:   Mon, 21 Aug 2017 10:59:36 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Juergen Gross <jgross@...e.com>,
        Mikko Rapeli <mikko.rapeli@....fi>,
        linux-kernel@...r.kernel.org, linux-api@...r.kernel.org
Cc:     xen-devel@...ts.xenproject.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>
Subject: Re: [PATCH v06 27/36] uapi xen/gntdev.h: include xen/privcmd.h and
 define grant_ref_t

On 08/21/2017 10:46 AM, Juergen Gross wrote:
> On 21/08/17 16:31, Boris Ostrovsky wrote:
>> On 08/21/2017 09:33 AM, Juergen Gross wrote:
>>> On 06/08/17 18:44, Mikko Rapeli wrote:
>>>> Both are needed to compile <xen/gntdev.h> in userspace. Fixes these
>>>> userspace compile errors:
>>>>
>>>> xen/gntdev.h:151:4: error: unknown type name ‘grant_ref_t’
>>>>     grant_ref_t ref;
>>>>     ^
>>>> xen/gntdev.h:153:4: error: unknown type name ‘domid_t’
>>>>     domid_t domid;
>>>>     ^
>>>>
>>>> Signed-off-by: Mikko Rapeli <mikko.rapeli@....fi>
>>>> Cc: Boris Ostrovsky <boris.ostrovsky@...cle.com>
>>>> Cc: Juergen Gross <jgross@...e.com>
>>>> Cc: xen-devel@...ts.xenproject.org
>>>> ---
>>>>  include/uapi/xen/gntdev.h           | 6 ++++++
>>>>  include/xen/interface/grant_table.h | 6 +-----
>>>>  2 files changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/include/uapi/xen/gntdev.h b/include/uapi/xen/gntdev.h
>>>> index d0661977667e..f208706b07c9 100644
>>>> --- a/include/uapi/xen/gntdev.h
>>>> +++ b/include/uapi/xen/gntdev.h
>>>> @@ -34,6 +34,12 @@
>>>>  #define __LINUX_PUBLIC_GNTDEV_H__
>>>>  
>>>>  #include <linux/types.h>
>>>> +#include <xen/privcmd.h>
>>>> +
>>>> +/*
>>>> + * Reference to a grant entry in a specified domain's grant table.
>>>> + */
>>>> +typedef __u32 grant_ref_t;
>>>>  
>>>>  struct ioctl_gntdev_grant_ref {
>>>>  	/* The domain ID of the grant to be mapped. */
>>>> diff --git a/include/xen/interface/grant_table.h b/include/xen/interface/grant_table.h
>>>> index 7fb7112d667c..29b3724aec2c 100644
>>>> --- a/include/xen/interface/grant_table.h
>>>> +++ b/include/xen/interface/grant_table.h
>>>> @@ -29,6 +29,7 @@
>>>>  #define __XEN_PUBLIC_GRANT_TABLE_H__
>>>>  
>>>>  #include <xen/interface/xen.h>
>>>> +#include <uapi/xen/gntdev.h> /* for grant_ref_t */
>>> Hmm, I really don't like this. We import the Xen headers from the Xen
>>> source repository. The Xen specific types shouldn't be taken from
>>> non-Xen headers.
>>>
>>>>  
>>>>  /***********************************
>>>>   * GRANT TABLE REPRESENTATION
>>>> @@ -85,11 +86,6 @@
>>>>   */
>>>>  
>>>>  /*
>>>> - * Reference to a grant entry in a specified domain's grant table.
>>>> - */
>>>> -typedef uint32_t grant_ref_t;
>>>> -
>>>> -/*
>>>>   * A grant table comprises a packed array of grant entries in one or more
>>>>   * page frames shared between Xen and a guest.
>>>>   * [XEN]: This field is written by Xen and read by the sharing guest.
>>>>
>>> I'd rather use __u32 instead of grant_ref_t in the uapi headers.
>>>
>>> Boris, what do you think?
>> I agree that including a non-Xen file into include/xen/interface is not
>> especially desirable.
>>
>> Can we ifdef __KERNEL__grant_ref_t usage in uapi header, with else
>> clause using __u32? And maybe stick BUILD_BUG_ON somewhere to check that
>> sizes are still the same ?
> I'd rather use __u32 unconditionally and have the BUILD_BUG_ON in the
> grant or privcmd driver.
>
> Otherwise we'd depend on the sequence of #includes (what happens if a
> driver includes the uapi header before xen/interface/xen.h?)

OK, good point.

-boris