linux-kernel - synchronization model: was: Re: [PATCH printk-rework 09/14] printk: introduce a kmsg

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 19 Feb 2021 18:57:08 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: synchronization model: was: Re: [PATCH printk-rework 09/14] printk:
 introduce a kmsg_dump iterator

On Thu 2021-02-18 09:18:12, John Ogness wrote:
> Rather than store the iterator information into the registered
> kmsg_dump structure, create a separate iterator structure. The
> kmsg_dump_iter structure can reside on the stack of the caller,
> thus allowing lockless use of the kmsg_dump functions.
> 
> This is in preparation for removal of @logbuf_lock.
> 
> diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
> index 4095a34db0fa..2fdb10ab1799 100644
> --- a/include/linux/kmsg_dump.h
> +++ b/include/linux/kmsg_dump.h
> @@ -29,6 +29,18 @@ enum kmsg_dump_reason {
>  	KMSG_DUMP_MAX
>  };
>  
> +/**
> + * struct kmsg_dumper_iter - iterator for kernel crash message dumper
> + * @active:	Flag that specifies if this is currently dumping
> + * @cur_seq:	Points to the oldest message to dump (private)
> + * @next_seq:	Points after the newest message to dump (private)
> + */
> +struct kmsg_dumper_iter {
> +	bool	active;
> +	u64	cur_seq;
> +	u64	next_seq;
> +};
> +

This is likely beyond the scope of this patchset.

I am still scratching my head about the synchronization if these dumpers.

There is the "active" flag. It has been introduced by the commit
e2ae715d66bf4becfb ("kmsg - kmsg_dump() use iterator to receive log
buffer content"). I do not see any explanation there.

It might prevent some misuse of the API. But the synchronization
model is not much clear:

	+ cur_seq and next_seq might be manipulated by
	  kmsg_dump_rewind() even when the flag is not set.

	+ It is possible to use the same dumper more times in parallel.
	  The API will fill the provided buffer of all callers
	  as long as the active flag is set.

	+ The "active" flag does not synchronize other operations with
	  the provided buffer. The "dump" callback is responsible
	  to provide some synchronization on its own.

In fact, it is not much clear how struct kmsg_dumper_iter, struct kmsg_dumper,
and the used buffers are connected with each other and synchronized.

It might some sense to have the iterator in a separate structure.
But the only safe scenario seems to be when all these three things
(both structures and the buffer) are connected together and
synchronized by the same lock. Also the "active" flag does not look
much helpful and can be removed.

As I said, this is likely beyond this patchset. This patch does more
or less just a refactoring and helps to understand the dependencies.

It is possible that it will be more clear the following week
with a fresh mind.

Best Regards,
Petr