[PATCH RT 0/6] Linux 5.4.54-rt33-rc1

[PATCH RT 0/6] Linux 5.4.54-rt33-rc1

[Steven Rostedt]

Dear RT Folks,

This is the RT stable review cycle of patch 5.4.54-rt33-rc1.

Please scream at me if I messed something up. Please test the patches too.

The -rc release will be uploaded to kernel.org and will be deleted when
the final release is out. This is just a review release (or release candidate).

The pre-releases will not be pushed to the git repository, only the
final release is.

If all goes well, this patch will be converted to the next main release
on 8/14/2020.

Enjoy,

-- Steve


To build 5.4.54-rt33-rc1 directly, the following patches should be applied:

  http://www.kernel.org/pub/linux/kernel/v5.x/linux-5.4.tar.xz

  http://www.kernel.org/pub/linux/kernel/v5.x/patch-5.4.54.xz

  http://www.kernel.org/pub/linux/kernel/projects/rt/5.4/patch-5.4.54-rt33-rc1.patch.xz

You can also build from 5.4.54-rt32 by applying the incremental patch:

http://www.kernel.org/pub/linux/kernel/projects/rt/5.4/incr/patch-5.4.54-rt32-rt33-rc1.patch.xz


Changes from 5.4.54-rt32:

---


Ahmed S. Darwish (1):
      net: phy: fixed_phy: Remove unused seqcount

Matt Fleming (1):
      signal: Prevent double-free of user struct

Sebastian Andrzej Siewior (3):
      workqueue: Sync with upstream
      Bluetooth: Acquire sk_lock.slock without disabling interrupts
      rwsem: Provide down_read_non_owner() and up_read_non_owner() for -RT

Steven Rostedt (VMware) (1):
      Linux 5.4.54-rt33-rc1

----
 drivers/net/phy/fixed_phy.c | 28 +++++++++++-----------------
 include/linux/swait.h       | 14 --------------
 kernel/locking/rwsem.c      |  8 ++++----
 kernel/signal.c             |  4 ++--
 kernel/workqueue.c          | 28 +++++++++++++++++++---------
 localversion-rt             |  2 +-
 net/bluetooth/rfcomm/sock.c |  7 ++-----
 7 files changed, 39 insertions(+), 52 deletions(-)

[PATCH RT 1/6] signal: Prevent double-free of user struct

[Steven Rostedt]

5.4.54-rt33-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Matt Fleming <matt@codeblueprint.co.uk>

The way user struct reference counting works changed significantly with,

  fda31c50292a ("signal: avoid double atomic counter increments for user accounting")

Now user structs are only freed once the last pending signal is
dequeued. Make sigqueue_free_current() follow this new convention to
avoid freeing the user struct multiple times and triggering this
warning:

 refcount_t: underflow; use-after-free.
 WARNING: CPU: 0 PID: 6794 at lib/refcount.c:288 refcount_dec_not_one+0x45/0x50
 Call Trace:
  refcount_dec_and_lock_irqsave+0x16/0x60
  free_uid+0x31/0xa0
  __dequeue_signal+0x17c/0x190
  dequeue_signal+0x5a/0x1b0
  do_sigtimedwait+0x208/0x250
  __x64_sys_rt_sigtimedwait+0x6f/0xd0
  do_syscall_64+0x72/0x200
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
Reported-by: Daniel Wagner <wagi@monom.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index def2e8e37f1f..aa924f0141cf 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -494,8 +494,8 @@ static void sigqueue_free_current(struct sigqueue *q)
 
 	up = q->user;
 	if (rt_prio(current->normal_prio) && !put_task_cache(current, q)) {
-		atomic_dec(&up->sigpending);
-		free_uid(up);
+		if (atomic_dec_and_test(&up->sigpending))
+			free_uid(up);
 	} else
 		  __sigqueue_free(q);
 }
-- 
2.28.0

[PATCH RT 2/6] workqueue: Sync with upstream

[Steven Rostedt]

5.4.54-rt33-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

