Re: [PATCH kvmtool 2/2] run: Check for ghost socket file upon VM creation

[Will Deacon <will.deacon@arm.com>]

On Fri, May 03, 2019 at 06:08:21PM +0100, Andre Przywara wrote:
> Kvmtool creates a (debug) UNIX socket file for each VM, using its
> (possibly auto-generated) name as the filename. There is a check using
> access(), which bails out with an error message if a socket with that
> name already exists.
> 
> Aside from this check being unnecessary, as the bind() call later would
> complain as well, this is also racy. But more annoyingly the bail out is
> not needed most of the time: an existing socket inode is most likely just
> an orphaned leftover from a previous kvmtool run, which just failed to
> remove that file, because of a crash, for instance.
> 
> Upon finding such a collision, let's first try to connect to that socket,
> to detect if there is still a kvmtool instance listening on the other
> end. If that fails, this socket will never come back to life, so we can
> safely clean it up and reuse the name for the new guest.
> However if the connect() succeeds, there is an actual live kvmtool
> instance using this name, so not proceeding is the only option.
> This should never happen with the (PID based) automatically generated
> names, though.
> 
> This avoids an annoying (and not helpful) error message and helps
> automated kvmtool runs to proceed in more cases.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  kvm-ipc.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/kvm-ipc.c b/kvm-ipc.c
> index d9a07595..06909171 100644
> --- a/kvm-ipc.c
> +++ b/kvm-ipc.c
> @@ -43,10 +43,6 @@ static int kvm__create_socket(struct kvm *kvm)
>  
>  	snprintf(full_name, sizeof(full_name), "%s/%s%s",
>  		 kvm__get_dir(), kvm->cfg.guest_name, KVM_SOCK_SUFFIX);
> -	if (access(full_name, F_OK) == 0) {
> -		pr_err("Socket file %s already exist", full_name);
> -		return -EEXIST;
> -	}
>  
>  	s = socket(AF_UNIX, SOCK_STREAM, 0);
>  	if (s < 0) {
> @@ -58,6 +54,32 @@ static int kvm__create_socket(struct kvm *kvm)
>  	strlcpy(local.sun_path, full_name, sizeof(local.sun_path));
>  	len = strlen(local.sun_path) + sizeof(local.sun_family);
>  	r = bind(s, (struct sockaddr *)&local, len);
> +	/* Check for an existing socket file */
> +	if (r < 0 && errno == EADDRINUSE) {
> +		r = connect(s, (struct sockaddr *)&local, len);
> +		if (r == 0) {
> +			/*
> +			 * If we could connect, there is already a guest
> +			 * using this same name. This should not happen
> +			 * for PID derived names, but could happen for user
> +			 * provided guest names.
> +			 */
> +			pr_err("Guest socket file %s already exists.",
> +			       full_name);
> +			r = -EEXIST;
> +			goto fail;
> +		}
> +		if (errno == ECONNREFUSED) {
> +			/*
> +			 * This is a ghost socket file, with no-one listening
> +			 * on the other end. Since kvmtool will only bind
> +			 * above when creating a new guest, there is no
> +			 * danger in just removing the file and re-trying.
> +			 */
> +			unlink(full_name);

Can we print a diagnostic when this happens please? (hopefully the same
message as whatever you end up doing in the first patch).

Will