Skip to contentSkip to conversation

kmsg: properly handle concurrent non-blocking read() from /proc/kmsg

PATCH In review Kay Sievers 2 participants 2 messages Jul 3, 2012 – Jul 4, 2012

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 kmsg: properly handle concurrent non-blocking read() from /proc/kmsg

From: Kay Sievers <[email protected]>
Subject: kmsg: properly handle concurrent non-blocking read() from /proc/kmsg

The /proc/kmsg read() interface is internally simply wired up to a sequence of syslog() syscalls, which might are racy between their checks and actions, regarding concurrency.

In the (very uncommon) case of concurrent readers of /dev/kmsg, relying on usual O_NONBLOCK behavior, the recently introduced mutex might block an O_NONBLOCK reader in read(), when poll() returns for it, but another process has already read the data in the meantime. We've seen that while running artificial test setups and tools that "fight" about /proc/kmsg data.

This restores the original /proc/kmsg behavior, where in case of concurrent read()s, poll() might wake up but the read() syscall will just return 0 to the caller, while another process has "stolen" the data.

This is in the general case not the expected behavior, but it is the exact same one, that can easily be triggered with a 3.4 kernel, and some tools might just rely on it.

The mutex is not needed, the original integrity issue which introduced it, is in the meantime covered by:
"fill buffer with more than a single message for SYSLOG_ACTION_READ" 116e90b23f74d303e8d607c7a7d54f60f14ab9f2

Cc Yuanhan Liu <[email protected]>
Cc Jan Beulich <[email protected]>
---
 kernel/printk.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)
kernel/printk.c +1 -8
@@ -1019,7 +1019,6 @@ int do_syslog(int type, char __user *buf
 {
 	bool clear = false;
 	static int saved_console_loglevel = -1;
-	static DEFINE_MUTEX(syslog_mutex);
 	int error;
 
 	error = check_syslog_permissions(type, from_file);
@@ -1046,17 +1045,11 @@ int do_syslog(int type, char __user *buf
 			error = -EFAULT;
 			goto out;
 		}
-		error = mutex_lock_interruptible(&syslog_mutex);
-		if (error)
-			goto out;
 		error = wait_event_interruptible(log_wait,
 						 syslog_seq != log_next_seq);
-		if (error) {
-			mutex_unlock(&syslog_mutex);
+		if (error)
 			goto out;
-		}
 		error = syslog_print(buf, len);
-		mutex_unlock(&syslog_mutex);
 		break;
 	/* Read/clear last kernel messages */
 	case SYSLOG_ACTION_READ_CLEAR:
Acked-by on v1 1/1
#1 lore raw
Quoted 22 lines
Quoted 1 line
Quoted 1 line

Kay Sievers <[email protected]> 07/03/12 8:17 PM >>>

From: Kay Sievers <[email protected]>
Subject: kmsg: properly handle concurrent non-blocking read() from /proc/kmsg

The /proc/kmsg read() interface is internally simply wired up to a sequence of syslog() syscalls, which might are racy between their checks and actions, regarding concurrency.

In the (very uncommon) case of concurrent readers of /dev/kmsg, relying on usual O_NONBLOCK behavior, the recently introduced mutex might block an O_NONBLOCK reader in read(), when poll() returns for it, but another process has already read the data in the meantime. We've seen that while running artificial test setups and tools that "fight" about /proc/kmsg data.

This restores the original /proc/kmsg behavior, where in case of concurrent read()s, poll() might wake up but the read() syscall will just return 0 to the caller, while another process has "stolen" the data.

This is in the general case not the expected behavior, but it is the exact same one, that can easily be triggered with a 3.4 kernel, and some tools might just rely on it.

Quoted 8 lines

The mutex is not needed, the original integrity issue which introduced it, is in the meantime covered by:
"fill buffer with more than a single message for SYSLOG_ACTION_READ" 116e90b23f74d303e8d607c7a7d54f60f14ab9f2

Cc Yuanhan Liu <[email protected]>
Cc Jan Beulich <[email protected]>

This being a revert of all but a single change of 4a77a5a06ec66ed05199b301e7c25f42f979afdc, which I had suggested to be reverted anyway: