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

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 28 Jun 2021 16:35:48 +0200
Petr Mladek <pmladek@suse.com> wrote:

> 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;
> }

That's a much more common approach to locking, that may not be as
efficient, but is much easier to keep straight, and less error prone.

-- Steve