From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: Problems with spin_lock_irqsave() and for_each_online_cpu() Date: Fri, 18 Oct 2013 15:55:19 +0100 Message-ID: <52614BD7.3030605@citrix.com> References: <52603BD1.40901@citrix.com> <52610B2902000078000FC000@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VXBSV-0005dN-ST for xen-devel@lists.xenproject.org; Fri, 18 Oct 2013 14:55:24 +0000 In-Reply-To: <52610B2902000078000FC000@nat28.tlf.novell.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: Jan Beulich Cc: xen-devel , Keir Fraser , Tim Deegan List-Id: xen-devel@lists.xenproject.org On 18/10/13 09:19, Jan Beulich wrote: >>>> On 17.10.13 at 21:34, Andrew Cooper wrote: >> I was triaging a Coverity issue (1055455) which was complaining about >> spinlock inbalance in common/trace.c:424. >> >> First of all, there is a latent bug here using "int flags" in a >> irqsave/irqrestore. It will be safe until bit 31 of RFLAGS is defined, >> but will introduce subtle corruption thereafter. >> >> This bug was not caught by the compiler because of the >> spin_lock_irqsave() macro which has slightly non-function-like >> semantics. Would it be acceptable to change spin_lock_irqsave() into a >> static inline so can be properly typed? (This would come with a huge >> amount of code churn as the function would have to take flags by pointer) > Why not simply add > > BUILD_BUG_ON(sizeof(f) != sizeof(_spin_lock_irqsave(l))) > > (or equivalent in case of header dependency issues) to the > macro? > > But then again I don't see the corruption to occur when RFLAGS > beyond bit 31 would become defined: the flags get passed to > local_irq_restore() only, and that one's effect is "defined" to set > IF to the intended state - what it does with the other flags isn't > really defined (and in fact I wonder whether it really is correct > to use a plain POPF there - imagine code fiddling with e.g. DF > at the same time as using the proper abstractions to control IF). > > Jan > The BUILD_BUG_ON() is a rather more simple solution to the problem. Also changing local_irq_restore() to a conditional sti will also be a good improvement. I shall see about spinning a patch for these issues. ~Andrew