| Message ID | 20210312092212.782255-3-groug@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | virtiofsd: Avoid potential deadlocks | expand |
On Fri, Mar 12, 2021 at 10:22:07AM +0100, Greg Kurz wrote: > Some message types, e.g. VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG, > can convey file descriptors. These must be closed before returning > from slave_read() to avoid being leaked. This can currently be done > in two different places: > > [1] just after the request has been processed > > [2] on the error path, under the goto label err: > > These path are supposed to be mutually exclusive but they are not > actually. If the VHOST_USER_NEED_REPLY_MASK flag was passed and the > sending of the reply fails, both [1] and [2] are performed with the > same descriptor values. This can potentially cause subtle bugs if one > of the descriptor was recycled by some other thread in the meantime. > > This code duplication complicates rollback for no real good benefit. > Do the closing in a unique place, under a new fdcleanup: goto label > at the end of the function. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/virtio/vhost-user.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 6af9b43a7232..acde1d293684 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -1475,13 +1475,6 @@ static void slave_read(void *opaque) ret = -EINVAL; } - /* Close the remaining file descriptors. */ - for (i = 0; i < fdsize; i++) { - if (fd[i] != -1) { - close(fd[i]); - } - } - /* * REPLY_ACK feature handling. Other reply types has to be managed * directly in their request handlers. @@ -1511,12 +1504,14 @@ static void slave_read(void *opaque) } } - return; + goto fdcleanup; err: qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL); close(u->slave_fd); u->slave_fd = -1; + +fdcleanup: for (i = 0; i < fdsize; i++) { if (fd[i] != -1) { close(fd[i]);
Some message types, e.g. VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG, can convey file descriptors. These must be closed before returning from slave_read() to avoid being leaked. This can currently be done in two different places: [1] just after the request has been processed [2] on the error path, under the goto label err: These path are supposed to be mutually exclusive but they are not actually. If the VHOST_USER_NEED_REPLY_MASK flag was passed and the sending of the reply fails, both [1] and [2] are performed with the same descriptor values. This can potentially cause subtle bugs if one of the descriptor was recycled by some other thread in the meantime. This code duplication complicates rollback for no real good benefit. Do the closing in a unique place, under a new fdcleanup: goto label at the end of the function. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/virtio/vhost-user.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)