On 23.01.2026 02:06, Stefano Stabellini wrote:
> @@ -555,8 +561,11 @@ static void console_switch_input(void)
>
> if ( next_rx++ >= max_console_rx )
> {
> - console_rx = 0;
> printk("*** Serial input to Xen");
> + nrspin_lock_irq(&console_lock);
> + console_rx = 0;
> + serial_rx_cons = serial_rx_prod;
> + nrspin_unlock_irq(&console_lock);
> break;
> }
>
> @@ -575,8 +584,12 @@ static void console_switch_input(void)
>
> rcu_unlock_domain(d);
>
> - console_rx = next_rx;
> printk("*** Serial input to DOM%u", domid);
> + nrspin_lock_irq(&console_lock);
> + console_rx = next_rx;
> + /* Don't let the next dom read the previous dom's unread data. */
> + serial_rx_cons = serial_rx_prod;
> + nrspin_unlock_irq(&console_lock);
> break;
> }
As in both cases you're also moving console_rx updates into the locked
region, what about readers of the variable, first and foremost
console_get_domain()? Else why the movement?
Also I think that the printk()s would better be last, indicating the
completion of the switching.
> @@ -605,8 +618,10 @@ static void __serial_rx(char c)
> * Deliver input to the hardware domain buffer, unless it is
> * already full.
> */
This comment goes stale with the buffer being used for whichever is the
focus domain in do_console_io().
> + nrspin_lock_irq(&console_lock);
> if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
> serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> + nrspin_unlock_irq(&console_lock);
I don't think you can unconditionally enable interrupts here, as this may
be running in the context of an IRQ handler.
> @@ -742,17 +758,36 @@ static long
> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> if ( copy_from_guest(kbuf, buffer, kcount) )
> return -EFAULT;
>
> - if ( is_hardware_domain(cd) )
> + /*
> + * Take both cons->lock and console_lock:
> + * - cons->lock protects cons->buf and cons->idx
> + * - console_lock protects console_send and is_focus_domain
> + * checks
> + *
> + * The order must be respected. guest_printk takes the
> + * console_lock so it is important that cons->lock is taken
> + * first.
> + */
> + spin_lock(&cons->lock);
> + nrspin_lock_irq(&console_lock);
> + if ( is_focus_domain(cd) )
Why would any of the domains possibly being permitted to be "focus" suddenly
gain direct access here? Full access in do_console_io() is still prevented by
the XSM check there, afaict. Cc-ing Daniel, as it's not quite clear to me
whether to introduce another XSM check here, whether to check ->is_console
directly, or yet something else.
> {
> + if ( cons->idx )
> + {
> + console_send(cons->buf, cons->idx, flags);
> + cons->idx = 0;
> + }
> + spin_unlock(&cons->lock);
> +
> /* Use direct console output as it could be interactive */
> - nrspin_lock_irq(&console_lock);
> console_send(kbuf, kcount, flags);
> nrspin_unlock_irq(&console_lock);
> }
> else
> {
> char *kin = kbuf, *kout = kbuf, c;
> - struct domain_console *cons = cd->console;
> +
> + nrspin_unlock_irq(&console_lock);
>
> /* Strip non-printable characters */
> do
> @@ -765,7 +800,6 @@ static long
> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> } while ( --kcount > 0 );
>
> *kout = '\0';
> - spin_lock(&cons->lock);
> kcount = kin - kbuf;
> if ( c != '\n' &&
> (cons->idx + (kout - kbuf) < (ARRAY_SIZE(cons->buf) - 1)) )
> @@ -815,6 +849,13 @@ long do_console_io(
> if ( count > INT_MAX )
> break;
>
> + nrspin_lock_irq(&console_lock);
> + if ( !is_focus_domain(current->domain) )
> + {
> + nrspin_unlock_irq(&console_lock);
> + return 0;
> + }
To avoid the extra unlock-and-return, how about simply setting "count" to 0,
leveraging ...
> rc = 0;
> while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
... the rhs check here?
> @@ -824,14 +865,28 @@ long do_console_io(
> len = SERIAL_RX_SIZE - idx;
> if ( (rc + len) > count )
> len = count - rc;
> + nrspin_unlock_irq(&console_lock);
This can be moved up a few lines, as none of the local variable manipulations
needs guarding.
> if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len)
> )
> - {
> - rc = -EFAULT;
> - break;
> - }
> + return -EFAULT;
> rc += len;
> +
> + /*
> + * First get console_lock again, then check is_focus_domain.
> + * It is possible that we switched focus domain during
> + * copy_to_guest_offset. In that case, serial_rx_cons and
Please can you append () to the function name, to identify it as a function,
as opposed to ...
> + * serial_rx_prod have been reset so it would be wrong to
> + * update serial_rx_cons here. Get out of the loop instead.
... the two variables referenced here?
Jan
