[PATCH 0/2] fix fcntl(F_SETFD) usage

[PATCH 0/2] fix fcntl(F_SETFD) usage

[Eric Blake]

As recently pointed out:
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03133.html
code that blindly calls fcntl(fd, F_SETFD, 1) rather than performing a
read-modify-write when it intends to add FD_CLOEXEC is broken, in that
it can inadvertently clear other bits.

Thankfully, the culprits fixed in this series are unlikely to be
clearing either FD_CLOFORK (if Linux ever follows Solaris' lead in
adding that), or the new FD_32BIT_MODE being proposed (as the fds in
question are unlikely to have that set) - but it is still better to
write proper code than to set up a bad example prone to copy-and-paste
propagation.  And as these usages are not new to 5.0, I don't see any
reason against waiting until 5.1 to apply them.

Eric Blake (2):
  hax: Fix setting of FD_CLOEXEC
  tools: Fix use of fcntl(F_SETFD) during socket activation

 target/i386/hax-posix.c | 6 +++---
 util/systemd.c          | 4 +++-
 2 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.26.1

[PATCH 1/2] hax: Fix setting of FD_CLOEXEC

[Eric Blake]

Blindly setting FD_CLOEXEC without a read-modify-write will
inadvertently clear any other intentionally-set bits, such as a
proposed new bit for designating a fd that must behave in 32-bit mode.
Use our wrapper function instead of an incorrect hand-rolled version.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 target/i386/hax-posix.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/i386/hax-posix.c b/target/i386/hax-posix.c
index 3bad89f13337..5f9d1b803dec 100644
--- a/target/i386/hax-posix.c
+++ b/target/i386/hax-posix.c
@@ -23,7 +23,7 @@ hax_fd hax_mod_open(void)
         fprintf(stderr, "Failed to open the hax module\n");
     }

-    fcntl(fd, F_SETFD, FD_CLOEXEC);
+    qemu_set_cloexec(fd);

     return fd;
 }
@@ -147,7 +147,7 @@ hax_fd hax_host_open_vm(struct hax_state *hax, int vm_id)
     fd = open(vm_name, O_RDWR);
     g_free(vm_name);

-    fcntl(fd, F_SETFD, FD_CLOEXEC);
+    qemu_set_cloexec(fd);

     return fd;
 }
@@ -200,7 +200,7 @@ hax_fd hax_host_open_vcpu(int vmid, int vcpuid)
     if (fd < 0) {
         fprintf(stderr, "Failed to open the vcpu devfs\n");
     }
-    fcntl(fd, F_SETFD, FD_CLOEXEC);
+    qemu_set_cloexec(fd);
     return fd;
 }

-- 
2.26.1

Re: [PATCH 1/2] hax: Fix setting of FD_CLOEXEC

[Eduardo Habkost]

On Mon, Apr 20, 2020 at 12:53:07PM -0500, Eric Blake wrote:
> Blindly setting FD_CLOEXEC without a read-modify-write will
> inadvertently clear any other intentionally-set bits, such as a
> proposed new bit for designating a fd that must behave in 32-bit mode.
> Use our wrapper function instead of an incorrect hand-rolled version.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Thanks, queued for 5.1.

-- 
Eduardo

Re: [PATCH 1/2] hax: Fix setting of FD_CLOEXEC

[Eric Blake]

On 4/20/20 5:07 PM, Eduardo Habkost wrote:
> On Mon, Apr 20, 2020 at 12:53:07PM -0500, Eric Blake wrote:
>> Blindly setting FD_CLOEXEC without a read-modify-write will
>> inadvertently clear any other intentionally-set bits, such as a
>> proposed new bit for designating a fd that must behave in 32-bit mode.
>> Use our wrapper function instead of an incorrect hand-rolled version.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Thanks, queued for 5.1.

Ping - I still don't see this upstream.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/2] hax: Fix setting of FD_CLOEXEC

[Colin Xu]

Looks good to me.

Reviewed-by: Colin Xu <colin.xu@intel.com>

--
Best Regards,
Colin Xu

On Tue, 21 Apr 2020, Eric Blake wrote:

