From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Jackson <Ian.Jackson@eu.citrix.com>
Subject: Re: [PATCH] xenconsole: Ensure exclusive access to
	console using locks
Date: Fri, 24 Jul 2015 14:32:16 +0100
Message-ID: <21938.15968.711376.627272@mariner.uk.xensource.com>
References: <1437738262.24746.79.camel@citrix.com>
	<1437742310-14193-1-git-send-email-martin@lucina.net>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <prvs=6403088e3=Ian.Jackson@citrix.com>)
	id 1ZId5K-0007ud-7O
	for xen-devel@lists.xenproject.org; Fri, 24 Jul 2015 13:32:22 +0000
In-Reply-To: <1437742310-14193-1-git-send-email-martin@lucina.net>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Martin Lucina <martin@lucina.net>
Cc: xen-devel@lists.xenproject.org, Wei Liu <wei.liu2@citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

Martin Lucina writes ("[PATCH] xenconsole: Ensure exclusive access to console using locks"):
> If more than one instance of xenconsole is run against the same DOMID
> then each instance will only get some data. This change ensures
> exclusive access to the console by creating and obtaining an exclusive
> lock on <XEN_LOCK_DIR>/xenconsole.<DOMID>.

It is a shame that we are still using ptys for the xenconsole
connection.  If it were a socket we could allow as many clients as we
like.  But I haven't got round to fixing this for the last n years so
I think the general plan you have makes sense.


> +static void console_lock(int domid)
> +{
> +	lockfile = malloc(PATH_MAX);
> +	if (lockfile == NULL)
> +		err(ENOMEM, "malloc");
> +	snprintf(lockfile, PATH_MAX - 1, "%s/xenconsole.%d", XEN_LOCK_DIR, domid);

Why not use asprintf ?


> +	lockfd = open(lockfile, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
> +	if (lockfd == -1)
> +		err(errno, "Could not open %s", lockfile);
> +	if (flock(lockfd, LOCK_EX|LOCK_NB) != 0)
> +		err(errno, "Could not lock %s", lockfile);
> +}

This locking strategy is not safe if the lockfile is ever unlinked,
because it allows:

    A   open         flock              .o{ I have the lock }
 
    B        unlink

    C                      open flock   .o{ I have the lock }

> +static void console_unlock(void)
> +{
> +	if (lockfd != -1) {
> +		flock(lockfd, LOCK_UN);
> +		close(lockfd);
> +	}
> +	if (lockfile != NULL)
> +		unlink(lockfile);

And this unlinking strategy is not safe even if we are more careful
with our locking.  You must only unlink with the lock held.


You should use the same recipe as with-lock-ex (from chiark-utils)

  http://www.chiark.greenend.org.uk/ucgi/~ian/git?p=chiark-utils.git;a=blob;f=cprogs/with-lock-ex.c;h=1850d1f0283bd61fc1da12458cba7ec0a227c572;hb=HEAD
  
(which we also use in tools/hotplug/Linux/locking.sh and
tools/libxl/libxl_internal.c:libxl__lock_domain_userdata)


Ian.