[PATCH for-4.19(?) 0/3] tools/libxs: More CLOEXEC fixes

[PATCH for-4.19(?) 0/3] tools/libxs: More CLOEXEC fixes

[Andrew Cooper]

More fixes to CLOEXEC handling in libxenstore.  For 4.19, because the first
attempt to fix this wasn't complete.

libxl is far worse, but I don't have time to get started on that mess.

Andrew Cooper (3):
  tools/libxs: Fix CLOEXEC handling in get_dev()
  tools/libxs: Fix CLOEXEC handling in get_socket()
  tools/libxs: Fix CLOEXEC handling in xs_fileno()

 tools/config.h.in     |  3 ++
 tools/configure       | 12 ++++++++
 tools/configure.ac    |  2 ++
 tools/libs/store/xs.c | 68 ++++++++++++++++++++++++++++++++++---------
 4 files changed, 72 insertions(+), 13 deletions(-)

-- 
2.39.2

[PATCH for-4.19 1/3] tools/libxs: Fix CLOEXEC handling in get_dev()

[Andrew Cooper]

Move the O_CLOEXEC compatibility outside of an #ifdef USE_PTHREAD block.

Introduce set_cloexec() to wrap fcntl() setting FD_CLOEXEC.  It will be reused
for other CLOEXEC fixes too.

Use set_cloexec() when O_CLOEXEC isn't available as a best-effort fallback.

Fixes: f4f2f3402b2f ("tools/libxs: Open /dev/xen/xenbus fds as O_CLOEXEC")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony@xenproject.org>
CC: Juergen Gross <jgross@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 tools/libs/store/xs.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 14985150737e..037e79d98b58 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -40,6 +40,10 @@
 #include <xentoolcore_internal.h>
 #include <xen_list.h>
 
