Re: [PATCH printk v3 3/7] printk: nbcon: Add buffer management

[Petr Mladek]

On Sun 2023-09-03 17:11:35, John Ogness wrote:
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> 
> In case of hostile takeovers it must be ensured that the previous
> owner cannot scribble over the output buffer of the emergency/panic
> context. This is achieved by:
> 
>  - Adding a global output buffer instance for the panic context.
>    This is the only situation where hostile takeovers can occur and
>    there is always at most 1 panic context.
> 
>  - Allocating an output buffer per console upon console
>    registration. This buffer is used by the console owner when not
>    in panic context.
> 
>  - Choosing the appropriate buffer is handled in the acquire/release
>    functions.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -503,6 +514,30 @@ static void nbcon_context_release(struct nbcon_context *ctxt)
>  		new.unsafe |= cur.unsafe_takeover;
>  
>  	} while (!nbcon_state_try_cmpxchg(con, &cur, &new));
> +
> +	ctxt->pbufs = NULL;
> +}
> +
> +/**
> + * nbcon_alloc - Allocate buffers needed by the nbcon console
> + * @con:	Console to initialize
> + *
> + * Return:	True on success. False otherwise and the console cannot
> + *		be used.
> + *
> + * This is not part of nbcon_init() because buffer allocation must
> + * be performed earlier in the console registration process.
> + */
> +bool nbcon_alloc(struct console *con)
> +{
> +
> +	con->pbufs = kmalloc(sizeof(*con->pbufs), GFP_KERNEL);

We might need to use memblock_alloc() at least for early consoles.

mm_core_init() is called after processing the kernel parameters.

For example, setup_log_buf() or vfs_caches_init_early() use
memblock_alloc() as well.

> +	if (!con->pbufs) {
> +		con_printk(KERN_ERR, con, "failed to allocate printing buffer\n");
> +		return false;
> +	}
> +
> +	return true;
>  }
>  
>  /**
> @@ -525,4 +563,6 @@ void nbcon_cleanup(struct console *con)
>  	struct nbcon_state state = { };
>  
>  	nbcon_state_set(con, &state);
> +	kfree(con->pbufs);
> +	con->pbufs = NULL;

It would be cleaner to create nbcon_free() as a counter part
for nbcon_alloc().

>  }

Best Regards,
Petr