linux-kernel - Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 4 Mar 2020 11:09:36 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrea Parri <parri.andrea@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        kexec@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: misc nits Re: [PATCH 1/2] printk: add lockless buffer

On Tue 2020-03-03 16:42:07, John Ogness wrote:
> On 2020-03-03, Petr Mladek <pmladek@...e.com> wrote:
> >>>>>> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..796257f226ee
> >>>>>> --- /dev/null
> >>>>>> +++ b/kernel/printk/printk_ringbuffer.c
> >>>>>> +/*
> >>>>>> + * Read the record @id and verify that it is committed and has the sequence
> >>>>>> + * number @seq. On success, 0 is returned.
> >>>>>> + *
> >>>>>> + * Error return values:
> >>>>>> + * -EINVAL: A committed record @seq does not exist.
> >>>>>> + * -ENOENT: The record @seq exists, but its data is not available. This is a
> >>>>>> + *          valid record, so readers should continue with the next seq.
> >>>>>> + */
> >>>>>> +static int desc_read_committed(struct prb_desc_ring *desc_ring,
> >>>>>> +			       unsigned long id, u64 seq,
> >>>>>> +			       struct prb_desc *desc)
> >>>>>> +{
> >>>
> @id _is_ very important because that is how descriptors are
> read. desc_read() takes @id as an argument and it is @id that identifies
> the descriptor. @seq is only meta-data within a descriptor. The only
> reason @seq is even checked is because of possible ABA issues with @id
> on 32-bit systems.

I think that the different view is because I look at this API
from the reader API side. It is called the following way:

prb_read_valid(, seq, )
  _prb_read_valid( , &seq, )
    prb_read( , *seq, )
        # id is read from address defined by seq
	rdesc = dr->descs[seq & MASK];
	id = rdesc->state_var && MASK_ID;

        desc_read_commited( , id, seq, )
	  desc_read( , id, )
	    # desc is the same as rdesc above because
	    # seq & MASK == id & MASK
	    desc = dr->descs[id & MASK];

Note that prb_read_valid() and prb_read() are addressed by seq.

It would be perfectly fine to pass only seq to desc_read_committed()
and read id from inside.

The name desc_read_committed() suggests that the important condition
is that the descriptor is in the committed state. It is not obvious
that seq is important as well.

>From my POV, it will be more clear to pass only seq and rename the
function to desc_read_by_seq() or so:

  + seq is enough for addressing
  + function returns true only when the stored seq matches
  + the stored seq is valid only when the state is committed
    or reusable


Please, do not reply to this mail. Either take the idea or keep
the code as is. I could live with it. And it is not important
enough to spend more time on it. I just wanted to explain my view.
But it is obviously just a personal preference.

Best Regards,
Petr