[PATCH 0/5] tools/xenstore: remove read-only socket

[PATCH 0/5] tools/xenstore: remove read-only socket

[Juergen Gross]

The read-only socket of Xenstore is usable for the daemon case only
and even there it is not really worth to be kept, as not all
Xesntore operations changing the state of Xenstore are blocked,
oxenstored ignoring the read-only semantics completely, and the
privileges for using the ro-socket being the same as for the normal
rw-socket.

So remove this feature with switching the related use cases to the
Xenstore-type agnostic open- and close-functions..

Juergen Gross (5):
  tools/xenstore: remove socket-only option from xenstore client
  tools/libs/store: ignore XS_OPEN_SOCKETONLY flag
  tools/libs/store: drop read-only functionality
  tools: drop all deprecated usages of xs_*_open() and friends in tools
  tools/xenstore: drop creation of read-only socket in xenstored

 docs/man/xenstore-chmod.1.pod           |  4 --
 docs/man/xenstore-ls.1.pod              |  4 --
 docs/man/xenstore-read.1.pod            |  4 --
 docs/man/xenstore-write.1.pod           |  4 --
 tools/console/client/main.c             |  2 +-
 tools/console/daemon/utils.c            |  4 +-
 tools/libs/light/libxl.c                |  6 +--
 tools/libs/light/libxl_exec.c           |  6 +--
 tools/libs/light/libxl_fork.c           |  2 +-
 tools/libs/stat/xenstat.c               |  4 +-
 tools/libs/store/include/xenstore.h     | 10 -----
 tools/libs/store/xs.c                   |  9 ++--
 tools/libs/vchan/init.c                 | 10 ++---
 tools/misc/xen-lowmemd.c                |  4 +-
 tools/python/xen/lowlevel/xs/xs.c       |  6 +--
 tools/tests/mce-test/tools/xen-mceinj.c |  4 +-
 tools/xenbackendd/xenbackendd.c         |  4 +-
 tools/xenpmd/xenpmd.c                   |  6 +--
 tools/xenstore/xenstore_client.c        |  8 +---
 tools/xenstore/xenstored_core.c         | 55 +++++--------------------
 tools/xenstore/xenstored_core.h         |  3 --
 tools/xenstore/xenstored_domain.c       |  4 +-
 tools/xenstore/xs_lib.c                 |  8 +---
 23 files changed, 46 insertions(+), 125 deletions(-)

-- 
2.26.2

[PATCH 1/5] tools/xenstore: remove socket-only option from xenstore client

[Juergen Gross]

The Xenstore access commands (xenstore-*) have the possibility to limit
connection to Xenstore to a local socket (option "-s"). This is an
option making no sense at all, as either there is only a socket, so
the option would be a nop, or there is no socket at all (in case
Xenstore is running in a stubdom or the client is called in a domU),
so specifying the option would just lead to failure.

So drop that option completely.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/man/xenstore-chmod.1.pod    | 4 ----
 docs/man/xenstore-ls.1.pod       | 4 ----
 docs/man/xenstore-read.1.pod     | 4 ----
 docs/man/xenstore-write.1.pod    | 4 ----
 tools/xenstore/xenstore_client.c | 8 ++------
 5 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/docs/man/xenstore-chmod.1.pod b/docs/man/xenstore-chmod.1.pod
index d76f34723d..d221f5dffc 100644
--- a/docs/man/xenstore-chmod.1.pod
+++ b/docs/man/xenstore-chmod.1.pod
@@ -46,10 +46,6 @@ write, and set permissions).
 
 Apply the permissions to the key and all its I<children>.
 
-=item B<-s>
-
-Connect to the Xenstore daemon using a local socket only.
-
 =item B<-u>
 
 Apply the permissions to the key and all its I<parents>.
diff --git a/docs/man/xenstore-ls.1.pod b/docs/man/xenstore-ls.1.pod
index 8dac931e94..a9f8b32653 100644
--- a/docs/man/xenstore-ls.1.pod
+++ b/docs/man/xenstore-ls.1.pod
@@ -50,10 +50,6 @@ I<and> the permissions for any domain not explicitly listed in
 subsequent entries.  The key owner always has full access (read,
 write, and set permissions).
 
-=item B<-s>
-
-Connect to the Xenstore daemon using a local socket only.
-
 =back
 
 =head1 BUGS
diff --git a/docs/man/xenstore-read.1.pod b/docs/man/xenstore-read.1.pod
index f5a7bb7e46..c7768cbbe5 100644
--- a/docs/man/xenstore-read.1.pod
+++ b/docs/man/xenstore-read.1.pod
@@ -16,10 +16,6 @@ Read values of one or more Xenstore I<PATH>s.
 
 Prefix value with key name.
 
-=item B<-s>
-
-Connect to the Xenstore daemon using a local socket only.
-
 =item B<-R>
 
 Read raw value, skip escaping non-printable characters (\x..).
diff --git a/docs/man/xenstore-write.1.pod b/docs/man/xenstore-write.1.pod
index d1b011236a..a0b1bca333 100644
--- a/docs/man/xenstore-write.1.pod
+++ b/docs/man/xenstore-write.1.pod
@@ -13,10 +13,6 @@ provided to write them at once - in one Xenstore transaction.
 
 =over
 
-=item B<-s>
-
-Connect to the Xenstore daemon using a local socket only.
-
 =item B<-R>
 
 Write raw value, skip parsing escaped characters (\x..).
diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
index ae7ed3eb9e..8015bfe5be 100644
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -530,7 +530,7 @@ main(int argc, char **argv)
 {
     struct xs_handle *xsh;
     xs_transaction_t xth = XBT_NULL;
-    int ret = 0, socket = 0;
+    int ret = 0;
     int prefix = 0;
     int tidy = 0;
     int upto = 0;
@@ -565,7 +565,6 @@ main(int argc, char **argv)
 	static struct option long_options[] = {
 	    {"help",    0, 0, 'h'},
 	    {"flat",    0, 0, 'f'}, /* MODE_ls */
-	    {"socket",  0, 0, 's'},
 	    {"prefix",  0, 0, 'p'}, /* MODE_read || MODE_list || MODE_ls */
 	    {"tidy",    0, 0, 't'}, /* MODE_rm */
 	    {"upto",    0, 0, 'u'}, /* MODE_chmod */
@@ -593,9 +592,6 @@ main(int argc, char **argv)
 		usage(mode, switch_argv, argv[0]);
 	    }
             break;
-        case 's':
-            socket = 1;
-            break;
 	case 'p':
 	    if ( mode == MODE_read || mode == MODE_list || mode == MODE_ls )
 		prefix = 1;
@@ -675,7 +671,7 @@ main(int argc, char **argv)
 	    max_width = ws.ws_col - 2;
     }
 
-    xsh = xs_open(socket ? XS_OPEN_SOCKETONLY : 0);
+    xsh = xs_open(0);
     if (xsh == NULL) err(1, "xs_open");
 
 again:
-- 
2.26.2

[PATCH 2/5] tools/libs/store: ignore XS_OPEN_SOCKETONLY flag

[Juergen Gross]

When opening the connection to Xenstore via xs_open() it makes no
sense to limit the connection to the socket based one. So just ignore
the XS_OPEN_SOCKETONLY flag.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libs/store/include/xenstore.h | 2 --
 tools/libs/store/xs.c               | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/libs/store/include/xenstore.h b/tools/libs/store/include/xenstore.h
index 25b31881c8..cbc7206a0f 100644
--- a/tools/libs/store/include/xenstore.h
+++ b/tools/libs/store/include/xenstore.h
@@ -66,8 +66,6 @@ typedef uint32_t xs_transaction_t;
  * * Connections made with xs_open(0) (which might be shared page or
  *   socket based) are only guaranteed to work in the parent after
  *   fork.
- * * Connections made with xs_open(XS_OPEN_SOCKETONLY) will be usable
- *   in either the parent or the child after fork, but not both.
  * * xs_daemon_open*() and xs_domain_open() are deprecated synonyms
  *   for xs_open(0).
  * * XS_OPEN_READONLY has no bearing on any of this.
diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index aa1d24b8b9..320734416f 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -319,7 +319,7 @@ struct xs_handle *xs_open(unsigned long flags)
 	else
 		xsh = get_handle(xs_daemon_socket());
 
-	if (!xsh && !(flags & XS_OPEN_SOCKETONLY))
+	if (!xsh)
 		xsh = get_handle(xs_domain_dev());
 
 	if (xsh && (flags & XS_UNWATCH_FILTER))
-- 
2.26.2

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

[Juergen Gross]

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);
 }
 
 struct xs_handle *xs_domain_open(void)
@@ -314,10 +314,7 @@ struct xs_handle *xs_open(unsigned long flags)
 {
 	struct xs_handle *xsh = NULL;
 
-	if (flags & XS_OPEN_READONLY)
-		xsh = get_handle(xs_daemon_socket_ro());
-	else
-		xsh = get_handle(xs_daemon_socket());
+	xsh = get_handle(xs_daemon_socket());
 
 	if (!xsh)
 		xsh = get_handle(xs_domain_dev());
-- 
2.26.2

[PATCH 4/5] tools: drop all deprecated usages of xs_*_open() and friends in tools

[Juergen Gross]

Switch all usages of xs_daemon_open*() and xs_domain_open() to use
xs_open() instead. While at it switch xs_daemon_close() users to
xs_close().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/console/client/main.c             |  2 +-
 tools/console/daemon/utils.c            |  4 ++--
 tools/libs/light/libxl.c                |  6 ++----
 tools/libs/light/libxl_exec.c           |  6 +++---
 tools/libs/light/libxl_fork.c           |  2 +-
 tools/libs/stat/xenstat.c               |  4 ++--
 tools/libs/vchan/init.c                 | 10 ++++------
 tools/misc/xen-lowmemd.c                |  4 ++--
 tools/python/xen/lowlevel/xs/xs.c       |  6 +++---
 tools/tests/mce-test/tools/xen-mceinj.c |  4 ++--
 tools/xenbackendd/xenbackendd.c         |  4 ++--
 tools/xenpmd/xenpmd.c                   |  6 +++---
 12 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index f92ad3d8cf..088be28dff 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -398,7 +398,7 @@ int main(int argc, char **argv)
 		exit(EINVAL);
 	}
 
-	xs = xs_daemon_open();
+	xs = xs_open(0);
 	if (xs == NULL) {
 		err(errno, "Could not contact XenStore");
 	}
