Re: [PATCH 3/5] tools/libs/store: drop read-only functionality

From: "Jürgen Groß" <jgross@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>
Cc: xen-devel@lists.xenproject.org, Ian Jackson <iwj@xenproject.org>
Subject: Re: [PATCH 3/5] tools/libs/store: drop read-only functionality
Date: Wed, 7 Oct 2020 14:45:29 +0200	[thread overview]
Message-ID: <72542057-f5d8-99d5-55d9-2a21b7cb2d93@suse.com> (raw)
In-Reply-To: <f4b6a87c-ac65-cb3e-d4b2-4504340b81e3@citrix.com>

On 07.10.20 13:50, Andrew Cooper wrote:
> On 07/10/2020 11:57, Jürgen Groß wrote:
>> On 07.10.20 12:54, Wei Liu wrote:
>>> On Fri, Oct 02, 2020 at 05:41:39PM +0200, Juergen Gross wrote:
>>>> Today it is possible to open the connection in read-only mode via
>>>> xs_daemon_open_readonly(). This is working only with Xenstore running
>>>> as a daemon in the same domain as the user. Additionally it doesn't
>>>> add any security as accessing the socket used for that functionality
>>>> requires the same privileges as the socket used for full Xenstore
>>>> access.
>>>>
>>>> So just drop the read-only semantics in all cases, leaving the
>>>> interface existing only for compatibility reasons. This in turn
>>>> requires to just ignore the XS_OPEN_READONLY flag.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>    tools/libs/store/include/xenstore.h | 8 --------
>>>>    tools/libs/store/xs.c               | 7 ++-----
>>>>    2 files changed, 2 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/tools/libs/store/include/xenstore.h
>>>> b/tools/libs/store/include/xenstore.h
>>>> index cbc7206a0f..158e69ef83 100644
>>>> --- a/tools/libs/store/include/xenstore.h
>>>> +++ b/tools/libs/store/include/xenstore.h
>>>> @@ -60,15 +60,12 @@ typedef uint32_t xs_transaction_t;
>>>>    /* Open a connection to the xs daemon.
>>>>     * Attempts to make a connection over the socket interface,
>>>>     * and if it fails, then over the  xenbus interface.
>>>> - * Mode 0 specifies read-write access, XS_OPEN_READONLY for
>>>> - * read-only access.
>>>>     *
>>>>     * * Connections made with xs_open(0) (which might be shared page or
>>>>     *   socket based) are only guaranteed to work in the parent after
>>>>     *   fork.
>>>>     * * xs_daemon_open*() and xs_domain_open() are deprecated synonyms
>>>>     *   for xs_open(0).
>>>> - * * XS_OPEN_READONLY has no bearing on any of this.
>>>>     *
>>>>     * Returns a handle or NULL.
>>>>     */
>>>> @@ -83,11 +80,6 @@ void xs_close(struct xs_handle *xsh /* NULL ok */);
>>>>     */
>>>>    struct xs_handle *xs_daemon_open(void);
>>>>    struct xs_handle *xs_domain_open(void);
>>>> -
>>>> -/* Connect to the xs daemon (readonly for non-root clients).
>>>> - * Returns a handle or NULL.
>>>> - * Deprecated, please use xs_open(XS_OPEN_READONLY) instead
>>>> - */
>>>>    struct xs_handle *xs_daemon_open_readonly(void);
>>>>      /* Close the connection to the xs daemon.
>>>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>>>> index 320734416f..4ac73ec317 100644
>>>> --- a/tools/libs/store/xs.c
>>>> +++ b/tools/libs/store/xs.c
>>>> @@ -302,7 +302,7 @@ struct xs_handle *xs_daemon_open(void)
>>>>      struct xs_handle *xs_daemon_open_readonly(void)
>>>>    {
>>>> -    return xs_open(XS_OPEN_READONLY);
>>>> +    return xs_open(0);
>>>>    }
>>>
>>> This changes the semantics of this function, isn't it? In that the user
>>> expects a readonly connection but in fact a read-write connection is
>>> returned.
>>>
>>> Maybe we should return an error here?
>>
>> No, as the behavior is this way already if any of the following is true:
>>
>> - a guest is opening the connection
>> - Xenstore is running in a stubdom
>> - oxenstored is being used
> 
> Right, and these are just some of the reasons why dropping the R/O
> socket is a sensible thing to do.
> 
> However, I do think xenstore.h needs large disclaimers next to the
> relevant constants and functions that both XS_OPEN_* flags are now
> obsolete and ignored (merged into appropriate patches in the series).

Fine with me.

Juergen