This is an all-on-one patch reverting the following commits:
  workqueue: Don't assume that the callback has interrupts disabled
  sched/swait: Add swait_event_lock_irq()
  workqueue: Use swait for wq_manager_wait
  workqueue: Convert the locks to raw type

and introducing the following commits from upstream:
  workqueue: Use rcuwait for wq_manager_wait
  workqueue: Convert the pool::lock and wq_mayday_lock to raw_spinlock_t

as an replacement.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/swait.h | 14 --------------
 kernel/workqueue.c    | 28 +++++++++++++++++++---------
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/include/linux/swait.h b/include/linux/swait.h
index 21ae66cd41d3..f426a0661aa0 100644
--- a/include/linux/swait.h
+++ b/include/linux/swait.h
@@ -299,18 +299,4 @@ do {									\
 	__ret;								\
 })
 
-#define __swait_event_lock_irq(wq, condition, lock, cmd)		\
-	___swait_event(wq, condition, TASK_UNINTERRUPTIBLE, 0,		\
-		       raw_spin_unlock_irq(&lock);			\
-		       cmd;						\
-		       schedule();					\
-		       raw_spin_lock_irq(&lock))
-
-#define swait_event_lock_irq(wq_head, condition, lock)			\
-	do {								\
-		if (condition)						\
-			break;						\
-		__swait_event_lock_irq(wq_head, condition, lock, );	\
-	} while (0)
-
 #endif /* _LINUX_SWAIT_H */
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 26341e8101ee..7c968aea01e1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -50,7 +50,6 @@
 #include <linux/uaccess.h>
 #include <linux/sched/isolation.h>
 #include <linux/nmi.h>
-#include <linux/swait.h>
 
 #include "workqueue_internal.h"
 
@@ -302,7 +301,8 @@ static struct workqueue_attrs *wq_update_unbound_numa_attrs_buf;
 static DEFINE_MUTEX(wq_pool_mutex);	/* protects pools and workqueues list */
 static DEFINE_MUTEX(wq_pool_attach_mutex); /* protects worker attach/detach */
 static DEFINE_RAW_SPINLOCK(wq_mayday_lock);	/* protects wq->maydays list */
-static DECLARE_SWAIT_QUEUE_HEAD(wq_manager_wait); /* wait for manager to go away */
+/* wait for manager to go away */
+static struct rcuwait manager_wait = __RCUWAIT_INITIALIZER(manager_wait);
 
 static LIST_HEAD(workqueues);		/* PR: list of all workqueues */
 static bool workqueue_freezing;		/* PL: have wqs started freezing? */
@@ -1615,11 +1615,9 @@ EXPORT_SYMBOL_GPL(queue_work_node);
 void delayed_work_timer_fn(struct timer_list *t)
 {
 	struct delayed_work *dwork = from_timer(dwork, t, timer);
-	unsigned long flags;
 
-	local_irq_save(flags);
+	/* should have been called from irqsafe timer with irq already off */
 	__queue_work(dwork->cpu, dwork->wq, &dwork->work);
-	local_irq_restore(flags);
 }
 EXPORT_SYMBOL(delayed_work_timer_fn);
 
@@ -2147,7 +2145,7 @@ static bool manage_workers(struct worker *worker)
 
 	pool->manager = NULL;
 	pool->flags &= ~POOL_MANAGER_ACTIVE;
-	swake_up_one(&wq_manager_wait);
+	rcuwait_wake_up(&manager_wait);
 	return true;
 }
 
@@ -3511,6 +3509,18 @@ static void rcu_free_pool(struct rcu_head *rcu)
 	kfree(pool);
 }
 
+/* This returns with the lock held on success (pool manager is inactive). */
+static bool wq_manager_inactive(struct worker_pool *pool)
+{
+	raw_spin_lock_irq(&pool->lock);
+
+	if (pool->flags & POOL_MANAGER_ACTIVE) {
+		raw_spin_unlock_irq(&pool->lock);
+		return false;
+	}
+	return true;
+}
+
 /**
  * put_unbound_pool - put a worker_pool
  * @pool: worker_pool to put
@@ -3546,10 +3556,10 @@ static void put_unbound_pool(struct worker_pool *pool)
 	 * Become the manager and destroy all workers.  This prevents
 	 * @pool's workers from blocking on attach_mutex.  We're the last
 	 * manager and @pool gets freed with the flag set.
+	 * Because of how wq_manager_inactive() works, we will hold the
+	 * spinlock after a successful wait.
 	 */