diff --git a/tools/console/daemon/utils.c b/tools/console/daemon/utils.c
index 97d7798b33..f9dd8a60c5 100644
--- a/tools/console/daemon/utils.c
+++ b/tools/console/daemon/utils.c
@@ -104,7 +104,7 @@ void daemonize(const char *pidfile)
 bool xen_setup(void)
 {
 	
-	xs = xs_daemon_open();
+	xs = xs_open(0);
 	if (xs == NULL) {
 		dolog(LOG_ERR,
 		      "Failed to contact xenstore (%m).  Is it running?");
@@ -131,7 +131,7 @@ bool xen_setup(void)
 
  out:
 	if (xs)
-		xs_daemon_close(xs);
+		xs_close(xs);
 	if (xc)
 		xc_interface_close(xc);
 	return false;
diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
index 621acc88f3..d2a87157a2 100644
--- a/tools/libs/light/libxl.c
+++ b/tools/libs/light/libxl.c
@@ -103,9 +103,7 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
         rc = ERROR_FAIL; goto out;
     }
 
-    ctx->xsh = xs_daemon_open();
-    if (!ctx->xsh)
-        ctx->xsh = xs_domain_open();
+    ctx->xsh = xs_open(0);
     if (!ctx->xsh) {
         LOGEV(ERROR, errno, "cannot connect to xenstore");
         rc = ERROR_FAIL; goto out;
@@ -171,7 +169,7 @@ int libxl_ctx_free(libxl_ctx *ctx)
 
     if (ctx->xch) xc_interface_close(ctx->xch);
     libxl_version_info_dispose(&ctx->version_info);
-    if (ctx->xsh) xs_daemon_close(ctx->xsh);
+    if (ctx->xsh) xs_close(ctx->xsh);
     if (ctx->xce) xenevtchn_close(ctx->xce);
 
     libxl__poller_put(ctx, ctx->poller_app);
diff --git a/tools/libs/light/libxl_exec.c b/tools/libs/light/libxl_exec.c
index 47c9c8f1ba..a8b949b193 100644
--- a/tools/libs/light/libxl_exec.c
+++ b/tools/libs/light/libxl_exec.c
@@ -178,7 +178,7 @@ int libxl__xenstore_child_wait_deprecated(libxl__gc *gc,
     unsigned int num;
     char **l = NULL;
 
-    xsh = xs_daemon_open();
+    xsh = xs_open(0);
     if (xsh == NULL) {
         LOG(ERROR, "Unable to open xenstore connection");
         goto err;
@@ -206,7 +206,7 @@ int libxl__xenstore_child_wait_deprecated(libxl__gc *gc,
 
         free(p);
         xs_unwatch(xsh, path, path);
-        xs_daemon_close(xsh);
+        xs_close(xsh);
         return rc;
 again:
         free(p);
@@ -226,7 +226,7 @@ again:
     LOG(ERROR, "%s not ready", what);
 
     xs_unwatch(xsh, path, path);
-    xs_daemon_close(xsh);
+    xs_close(xsh);
 err:
     return -1;
 }
diff --git a/tools/libs/light/libxl_fork.c b/tools/libs/light/libxl_fork.c
index 9a4709b9a4..5d47dceb8a 100644
--- a/tools/libs/light/libxl_fork.c
+++ b/tools/libs/light/libxl_fork.c
@@ -663,7 +663,7 @@ int libxl__ev_child_xenstore_reopen(libxl__gc *gc, const char *what) {
     int rc;
 
     assert(!CTX->xsh);
-    CTX->xsh = xs_daemon_open();
+    CTX->xsh = xs_open(0);
     if (!CTX->xsh) {
         LOGE(ERROR, "%s: xenstore reopen failed", what);
         rc = ERROR_FAIL;  goto out;
diff --git a/tools/libs/stat/xenstat.c b/tools/libs/stat/xenstat.c
index 6f93d4e982..e49689aa2d 100644
--- a/tools/libs/stat/xenstat.c
+++ b/tools/libs/stat/xenstat.c
@@ -107,7 +107,7 @@ xenstat_handle *xenstat_init(void)
 		return NULL;
 	}
 
-	handle->xshandle = xs_daemon_open_readonly(); /* open handle to xenstore*/
+	handle->xshandle = xs_open(0); /* open handle to xenstore*/
 	if (handle->xshandle == NULL) {
 		perror("unable to open xenstore");
 		xc_interface_close(handle->xc_handle);
@@ -125,7 +125,7 @@ void xenstat_uninit(xenstat_handle * handle)
 		for (i = 0; i < NUM_COLLECTORS; i++)
 			collectors[i].uninit(handle);
 		xc_interface_close(handle->xc_handle);
-		xs_daemon_close(handle->xshandle);
+		xs_close(handle->xshandle);
 		free(handle->priv);
 		free(handle);
 	}
diff --git a/tools/libs/vchan/init.c b/tools/libs/vchan/init.c
index ad4b64fbe3..c8510e6ce9 100644
--- a/tools/libs/vchan/init.c
+++ b/tools/libs/vchan/init.c
@@ -251,7 +251,7 @@ static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base
 	char ref[16];
 	char* domid_str = NULL;
 	xs_transaction_t xs_trans = XBT_NULL;
-	xs = xs_domain_open();
+	xs = xs_open(0);
 	if (!xs)
 		goto fail;
 	domid_str = xs_read(xs, 0, "domid", NULL);
@@ -293,7 +293,7 @@ retry_transaction:
 	}
  fail_xs_open:
 	free(domid_str);
-	xs_daemon_close(xs);
+	xs_close(xs);
  fail:
 	return ret;
 }
@@ -408,9 +408,7 @@ struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
 	ctrl->write.order = ctrl->read.order = 0;
 	ctrl->is_server = 0;
 
-	xs = xs_daemon_open();
-	if (!xs)
-		xs = xs_domain_open();
+	xs = xs_open(0);
 	if (!xs)
 		goto fail;
 
@@ -452,7 +450,7 @@ struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
 
  out:
 	if (xs)
-		xs_daemon_close(xs);
+		xs_close(xs);
 	return ctrl;
  fail:
 	libxenvchan_close(ctrl);
diff --git a/tools/misc/xen-lowmemd.c b/tools/misc/xen-lowmemd.c
index 79ad34cb4a..a3a2741242 100644
--- a/tools/misc/xen-lowmemd.c
+++ b/tools/misc/xen-lowmemd.c
@@ -24,7 +24,7 @@ void cleanup(void)
     if (xch)
         xc_interface_close(xch);
     if (xs_handle)
-        xs_daemon_close(xs_handle);
+        xs_close(xs_handle);
 }
 
 /* Never shrink dom0 below 1 GiB */
@@ -102,7 +102,7 @@ int main(int argc, char *argv[])
         return 2;
     }
 
-    xs_handle = xs_daemon_open();
+    xs_handle = xs_open(0);
     if (xs_handle == NULL)
     {
         perror("Failed to open xenstore connection");
diff --git a/tools/python/xen/lowlevel/xs/xs.c b/tools/python/xen/lowlevel/xs/xs.c
index b7d4b6ef5d..0dad7fa5f2 100644
--- a/tools/python/xen/lowlevel/xs/xs.c
+++ b/tools/python/xen/lowlevel/xs/xs.c
@@ -791,7 +791,7 @@ static PyObject *xspy_close(XsHandle *self)
         PySequence_SetItem(self->watches, i, Py_None);
     }
 
-    xs_daemon_close(xh);
+    xs_close(xh);
     self->xh = NULL;
 
     Py_INCREF(Py_None);
@@ -985,7 +985,7 @@ xshandle_init(XsHandle *self, PyObject *args, PyObject *kwds)
                                      &readonly))
         goto fail;
 
