set_cloexec() had a bit too much copy&pate from setnonblock(), and
insufficient testing on ancient versions of Linux...

As written (emulating ancient linux by undef'ing O_CLOEXEC), strace shows:

  open("/dev/xen/xenbus", O_RDWR)         = 3
  fcntl(3, F_GETFL)                       = 0x8002 (flags O_RDWR|O_LARGEFILE)
  fcntl(3, 0x8003 /* F_??? */, 0x7ffe4a771d90) = -1 EINVAL (Invalid argument)
  close(3)                                = 0

which is obviously nonsense.

Switch F_GETFL -> F_GETFD, and fix the second invocation to use F_SETFD.  With
this, strace is rather happer:

  open("/dev/xen/xenbus", O_RDWR)         = 3
  fcntl(3, F_GETFD)                       = 0
  fcntl(3, F_SETFD, FD_CLOEXEC)           = 0

Fixes: bf7c1464706a ("tools/libxs: Fix CLOEXEC handling in get_dev()")
Reported-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Juergen Gross <jgross@suse.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Jan Beulich <JBeulich@suse.com>

I'm embarassed to say that this was only spotted by Ross when I was
cherrypicking fixes from staging-4.17 into the XenServer patchqueue.

This is urgent to take for 4.19, and to backport into 4.17/18 seeing as the
breakage has been backported already.
---
 tools/libs/store/xs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index c8845b69e284..38a6ce3cf2ea 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -182,12 +182,12 @@ static bool setnonblock(int fd, int nonblock) {
 
 static bool set_cloexec(int fd)
 {
-	int flags = fcntl(fd, F_GETFL);
+	int flags = fcntl(fd, F_GETFD);
 
 	if (flags < 0)
 		return false;
 
-	return fcntl(fd, flags | FD_CLOEXEC) >= 0;
+	return fcntl(fd, F_SETFD, flags | FD_CLOEXEC) >= 0;
 }
 
 static int pipe_cloexec(int fds[2])
-- 
2.39.2

On 08.07.24 17:15, Andrew Cooper wrote:
> set_cloexec() had a bit too much copy&pate from setnonblock(), and
> insufficient testing on ancient versions of Linux...
> 
> As written (emulating ancient linux by undef'ing O_CLOEXEC), strace shows:
> 
>    open("/dev/xen/xenbus", O_RDWR)         = 3
>    fcntl(3, F_GETFL)                       = 0x8002 (flags O_RDWR|O_LARGEFILE)
>    fcntl(3, 0x8003 /* F_??? */, 0x7ffe4a771d90) = -1 EINVAL (Invalid argument)
>    close(3)                                = 0
> 
> which is obviously nonsense.
> 
> Switch F_GETFL -> F_GETFD, and fix the second invocation to use F_SETFD.  With
> this, strace is rather happer:
> 
>    open("/dev/xen/xenbus", O_RDWR)         = 3
>    fcntl(3, F_GETFD)                       = 0
>    fcntl(3, F_SETFD, FD_CLOEXEC)           = 0
> 
> Fixes: bf7c1464706a ("tools/libxs: Fix CLOEXEC handling in get_dev()")
> Reported-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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


Juergen

On Mon, Jul 8, 2024 at 4:15 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> set_cloexec() had a bit too much copy&pate from setnonblock(), and
> insufficient testing on ancient versions of Linux...
>
> As written (emulating ancient linux by undef'ing O_CLOEXEC), strace shows:
>
>   open("/dev/xen/xenbus", O_RDWR)         = 3
>   fcntl(3, F_GETFL)                       = 0x8002 (flags O_RDWR|O_LARGEFILE)
>   fcntl(3, 0x8003 /* F_??? */, 0x7ffe4a771d90) = -1 EINVAL (Invalid argument)
>   close(3)                                = 0
>
> which is obviously nonsense.
>
> Switch F_GETFL -> F_GETFD, and fix the second invocation to use F_SETFD.  With
> this, strace is rather happer:
>
>   open("/dev/xen/xenbus", O_RDWR)         = 3
>   fcntl(3, F_GETFD)                       = 0
>   fcntl(3, F_SETFD, FD_CLOEXEC)           = 0
>
> Fixes: bf7c1464706a ("tools/libxs: Fix CLOEXEC handling in get_dev()")
> Reported-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Thanks