exec: Transform exec_update_mutex into a rw_semaphore

In preparation for converting exec_update_mutex to a rwsem so that multiple readers can execute in parallel and not deadlock, add down_read_killable_nested. This is needed so that kcmp_lock can be converted from working on a mutexes to working on rw_semaphores.

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Will Deacon <[email protected]>

Signed-off-by Eric W. Biederman <[email protected]>

---
  include/linux/rwsem.h  |  2 ++
  kernel/locking/rwsem.c | 14 ++++++++++++++
  2 files changed, 16 insertions(+)

include/linux/rwsem.h +2 -0

index 25e3fde85617..13021b08b2ed 100644
@@ -171,6 +171,7 @@ extern void downgrade_write(struct rw_semaphore *sem);
   * See Documentation/locking/lockdep-design.rst for more details.)
   */
  extern void down_read_nested(struct rw_semaphore *sem, int subclass);
+extern int __must_check down_read_killable_nested(struct rw_semaphore *sem, int subclass);
  extern void down_write_nested(struct rw_semaphore *sem, int subclass);
  extern int down_write_killable_nested(struct rw_semaphore *sem, int subclass);
  extern void _down_write_nest_lock(struct rw_semaphore *sem, struct lockdep_map *nest_lock);

@@ -191,6 +192,7 @@ extern void down_read_non_owner(struct rw_semaphore *sem);
  extern void up_read_non_owner(struct rw_semaphore *sem);
  #else
  # define down_read_nested(sem, subclass)		down_read(sem)
+# define down_read_killable_nested(sem, subclass)	down_read_killable(sem)
  # define down_write_nest_lock(sem, nest_lock)	down_write(sem)
  # define down_write_nested(sem, subclass)	down_write(sem)
  # define down_write_killable_nested(sem, subclass)	down_write_killable(sem)

kernel/locking/rwsem.c +7 -0

index f11b9bd3431d..54d11cb97551 100644
@@ -1605,6 +1605,20 @@ void down_read_nested(struct rw_semaphore *sem, int subclass)
  }
  EXPORT_SYMBOL(down_read_nested);
 
