On 2021-02-01, Petr Mladek <pmla...@suse.com> wrote:
>> The global variables @syslog_seq, @syslog_partial, @syslog_time
>> and write access to @clear_seq are protected by @logbuf_lock.
>> Once @logbuf_lock is removed, these variables will need their
>> own synchronization method. Introduce @syslog_lock for this
>> purpose.
>
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -390,8 +390,12 @@ DEFINE_RAW_SPINLOCK(logbuf_lock);
>> printk_safe_exit_irqrestore(flags); \
>> } while (0)
>>
>> +/* syslog_lock protects syslog_* variables and write access to clear_seq. */
>> +static DEFINE_RAW_SPINLOCK(syslog_lock);
>
> I am not expert on RT code but I think that it prefers the generic
> spinlocks. syslog_lock seems to be used in a normal context.
> IMHO, it does not need to be a raw spinlock.
>
> Note that using normal spinlock would require switching the locking
> order. logbuf_lock is a raw lock. Normal spinlock must not be taken
> under a raw spinlock.
>
> Or we could switch syslog_lock to the normal spinlock later
> after logbuf_lock is removed.
I was planning on this last option because I think it is the
simplest. There are places such as syslog_print_all() where the
printk_safe_enter() and logbuf_lock locking are not at the same place as
the syslog_lock locking (and syslog_lock is inside).
Once the safe buffers are removed, syslog_lock can transition to a
spinlock. (spinlock's must not be under local_irq_save().)
>> +
>> #ifdef CONFIG_PRINTK
>> DECLARE_WAIT_QUEUE_HEAD(log_wait);
>> +/* All 3 protected by @syslog_lock. */
>> /* the next printk record to read by syslog(READ) or /proc/kmsg */
>> static u64 syslog_seq;
>> static size_t syslog_partial;
>> @@ -1631,6 +1643,7 @@ int do_syslog(int type, char __user *buf, int len, int
>> source)
>> bool clear = false;
>> static int saved_console_loglevel = LOGLEVEL_DEFAULT;
>> int error;
>> + u64 seq;
>
> This allows to remove definition of the same temporary variable
> for case SYSLOG_ACTION_SIZE_UNREAD.
Right. I missed that.
>>
>> error = check_syslog_permissions(type, source);
>> if (error)
>> @@ -1648,8 +1661,14 @@ int do_syslog(int type, char __user *buf, int len,
>> int source)
>> return 0;
>> if (!access_ok(buf, len))
>> return -EFAULT;
>> +
>> + /* Get a consistent copy of @syslog_seq. */
>> + raw_spin_lock_irq(&syslog_lock);
>> + seq = syslog_seq;
>> + raw_spin_unlock_irq(&syslog_lock);
>> +
>> error = wait_event_interruptible(log_wait,
>> - prb_read_valid(prb, syslog_seq, NULL));
>> + prb_read_valid(prb, seq, NULL));
>
> Hmm, this will not detect when syslog_seq gets cleared in parallel.
> I hope that nobody rely on this behavior. But who knows?
>
> A solution might be to have also syslog_seq latched. But I am
> not sure if it is worth it.
>
> I am for taking the risk and use the patch as it is now. Let's keep
> the code for now. We could always use the latched variable when
> anyone complains. Just keep it in mind.
We could add a simple helper:
/* Get a consistent copy of @syslog_seq. */
static u64 syslog_seq_read(void)
{
unsigned long flags;
raw_spin_lock_irqsave(&syslog_lock, flags);
seq = syslog_seq;
raw_spin_unlock_irqrestore(&syslog_lock, flags);
return seq;
}
Then change the code to:
error = wait_event_interruptible(log_wait,
prb_read_valid(prb, read_syslog_seq(), NULL));
register_console() could also make use of the function. (That is why I
am suggesting the flags variant.)
John Ogness