> Blindly setting FD_CLOEXEC without a read-modify-write will
> inadvertently clear any other intentionally-set bits, such as a
> proposed new bit for designating a fd that must behave in 32-bit mode.
> Use our wrapper function instead of an incorrect hand-rolled version.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> target/i386/hax-posix.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target/i386/hax-posix.c b/target/i386/hax-posix.c
> index 3bad89f13337..5f9d1b803dec 100644
> --- a/target/i386/hax-posix.c
> +++ b/target/i386/hax-posix.c
> @@ -23,7 +23,7 @@ hax_fd hax_mod_open(void)
>         fprintf(stderr, "Failed to open the hax module\n");
>     }
>
> -    fcntl(fd, F_SETFD, FD_CLOEXEC);
> +    qemu_set_cloexec(fd);
>
>     return fd;
> }
> @@ -147,7 +147,7 @@ hax_fd hax_host_open_vm(struct hax_state *hax, int vm_id)
>     fd = open(vm_name, O_RDWR);
>     g_free(vm_name);
>
> -    fcntl(fd, F_SETFD, FD_CLOEXEC);
> +    qemu_set_cloexec(fd);
>
>     return fd;
> }
> @@ -200,7 +200,7 @@ hax_fd hax_host_open_vcpu(int vmid, int vcpuid)
>     if (fd < 0) {
>         fprintf(stderr, "Failed to open the vcpu devfs\n");
>     }
> -    fcntl(fd, F_SETFD, FD_CLOEXEC);
> +    qemu_set_cloexec(fd);
>     return fd;
> }
>
> -- 
> 2.26.1
>
>

[PATCH 2/2] tools: Fix use of fcntl(F_SETFD) during socket activation

[Eric Blake]

Blindly setting FD_CLOEXEC without a read-modify-write will
inadvertently clear any other intentionally-set bits, such as a
proposed new bit for designating a fd that must behave in 32-bit mode.
However, we cannot use our wrapper qemu_set_cloexec(), because that
wrapper intentionally abort()s on failure, whereas the probe here
intentionally tolerates failure to deal with incorrect socket
activation gracefully.  Instead, fix the code to do the proper
read-modify-write.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 util/systemd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/util/systemd.c b/util/systemd.c
index 1dd0367d9a84..5bcac9b40169 100644
--- a/util/systemd.c
+++ b/util/systemd.c
@@ -23,6 +23,7 @@ unsigned int check_socket_activation(void)
     unsigned long nr_fds;
     unsigned int i;
     int fd;
+    int f;
     int err;

     s = getenv("LISTEN_PID");
@@ -54,7 +55,8 @@ unsigned int check_socket_activation(void)
     /* So the file descriptors don't leak into child processes. */
     for (i = 0; i < nr_fds; ++i) {
         fd = FIRST_SOCKET_ACTIVATION_FD + i;
-        if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) {
+        f = fcntl(fd, F_GETFD);
+        if (f == -1 || fcntl(fd, F_SETFD, f | FD_CLOEXEC) == -1) {
             /* If we cannot set FD_CLOEXEC then it probably means the file
              * descriptor is invalid, so socket activation has gone wrong
              * and we should exit.
-- 
2.26.1

Re: [PATCH 2/2] tools: Fix use of fcntl(F_SETFD) during socket activation

[Eric Blake]

On 4/20/20 12:53 PM, Eric Blake wrote:
> Blindly setting FD_CLOEXEC without a read-modify-write will
> inadvertently clear any other intentionally-set bits, such as a
> proposed new bit for designating a fd that must behave in 32-bit mode.
> However, we cannot use our wrapper qemu_set_cloexec(), because that
> wrapper intentionally abort()s on failure, whereas the probe here
> intentionally tolerates failure to deal with incorrect socket
> activation gracefully.  Instead, fix the code to do the proper
> read-modify-write.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   util/systemd.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

As qemu-nbd is impacted, I'll queue 2/2 through my NBD tree if 
qemu-trivial doesn't pick it up first.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 0/2] fix fcntl(F_SETFD) usage

[Peter Maydell]

On Mon, 20 Apr 2020 at 18:53, Eric Blake <eblake@redhat.com> wrote:
>
> As recently pointed out:
> https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03133.html
> code that blindly calls fcntl(fd, F_SETFD, 1) rather than performing a
> read-modify-write when it intends to add FD_CLOEXEC is broken, in that
> it can inadvertently clear other bits.
>
> Thankfully, the culprits fixed in this series are unlikely to be
> clearing either FD_CLOFORK (if Linux ever follows Solaris' lead in
> adding that), or the new FD_32BIT_MODE being proposed (as the fds in
> question are unlikely to have that set) - but it is still better to
> write proper code than to set up a bad example prone to copy-and-paste
> propagation.  And as these usages are not new to 5.0, I don't see any
> reason against waiting until 5.1 to apply them.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM