Xen devel - Jan 2012 - [PATCH] x86: xen: size struct xen_spinlock to always fit in arch_spinlock_t

From: David Vrabel <david.vrabel@citrix.com>

If NR_CPUS < 256 then arch_spinlock_t is only 16 bits wide but struct
xen_spinlock is 32 bits.  When a spin lock is contended and
xl->spinners is modified the two bytes immediately after the spin lock
would be corrupted.

This is a regression caused by 84eb950db13ca40a0572ce9957e14723500943d6
(x86, ticketlock: Clean up types and accessors) which reduced the size
of arch_spinlock_t.

Fix this by making xl->spinners a u8 if NR_CPUS < 256.  A
BUILD_BUG_ON() is also added to check the sizes of the two structures
are compatible.

In many cases this was not noticable as there would often be padding
bytes after the lock (e.g., if any of CONFIG_GENERIC_LOCKBREAK,
CONFIG_DEBUG_SPINLOCK, or CONFIG_DEBUG_LOCK_ALLOC were enabled).

The bnx2 driver is affected. In struct bnx2, phy_lock and
indirect_lock may have no padding after them.  Contention on phy_lock
would corrupt indirect_lock making it appear locked and the driver
would deadlock.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
---
 arch/x86/xen/spinlock.c |   27 ++++++++++++++++++++++-----
 1 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index cc9b1e1..d69cc6c 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -116,9 +116,26 @@ static inline void spin_time_accum_blocked(u64 start)
 }
 #endif  /* CONFIG_XEN_DEBUG_FS */
 
+/*
+ * Size struct xen_spinlock so it''s the same as arch_spinlock_t.
+ */
+#if NR_CPUS < 256
+typedef u8 xen_spinners_t;
+# define inc_spinners(xl) \
+	asm(LOCK_PREFIX " incb %0" : "+m" ((xl)->spinners) : :
"memory");
+# define dec_spinners(xl) \
+	asm(LOCK_PREFIX " decb %0" : "+m" ((xl)->spinners) : :
"memory");
+#else
+typedef u16 xen_spinners_t;
+# define inc_spinners(xl) \
+	asm(LOCK_PREFIX " incw %0" : "+m" ((xl)->spinners) : :
"memory");
+# define dec_spinners(xl) \
+	asm(LOCK_PREFIX " decw %0" : "+m" ((xl)->spinners) : :
"memory");
+#endif
+
 struct xen_spinlock {
 	unsigned char lock;		/* 0 -> free; 1 -> locked */
-	unsigned short spinners;	/* count of waiting cpus */
+	xen_spinners_t spinners;	/* count of waiting cpus */
 };
 
 static int xen_spin_is_locked(struct arch_spinlock *lock)
@@ -164,8 +181,7 @@ static inline struct xen_spinlock *spinning_lock(struct
xen_spinlock *xl)
 
 	wmb();			/* set lock of interest before count */
 
-	asm(LOCK_PREFIX " incw %0"
-	    : "+m" (xl->spinners) : : "memory");
+	inc_spinners(xl);
 
 	return prev;
 }
@@ -176,8 +192,7 @@ static inline struct xen_spinlock *spinning_lock(struct
xen_spinlock *xl)
  */
 static inline void unspinning_lock(struct xen_spinlock *xl, struct xen_spinlock
*prev)
 {
-	asm(LOCK_PREFIX " decw %0"
-	    : "+m" (xl->spinners) : : "memory");
+	dec_spinners(xl);
 	wmb();			/* decrement count before restoring lock */
 	__this_cpu_write(lock_spinners, prev);
 }
@@ -373,6 +388,8 @@ void xen_uninit_lock_cpu(int cpu)
 
 void __init xen_init_spinlocks(void)
 {
+	BUILD_BUG_ON(sizeof(struct xen_spinlock) > sizeof(arch_spinlock_t));
+
 	pv_lock_ops.spin_is_locked = xen_spin_is_locked;
 	pv_lock_ops.spin_is_contended = xen_spin_is_contended;
 	pv_lock_ops.spin_lock = xen_spin_lock;
-- 
1.7.2.5

On Mon, Jan 23, 2012 at 07:32:25PM +0000, David Vrabel
wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> If NR_CPUS < 256 then arch_spinlock_t is only 16 bits wide but struct
> xen_spinlock is 32 bits.  When a spin lock is contended and
> xl->spinners is modified the two bytes immediately after the spin lock
> would be corrupted.
> 
> This is a regression caused by 84eb950db13ca40a0572ce9957e14723500943d6
> (x86, ticketlock: Clean up types and accessors) which reduced the size
> of arch_spinlock_t.
> 
> Fix this by making xl->spinners a u8 if NR_CPUS < 256.  A
> BUILD_BUG_ON() is also added to check the sizes of the two structures
> are compatible.
> 
> In many cases this was not noticable as there would often be padding
> bytes after the lock (e.g., if any of CONFIG_GENERIC_LOCKBREAK,
> CONFIG_DEBUG_SPINLOCK, or CONFIG_DEBUG_LOCK_ALLOC were enabled).
> 
> The bnx2 driver is affected. In struct bnx2, phy_lock and
> indirect_lock may have no padding after them.  Contention on phy_lock
> would corrupt indirect_lock making it appear locked and the driver
> would deadlock.
Nice find. I think it also affected the ahci driver, and some of the USB
ones - at least those I saw starting to hang with:

[   79.317407] BUG: soft lockup - CPU#1 stuck for 22s! [modprobe:2244]

Do you have some of those messages by any chance?

Going to test this overnight to see if it fixes the mysterious rash of
32-bit v3.3-rc1 build dying and showing:

 [<c1308681>] ? xen_poll_irq_timeout+0x41/0x60
 [<c13086ac>] xen_poll_irq+0xc/0x10
 [<c104447a>] xen_spin_lock_slow+0xca/0x210
 [<c10447bc>] xen_spin_lock+0xec/0x100
 [<c154a561>] _raw_spin_lock+0x21/0x30

Thanks!
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> ---
>  arch/x86/xen/spinlock.c |   27 ++++++++++++++++++++++-----
>  1 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
> index cc9b1e1..d69cc6c 100644
> --- a/arch/x86/xen/spinlock.c
> +++ b/arch/x86/xen/spinlock.c
> @@ -116,9 +116,26 @@ static inline void spin_time_accum_blocked(u64 start)
>  }
>  #endif  /* CONFIG_XEN_DEBUG_FS */
>  
> +/*
> + * Size struct xen_spinlock so it''s the same as arch_spinlock_t.
> + */
> +#if NR_CPUS < 256
> +typedef u8 xen_spinners_t;
> +# define inc_spinners(xl) \
> +	asm(LOCK_PREFIX " incb %0" : "+m" ((xl)->spinners)
: : "memory");
> +# define dec_spinners(xl) \
> +	asm(LOCK_PREFIX " decb %0" : "+m" ((xl)->spinners)
: : "memory");
> +#else
> +typedef u16 xen_spinners_t;
> +# define inc_spinners(xl) \
> +	asm(LOCK_PREFIX " incw %0" : "+m" ((xl)->spinners)
: : "memory");
> +# define dec_spinners(xl) \
> +	asm(LOCK_PREFIX " decw %0" : "+m" ((xl)->spinners)
: : "memory");
> +#endif
> +
>  struct xen_spinlock {
>  	unsigned char lock;		/* 0 -> free; 1 -> locked */
> -	unsigned short spinners;	/* count of waiting cpus */
> +	xen_spinners_t spinners;	/* count of waiting cpus */
>  };
>  
>  static int xen_spin_is_locked(struct arch_spinlock *lock)
> @@ -164,8 +181,7 @@ static inline struct xen_spinlock *spinning_lock(struct
xen_spinlock *xl)
>  
>  	wmb();			/* set lock of interest before count */
>  
> -	asm(LOCK_PREFIX " incw %0"
> -	    : "+m" (xl->spinners) : : "memory");
> +	inc_spinners(xl);
>  
>  	return prev;
>  }
> @@ -176,8 +192,7 @@ static inline struct xen_spinlock *spinning_lock(struct
xen_spinlock *xl)
>   */
>  static inline void unspinning_lock(struct xen_spinlock *xl, struct
xen_spinlock *prev)
>  {
> -	asm(LOCK_PREFIX " decw %0"
> -	    : "+m" (xl->spinners) : : "memory");
> +	dec_spinners(xl);
>  	wmb();			/* decrement count before restoring lock */
>  	__this_cpu_write(lock_spinners, prev);
>  }
> @@ -373,6 +388,8 @@ void xen_uninit_lock_cpu(int cpu)
>  
>  void __init xen_init_spinlocks(void)
>  {
> +	BUILD_BUG_ON(sizeof(struct xen_spinlock) > sizeof(arch_spinlock_t));
> +
>  	pv_lock_ops.spin_is_locked = xen_spin_is_locked;
>  	pv_lock_ops.spin_is_contended = xen_spin_is_contended;
>  	pv_lock_ops.spin_lock = xen_spin_lock;
> -- 
> 1.7.2.5

On Jan 23, 2012, at 11:32 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> If NR_CPUS < 256 then arch_spinlock_t is only 16 bits wide but struct
> xen_spinlock is 32 bits.  When a spin lock is contended and
> xl->spinners is modified the two bytes immediately after the spin lock
> would be corrupted.
> 
> This is a regression caused by 84eb950db13ca40a0572ce9957e14723500943d6
> (x86, ticketlock: Clean up types and accessors) which reduced the size
> of arch_spinlock_t.
Oh, *ouch*.  Very sorry about that one.

	J

On 23/01/2012 19:50, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 23, 2012 at 07:32:25PM +0000, David Vrabel wrote:
>> From: David Vrabel<david.vrabel@citrix.com>
>>
>> If NR_CPUS<  256 then arch_spinlock_t is only 16 bits wide but
struct
>> xen_spinlock is 32 bits.  When a spin lock is contended and
>> xl->spinners is modified the two bytes immediately after the spin
lock
>> would be corrupted.
>>
>> This is a regression caused by 84eb950db13ca40a0572ce9957e14723500943d6
>> (x86, ticketlock: Clean up types and accessors) which reduced the size
>> of arch_spinlock_t.
>>
>> Fix this by making xl->spinners a u8 if NR_CPUS<  256.  A
>> BUILD_BUG_ON() is also added to check the sizes of the two structures
>> are compatible.
>>
>> In many cases this was not noticable as there would often be padding
>> bytes after the lock (e.g., if any of CONFIG_GENERIC_LOCKBREAK,
>> CONFIG_DEBUG_SPINLOCK, or CONFIG_DEBUG_LOCK_ALLOC were enabled).
>>
>> The bnx2 driver is affected. In struct bnx2, phy_lock and
>> indirect_lock may have no padding after them.  Contention on phy_lock
>> would corrupt indirect_lock making it appear locked and the driver
>> would deadlock.
>
> Nice find. I think it also affected the ahci driver, and some of the USB
> ones - at least those I saw starting to hang with:
It''s possible but keep in mind that this isn''t a recent
regression.  I
think it''s been around since 3.0 and maybe even earlier.

David

On Mon, Jan 23, 2012 at 09:44:33PM +0000, David Vrabel
wrote:
> On 23/01/2012 19:50, Konrad Rzeszutek Wilk wrote:
> >On Mon, Jan 23, 2012 at 07:32:25PM +0000, David Vrabel wrote:
> >>From: David Vrabel<david.vrabel@citrix.com>
> >>
> >>If NR_CPUS<  256 then arch_spinlock_t is only 16 bits wide but
struct
> >>xen_spinlock is 32 bits.  When a spin lock is contended and
> >>xl->spinners is modified the two bytes immediately after the
spin lock
> >>would be corrupted.
> >>
> >>This is a regression caused by
84eb950db13ca40a0572ce9957e14723500943d6
> >>(x86, ticketlock: Clean up types and accessors) which reduced the
size
> >>of arch_spinlock_t.
> >>
> >>Fix this by making xl->spinners a u8 if NR_CPUS<  256.  A
> >>BUILD_BUG_ON() is also added to check the sizes of the two
structures
> >>are compatible.
> >>
> >>In many cases this was not noticable as there would often be
padding
> >>bytes after the lock (e.g., if any of CONFIG_GENERIC_LOCKBREAK,
> >>CONFIG_DEBUG_SPINLOCK, or CONFIG_DEBUG_LOCK_ALLOC were enabled).
> >>
> >>The bnx2 driver is affected. In struct bnx2, phy_lock and
> >>indirect_lock may have no padding after them.  Contention on
phy_lock
> >>would corrupt indirect_lock making it appear locked and the driver
> >>would deadlock.
> >
> >Nice find. I think it also affected the ahci driver, and some of the
USB
> >ones - at least those I saw starting to hang with:
> 
> It''s possible but keep in mind that this isn''t a recent
regression.  I
> think it''s been around since 3.0 and maybe even earlier.
konrad@phenom:~/work/linux.mm$ git tag --contains
84eb950db13ca40a0572ce9957e14723500943d6 | grep -v rc
v3.2

So since v3.2, hmm, I should double-check those serial logs
to see if the problems I had were in 3.2 as well.

On Mon, 2012-01-23 at 19:32 +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> If NR_CPUS < 256 then arch_spinlock_t is only 16 bits wide but struct
> xen_spinlock is 32 bits.  When a spin lock is contended and
> xl->spinners is modified the two bytes immediately after the spin lock
> would be corrupted.
> 
> This is a regression caused by 84eb950db13ca40a0572ce9957e14723500943d6
> (x86, ticketlock: Clean up types and accessors) which reduced the size
> of arch_spinlock_t.
> 
> Fix this by making xl->spinners a u8 if NR_CPUS < 256.  A
> BUILD_BUG_ON() is also added to check the sizes of the two structures
> are compatible.
> 
> In many cases this was not noticable as there would often be padding
> bytes after the lock (e.g., if any of CONFIG_GENERIC_LOCKBREAK,
> CONFIG_DEBUG_SPINLOCK, or CONFIG_DEBUG_LOCK_ALLOC were enabled).
> 
> The bnx2 driver is affected. In struct bnx2, phy_lock and
> indirect_lock may have no padding after them.  Contention on phy_lock
> would corrupt indirect_lock making it appear locked and the driver
> would deadlock.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Very good catch!
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> ---
>  arch/x86/xen/spinlock.c |   27 ++++++++++++++++++++++-----
>  1 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
> index cc9b1e1..d69cc6c 100644
> --- a/arch/x86/xen/spinlock.c
> +++ b/arch/x86/xen/spinlock.c
> @@ -116,9 +116,26 @@ static inline void spin_time_accum_blocked(u64 start)
>  }
>  #endif  /* CONFIG_XEN_DEBUG_FS */
>  
> +/*
> + * Size struct xen_spinlock so it''s the same as arch_spinlock_t.
> + */
> +#if NR_CPUS < 256
> +typedef u8 xen_spinners_t;
> +# define inc_spinners(xl) \
> +	asm(LOCK_PREFIX " incb %0" : "+m" ((xl)->spinners)
: : "memory");
> +# define dec_spinners(xl) \
> +	asm(LOCK_PREFIX " decb %0" : "+m" ((xl)->spinners)
: : "memory");
> +#else
> +typedef u16 xen_spinners_t;
> +# define inc_spinners(xl) \
> +	asm(LOCK_PREFIX " incw %0" : "+m" ((xl)->spinners)
: : "memory");
> +# define dec_spinners(xl) \
> +	asm(LOCK_PREFIX " decw %0" : "+m" ((xl)->spinners)
: : "memory");
> +#endif
> +
>  struct xen_spinlock {
>  	unsigned char lock;		/* 0 -> free; 1 -> locked */
> -	unsigned short spinners;	/* count of waiting cpus */
> +	xen_spinners_t spinners;	/* count of waiting cpus */
>  };
>  
>  static int xen_spin_is_locked(struct arch_spinlock *lock)
> @@ -164,8 +181,7 @@ static inline struct xen_spinlock *spinning_lock(struct
xen_spinlock *xl)
>  
>  	wmb();			/* set lock of interest before count */
>  
> -	asm(LOCK_PREFIX " incw %0"
> -	    : "+m" (xl->spinners) : : "memory");
> +	inc_spinners(xl);
>  
>  	return prev;
>  }
> @@ -176,8 +192,7 @@ static inline struct xen_spinlock *spinning_lock(struct
xen_spinlock *xl)
>   */
>  static inline void unspinning_lock(struct xen_spinlock *xl, struct
xen_spinlock *prev)
>  {
> -	asm(LOCK_PREFIX " decw %0"
> -	    : "+m" (xl->spinners) : : "memory");
> +	dec_spinners(xl);
>  	wmb();			/* decrement count before restoring lock */
>  	__this_cpu_write(lock_spinners, prev);
>  }
> @@ -373,6 +388,8 @@ void xen_uninit_lock_cpu(int cpu)
>  
>  void __init xen_init_spinlocks(void)
>  {
> +	BUILD_BUG_ON(sizeof(struct xen_spinlock) > sizeof(arch_spinlock_t));
> +
>  	pv_lock_ops.spin_is_locked = xen_spin_is_locked;
>  	pv_lock_ops.spin_is_contended = xen_spin_is_contended;
>  	pv_lock_ops.spin_lock = xen_spin_lock;