-	raw_spin_lock_irq(&pool->lock);
-	swait_event_lock_irq(wq_manager_wait,
-			    !(pool->flags & POOL_MANAGER_ACTIVE), pool->lock);
+	rcuwait_wait_event(&manager_wait, wq_manager_inactive(pool));
 	pool->flags |= POOL_MANAGER_ACTIVE;
 
 	while ((worker = first_idle_worker(pool)))
-- 
2.28.0

[PATCH RT 3/6] Bluetooth: Acquire sk_lock.slock without disabling interrupts

[Steven Rostedt]

5.4.54-rt33-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

[ Upstream commit e6da0edc24eecef2f6964d92fa9044e1821deace ]

There was a lockdep which led to commit
   fad003b6c8e3d ("Bluetooth: Fix inconsistent lock state with RFCOMM")

Lockdep noticed that `sk->sk_lock.slock' was acquired without disabling
the softirq while the lock was also used in softirq context.
Unfortunately the solution back then was to disable interrupts before
acquiring the lock which however made lockdep happy.
It would have been enough to simply disable the softirq. Disabling
interrupts before acquiring a spinlock_t is not allowed on PREEMPT_RT
because these locks are converted to 'sleeping' spinlocks.

Use spin_lock_bh() in order to acquire the `sk_lock.slock'.

Cc: stable-rt@vger.kernel.org
Reported-by: Luis Claudio R. Goncalves <lclaudio@uudg.org>
Reported-by: kbuild test robot <lkp@intel.com> [missing unlock]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 net/bluetooth/rfcomm/sock.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 90bb53aa4bee..2c27aa2acf1c 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -64,15 +64,13 @@ static void rfcomm_sk_data_ready(struct rfcomm_dlc *d, struct sk_buff *skb)
 static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
 {
 	struct sock *sk = d->owner, *parent;
-	unsigned long flags;
 
 	if (!sk)
 		return;
 
 	BT_DBG("dlc %p state %ld err %d", d, d->state, err);
 
-	local_irq_save(flags);
-	bh_lock_sock(sk);
+	spin_lock_bh(&sk->sk_lock.slock);
 
 	if (err)
 		sk->sk_err = err;
@@ -93,8 +91,7 @@ static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
 		sk->sk_state_change(sk);
 	}
 