-    self->xh = (readonly ? xs_daemon_open_readonly() : xs_daemon_open());
+    self->xh = xs_open(0);
     if (!self->xh)
         goto fail;
 
@@ -999,7 +999,7 @@ xshandle_init(XsHandle *self, PyObject *args, PyObject *kwds)
 static void xshandle_dealloc(XsHandle *self)
 {
     if (self->xh) {
-        xs_daemon_close(self->xh);
+        xs_close(self->xh);
         self->xh = NULL;
     }
 
diff --git a/tools/tests/mce-test/tools/xen-mceinj.c b/tools/tests/mce-test/tools/xen-mceinj.c
index 380e42190c..1187d01e5f 100644
--- a/tools/tests/mce-test/tools/xen-mceinj.c
+++ b/tools/tests/mce-test/tools/xen-mceinj.c
@@ -411,13 +411,13 @@ static long xs_get_dom_mem(int domid)
     unsigned int plen;
     struct xs_handle *xs;
 
-    xs = xs_daemon_open();
+    xs = xs_open(0);
     if (!xs)
         return -1;
 
     sprintf(path, "/local/domain/%d/memory/target", domid);
     memstr = xs_read(xs, XBT_NULL, path, &plen);
-    xs_daemon_close(xs);
+    xs_close(xs);
 
     if (!memstr || !plen)
         return -1;
diff --git a/tools/xenbackendd/xenbackendd.c b/tools/xenbackendd/xenbackendd.c
index b6d92984e0..21884af772 100644
--- a/tools/xenbackendd/xenbackendd.c
+++ b/tools/xenbackendd/xenbackendd.c
@@ -122,7 +122,7 @@ usage(void)
 static int
 xen_setup(void)
 {
-	xs = xs_daemon_open();
+	xs = xs_open(0);
 	if (xs == NULL) {
 		dolog(LOG_ERR,
 		    "Failed to contact xenstore (%s).  Is it running?",
@@ -138,7 +138,7 @@ xen_setup(void)
 
  out:
 	if (xs) {
-		xs_daemon_close(xs);
+		xs_close(xs);
 		xs = NULL;
 	}
 	return -1;
diff --git a/tools/xenpmd/xenpmd.c b/tools/xenpmd/xenpmd.c
index 1c801caa71..35fd1c931a 100644
--- a/tools/xenpmd/xenpmd.c
+++ b/tools/xenpmd/xenpmd.c
@@ -502,18 +502,18 @@ int main(int argc, char *argv[])
 #ifndef RUN_STANDALONE
     daemonize();
 #endif
-    xs = (struct xs_handle *)xs_daemon_open();
+    xs = xs_open(0);
     if ( xs == NULL ) 
         return -1;
 
     if ( write_one_time_battery_info() == 0 ) 
     {
-        xs_daemon_close(xs);
+        xs_close(xs);
         return -1;
     }
 
     wait_for_and_update_battery_status_request();
-    xs_daemon_close(xs);
+    xs_close(xs);
     return 0;
 }
 
-- 
2.26.2

[PATCH 5/5] tools/xenstore: drop creation of read-only socket in xenstored

[Juergen Gross]

With xs_daemon_open_readonly() now no longer using the read-only socket
the creation of that socket can be dropped.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c   | 55 +++++++------------------------
 tools/xenstore/xenstored_core.h   |  3 --
 tools/xenstore/xenstored_domain.c |  4 +--
 tools/xenstore/xs_lib.c           |  8 +----
 4 files changed, 14 insertions(+), 56 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 9700772d40..b4be374d3f 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -71,7 +71,6 @@ static unsigned int current_array_size;
 static unsigned int nr_fds;
 
 static int sock = -1;
-static int ro_sock = -1;
 
 static bool verbose = false;
 LIST_HEAD(connections);
@@ -311,8 +310,7 @@ fail:
 	return -1;
 }
 
-static void initialize_fds(int *p_sock_pollfd_idx, int *p_ro_sock_pollfd_idx,
-			   int *ptimeout)
+static void initialize_fds(int *p_sock_pollfd_idx, int *ptimeout)
 {
 	struct connection *conn;
 	struct wrl_timestampt now;
@@ -325,8 +323,6 @@ static void initialize_fds(int *p_sock_pollfd_idx, int *p_ro_sock_pollfd_idx,
 
 	if (sock != -1)
 		*p_sock_pollfd_idx = set_fd(sock, POLLIN|POLLPRI);
-	if (ro_sock != -1)
-		*p_ro_sock_pollfd_idx = set_fd(ro_sock, POLLIN|POLLPRI);
 	if (reopen_log_pipe[0] != -1)
 		reopen_log_pipe0_pollfd_idx =
 			set_fd(reopen_log_pipe[0], POLLIN|POLLPRI);
@@ -472,9 +468,6 @@ static enum xs_perm_type perm_for_conn(struct connection *conn,
 	unsigned int i;
 	enum xs_perm_type mask = XS_PERM_READ|XS_PERM_WRITE|XS_PERM_OWNER;
 
-	if (!conn->can_write)
-		mask &= ~XS_PERM_WRITE;
-
 	/* Owners and tools get it all... */
 	if (!domain_is_unprivileged(conn) || perms[0].id == conn->id
                 || (conn->target && perms[0].id == conn->target->id))
@@ -1422,7 +1415,6 @@ struct connection *new_connection(connwritefn_t *write, connreadfn_t *read)
 	new->pollfd_idx = -1;
 	new->write = write;
 	new->read = read;
-	new->can_write = true;
 	new->transaction_started = 0;
 	INIT_LIST_HEAD(&new->out_list);
 	INIT_LIST_HEAD(&new->watches);
@@ -1435,7 +1427,7 @@ struct connection *new_connection(connwritefn_t *write, connreadfn_t *read)
 }
 
 #ifdef NO_SOCKETS
-static void accept_connection(int sock, bool canwrite)
+static void accept_connection(int sock)
 {
 }
 #else
@@ -1477,7 +1469,7 @@ static int readfd(struct connection *conn, void *data, unsigned int len)
 	return rc;
 }
 
-static void accept_connection(int sock, bool canwrite)
+static void accept_connection(int sock)
 {
 	int fd;
 	struct connection *conn;
@@ -1487,10 +1479,9 @@ static void accept_connection(int sock, bool canwrite)
 		return;
 
 	conn = new_connection(writefd, readfd);
-	if (conn) {
+	if (conn)
 		conn->fd = fd;
-		conn->can_write = canwrite;
-	} else
+	else
 		close(fd);
 }
 #endif
@@ -1794,28 +1785,21 @@ static void destroy_fds(void)
 {
 	if (sock >= 0)
 		close(sock);
-	if (ro_sock >= 0)
-		close(ro_sock);
 }
 
 static void init_sockets(void)
 {
 	struct sockaddr_un addr;
 	const char *soc_str = xs_daemon_socket();
-	const char *soc_str_ro = xs_daemon_socket_ro();
 
 	/* Create sockets for them to listen to. */
 	atexit(destroy_fds);
 	sock = socket(PF_UNIX, SOCK_STREAM, 0);
 	if (sock < 0)
 		barf_perror("Could not create socket");
-	ro_sock = socket(PF_UNIX, SOCK_STREAM, 0);
-	if (ro_sock < 0)
-		barf_perror("Could not create socket");
 
 	/* FIXME: Be more sophisticated, don't mug running daemon. */
 	unlink(soc_str);
-	unlink(soc_str_ro);
 
 	addr.sun_family = AF_UNIX;
 
@@ -1825,17 +1809,10 @@ static void init_sockets(void)
 	if (bind(sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
 		barf_perror("Could not bind socket to %s", soc_str);
 
-	if(strlen(soc_str_ro) >= sizeof(addr.sun_path))
-		barf_perror("socket string '%s' too long", soc_str_ro);
-	strcpy(addr.sun_path, soc_str_ro);
-	if (bind(ro_sock, (struct sockaddr *)&addr, sizeof(addr)) != 0)
-		barf_perror("Could not bind socket to %s", soc_str_ro);
-
-	if (chmod(soc_str, 0600) != 0
-	    || chmod(soc_str_ro, 0660) != 0)
+	if (chmod(soc_str, 0600) != 0)
 		barf_perror("Could not chmod sockets");
 
-	if (listen(sock, 1) != 0 || listen(ro_sock, 1) != 0)
+	if (listen(sock, 1) != 0)
 		barf_perror("Could not listen on sockets");
 }
 #endif
@@ -1893,7 +1870,7 @@ int priv_domid = 0;
 int main(int argc, char *argv[])
 {
 	int opt;
-	int sock_pollfd_idx = -1, ro_sock_pollfd_idx = -1;
+	int sock_pollfd_idx = -1;
 	bool dofork = true;
 	bool outputpid = false;
 	bool no_domain_init = false;
@@ -2010,7 +1987,7 @@ int main(int argc, char *argv[])
 		tracefile = talloc_strdup(NULL, tracefile);
 
 	/* Get ready to listen to the tools. */
-	initialize_fds(&sock_pollfd_idx, &ro_sock_pollfd_idx, &timeout);
+	initialize_fds(&sock_pollfd_idx, &timeout);
 
 	/* Tell the kernel we're up and running. */
 	xenbus_notify_running();
@@ -2051,21 +2028,11 @@ int main(int argc, char *argv[])
 				barf_perror("sock poll failed");
 				break;
 			} else if (fds[sock_pollfd_idx].revents & POLLIN) {
-				accept_connection(sock, true);
+				accept_connection(sock);
 				sock_pollfd_idx = -1;
 			}
 		}
 
-		if (ro_sock_pollfd_idx != -1) {
-			if (fds[ro_sock_pollfd_idx].revents & ~POLLIN) {
-				barf_perror("ro sock poll failed");
-				break;
-			} else if (fds[ro_sock_pollfd_idx].revents & POLLIN) {
-				accept_connection(ro_sock, false);
-				ro_sock_pollfd_idx = -1;
-			}
-		}
-
 		if (xce_pollfd_idx != -1) {
 			if (fds[xce_pollfd_idx].revents & ~POLLIN) {
 				barf_perror("xce_handle poll failed");
@@ -2128,7 +2095,7 @@ int main(int argc, char *argv[])
 			}
 		}
 
-		initialize_fds(&sock_pollfd_idx, &ro_sock_pollfd_idx, &timeout);
+		initialize_fds(&sock_pollfd_idx, &timeout);
 	}
 }
 
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index c4c32bc88f..1df6ad94ab 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -77,9 +77,6 @@ struct connection
 	/* Who am I? 0 for socket connections. */
 	unsigned int id;
 
-	/* Is this a read-only connection? */
-	bool can_write;
-
 	/* Buffered incoming data. */
 	struct buffered_data *in;
 
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 0d5495745b..a2f144f6dd 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -372,7 +372,7 @@ int do_introduce(struct connection *conn, struct buffered_data *in)
 	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec))
 		return EINVAL;
 
