On Tue 2021-03-30 17:35:11, John Ogness wrote: > @syslog_lock was a raw_spin_lock to simplify the transition of > removing @logbuf_lock and the safe buffers. With that transition > complete, and since all uses of @syslog_lock are within sleepable > contexts, @syslog_lock can become a mutex.
It makes perfect sense. > --- > Note: The removal of read_syslog_seq_irq() is technically a small > step backwards. But the follow-up patch moves forward again > and closes a window that existed with read_syslog_seq_irq() > and @syslog_lock as a spin_lock. This change would deserve a comment in the commit message. Well, I do not think that is a step backward, see below. > kernel/printk/printk.c | 49 +++++++++++++++++------------------------- > 1 file changed, 20 insertions(+), 29 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index f090d6a1b39e..b771aae46445 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1603,21 +1603,9 @@ static int syslog_print_all(char __user *buf, int > size, bool clear) > > static void syslog_clear(void) > { > - raw_spin_lock_irq(&syslog_lock); > + mutex_lock(&syslog_lock); > latched_seq_write(&clear_seq, prb_next_seq(prb)); > - raw_spin_unlock_irq(&syslog_lock); > -} > - > -/* Return a consistent copy of @syslog_seq. */ > -static u64 read_syslog_seq_irq(void) > -{ > - u64 seq; > - > - raw_spin_lock_irq(&syslog_lock); > - seq = syslog_seq; > - raw_spin_unlock_irq(&syslog_lock); > - > - return seq; > + mutex_unlock(&syslog_lock); > } > > int do_syslog(int type, char __user *buf, int len, int source) > @@ -1644,8 +1633,12 @@ int do_syslog(int type, char __user *buf, int len, int > source) > if (!access_ok(buf, len)) > return -EFAULT; > > - error = wait_event_interruptible(log_wait, > - prb_read_valid(prb, read_syslog_seq_irq(), > NULL)); > + /* Get a consistent copy of @syslog_seq. */ > + mutex_lock(&syslog_lock); > + seq = syslog_seq; > + mutex_unlock(&syslog_lock); > + > + error = wait_event_interruptible(log_wait, prb_read_valid(prb, > seq, NULL)); Honestly, I am not sure how the syslog interface should work when there are two readers in the system. They both share the same "syslog_seq". This either fixes a historic bug. The caller of SYSLOG_ACTION_READ might miss the new message when another reader did read it in the meantime. Or it might introduce a regression when two readers would read the same message. Or it does not matter because the behavior is racy by definition. Best Regards, Petr PS: I am going to look at this more with a fresh head after Easter holidays. The answer is important also for the next patch that basically restores the original behavior.