-	bh_unlock_sock(sk);
-	local_irq_restore(flags);
+	spin_unlock_bh(&sk->sk_lock.slock);
 
 	if (parent && sock_flag(sk, SOCK_ZAPPED)) {
 		/* We have to drop DLC lock here, otherwise
-- 
2.28.0

[PATCH RT 4/6] net: phy: fixed_phy: Remove unused seqcount

[Steven Rostedt]

5.4.54-rt33-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: "Ahmed S. Darwish" <a.darwish@linutronix.de>

Commit bf7afb29d545 ("phy: improve safety of fixed-phy MII register
reading") protected the fixed PHY status with a sequence counter.

Two years later, commit d2b977939b18 ("net: phy: fixed-phy: remove
fixed_phy_update_state()") removed the sequence counter's write side
critical section -- neutralizing its read side retry loop.

Remove the unused seqcount.

Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: David S. Miller <davem@davemloft.net>
(cherry picked from v5.8-rc1 commit 79cbb6bc3332da7162c2581e151659ab8ebaa528)
Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 drivers/net/phy/fixed_phy.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 4190f9ed5313..9ed715e9be40 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -19,7 +19,6 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/gpio/consumer.h>
-#include <linux/seqlock.h>
 #include <linux/idr.h>
 #include <linux/netdevice.h>
 #include <linux/linkmode.h>
@@ -34,7 +33,6 @@ struct fixed_mdio_bus {
 struct fixed_phy {
 	int addr;
 	struct phy_device *phydev;
-	seqcount_t seqcount;
 	struct fixed_phy_status status;
 	bool no_carrier;
 	int (*link_update)(struct net_device *, struct fixed_phy_status *);
@@ -80,19 +78,17 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num)
 	list_for_each_entry(fp, &fmb->phys, node) {
 		if (fp->addr == phy_addr) {
 			struct fixed_phy_status state;
-			int s;
-
-			do {
-				s = read_seqcount_begin(&fp->seqcount);
-				fp->status.link = !fp->no_carrier;
-				/* Issue callback if user registered it. */
-				if (fp->link_update)
-					fp->link_update(fp->phydev->attached_dev,
-							&fp->status);
-				/* Check the GPIO for change in status */
-				fixed_phy_update(fp);
-				state = fp->status;
-			} while (read_seqcount_retry(&fp->seqcount, s));
+
+			fp->status.link = !fp->no_carrier;
+
+			/* Issue callback if user registered it. */
+			if (fp->link_update)
+				fp->link_update(fp->phydev->attached_dev,
+						&fp->status);
+
+			/* Check the GPIO for change in status */
+			fixed_phy_update(fp);
+			state = fp->status;
 
 			return swphy_read_reg(reg_num, &state);
 		}
@@ -150,8 +146,6 @@ static int fixed_phy_add_gpiod(unsigned int irq, int phy_addr,
 	if (!fp)
 		return -ENOMEM;
 
-	seqcount_init(&fp->seqcount);
-
 	if (irq != PHY_POLL)
 		fmb->mii_bus->irq[phy_addr] = irq;
 
-- 
2.28.0

[PATCH RT 5/6] rwsem: Provide down_read_non_owner() and up_read_non_owner() for -RT

[Steven Rostedt]

5.4.54-rt33-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

The rwsem implementation on -RT allows multiple reader and there is no
owner tracking anymore.
We can provide down_read_non_owner() and up_read_non_owner() by skipping
the owner check bits which are only available in the !RT implementation.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/locking/rwsem.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 0d11ba11a32a..0325c721f9b3 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -1616,15 +1616,15 @@ void _down_write_nest_lock(struct rw_semaphore *sem, struct lockdep_map *nest)
 }
 EXPORT_SYMBOL(_down_write_nest_lock);
 
-#ifndef CONFIG_PREEMPT_RT
 void down_read_non_owner(struct rw_semaphore *sem)
 {
 	might_sleep();
 	__down_read(sem);
+#ifndef CONFIG_PREEMPT_RT
 	__rwsem_set_reader_owned(sem, NULL);
+#endif
 }
 EXPORT_SYMBOL(down_read_non_owner);
-#endif
 
 void down_write_nested(struct rw_semaphore *sem, int subclass)
 {
@@ -1649,13 +1649,13 @@ int __sched down_write_killable_nested(struct rw_semaphore *sem, int subclass)
 }
 EXPORT_SYMBOL(down_write_killable_nested);
 
-#ifndef CONFIG_PREEMPT_RT
 void up_read_non_owner(struct rw_semaphore *sem)
 {
+#ifndef CONFIG_PREEMPT_RT
 	DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
+#endif
 	__up_read(sem);
 }
 EXPORT_SYMBOL(up_read_non_owner);
-#endif
 
 #endif
-- 
2.28.0

[PATCH RT 6/6] Linux 5.4.54-rt33-rc1

[Steven Rostedt]

5.4.54-rt33-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

---
 localversion-rt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/localversion-rt b/localversion-rt
index ce6a482618d5..c8c4d2166008 100644
--- a/localversion-rt
+++ b/localversion-rt
@@ -1 +1 @@
--rt32
+-rt33-rc1
-- 
2.28.0

Re: [PATCH RT 1/6] signal: Prevent double-free of user struct

[Rasmus Villemoes]

