Re: Problems with spin_lock_irqsave() and for_each_online_cpu()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/10/13 09:19, Jan Beulich wrote:
>>>> On 17.10.13 at 21:34, Andrew Cooper <andrew.cooper3@citrix.com> 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