From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 for 4.5 1/8] libxl: rework domain userdata file lock Date: Tue, 16 Sep 2014 19:50:44 +0100 Message-ID: <1410893444.23505.26.camel@citrix.com> References: <1410861678-31428-1-git-send-email-wei.liu2@citrix.com> <1410861678-31428-2-git-send-email-wei.liu2@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1410861678-31428-2-git-send-email-wei.liu2@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: ian.jackson@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Tue, 2014-09-16 at 11:01 +0100, Wei Liu wrote: > The lock introduced in d2cd9d4f ("libxl: functions to lock / unlock > libxl userdata store") has a bug that can leak the lock file when domain > destruction races with other functions that try to get hold of the lock. > > There are several issues: > 1. The lock is released too early with libxl__userdata_destroyall > deletes everything in userdata store, including the lock file. > 2. The check of domain existence is only done at the beginning of lock > function, by the time the lock is acquired, the domain might have > been gone already. > > The effect of this two issues is we can run into such situation: > > Process 1 Process 2 domain destruction > # LOCK FUNCTION # LOCK FUNCTION > check domain existence check domain existence > acquire lock (file created) > # LOCK FUNCTION > destroy all files (lock file deleted, > lock released) > acquire lock (file created) > # LOCK FUNCTION destroy domain > # UNLOCK (close fd only) > [ lock file leaked ] > > Fix this problem by deploying following changes: > > 1. Unlink lock file in unlock function. > 2. Modify libxl__userdata_destroyall to not delete domain-userdata-lock, > so that the lock remains held until unlock function is called. > 3. Check domain still exists when the lock is acquired, unlock if > domain is already gone. > > Signed-off-by: Wei Liu Acked-by: Ian Campbell Iff you end up resending the series (I'm hoping not) it might be nice to s/lock_carefd/carefd/ in the struct (since the usages are lock->thing so the lock prefix is redundant). Also in the unlock perhaps comment that the unlock must be done first before the close, since that is quite important. Ian