Re: [PATCH printk v2 08/26] printk: nbcon: Implement processing in port->lock wrapper

[Petr Mladek <pmladek@suse.com>]

On Fri 2024-03-15 16:10:18, John Ogness wrote:
> On 2024-03-14, Petr Mladek <pmladek@suse.com> wrote:
> > Well, it brings another question. Does this allow to have
> > the following situation?
> >
> > CPU0				CPU1
> >
> >   some_function()
> >     uart_port_lock()
> >       // locked just with up->lock
> >       // doing something with the port
> >
> > 				register_console()
> > 				  // add struct console using the same
> > 				  // port as CPU0
> > 				  printk()
> > 				    console_try_lock()
> > 				    console_unlock()
> > 				      console_flush_all()
> > 					// acquire context for the newly
> > 					// registered nbcon
> > 					nbcon_context_try_acquire(ctxt)
> > 					  con->write()
> >
> > BANG: Both CPU0 and CPU1 are writing to the same port.
> >
> > Reason: CPU0 locked only via port->lock.
> > 	CPU1 locked only by acquiring nbcon context.
> 
> Great catch! Yes, this is possible. :-/
> 
> When the kthread series part is introduced, there will be additional
> callbacks that nbcon consoles must implement
> (driver_enter()/driver_exit()). These provide driver-level
> synchronization. In the case of serial uarts, the callbacks map to
> locking/unlocking the port lock.
> 
> If I were to introduce those callbacks in _this_ series, they can be
> used when adding a console to the list in register_console(). This
> changes your example to:
> 
> CPU0				CPU1
> 
>   some_function()
>     uart_port_lock()
>       // locked just with up->lock
>       // doing something with the port
> 
> 				register_console()
> 				  // add struct console using the same
> 				  // port as CPU0
> 				  newcon->driver_enter()
> 				    spin_lock(port_lock)
> 				    // spin on CPU0
>     uart_port_unlock()
> 				  // add new console to console list
> 				  newcon->driver_exit()
> 				    spin_unlock(port_lock)
> 				  ...
> 
> If any other CPUs come in and call uart_port_lock(), they will see the
> console as registered and will acquire the nbcon to avoid the BANG.

Looks good. See below.

> > Maybe, this is not possible because the console is registered when
> > the struct uart_port is being initialized and nobody could
> > use the same port in parallel, except for the early console.
> > Where the early console is serialized using the console_lock().
> 
> Yes, it is possible. Just check out:
> 
>     find /sys/ -name console -type f
> 
> If you echo 'Y' or 'N' into any of those files, you can dynamically
> register and unregister those consoles, respectively.
> 
> I just ran some tests to verify this and was even able to trigger a
> mainline bug because probe_baud() of the 8250 driver is not called under
> the port lock. This is essentially the same scenario you
> illustrated. But the 8250 probe_baud() issue is a driver bug and not
> related to this series.

Thanks a lot for checking it.

> Getting back to this series, my proposal would change register_console()
> like this:
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 68657d4d6649..25a0a81e8397 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3733,6 +3733,7 @@ void register_console(struct console *newcon)
>  	struct console *con;
>  	bool bootcon_registered = false;
>  	bool realcon_registered = false;
> +	unsigned long flags;
>  	int err;
>  
>  	console_list_lock();
> @@ -3831,6 +3832,19 @@ void register_console(struct console *newcon)
>  	if (newcon->flags & CON_BOOT)
>  		have_boot_console = true;
>  
> +	/*
> +	 * If another context is actively using the hardware of this new
> +	 * console, it will not be aware of the nbcon synchronization. This
> +	 * is a risk that two contexts could access the hardware
> +	 * simultaneously if this new console is used for atomic printing
> +	 * and the other context is still using the hardware.
> +	 * 
> +	 * Use the driver synchronization to ensure that the hardware is not
> +	 * in use while this new console transitions to being registered.
> +	 */
> +	if ((newcon->flags & CON_NBCON) && newcon->write_atomic)
> +		newcon->driver_enter(newcon, &flags);
> +
>  	/*
>  	 * Put this console in the list - keep the
>  	 * preferred driver at the head of the list.
> @@ -3855,6 +3869,10 @@ void register_console(struct console *newcon)
>  	 * register_console() completes.
>  	 */
>  
> +	/* This new console is now registered. */
> +	if ((newcon->flags & CON_NBCON) && newcon->write_atomic)
> +		newcon->driver_exit(newcon, flags);
> +
>  	console_sysfs_notify();
>  
>  	/*
> 
> > One solution would be to add nbcon consoles into the console_list
> > under uart_port_lock().
> 
> This is what I have proposed and I think it is the most straight forward
> solution.
> 
> > Another solution would be to make sure that any code serialized
> > by uart_port_lock() will be already synchronized by nbcon context
> > while the nbcon is added into the console_list.
> 
> I do not think this would be acceptable. It would mean that non-console
> ports would need to lock the nbcon. Not only will that slow down the
> non-console ports, but it will also cause serious contention between the
> ports. (Remember, all the ports share the same struct console.)

I actually did not want to lock the nbcon for all ports. This is why
I proposed to do it in con->setup() where con->index is already set.
It might solve the problem without adding yet another callbacks.

That said, I like your solution with newcon->driver_enter()/exit()
callbacks. It seems to have an easier and more straightforward semantic.

Go for it, especially when you need these callbacks later in
the printk kthread.

Nit: I think about renaming the callbacks to"device_lock()
     and device_unlock().

     "(un)lock" probably better describes what the callbacks do.
     register_console() does not want to do any operations
     on the serial port. It just needs to serialize adding
     the console into the list.

     I suggest "device" because the callbacks will lock/unlock
     the tty_driver pointed by "con->device".

> 
> > Maybe, we could do this in con->setup() callback. Something like:
> 
> This proposal would work, but IMHO it adds too much complexity by
> requiring console drivers to implement the callbacks and do special
> things in those callbacks.

Fair enough.

Best Regards,
Petr