Pkg xen devel - Oct 2018 - Ill-advised use of xs_open flag 1UL<<2 by Debian

tl;dr
  The Debian Xen packages have had a very bad patch which I propose to
  simply drop, with minor compatibility implications, unless someone
  can explain what it is for and why it is still needed, and/or has a
  better plan.



I have been going through delta queue in the Debian Xen package.
I found a commit (patch) describing itself only this way:
   xenstore/tools-xenstore-compatibility.diff
Yes, that was the whole of the description.

The patch
 - defines XS_OPEN_DOMAINONLY to 1UL<<2
 - arranges that when this flag is passed, xs_open will never try
   the socket, only the xenstore kernel device
 - always passes that flag in the xenstore utilities.

Digging into snapshot.d.o I found this changelog entry in 4.1.0-2,
  * Workaround incompatibility with xenstored of Xen 4.0.
which I can tell by diffing 4.1.0-1 and 4.1.0-2 must refer to this.
I was not able to discover what the incompatibility was.  There
did not seem to be any related archived bugs, or commentary, or
anything.  It may be related to one of these
  d31038383192 xenstore: new XS_OPEN_SOCKETONLY flag ...
  999241f7aa45 Adds an open xenstore connection function ...
(commitids are from usptream xen.git)

In any case, if this patch previously served any purpose it doesn't
seem to any more.  xenstore socket connections work just fine in dom0,
and attempting them in domUs does no harm.  I built salsa/master with
this patch reverted and both an HVM and PV domU had no trouble
accessing xenstore.

This patch was extremely ill-advised because it stole a bit from the
xs_open flag bitmap without coordination with upstream.

In Xen 4.3, upstream instroduced XS_UNWATCH_FILTER with value 1UL<<2.

In April 2011, the Debian Xen packaging was updated to Xen 4.4; this
included rebasing the patch queue.  At this time, this patch would
have produced a conflict - both textual (context for the Debian patch
changed due to the upstream addition) and semantic (same value used
twice).  Unfortunately the conflict was resolved textually, and the
semantic conflict was ignored.  Debian's 4.4 simply took both
additions without noticing that the value 1UL<<2 was used for two
different purposes.

This simple #define style of flag allocation does not provide any
programmatic way of detecting this kind of situation.  But perhaps a
contributing factor to this further error was that, upstream, while
XS_UNWATCH_FILTER is a flag to xs_open, the name chosen did not match
XS_OPEN_*.

Since then, the Debian Xen packages have conflated these two flags:

Attempts by any application or program linked against libxenstore to
use XS_UNWATCH_FILTER will have had as a side effect that they would
use only the xenstore domain connection (via the kernel device) and
not the socket.  I think that is almost always almost entirely
harmless.

The converse is that any programs trying to use XS_OPEN_DOMAINONLY
would get the watch filtering effect.  In a complex application the
watch filtering effect could cause lost xenstore watch events.
Luckily I think the only thing that is likely to pass this flag is the
xenstore utilities in the Debian Xen packages and those are not
complicated enough to be affected.

I propose to resolve this situation in Debian by summarily dropping
this patch in the next Debian stable release.  (We will leave well
enough alone in the previous two Debian releases.)  I think the only
user of XS_OPEN_DOMAINONLY is probably in src:xen itself.

I also observe that, upstream, these flag values are not enclosed in
( ) as they clearly should be.  I will send a patch upstream to fix
that.


If anyone has any information about this situation, particularly
information or analysis which might suggest that just dropping the
patch would be the wrong thing to do, please let us (me, and
pkg-xen-deve) know.

Thanks,
Ian.

-- 
Ian Jackson <ijackson at chiark.greenend.org.uk>   These opinions are my
own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

Hi Ian

On Tue, Oct 09, 2018 at 04:04:24PM +0100, Ian Jackson
wrote:
>   The Debian Xen packages have had a very bad patch which I propose to
>   simply drop, with minor compatibility implications, unless someone
>   can explain what it is for and why it is still needed, and/or has a
>   better plan.
It is still needed, as the underlying change (done by you) constitutes
an ABI change of the library.  I fixed it because I found that tools
where not longer able to connect to XenStore.

commit d310383831923070727745af97776dcb6078e4f7
Author: Ian Jackson <Ian.Jackson at eu.citrix.com>
Date:   Tue Dec 14 16:56:54 2010 +0000

Bastian

-- 
Those who hate and fight must stop themselves -- otherwise it is not stopped.
		-- Spock, "Day of the Dove", stardate unknown

Hi.  Thanks for taking the time to help.

Bastian Blank writes ("Re: [Pkg-xen-devel] Ill-advised use of xs_open flag
1UL<<2 by Debian"):
> On Tue, Oct 09, 2018 at 04:04:24PM +0100, Ian Jackson wrote:
> >   The Debian Xen packages have had a very bad patch which I propose to
> >   simply drop, with minor compatibility implications, unless someone
> >   can explain what it is for and why it is still needed, and/or has a
> >   better plan.
> 
> It is still needed, as the underlying change (done by you) constitutes
> an ABI change of the library.
...
> commit d310383831923070727745af97776dcb6078e4f7
> Author: Ian Jackson <Ian.Jackson at eu.citrix.com>
> Date:   Tue Dec 14 16:56:54 2010 +0000
I looked at that change and I think I see a glimmer of an ABI problem:
That change introduces a new XS_OPEN_SOCKETONLY flag which is ignored
by old libraries, and changes the tools to use it.

So if the xenstore tools are upgraded first, they will call xs_open
with XS_OPEN_SOCKETONLY (rather than, previously xs_daemon_open), but
XS_OPEN_SOCKETONLY would be ignored and the problem for which it was
invented would occur.

I think this means we should have bumped the library (or symbol) minor
version and thereby caused a dependency to ensure that the library
would be upgraded first.

However, I don't think this can be the whole picture because I don't
see how the XS_OPEN_DOMAINONLY flag would avoid this problem.  Or,
indeed, any ABI problem.  It is passed, and understood, only by new
utilities and libraries.

But that was all a long time ago.  There is no longer any need to make
upgrades from squeeze or whatever work correctly.


If this functionality (XS_OPEN_DOMAINONLY) is still needed then we
need to properly allocate it a new flag bit which won't clash with
upstream.

But I'm not convinced it is needed.  As I say, I reverted this patch
and the resulting setup works fine.  I tested it with new binaries,
without XS_OPEN_DOMAINONLY, and also a new library which doesn't
honour 1<<2 that way, but I don't see how there would be a difficulty
with upgrade skew either.


Thanks,
Ian.