On 2021-02-09, Petr Mladek <pmla...@suse.com> wrote:
>> @@ -1629,9 +1631,13 @@ int do_syslog(int type, char __user *buf, int len,
>> int source)
>> /* Number of chars in the log buffer */
>> case SYSLOG_ACTION_SIZE_UNREAD:
>> logbuf_lock_irq();
>> - if (syslog_seq < prb_first_valid_seq(prb)) {
>> - /* messages are gone, move to first one */
>> - syslog_seq = prb_first_valid_seq(prb);
>> + if (prb_read_valid_info(prb, syslog_seq, &info, NULL)) {
>> + if (info.seq != syslog_seq) {
>> + /* messages are gone, move to first one */
>> + syslog_seq = info.seq;
>> + syslog_partial = 0;
>> + }
>> + } else {
>> syslog_partial = 0;
>
> I am scratching my head when prb_read_valid_info(prb,
> syslog_seq, &info, NULL)) might fail.
It can fail because the descriptor has been invalidated/recycled by
writers and perhaps there is no valid record that has yet come after it.
> It might fail when syslog_seq points to the next message
> after the last valid one. In this case, we could return
> immediately (after releasing the lock) because there are
> zero unread messages.
Yes, we could just return 0 in this case. If we are returning and not
modifying @syslog_seq, then there is no need to reset
@syslog_partial. At some point a reader will notice that the record is
gone and reset @syslog_partial accordingly.
> Anyway, syslog_partial must be zero in this case. syslog_seq
> should stay when the last read was partial. And there should
> always be at least one valid message in the log buffer
> be design.
A record can be invalidated at any time. It is a normal case that a
re-read of a record (to get the rest of the partial) can lead to the
record no longer being available.
> IMHO, it would deserve a comment and maybe even a warning.
I don't think we need a warning. It is something that can happen and it
is not a problem.
> What about something like?
>
> /* Number of chars in the log buffer */
> case SYSLOG_ACTION_SIZE_UNREAD:
> logbuf_lock_irq();
> if (!prb_read_valid_info(prb, syslog_seq, &info, NULL)) {
> /* No unread message */
> if (syslog_partial) {
> /* This should never happen. */
> pr_err_once("Unable to read any message even
> when the last syslog read was partial: %zu", syslog_partial);
> syslog_partial = 0;
> }
> logbuf_unlock_irq();
> return 0;
> }
I recommend changing your suggestion to:
> if (!prb_read_valid_info(prb, syslog_seq, &info, NULL)) {
> /*
> * No unread messages. No need to check/reset
> * syslog_partial. When a reader does read a new
> * message it will notice and appropriately update
> * syslog_seq and reset syslog_partial.
> */
> logbuf_unlock_irq();
> return 0;
> }
> if (info.seq != syslog_seq) {
> /* messages are gone, move to first one */
> syslog_seq = info.seq;
> syslog_partial = 0;
> }
John Ogness