-	if (domain_is_unprivileged(conn) || !conn->can_write)
+	if (domain_is_unprivileged(conn))
 		return EACCES;
 
 	domid = atoi(vec[0]);
@@ -438,7 +438,7 @@ int do_set_target(struct connection *conn, struct buffered_data *in)
 	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec))
 		return EINVAL;
 
-	if (domain_is_unprivileged(conn) || !conn->can_write)
+	if (domain_is_unprivileged(conn))
 		return EACCES;
 
 	domid = atoi(vec[0]);
diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
index 3e43f8809d..9f1dc6d559 100644
--- a/tools/xenstore/xs_lib.c
+++ b/tools/xenstore/xs_lib.c
@@ -63,13 +63,7 @@ const char *xs_daemon_socket(void)
 
 const char *xs_daemon_socket_ro(void)
 {
-	static char buf[PATH_MAX];
-	const char *s = xs_daemon_path();
-	if (s == NULL)
-		return NULL;
-	if (snprintf(buf, sizeof(buf), "%s_ro", s) >= PATH_MAX)
-		return NULL;
-	return buf;
+	return xs_daemon_path();
 }
 
 const char *xs_domain_dev(void)
-- 
2.26.2

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

[Wei Liu]

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?

Wei.

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

[Jürgen Groß]

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