On 13/08/2020 03.45, Steven Rostedt wrote:
> 5.4.54-rt33-rc1 stable review patch.
> If anyone has any objections, please let me know.
>

No objections, quite the contrary. I think this should also be applied
to 4.19-rt:

Commit fda31c50292a is also in 4.19.y (as 797479da0ae9), since 4.19.112
and hence also 4.19.112-rt47. For a while we've tried to track down a
hang that at least sometimes manifests quite similarly

refcount_t: underflow; use-after-free.
WARNING: CPU: 0 PID: 14 at lib/refcount.c:280 refcount_dec_not_one+0xc0/0xd8
...
Call Trace:
[cf45be10] [c0238258] refcount_dec_not_one+0xc0/0xd8 (unreliable)
[cf45be20] [c02383c8] refcount_dec_and_lock_irqsave+0x20/0xa4
[cf45be40] [c0024a70] free_uid+0x2c/0xa0
[cf45be60] [c00384f0] put_cred_rcu+0x58/0x8c
[cf45be70] [c005f048] rcu_cpu_kthread+0x364/0x49c
[cf45bee0] [c003a0d0] smpboot_thread_fn+0x21c/0x29c
[cf45bf10] [c0036464] kthread+0xe0/0x10c
[cf45bf40] [c000f1cc] ret_from_kernel_thread+0x14/0x1c

But our reproducer is rather complicated and involves cutting power to
neighbouring boards, and takes many minutes to trigger. So I tried
Daniel's reproducer

  sigwaittest -t -a -p 98

and almost immediately got a trace much more similar to the one in the
commit message

refcount_t: underflow; use-after-free.
WARNING: CPU: 0 PID: 1526 at lib/refcount.c:280
refcount_dec_not_one+0xc0/0xd8
...
Call Trace:
[cebc9e00] [c0238258] refcount_dec_not_one+0xc0/0xd8 (unreliable)
[cebc9e10] [c02383c8] refcount_dec_and_lock_irqsave+0x20/0xa4
[cebc9e30] [c0024a70] free_uid+0x2c/0xa0
[cebc9e50] [c002574c] dequeue_signal+0x90/0x1a4
[cebc9e80] [c0028f74] sys_rt_sigtimedwait+0x24c/0x288
[cebc9f40] [c000f12c] ret_from_syscall+0x0/0x40

With this patch applied, the sigwaittest has now run for 10 minutes
without problems.

I'll have to run some more tests with our reproducer to see if it really
is the same issue, but even if not, the fact that the sigwaittest fails
should be enough to put this in 4.19-rt.

Three requests (in order of importance):

* pull this into 4.19-rt
* add a note about the sigwaittest reproducer to the commit log
* do publish the rt-rcs in some git repository; that makes it a lot
easier to cherry-pick and test patches

Thanks,
Rasmus

Re: [PATCH RT 1/6] signal: Prevent double-free of user struct

[Steven Rostedt]

On Thu, 13 Aug 2020 10:25:45 +0200
Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:

> On 13/08/2020 03.45, Steven Rostedt wrote:
> > 5.4.54-rt33-rc1 stable review patch.
> > If anyone has any objections, please let me know.
> >  
> 
> No objections, quite the contrary. I think this should also be applied
> to 4.19-rt:

Yep. We have a rule that no earlier supported stable release should
have a fix that a more recent stable release does not have. So this
needs to be accepted in 5.4-rt before 4.19-rt can have it. And those
maintainers have been waiting patiently for me to push this ;-)


> Three requests (in order of importance):
> 
> * pull this into 4.19-rt
> * add a note about the sigwaittest reproducer to the commit log

We don't usually add comments to the commit log for backported patches.

> * do publish the rt-rcs in some git repository; that makes it a lot
> easier to cherry-pick and test patches

This has been talked about before. Perhaps you should bring up posting
stable RT RC releases into git repositories at the RT microconference
at Plumbers ;-) Especially since the last time I asked about it, people
said it wasn't necessary.

https://linuxplumbersconf.org/event/7/page/80-accepted-microconferences#rt-cr

-- Steve