linux-kernel - Re: [PATCH] x86/spinlocks: Fix regression in spinlock contention detection

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 05 May 2015 16:08:22 +0530
From:	Raghavendra K T <raghavendra.kt@...ux.vnet.ibm.com>
To:	Tahsin Erdogan <tahsin@...gle.com>
CC:	Peter Zijlstra <peterz@...radead.org>, tglx@...utronix.de,
	mingo@...hat.com, hpa@...or.com, x86@...nel.org,
	Waiman.Long@...com, borntraeger@...ibm.com, oleg@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/spinlocks: Fix regression in spinlock contention
 detection

On 05/05/2015 02:47 PM, Peter Zijlstra wrote:
> On Mon, May 04, 2015 at 09:15:31PM -0700, Tahsin Erdogan wrote:
>> A spinlock is regarded as contended when there is at least one waiter.
>> Currently, the code that checks whether there are any waiters rely on
>> tail value being greater than head. However, this is not true if tail
>> reaches the max value and wraps back to zero, so arch_spin_is_contended()
>> incorrectly returns 0 (not contended) when tail is smaller than head.
>>
>> The original code (before regression) handled this case by casting the
>> (tail - head) to an unsigned value. This change simply restores that
>> behavior.
>>
>> Fixes: d6abfdb20223 ("x86/spinlocks/paravirt: Fix memory corruption on
>> unlock")
>> Signed-off-by: Tahsin Erdogan <tahsin@...gle.com>
>> ---
>>   arch/x86/include/asm/spinlock.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/spinlock.h b/arch/x86/include/asm/spinlock.h
>> index cf87de3..64b6117 100644
>> --- a/arch/x86/include/asm/spinlock.h
>> +++ b/arch/x86/include/asm/spinlock.h
>> @@ -169,7 +169,7 @@ static inline int arch_spin_is_contended(arch_spinlock_t *lock)
>>   	struct __raw_tickets tmp = READ_ONCE(lock->tickets);
>>
>>   	tmp.head &= ~TICKET_SLOWPATH_FLAG;
>> -	return (tmp.tail - tmp.head) > TICKET_LOCK_INC;
>> +	return (__ticket_t)(tmp.tail - tmp.head) > TICKET_LOCK_INC;
>
> I'm not seeing it, everything in that expression is of __ticket_t type
> (tail, head and TICKET_LOCK_INC), nothing should cause it to be cast to
> another type due to conversion rules.
>
> Or does - always cast to a signed type? Lemme go grab the C rules again.
>
> I'm not seeing it.. Please explain better, iow. your changelog fails to
> properly explain the problem.
>

Same from me here.

Did you really see a regression?

I am not seeing two unsigned value subtraction resulting in a negative
value.

================
#include <stdio.h>

#define LOCK_INC 2

int main()
{
         unsigned int head = 32700, tail=2;

         if ((tail - head) > LOCK_INC)
                 printf(" tail - head > LOCK_INC \n");
         else
                 printf(" tail - head < LOCK_INC \n");

         return 0;
}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/