Juergen

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

[Andrew Cooper]

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).

~Andrew

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

[Jürgen Groß]

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

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

[Wei Liu]

On Wed, Oct 07, 2020 at 02:45:29PM +0200, Jürgen Groß wrote:
> 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.

+1 on this. Let me check other patches first. If there is no need for
another round of posting of this series, the addition of the disclaimers
can come in a separate patch.

Wei.

> 
> 
> Juergen
> 

Re: [PATCH 0/5] tools/xenstore: remove read-only socket

[Wei Liu]

On Fri, Oct 02, 2020 at 05:41:36PM +0200, Juergen Gross wrote:
> The read-only socket of Xenstore is usable for the daemon case only
> and even there it is not really worth to be kept, as not all
> Xesntore operations changing the state of Xenstore are blocked,
> oxenstored ignoring the read-only semantics completely, and the
> privileges for using the ro-socket being the same as for the normal
> rw-socket.
> 
> So remove this feature with switching the related use cases to the
> Xenstore-type agnostic open- and close-functions..
> 
> Juergen Gross (5):
>   tools/xenstore: remove socket-only option from xenstore client
>   tools/libs/store: ignore XS_OPEN_SOCKETONLY flag
>   tools/libs/store: drop read-only functionality
>   tools: drop all deprecated usages of xs_*_open() and friends in tools
>   tools/xenstore: drop creation of read-only socket in xenstored

Acked + applied.

Please submit a follow-up patch for adding some comments to xenstore.h.

Wei.