Re: [PATCH printk v3 6/6] printk: syslog: close window between wait and read

[Petr Mladek]

On Fri 2021-06-25 09:33:54, Steven Rostedt wrote:
> On Thu, 24 Jun 2021 13:17:48 +0206
> John Ogness <john.ogness@xxxxxxxxxxxxx> wrote:
> 
> > +	 * @syslog_lock is held when entering the read loop to prevent
> > +	 * another reader from modifying @syslog_seq.
> 
> You should add to the above comment:
> 
> 	 * And the @syslog_lock is released before exiting the loop.
> 
> Because it's not normal to enter a loop locked, and have it unlocked
> when exiting the loop. And I can envision in the future, someone might
> add a break (for error) while still holding the lock.

I was double checking the code and the locking is really hard to
follow. I would if the following approach make it easier. The main
trick is that the lock is taken at the beginnig and release at
the end. It is only temporary released around a single line
when needed.

static int syslog_print(char __user *buf, int size)
{
	struct printk_info info;
	struct printk_record r;
	char *text;
	int len = 0;
	u64 seq;

	text = kmalloc(CONSOLE_LOG_MAX, GFP_KERNEL);
	if (!text)
		return -ENOMEM;

	prb_rec_init_rd(&r, &info, text, CONSOLE_LOG_MAX);

	mutex_lock(&syslog_lock);

	/*
	 * Wait for the @syslog_seq record to be vailable. @syslog_seq may
	 * change while waiting.
	 */
	do {
		seq = syslog_seq;

		mutex_unlock(&syslog_lock);
		len = wait_event_interruptible(log_wait, prb_read_valid(prb, seq, NULL));
		mutex_lock(&syslog_lock);

		if (len)
			goto out;
	} while (syslog_seq != seq);

	/*
	 * Copy records that fit into the buffer. The above cycle makes sure
	 * that the first record is always available.
	 */
	do {
		size_t n;
		size_t skip;
		unsigned long err;

		if (!prb_read_valid(prb, syslog_seq, &r))
			break;

		if (r.info->seq != syslog_seq) {
			/* message is gone, move to next valid one */
			syslog_seq = r.info->seq;
			syslog_partial = 0;
		}

		/*
		 * To keep reading/counting partial line consistent,
		 * use printk_time value as of the beginning of a line.
		 */
		if (!syslog_partial)
			syslog_time = printk_time;

		skip = syslog_partial;
		n = record_print_text(&r, true, syslog_time);
		if (n - syslog_partial <= size) {
			/* message fits into buffer, move forward */
			syslog_seq = r.info->seq + 1;
			n -= syslog_partial;
			syslog_partial = 0;
		} else if (!len){
			/* partial read(), remember position */
			n = size;
			syslog_partial += n;
		} else
			n = 0;

		if (!n)
			break;

		mutex_unlock(&syslog_lock);
		err = copy_to_user(buf, text + skip, n);
		mutex_lock(&syslog_lock);

		if (err && !len) {
			len = -EFAULT;
			break;
		}

		len += n;
		size -= n;
		buf += n;
	} while(size);
out:
	mutex_unlock(&syslog_lock);
	kfree(text);
	return len;
}

Best Regards,
Petr