+int down_read_killable_nested(struct rw_semaphore *sem, int subclass)
+{
+	might_sleep();
+	rwsem_acquire_read(&sem->dep_map, subclass, 0, _RET_IP_);
+
+	if (LOCK_CONTENDED_RETURN(sem, __down_read_trylock, __down_read_killable)) {
+		rwsem_release(&sem->dep_map, _RET_IP_);

In preparation for converting exec_update_mutex to a rwsem so that multiple readers can execute in parallel and not deadlock, add down_read_interruptible. This is needed for perf_event_open to be converted (with no semantic changes) from working on a mutex to wroking on a rwsem.

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Will Deacon <[email protected]>

Signed-off-by Eric W. Biederman <[email protected]>

---
  include/linux/rwsem.h  |  1 +
  kernel/locking/rwsem.c | 26 ++++++++++++++++++++++++++
  2 files changed, 27 insertions(+)

include/linux/rwsem.h +1 -0

index 13021b08b2ed..4c715be48717 100644
@@ -123,6 +123,7 @@ static inline int rwsem_is_contended(struct rw_semaphore *sem)
   * lock for reading
   */
  extern void down_read(struct rw_semaphore *sem);
+extern int __must_check down_read_interruptible(struct rw_semaphore *sem);
  extern int __must_check down_read_killable(struct rw_semaphore *sem);
 
  /*

kernel/locking/rwsem.c +19 -0

index 54d11cb97551..a163542d178e 100644
@@ -1345,6 +1345,18 @@ static inline void __down_read(struct rw_semaphore *sem)
  	}
  }
 
+static inline int __down_read_interruptible(struct rw_semaphore *sem)
+{
+	if (!rwsem_read_trylock(sem)) {
+		if (IS_ERR(rwsem_down_read_slowpath(sem, TASK_INTERRUPTIBLE)))
+			return -EINTR;
+		DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
+	} else {
+		rwsem_set_reader_owned(sem);
+	}
+	return 0;
+}
+
  static inline int __down_read_killable(struct rw_semaphore *sem)
  {
  	if (!rwsem_read_trylock(sem)) {

@@ -1495,6 +1507,20 @@ void __sched down_read(struct rw_semaphore *sem)
  }
  EXPORT_SYMBOL(down_read);
 
+int __sched down_read_interruptible(struct rw_semaphore *sem)
+{
+	might_sleep();
+	rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_);
+
+	if (LOCK_CONTENDED_RETURN(sem, __down_read_trylock, __down_read_interruptible)) {
+		rwsem_release(&sem->dep_map, _RET_IP_);

Quoted 29 lines · Eric W. Biederman

(quoted diff) +23 -0

+static inline int __down_read_interruptible(struct rw_semaphore *sem)
+{
+	if (!rwsem_read_trylock(sem)) {
+		if (IS_ERR(rwsem_down_read_slowpath(sem, TASK_INTERRUPTIBLE)))
+			return -EINTR;
+		DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
+	} else {
+		rwsem_set_reader_owned(sem);
+	}
+	return 0;
+}
+
  static inline int __down_read_killable(struct rw_semaphore *sem)
  {
  	if (!rwsem_read_trylock(sem)) {

@@ -1495,6 +1507,20 @@ void __sched down_read(struct rw_semaphore *sem)
  }
  EXPORT_SYMBOL(down_read);
+int __sched down_read_interruptible(struct rw_semaphore *sem)
+{
+	might_sleep();
+	rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_);
+
+	if (LOCK_CONTENDED_RETURN(sem, __down_read_trylock, __down_read_interruptible)) {
+		rwsem_release(&sem->dep_map, _RET_IP_);
+		return -EINTR;
+	}
+
+	return 0;

Review void __sched down_read(struct rw_semaphore *sem) · line 1525

Quoted 6 lines

(quoted diff) +3 -0

+}
+EXPORT_SYMBOL(down_read_interruptible);
+
  int __sched down_read_killable(struct rw_semaphore *sem)
  {
  	might_sleep();

Acked-by Waiman Long <[email protected]>

Quoted 29 lines · Waiman Long

Quoted 28 lines · Eric W. Biederman

(quoted diff) +22 -0

+static inline int __down_read_interruptible(struct rw_semaphore *sem)
+{
+	if (!rwsem_read_trylock(sem)) {
+		if (IS_ERR(rwsem_down_read_slowpath(sem, TASK_INTERRUPTIBLE)))
+			return -EINTR;
+		DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
+	} else {
+		rwsem_set_reader_owned(sem);
+	}
+	return 0;
+}
+
   static inline int __down_read_killable(struct rw_semaphore *sem)
   {
   	if (!rwsem_read_trylock(sem)) {

@@ -1495,6 +1507,20 @@ void __sched down_read(struct rw_semaphore *sem)
   }
   EXPORT_SYMBOL(down_read);
+int __sched down_read_interruptible(struct rw_semaphore *sem)
+{
+	might_sleep();
+	rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_);
+
+	if (LOCK_CONTENDED_RETURN(sem, __down_read_trylock, __down_read_interruptible)) {
+		rwsem_release(&sem->dep_map, _RET_IP_);
+		return -EINTR;
+	}
+

Quoted 8 lines

Review void __sched down_read(struct rw_semaphore *sem) · line 1525

Quoted 7 lines

(quoted diff) +4 -0

+	return 0;
+}
+EXPORT_SYMBOL(down_read_interruptible);
+
   int __sched down_read_killable(struct rw_semaphore *sem)
   {
   	might_sleep();

Acked-by Waiman Long <[email protected]>

Yeah, that seems correct.. There's an unfortunate amount of copy-paste there though.

Do we want to follow that up with something like this?

Sent: 07 December 2020 15:34

Quoted 38 lines · Peter Zijlstra

Quoted 37 lines · Waiman Long

Quoted 25 lines · Eric W. Biederman

(quoted diff) +19 -0

+static inline int __down_read_interruptible(struct rw_semaphore *sem)
+{
+	if (!rwsem_read_trylock(sem)) {
+		if (IS_ERR(rwsem_down_read_slowpath(sem, TASK_INTERRUPTIBLE)))
+			return -EINTR;
+		DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
+	} else {
+		rwsem_set_reader_owned(sem);
+	}
+	return 0;
+}
+
   static inline int __down_read_killable(struct rw_semaphore *sem)
   {
   	if (!rwsem_read_trylock(sem)) {

@@ -1495,6 +1507,20 @@ void __sched down_read(struct rw_semaphore *sem)
   }
   EXPORT_SYMBOL(down_read);
+int __sched down_read_interruptible(struct rw_semaphore *sem)
+{
+	might_sleep();
+	rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_);
+
+	if (LOCK_CONTENDED_RETURN(sem, __down_read_trylock, __down_read_interruptible)) {
+		rwsem_release(&sem->dep_map, _RET_IP_);

Review void __sched down_read(struct rw_semaphore *sem) · line 1525

Quoted 10 lines

(quoted diff) +7 -0

+		return -EINTR;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(down_read_interruptible);
+
   int __sched down_read_killable(struct rw_semaphore *sem)
   {
   	might_sleep();

Acked-by Waiman Long <[email protected]>

Quoted 4 lines

Yeah, that seems correct.. There's an unfortunate amount of copy-paste there though.

Do we want to follow that up with something like this?

I am actually thinking about similar streamlining once the patch lands.

Your suggested changes look fine to me.

From: Waiman Long

Quoted 47 lines

Sent: 07 December 2020 15:34

Quoted 38 lines · Peter Zijlstra

Quoted 29 lines · Waiman Long

Quoted 28 lines · Eric W. Biederman

(quoted diff) +22 -0

+static inline int __down_read_interruptible(struct rw_semaphore *sem)
+{
+	if (!rwsem_read_trylock(sem)) {
+		if (IS_ERR(rwsem_down_read_slowpath(sem, TASK_INTERRUPTIBLE)))
+			return -EINTR;
+		DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
+	} else {
+		rwsem_set_reader_owned(sem);
+	}
+	return 0;
+}
+
    static inline int __down_read_killable(struct rw_semaphore *sem)
    {
    	if (!rwsem_read_trylock(sem)) {

@@ -1495,6 +1507,20 @@ void __sched down_read(struct rw_semaphore *sem)
    }
    EXPORT_SYMBOL(down_read);
+int __sched down_read_interruptible(struct rw_semaphore *sem)
+{
+	might_sleep();
+	rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_);
+
+	if (LOCK_CONTENDED_RETURN(sem, __down_read_trylock, __down_read_interruptible)) {
+		rwsem_release(&sem->dep_map, _RET_IP_);
+		return -EINTR;
+	}
+

Quoted 8 lines

Review void __sched down_read(struct rw_semaphore *sem) · line 1525

Quoted 7 lines

(quoted diff) +4 -0

+	return 0;
+}
+EXPORT_SYMBOL(down_read_interruptible);
+
    int __sched down_read_killable(struct rw_semaphore *sem)
    {
    	might_sleep();

Acked-by Waiman Long <[email protected]>

Quoted 4 lines

Yeah, that seems correct.. There's an unfortunate amount of copy-paste there though.

Do we want to follow that up with something like this?

I am actually thinking about similar streamlining once the patch lands.

Quoted 1 line

Your suggested changes look fine to me.

How much more difficult would it be to also add a timeout option? I looked at adding one to the mutex code - and fell into a big pile of replicated code.

ISTM that one the initial locked exchange (and spin) fails a few extra instructions when heading for the sleep don't really matter

Sent: 07 December 2020 19:02

Quoted 7 lines

How much more difficult would it be to also add a timeout option? I looked at adding one to the mutex code - and fell into a big pile of replicated code.

ISTM that one the initial locked exchange (and spin) fails a few extra instructions when heading for the sleep don't really matter

Actually, I had tried that before. See

https://lore.kernel.org/lkml/[email protected]/

That is for rwsem, but the same can be done for mutex. However, Peter didn't seem to like the idea of a timeout parameter. Anyway, it is certainly doable if there is a good use case for it.

From: Waiman Long

Quoted 1 line

Sent: 07 December 2020 19:02

...

Quoted 11 lines

Quoted 7 lines

How much more difficult would it be to also add a timeout option? I looked at adding one to the mutex code - and fell into a big pile of replicated code.

ISTM that one the initial locked exchange (and spin) fails a few extra instructions when heading for the sleep don't really matter

Actually, I had tried that before. See

https://lore.kernel.org/lkml/[email protected]/

Quoted 3 lines

That is for rwsem, but the same can be done for mutex. However, Peter didn't seem to like the idea of a timeout parameter. Anyway, it is certainly doable if there is a good use case for it.

'Unfortunately' my use-case if for an out-of-tree driver.

The problem I was solving is a status call blocking because some other code is 'stuck' (probably an oops) with a mutex held.

Sent: 08 December 2020 12:32

Quoted 18 lines · David Laight

From: Waiman Long

Quoted 1 line

Sent: 07 December 2020 19:02

...

Quoted 14 lines

Quoted 7 lines

How much more difficult would it be to also add a timeout option? I looked at adding one to the mutex code - and fell into a big pile of replicated code.

ISTM that one the initial locked exchange (and spin) fails a few extra instructions when heading for the sleep don't really matter

Actually, I had tried that before. See

https://lore.kernel.org/lkml/[email protected]/

That is for rwsem, but the same can be done for mutex. However, Peter didn't seem to like the idea of a timeout parameter. Anyway, it is certainly doable if there is a good use case for it.

Quoted 4 lines

'Unfortunately' my use-case if for an out-of-tree driver.

The problem I was solving is a status call blocking because some other code is 'stuck' (probably an oops) with a mutex held.

Working around oopses is not a sane use-case. If you oops, you get to keep all the pieces.

From: Waiman Long

Quoted 1 line

Sent: 07 December 2020 19:02

...

Quoted 7 lines

Quoted 7 lines

How much more difficult would it be to also add a timeout option? I looked at adding one to the mutex code - and fell into a big pile of replicated code.

ISTM that one the initial locked exchange (and spin) fails a few extra instructions when heading for the sleep don't really matter

Quoted 7 lines

Actually, I had tried that before. See

https://lore.kernel.org/lkml/[email protected]/

That is for rwsem, but the same can be done for mutex. However, Peter didn't seem to like the idea of a timeout parameter. Anyway, it is certainly doable if there is a good use case for it.

'Unfortunately' my use-case if for an out-of-tree driver.

The problem I was solving is a status call blocking because some other code is 'stuck' (probably an oops) with a mutex held.

The code used to use down_timeout() (it was written for 2.4). When I changed to mutex_(to get optimistic spinning) I lost the ability to do the timeouts.

Sent: 08 December 2020 15:34

Quoted 22 lines · David Laight

From: Waiman Long

Quoted 1 line

Sent: 07 December 2020 19:02

...

Quoted 7 lines

Quoted 7 lines

How much more difficult would it be to also add a timeout option? I looked at adding one to the mutex code - and fell into a big pile of replicated code.

ISTM that one the initial locked exchange (and spin) fails a few extra instructions when heading for the sleep don't really matter

Quoted 7 lines

Actually, I had tried that before. See

https://lore.kernel.org/lkml/[email protected]/

That is for rwsem, but the same can be done for mutex. However, Peter didn't seem to like the idea of a timeout parameter. Anyway, it is certainly doable if there is a good use case for it.

'Unfortunately' my use-case if for an out-of-tree driver.

The problem I was solving is a status call blocking because some other code is 'stuck' (probably an oops) with a mutex held.

Quoted 3 lines

The code used to use down_timeout() (it was written for 2.4). When I changed to mutex_(to get optimistic spinning) I lost the ability to do the timeouts.

The primary reason for sending out that patchset was to work around some circular locking problem in existing code even though these circular locking scenarios are not likely to happen. Your case is certainly another potential circular locking problem as well.

Quoted 31 lines · Waiman Long

Quoted 29 lines · Eric W. Biederman

(quoted diff) +23 -0

+static inline int __down_read_interruptible(struct rw_semaphore *sem)
+{
+	if (!rwsem_read_trylock(sem)) {
+		if (IS_ERR(rwsem_down_read_slowpath(sem, TASK_INTERRUPTIBLE)))
+			return -EINTR;
+		DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
+	} else {
+		rwsem_set_reader_owned(sem);
+	}
+	return 0;
+}
+
  static inline int __down_read_killable(struct rw_semaphore *sem)
  {
  	if (!rwsem_read_trylock(sem)) {

@@ -1495,6 +1507,20 @@ void __sched down_read(struct rw_semaphore *sem)
  }
  EXPORT_SYMBOL(down_read);
+int __sched down_read_interruptible(struct rw_semaphore *sem)
+{
+	might_sleep();
+	rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_);
+
+	if (LOCK_CONTENDED_RETURN(sem, __down_read_trylock, __down_read_interruptible)) {
+		rwsem_release(&sem->dep_map, _RET_IP_);
+		return -EINTR;
+	}
+
+	return 0;

Quoted 8 lines

Review void __sched down_read(struct rw_semaphore *sem) · line 1525

Quoted 6 lines

(quoted diff) +3 -0

+}
+EXPORT_SYMBOL(down_read_interruptible);
+
  int __sched down_read_killable(struct rw_semaphore *sem)
  {
  	might_sleep();

Acked-by Waiman Long <[email protected]>

Yeah, that seems correct.. There's an unfortunate amount of copy-paste there though.

Do we want to follow that up with something like this?

---

kernel/locking/rwsem.c +45 -51

@@ -275,7 +275,25 @@ static inline bool rwsem_read_trylock(st
 	long cnt = atomic_long_add_return_acquire(RWSEM_READER_BIAS, &sem->count);
 	if (WARN_ON_ONCE(cnt < 0))
 		rwsem_set_nonspinnable(sem);
-	return !(cnt & RWSEM_READ_FAILED_MASK);
+
+	if (!(cnt & RWSEM_READ_FAILED_MASK)) {
+		rwsem_set_reader_owned(sem);
+		return true;
+	}
+
+	return false;
+}
+
+static inline bool rwsem_write_trylock(struct rw_semaphore *sem)
+{
+	long tmp = RWSEM_UNLOCKED_VALUE;
+
+	if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp, RWSEM_WRITER_LOCKED)) {
+		rwsem_set_owner(sem);
+		return true;
+	}
+
+	return false;
 }
 
 /*

@@ -1335,38 +1353,29 @@ static struct rw_semaphore *rwsem_downgr
 /*
  * lock for reading
  */
-static inline void __down_read(struct rw_semaphore *sem)
+static inline int __down_read_common(struct rw_semaphore *sem, int state)
 {
 	if (!rwsem_read_trylock(sem)) {
-		rwsem_down_read_slowpath(sem, TASK_UNINTERRUPTIBLE);
+		if (IS_ERR(rwsem_down_read_slowpath(sem, state)))
+			return -EINTR;
 		DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
-	} else {
-		rwsem_set_reader_owned(sem);
 	}
+	return 0;
 }
 
-static inline int __down_read_interruptible(struct rw_semaphore *sem)
+static __always_inline void __down_read(struct rw_semaphore *sem)
 {
-	if (!rwsem_read_trylock(sem)) {
-		if (IS_ERR(rwsem_down_read_slowpath(sem, TASK_INTERRUPTIBLE)))
-			return -EINTR;
-		DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
-	} else {
-		rwsem_set_reader_owned(sem);
-	}
-	return 0;
+	__down_read_common(sem, TASK_UNINTERRUPTIBLE);
 }
 
-static inline int __down_read_killable(struct rw_semaphore *sem)
+static __always_inline int __down_read_interruptible(struct rw_semaphore *sem)
 {
-	if (!rwsem_read_trylock(sem)) {
-		if (IS_ERR(rwsem_down_read_slowpath(sem, TASK_KILLABLE)))
-			return -EINTR;
-		DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
-	} else {
-		rwsem_set_reader_owned(sem);
-	}
-	return 0;
+	return __down_read_common(sem, TASK_INTERRUPTIBLE);
+}
+
+static __always_inline int __down_read_killable(struct rw_semaphore *sem)
+{
+	return __down_read_common(sem, TASK_KILLABLE);
 }
 
 static inline int __down_read_trylock(struct rw_semaphore *sem)

@@ -1392,44 +1401,29 @@ static inline int __down_read_trylock(st
 /*
  * lock for writing
  */
-static inline void __down_write(struct rw_semaphore *sem)
+static inline int __down_write_common(struct rw_semaphore *sem, int state)
 {
-	long tmp = RWSEM_UNLOCKED_VALUE;
-
-	if (unlikely(!atomic_long_try_cmpxchg_acquire(&sem->count, &tmp,
-						      RWSEM_WRITER_LOCKED)))
-		rwsem_down_write_slowpath(sem, TASK_UNINTERRUPTIBLE);
-	else
-		rwsem_set_owner(sem);
+	if (unlikely(!rwsem_write_trylock(sem))) {
+		if (IS_ERR(rwsem_down_write_slowpath(sem, state)))
+			return -EINTR;
+	}
+	return 0;
 }
 
-static inline int __down_write_killable(struct rw_semaphore *sem)
+static __always_inline void __down_write(struct rw_semaphore *sem)
 {
-	long tmp = RWSEM_UNLOCKED_VALUE;
+	__down_write_common(sem, TASK_UNINTERRUPTIBLE);
+}
 
-	if (unlikely(!atomic_long_try_cmpxchg_acquire(&sem->count, &tmp,
-						      RWSEM_WRITER_LOCKED))) {
-		if (IS_ERR(rwsem_down_write_slowpath(sem, TASK_KILLABLE)))
-			return -EINTR;
-	} else {
-		rwsem_set_owner(sem);
-	}
-	return 0;
+static __always_inline int __down_write_killable(struct rw_semaphore *sem)
+{
+	return __down_write_common(sem, TASK_KILLABLE);
 }
 
 static inline int __down_write_trylock(struct rw_semaphore *sem)
 {
-	long tmp;
-
 	DEBUG_RWSEMS_WARN_ON(sem->magic != sem, sem);
-
-	tmp  = RWSEM_UNLOCKED_VALUE;
-	if (atomic_long_try_cmpxchg_acquire(&sem->count, &tmp,
-					    RWSEM_WRITER_LOCKED)) {
-		rwsem_set_owner(sem);
-		return true;
-	}
-	return false;
+	return rwsem_write_trylock(sem);
 }
 
 /*

Do you want to pull these two into a topic branch in the tip tree based on v10-rc1?

Quoted 2 lines · Eric W. Biederman

Do you want to pull these two into a topic branch in the tip tree based on v10-rc1?

I'll go do that. I'll let the robots chew on it before pushing it out though, I'll reply once it's in tip.git.

Quoted 7 lines · Peter Zijlstra

Quoted 2 lines · Eric W. Biederman

Do you want to pull these two into a topic branch in the tip tree based on v10-rc1?

I'll go do that. I'll let the robots chew on it before pushing it out though, I'll reply once it's in tip.git.

Thanks,

Quoted 7 lines · Eric W. Biederman

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines · Eric W. Biederman

Do you want to pull these two into a topic branch in the tip tree based on v10-rc1?

Quoted 4 lines

Quoted 2 lines

I'll go do that. I'll let the robots chew on it before pushing it out though, I'll reply once it's in tip.git.

Thanks,

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking/rwsem

Quoted 11 lines · Peter Zijlstra

Quoted 10 lines · Eric W. Biederman

Quoted 7 lines · Peter Zijlstra

Quoted 2 lines · Eric W. Biederman

Do you want to pull these two into a topic branch in the tip tree based on v10-rc1?

I'll go do that. I'll let the robots chew on it before pushing it out though, I'll reply once it's in tip.git.

Quoted 3 lines

Quoted 1 line

Thanks,

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking/rwsem

Is that branch supposed to be against 34816d20f173
("Merge tag 'gfs2-v5.10-rc5-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2")

If so I can live with that, but it is a little awkward to work with a base that recent. As all of my other branches have an older base.

kernel/kcmp.c +4 -3

+		err = down_read_killable_nested(l2, SINGLE_DEPTH_NESTING);
 		if (err)
-			mutex_unlock(m1);
+			up_read(l1);
 	}
 
 	return err;

@@ -156,8 +156,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
 	/*
 	 * One should have enough rights to inspect task details.
 	 */
-	ret = kcmp_lock(&task1->signal->exec_update_mutex,
-			&task2->signal->exec_update_mutex);
+	ret = kcmp_lock(&task1->signal->exec_update_lock,
+			&task2->signal->exec_update_lock);
 	if (ret)
 		goto err;
 	if (!ptrace_may_access(task1, PTRACE_MODE_READ_REALCREDS) ||

kernel/kcmp.c get_file_raw_ptr(struct task_struct *task, unsigned int idx) · line 75

Quoted 9 lines

kernel/kcmp.c +2 -4

-static void kcmp_unlock(struct mutex *m1, struct mutex *m2)
+static void kcmp_unlock(struct rw_semaphore *l1, struct rw_semaphore *l2)
 {
-     if (likely(m2 != m1))
-             mutex_unlock(m2);
-     mutex_unlock(m1);
+     if (likely(l2 != l1))

is this still necessary ?

Quoted 9 lines

kernel/kcmp.c +3 -1

+             up_read(l2);
+     up_read(l1);
 }
 
-static int kcmp_lock(struct mutex *m1, struct mutex *m2)
+static int kcmp_lock(struct rw_semaphore *l1, struct rw_semaphore *l2)
 {
      int err;

kernel/kcmp.c get_file_raw_ptr(struct task_struct *task, unsigned int idx) · line 85

Quoted 4 lines

kernel/kcmp.c +2 -2

-     if (m2 > m1)
-             swap(m1, m2);
+     if (l2 > l1)
+             swap(l1, l2);

and this is probably also no longer necessary?

Quoted 21 lines · Bernd Edlinger

kernel/kcmp.c get_file_raw_ptr(struct task_struct *task, unsigned int idx) · line 75

Quoted 9 lines

kernel/kcmp.c +2 -4

-static void kcmp_unlock(struct mutex *m1, struct mutex *m2)
+static void kcmp_unlock(struct rw_semaphore *l1, struct rw_semaphore *l2)
 {
-     if (likely(m2 != m1))
-             mutex_unlock(m2);
-     mutex_unlock(m1);
+     if (likely(l2 != l1))

is this still necessary ?

Quoted 9 lines

kernel/kcmp.c +3 -1

+             up_read(l2);
+     up_read(l1);
 }
 
-static int kcmp_lock(struct mutex *m1, struct mutex *m2)
+static int kcmp_lock(struct rw_semaphore *l1, struct rw_semaphore *l2)
 {
      int err;

Quoted 6 lines

kernel/kcmp.c get_file_raw_ptr(struct task_struct *task, unsigned int idx) · line 85

Quoted 4 lines

kernel/kcmp.c +2 -2

-     if (m2 > m1)
-             swap(m1, m2);
+     if (l2 > l1)
+             swap(l1, l2);

and this is probably also no longer necessary?

These are still necessary, because even a recursive read lock can still block on a writer trying to come in between the two read locks due to fairness guarantees.

So taking the same read lock twice is still a source of possible deadlocks.

For the same reason, read locks still have ABBA deadlock and need to be taken in order.

So switching from a mutex to a rwlock doesn't really change the locking rules in this respect.

In fact, I'm not convinced this change even fixes the deadlock that syzbot reported, for the same reason: it just requires a write lock in between two read locks to deadlock.

From a deadlock perspective the change is strictly better than what we have today. The readers will no longer block on each other.

For the specific case that syzbot reported it is readers who were blocking on each other so that specific case if fixed.

On the write side of exec_update_lock we have:

cred_guard_mutex -> exec_update_lock

Which means that to get an ABBA deadlock cred_guard_mutex would need to be involved

Quoted 3 lines · Eric W. Biederman

From a deadlock perspective the change is strictly better than what we have today. The readers will no longer block on each other.

No, agreed, it's better regardless.

Quoted 2 lines

For the specific case that syzbot reported it is readers who were blocking on each other so that specific case if fixed.

So the thing is, a reader can still block another reader if a writer comes in between them. Which is why I was thinking that the same deadlock could happen if somebody does an execve at just the right point.

Quoted 6 lines

On the write side of exec_update_lock we have:

cred_guard_mutex -> exec_update_lock

Which means that to get an ABBA deadlock cred_guard_mutex would need to be involved

No, see above: you can get a deadlock with

- first reader gets exec_update_lock

- writer on exec_update_lock blocks on first reader (this is exec)

- second reader of exec_update_lock now blocks on the writer.

So if that second reader holds something that the first one wants to get (or is the same thread as the first one), you have a deadlock: the first reader will never make any progress, will never release the lock, and the writer will never get it, and the second reader will forever wait for the writer that is ahead of it in the queue.

cred_guard_mutex is immaterial, it's not involved in the deadlock. Yes, the writer holds it, but it's not relevant for anything else.

And that deadlock looks very much like what syzcaller detected, in exactly that scenario:

  Chain exists of:
    &sig->exec_update_mutex --> sb_writers#4 --> &p->lock

   Possible unsafe locking scenario:

         CPU0                    CPU1
         ----                    ----
    lock(&p->lock);
                                 lock(sb_writers#4);
                                 lock(&p->lock);
    lock(&sig->exec_update_mutex);

   *** DEADLOCK ***

No? The only thing that is missing is that writer that causes the exec_update_mutex readers to block each other - exactly like they did when it was a mutex.

But I may be missing something entirely obvious that keeps this from happening.

   perf_event_open  (exec_update_mutex -> ovl_i_mutex)
   chown            (ovl_i_mutex       -> sb_writes)
   sendfile         (sb_writes         -> p->lock)

by reading from a proc file and writing to overlayfs
proc_pid_syscall (p->lock -> exec_update_mutex)

Linus

Quoted 2 lines

   perf_event_open  (exec_update_mutex -> ovl_i_mutex)

Quoted 2 lines · Bernd Edlinger

Quoted 2 lines

   perf_event_open  (exec_update_mutex -> ovl_i_mutex)

Side note: this one looks like it should be easy to fix.

Is there any real reason why exec_update_mutex is actually gotten that early, and held for that long in the perf event code?

I _think_ we could move the ptrace check to be much later, to _just_ before that

* This is the point on no return; we cannot fail hereafter.

point in the perf event install chain..

I don't think it needs to be moved down even that much, I think it would be sufficient to move it down below the "perf_event_alloc()", but I didn't check very much.

Quoted 9 lines · Linus Torvalds

Quoted 2 lines · Bernd Edlinger

Quoted 2 lines

   perf_event_open  (exec_update_mutex -> ovl_i_mutex)

Side note: this one looks like it should be easy to fix.

Is there any real reason why exec_update_mutex is actually gotten that early, and held for that long in the perf event code?

afaict just to validate the whole operation early. Per 79c9ce57eb2 the mutex will guard the check and the perf_install_in_context vs exec.

Quoted 6 lines

I _think_ we could move the ptrace check to be much later, to _just_ before that

* This is the point on no return; we cannot fail hereafter.

point in the perf event install chain..

Peter had the idea of doing the ptrace_may_access() check twice: first lockless and early, then under exec_update_mutex when it mattered right before perf_install_in_context():

https://lore.kernel.org/linux-fsdevel/[email protected]/

Quoted 1 line

Quoted 3 lines

I don't think it needs to be moved down even that much, I think it would be sufficient to move it down below the "perf_event_alloc()", but I didn't check very much.

Yeah we could just keep a single ptrace_may_access() check just further down until it won't deadlock.

I am trying to understand why the permission check is there.

Quoted 2 lines · Bernd Edlinger

Quoted 2 lines

   perf_event_open  (exec_update_mutex -> ovl_i_mutex)

Side note: this one looks like it should be easy to fix.

PeterZ, is there something I'm missing?

Quoted 2 lines

PeterZ, is there something I'm missing?

Like this?

https://lkml.kernel.org/r/[email protected]

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines

PeterZ, is there something I'm missing?

Like this?

Quoted 1 line

https://lkml.kernel.org/r/[email protected]

Yes, except I think you should remove the old ptrace_may_access() check.

I don't see any point at all in checking privileges twice, and I do see real downsides. Not just that KCSAN issue, but also lack of coverage (ie the second check will then effectively never be tested, which is bad too).

I suppose I'll queue the below into tip/perf/core for next merge window, unless you want it in a hurry?

I suppose I'll queue the below into tip/perf/core for next merge window, unless you want it in a hurry?

Quoted 2 lines · Peter Zijlstra

I suppose I'll queue the below into tip/perf/core for next merge window, unless you want it in a hurry?

I'm thinking we'd still want Eric's series on top of this for the reader benefits of the lock.

Quoted 3 lines · Eric W. Biederman

From a deadlock perspective the change is strictly better than what we have today. The readers will no longer block on each other.

No, agreed, it's better regardless.

Quoted 2 lines

For the specific case that syzbot reported it is readers who were blocking on each other so that specific case if fixed.

So the thing is, a reader can still block another reader if a writer comes in between them. Which is why I was thinking that the same deadlock could happen if somebody does an execve at just the right point.

Quoted 6 lines

On the write side of exec_update_lock we have:

cred_guard_mutex -> exec_update_lock

Which means that to get an ABBA deadlock cred_guard_mutex would need to be involved

No, see above: you can get a deadlock with

- first reader gets exec_update_lock

- writer on exec_update_lock blocks on first reader (this is exec)

- second reader of exec_update_lock now blocks on the writer.

So if that second reader holds something that the first one wants to get (or is the same thread as the first one), you have a deadlock: the first reader will never make any progress, will never release the lock, and the writer will never get it, and the second reader will forever wait for the writer that is ahead of it in the queue.

And that deadlock looks very much like what syzcaller detected, in exactly that scenario:

  Chain exists of:
    &sig->exec_update_mutex --> sb_writers#4 --> &p->lock

   Possible unsafe locking scenario:

         CPU0                    CPU1
         ----                    ----
    lock(&p->lock);
                                 lock(sb_writers#4);
                                 lock(&p->lock);
    lock(&sig->exec_update_mutex);

   *** DEADLOCK ***

No? The only thing that is missing is that writer that causes the exec_update_mutex readers to block each other - exactly like they did when it was a mutex.

But I may be missing something entirely obvious that keeps this from happening.

Hi Eric,

I think I remembered from a previous discussion about this topic, that it was unclear if the rw_semaphores are working the same in RT-Linux. Will this fix work in RT as well?

Quoted 6 lines

kernel/kcmp.c +0 -0

@@ -70,25 +70,25 @@ get_file_raw_ptr(struct task_struct *task, unsigned int idx)
 	return file;
 }
 

kernel/kcmp.c get_file_raw_ptr(struct task_struct *task, unsigned int idx) · line 75

Quoted 7 lines

kernel/kcmp.c +2 -4

-static void kcmp_unlock(struct mutex *m1, struct mutex *m2)
+static void kcmp_unlock(struct rw_semaphore *l1, struct rw_semaphore *l2)
 {
-	if (likely(m2 != m1))
-		mutex_unlock(m2);
-	mutex_unlock(m1);
+	if (likely(l2 != l1))

is this still necessary ?

Quoted 10 lines

kernel/kcmp.c +3 -1

+		up_read(l2);
+	up_read(l1);
 }
 
-static int kcmp_lock(struct mutex *m1, struct mutex *m2)
+static int kcmp_lock(struct rw_semaphore *l1, struct rw_semaphore *l2)
 {
 	int err;
 

kernel/kcmp.c get_file_raw_ptr(struct task_struct *task, unsigned int idx) · line 85

Quoted 4 lines

kernel/kcmp.c +2 -2

-	if (m2 > m1)
-		swap(m1, m2);
+	if (l2 > l1)
+		swap(l1, l2);

and this is probably also no longer necessary?

kernel/kcmp.c get_file_raw_ptr(struct task_struct *task, unsigned int idx) · line 88

Quoted 6 lines

kernel/kcmp.c +2 -3

-	err = mutex_lock_killable(m1);
-	if (!err && likely(m1 != m2)) {
-		err = mutex_lock_killable_nested(m2, SINGLE_DEPTH_NESTING);
+	err = down_read_killable(l1);
+	if (!err && likely(l1 != l2)) {

and this can now be unconditionally, right?

Quoted 19 lines

kernel/kcmp.c +4 -3

+		err = down_read_killable_nested(l2, SINGLE_DEPTH_NESTING);
 		if (err)
-			mutex_unlock(m1);
+			up_read(l1);
 	}
 
 	return err;

@@ -156,8 +156,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
 	/*
 	 * One should have enough rights to inspect task details.
 	 */
-	ret = kcmp_lock(&task1->signal->exec_update_mutex,
-			&task2->signal->exec_update_mutex);
+	ret = kcmp_lock(&task1->signal->exec_update_lock,
+			&task2->signal->exec_update_lock);
 	if (ret)
 		goto err;
 	if (!ptrace_may_access(task1, PTRACE_MODE_READ_REALCREDS) ||

kernel/kcmp.c SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type, · line 219

Quoted 10 lines

kernel/kcmp.c +2 -2

@@ -212,8 +212,8 @@ SYSCALL_DEFINE5(kcmp, pid_t, pid1, pid_t, pid2, int, type,
 	}
 
 err_unlock:
-	kcmp_unlock(&task1->signal->exec_update_mutex,
-		    &task2->signal->exec_update_mutex);
+	kcmp_unlock(&task1->signal->exec_update_lock,
+		    &task2->signal->exec_update_lock);
 err:
 	put_task_struct(task1);
 	put_task_struct(task2);

Thanks
Bernd.

The simplest and most robust solution appears to be making exec_update_mutex a read/write lock and having everything execept for exec take the lock for read.

Quoted 3 lines · Eric W. Biederman

The simplest and most robust solution appears to be making exec_update_mutex a read/write lock and having everything execept for exec take the lock for read.

Looks sane to me.

I'd like the locking people to look at the down_read_*() functions, even if they look simple. Adding Waiman and Davidlohr to the cc just in case they would look at that too, since they've worked on that code.

Quoted 4 lines · Eric W. Biederman

The simplest and most robust solution appears to be making exec_update_mutex a read/write lock and having everything execept for exec take the lock for read.

Looks sane to me.

I'd like the locking people to look at the down_read_*() functions, even if they look simple. Adding Waiman and Davidlohr to the cc just in case they would look at that too, since they've worked on that code.