Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk v3 6/6] printk: syslog: close window between wait and read | Date | Fri, 25 Jun 2021 10:17:40 +0206 |
| |
On 2021-06-24, Petr Mladek <pmladek@suse.com> wrote: >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c >> index 90954cb5a0ab..4737804d6c6d 100644 >> --- a/kernel/printk/printk.c >> +++ b/kernel/printk/printk.c >> @@ -1542,8 +1570,13 @@ static int syslog_print(char __user *buf, int size) >> len += n; >> size -= n; >> buf += n; >> - } >> >> + if (!size) >> + break; > > This looks like an unrelated optimization. If I get it correctly, it > does not change the existing behavior.
It was a necessary change in order to preserve the existing logic but allow the lock to be held when enterring the loop. Before the patch we have:
...get seq to read...
while (size > 0) { mutex_lock(&syslog_lock); ...read record... mutex_unlock(&syslog_lock); ...copy record... }
After the patch we enter the loop with the lock already held. So this changes the code to:
mutex_lock(&syslog_lock); ...get seq to read...
for (;;) { ...read record... mutex_unlock(&syslog_lock); ...copy record... if (!size) break; mutex_lock(&syslog_lock); }
Note that @size always starts with >0, so there is no need to check it at the beginning of the loop. And checking for !0 instead of >0 is also ok, since @size will never be less than zero.
> The next cycle would end up with n == 0 and break anyway.
Doing an extra loop of reading more data and sprinting it into the temporary buffer even though we know the user buffer is not desirable.
If you insisted on keeping the "while (size > 0)" loop, then there would be an unnecessary lock/unlock call and the code gets even more complex.
I could add some comments to the implementation if you prefer.
> The patch itself makes sense. It somehow fixes a long standing race. > Even though the result still might be racy. The lock is released > when each record is copied to the user-provided buffer.
I do not understand this conclusion. The existing race is real. SYSLOG_ACTION_READ could return with no data, not because there is no records available, but because the race was hit. With this patch that race is closed: SYSLOG_ACTION_READ will either return with data or with an error.
You claim the result is still racy, but I do not know what you are referring to. If you have multiple readers, they will get different records (and record pieces), but collectively no data would be lost and no data would be redundant. And no readers would return from SYSLOG_ACTION_READ without data.
> I would feel more comfortable if we handled the optimization one of > the suggested way.
There is no optimization here. Perhaps you have missed that the loop changes from "while (size > 0)" to "for (;;)".
John Ogness
| |