[PATCH] seqlock: fix seqlock_write_unlock_impl function

[PATCH] seqlock: fix seqlock_write_unlock_impl function

[Luc Michel]

The seqlock write unlock function was incorrectly calling
seqlock_write_begin() instead of seqlock_write_end(), and was releasing
the lock before incrementing the sequence. This could lead to a race
condition and a corrupted sequence number becoming odd even though the
lock is not held.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
---
 include/qemu/seqlock.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qemu/seqlock.h b/include/qemu/seqlock.h
index fd408b7ec5..8b6b4ee4bb 100644
--- a/include/qemu/seqlock.h
+++ b/include/qemu/seqlock.h
@@ -53,15 +53,15 @@ static inline void seqlock_write_lock_impl(QemuSeqLock *sl, QemuLockable *lock)
     seqlock_write_begin(sl);
 }
 #define seqlock_write_lock(sl, lock) \
     seqlock_write_lock_impl(sl, QEMU_MAKE_LOCKABLE(lock))
 
-/* Lock out other writers and update the count.  */
+/* Update the count and release the lock.  */
 static inline void seqlock_write_unlock_impl(QemuSeqLock *sl, QemuLockable *lock)
 {
+    seqlock_write_end(sl);
     qemu_lockable_unlock(lock);
-    seqlock_write_begin(sl);
 }
 #define seqlock_write_unlock(sl, lock) \
     seqlock_write_unlock_impl(sl, QEMU_MAKE_LOCKABLE(lock))
 
 
-- 
2.25.0

Re: [PATCH] seqlock: fix seqlock_write_unlock_impl function

[Philippe Mathieu-Daudé]

On 1/29/20 3:49 PM, Luc Michel wrote:
> The seqlock write unlock function was incorrectly calling
> seqlock_write_begin() instead of seqlock_write_end(), and was releasing
> the lock before incrementing the sequence. This could lead to a race
> condition and a corrupted sequence number becoming odd even though the
> lock is not held.

I'm surprised it took 18 months to figure this out.

Fixes: 988fcafc730
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> ---
>   include/qemu/seqlock.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/seqlock.h b/include/qemu/seqlock.h
> index fd408b7ec5..8b6b4ee4bb 100644
> --- a/include/qemu/seqlock.h
> +++ b/include/qemu/seqlock.h
> @@ -53,15 +53,15 @@ static inline void seqlock_write_lock_impl(QemuSeqLock *sl, QemuLockable *lock)
>       seqlock_write_begin(sl);
>   }
>   #define seqlock_write_lock(sl, lock) \
>       seqlock_write_lock_impl(sl, QEMU_MAKE_LOCKABLE(lock))
>   
> -/* Lock out other writers and update the count.  */
> +/* Update the count and release the lock.  */
>   static inline void seqlock_write_unlock_impl(QemuSeqLock *sl, QemuLockable *lock)
>   {
> +    seqlock_write_end(sl);
>       qemu_lockable_unlock(lock);
> -    seqlock_write_begin(sl);
>   }
>   #define seqlock_write_unlock(sl, lock) \
>       seqlock_write_unlock_impl(sl, QEMU_MAKE_LOCKABLE(lock))
>   
>   
> 

Re: [PATCH] seqlock: fix seqlock_write_unlock_impl function

[Paolo Bonzini]

On 29/01/20 15:49, Luc Michel wrote:
> The seqlock write unlock function was incorrectly calling
> seqlock_write_begin() instead of seqlock_write_end(), and was releasing
> the lock before incrementing the sequence. This could lead to a race
> condition and a corrupted sequence number becoming odd even though the
> lock is not held.
> 
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> ---
>  include/qemu/seqlock.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/seqlock.h b/include/qemu/seqlock.h
> index fd408b7ec5..8b6b4ee4bb 100644
> --- a/include/qemu/seqlock.h
> +++ b/include/qemu/seqlock.h
> @@ -53,15 +53,15 @@ static inline void seqlock_write_lock_impl(QemuSeqLock *sl, QemuLockable *lock)
>      seqlock_write_begin(sl);
>  }
>  #define seqlock_write_lock(sl, lock) \
>      seqlock_write_lock_impl(sl, QEMU_MAKE_LOCKABLE(lock))
>  
> -/* Lock out other writers and update the count.  */
> +/* Update the count and release the lock.  */
>  static inline void seqlock_write_unlock_impl(QemuSeqLock *sl, QemuLockable *lock)
>  {
> +    seqlock_write_end(sl);
>      qemu_lockable_unlock(lock);
> -    seqlock_write_begin(sl);
>  }
>  #define seqlock_write_unlock(sl, lock) \
>      seqlock_write_unlock_impl(sl, QEMU_MAKE_LOCKABLE(lock))
>  
>  
> 

Queued, thanks very much.

Paolo