On Wed 2021-02-10 19:32:10, John Ogness wrote:
> On 2021-02-09, Petr Mladek <[email protected]> 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.
I see. From some reasons I though that there should always be at
least one message in the commited state. But it is enough when
it is in reusable state. I should have double checked it.
> 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.
> > */
The following comment might be enough after all.
/* No unread messages. */
My main concern was that we cleared syslog_partial and continued.
I thought that we might miss a bug this way. But it seems to
be perfectly fine. I just have to update my mental picture.
Otherwise. the fact that syslog_partial will be fixed by the next
successful call is more or less obvious if we change the code as you
propose.
Please, send an updated patch.
Best Regards,
Petr
> > 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
