linux-kernel - Re: [PATCH printk v3 5/7] printk: nbcon: Add sequence handling

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]

Message-ID: <ZPl_rvMGCg_u538g@alley>
Date:   Thu, 7 Sep 2023 09:45:50 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v3 5/7] printk: nbcon: Add sequence handling

On Sun 2023-09-03 17:11:37, John Ogness wrote:
> From: Thomas Gleixner <tglx@...utronix.de>
> 
> Add an atomic_long_t field @nbcon_seq to the console struct to
> store the sequence number for nbcon consoles. For nbcon consoles
> this will be used instead of the non-atomic @seq field. The new
> field allows for safe atomic sequence number updates without
> requiring any locking.
> 
> On 64bit systems the new field stores the full sequence number.
> On 32bit systems the new field stores the lower 32 bits of the
> sequence number, which are expanded to 64bit as needed by
> folding the values based on the sequence numbers available in
> the ringbuffer.
> 
> For 32bit systems, having a 32bit representation in the console
> is sufficient. If a console ever gets more than 2^31 records
> behind the ringbuffer then this is the least of the problems.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> +/**
> + * nbcon_seq_try_update - Try to update the console sequence number
> + * @ctxt:	Pointer to an acquire context that contains
> + *		all information about the acquire mode
> + * @new_seq:	The new sequence number to set
> + *
> + * @ctxt->seq is updated to the new value of @con::nbcon_seq. This could
> + * be a different value than @new_seq if nbcon_seq_force() was used or the
> + * current context no longer owns the console. In the later case, it will
> + * stop printing anyway.
> + */
> +__maybe_unused
> +static void nbcon_seq_try_update(struct nbcon_context *ctxt, u64 new_seq)
> +{
> +	unsigned long nbcon_seq = __seq_to_nbcon_seq(ctxt->seq);
> +	struct console *con = ctxt->console;
> +
> +	if (atomic_long_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_seq), &nbcon_seq,
> +				    __seq_to_nbcon_seq(new_seq))) {
> +		ctxt->seq = new_seq;
> +	} else {
> +		ctxt->seq = atomic_long_read(&ACCESS_PRIVATE(con, nbcon_seq));

This should be:

		ctxt->seq = nbcon_seq_read(con);

so that it extends the 32-bit nbcon_seq to the full 64-bit seq on
32-bit systems.

> +	}
> +}
> +
>  /**
>   * nbcon_context_try_acquire_direct - Try to acquire directly
>   * @ctxt:	The context of the caller

Otherwise, it looks good. With the above fix:

Reviewed-by: Petr Mladek <pmladek@...e.com>

Best Regards,
Petr