+#ifndef O_CLOEXEC
+#define O_CLOEXEC 0
+#endif
+
 struct xs_stored_msg {
 	XEN_TAILQ_ENTRY(struct xs_stored_msg) list;
 	struct xsd_sockmsg hdr;
@@ -54,10 +58,6 @@ struct xs_stored_msg {
 #include <dlfcn.h>
 #endif
 
-#ifndef O_CLOEXEC
-#define O_CLOEXEC 0
-#endif
-
 struct xs_handle {
 	/* Communications channel to xenstore daemon. */
 	int fd;
@@ -176,6 +176,16 @@ static bool setnonblock(int fd, int nonblock) {
 	return true;
 }
 
+static bool set_cloexec(int fd)
+{
+	int flags = fcntl(fd, F_GETFL);
+
+	if (flags < 0)
+		return false;
+
+	return fcntl(fd, flags | FD_CLOEXEC) >= 0;
+}
+
 int xs_fileno(struct xs_handle *h)
 {
 	char c = 0;
@@ -230,8 +240,24 @@ static int get_socket(const char *connect_to)
 
 static int get_dev(const char *connect_to)
 {
-	/* We cannot open read-only because requests are writes */
-	return open(connect_to, O_RDWR | O_CLOEXEC);
+	int fd, saved_errno;
+
+	fd = open(connect_to, O_RDWR | O_CLOEXEC);
+	if (fd < 0)
+		return -1;
+
+	/* Compat for non-O_CLOEXEC environments.  Racy. */
+	if (!O_CLOEXEC && !set_cloexec(fd))
+		goto error;
+
+	return fd;
+
+error:
+	saved_errno = errno;
+	close(fd);
+	errno = saved_errno;
+
+	return -1;
 }
 
 static int all_restrict_cb(Xentoolcore__Active_Handle *ah, domid_t domid) {
-- 
2.39.2

[PATCH for-4.19 2/3] tools/libxs: Fix CLOEXEC handling in get_socket()

[Andrew Cooper]

get_socket() opens a socket, then uses fcntl() to set CLOEXEC.  This is racy
with exec().

Open the socket with SOCK_CLOEXEC.  Use the same compatibility strategy as
O_CLOEXEC on ancient versions of Linux.

Reported-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony@xenproject.org>
CC: Juergen Gross <jgross@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 tools/libs/store/xs.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 037e79d98b58..11a766c50887 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -44,6 +44,10 @@
 #define O_CLOEXEC 0
 #endif
 
+#ifndef SOCK_CLOEXEC
+#define SOCK_CLOEXEC 0
+#endif
+
 struct xs_stored_msg {
 	XEN_TAILQ_ENTRY(struct xs_stored_msg) list;
 	struct xsd_sockmsg hdr;
@@ -207,16 +211,14 @@ int xs_fileno(struct xs_handle *h)
 static int get_socket(const char *connect_to)
 {
 	struct sockaddr_un addr;
-	int sock, saved_errno, flags;
+	int sock, saved_errno;
 
-	sock = socket(PF_UNIX, SOCK_STREAM, 0);
+	sock = socket(PF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
 	if (sock < 0)
 		return -1;
 
-	if ((flags = fcntl(sock, F_GETFD)) < 0)
-		goto error;
-	flags |= FD_CLOEXEC;
-	if (fcntl(sock, F_SETFD, flags) < 0)
+	/* Compat for non-SOCK_CLOEXEC environments.  Racy. */
+	if (!SOCK_CLOEXEC && !set_cloexec(sock))
 		goto error;
 
 	addr.sun_family = AF_UNIX;
-- 
2.39.2

[PATCH for-4.19 3/3] tools/libxs: Fix CLOEXEC handling in xs_fileno()

[Andrew Cooper]

xs_fileno() opens a pipe on first use to communicate between the watch thread
and the main thread.  Nothing ever sets CLOEXEC on the file descriptors.

Check for the availability of the pipe2() function with configure.  Despite
starting life as Linux-only, FreeBSD and NetBSD have gained it.

When pipe2() isn't available, try our best with pipe() and set_cloexec().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony@xenproject.org>
CC: Juergen Gross <jgross@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 tools/config.h.in     |  3 +++
 tools/configure       | 12 ++++++++++++
 tools/configure.ac    |  2 ++
 tools/libs/store/xs.c | 16 +++++++++++++++-
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/config.h.in b/tools/config.h.in
index 0bb2fe08a143..50ad60fcb091 100644
--- a/tools/config.h.in
+++ b/tools/config.h.in
@@ -39,6 +39,9 @@
 /* Define to 1 if you have the <memory.h> header file. */
 #undef HAVE_MEMORY_H
 
+/* Define to 1 if you have the `pipe2' function. */
+#undef HAVE_PIPE2
+
 /* pygrub enabled */
 #undef HAVE_PYGRUB
 
diff --git a/tools/configure b/tools/configure
index 459bfb56520e..a6b43bfc6064 100755
--- a/tools/configure
+++ b/tools/configure
@@ -9751,6 +9751,18 @@ if test "$ax_found" = "0"; then :
 fi
 
 
+for ac_func in pipe2
+do :
+  ac_fn_c_check_func "$LINENO" "pipe2" "ac_cv_func_pipe2"
+if test "x$ac_cv_func_pipe2" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_PIPE2 1
+_ACEOF
+
+fi
+done
+
+
 cat >confcache <<\_ACEOF
 # This file is a shell script that caches the results of configure
 # tests run on this system so they can be shared between configure
diff --git a/tools/configure.ac b/tools/configure.ac
index 851887080c5e..ac0fdc4314c4 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -543,4 +543,6 @@ AS_IF([test "x$pvshim" = "xy"], [
 
 AX_FIND_HEADER([INCLUDE_ENDIAN_H], [endian.h sys/endian.h])
 
+AC_CHECK_FUNCS([pipe2])
+
 AC_OUTPUT()
diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 11a766c50887..27bd20933efd 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -190,13 +190,27 @@ static bool set_cloexec(int fd)
 	return fcntl(fd, flags | FD_CLOEXEC) >= 0;
 }
 
+static int pipe_cloexec(int fds[2])
+{
+#if HAVE_PIPE2
+	return pipe2(fds, O_CLOEXEC);
+#else
+	if (pipe(fds) < 0)
+		return -1;
+	/* Best effort to set CLOEXEC. Racy. */
+	set_cloexec(fds[0]);
+	set_cloexec(fds[1]);
+	return 0;
+#endif
+}
+
 int xs_fileno(struct xs_handle *h)
 {
 	char c = 0;
 
 	mutex_lock(&h->watch_mutex);
 
-	if ((h->watch_pipe[0] == -1) && (pipe(h->watch_pipe) != -1)) {
+	if ((h->watch_pipe[0] == -1) && (pipe_cloexec(h->watch_pipe) != -1)) {
 		/* Kick things off if the watch list is already non-empty. */
 		if (!XEN_TAILQ_EMPTY(&h->watch_list))
 			while (write(h->watch_pipe[1], &c, 1) != 1)
-- 
2.39.2

Re: [PATCH for-4.19(?) 0/3] tools/libxs: More CLOEXEC fixes

[Jürgen Groß]

On 28.06.24 16:31, Andrew Cooper wrote:
> More fixes to CLOEXEC handling in libxenstore.  For 4.19, because the first
> attempt to fix this wasn't complete.
> 
> libxl is far worse, but I don't have time to get started on that mess.
> 
> Andrew Cooper (3):
>    tools/libxs: Fix CLOEXEC handling in get_dev()
>    tools/libxs: Fix CLOEXEC handling in get_socket()
>    tools/libxs: Fix CLOEXEC handling in xs_fileno()
> 
>   tools/config.h.in     |  3 ++
>   tools/configure       | 12 ++++++++
>   tools/configure.ac    |  2 ++
>   tools/libs/store/xs.c | 68 ++++++++++++++++++++++++++++++++++---------
>   4 files changed, 72 insertions(+), 13 deletions(-)
> 

For the series:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH for-4.19(?) 0/3] tools/libxs: More CLOEXEC fixes

[Anthony PERARD]

On Mon, Jul 01, 2024 at 11:03:02AM +0200, Jürgen Groß wrote:
> On 28.06.24 16:31, Andrew Cooper wrote:
> > More fixes to CLOEXEC handling in libxenstore.  For 4.19, because the first
> > attempt to fix this wasn't complete.
> >
> > libxl is far worse, but I don't have time to get started on that mess.
> >
> > Andrew Cooper (3):
> >    tools/libxs: Fix CLOEXEC handling in get_dev()
> >    tools/libxs: Fix CLOEXEC handling in get_socket()
> >    tools/libxs: Fix CLOEXEC handling in xs_fileno()
> >
> >   tools/config.h.in     |  3 ++
> >   tools/configure       | 12 ++++++++
> >   tools/configure.ac    |  2 ++
> >   tools/libs/store/xs.c | 68 ++++++++++++++++++++++++++++++++++---------
> >   4 files changed, 72 insertions(+), 13 deletions(-)
> >
>
> For the series:
>
> Reviewed-by: Juergen Gross <jgross@suse.com>

Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

--

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH for-4.19(?) 0/3] tools/libxs: More CLOEXEC fixes

[Oleksii]

On Fri, 2024-06-28 at 15:31 +0100, Andrew Cooper wrote:
> More fixes to CLOEXEC handling in libxenstore.  For 4.19, because the
> first
> attempt to fix this wasn't complete.
> 
> libxl is far worse, but I don't have time to get started on that
> mess.
> 
> Andrew Cooper (3):
>   tools/libxs: Fix CLOEXEC handling in get_dev()
>   tools/libxs: Fix CLOEXEC handling in get_socket()
>   tools/libxs: Fix CLOEXEC handling in xs_fileno()
> 
>  tools/config.h.in     |  3 ++
>  tools/configure       | 12 ++++++++
>  tools/configure.ac    |  2 ++
>  tools/libs/store/xs.c | 68 ++++++++++++++++++++++++++++++++++-------
> --
>  4 files changed, 72 insertions(+), 13 deletions(-)
> 
For the whole series:
 Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii