On Wed, 28 Jan 2026, Jan Beulich wrote:
> 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?
You're right. I've added locking in console_get_domain() to read
console_rx under console_lock into a local variable, ensuring
consistency with the now-locked writes.
> Also I think that the printk()s would better be last, indicating the
> completion of the switching.
OK
> > @@ -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.
Good catch! I've changed __serial_rx() to use nrspin_lock_irqsave()/
nrspin_unlock_irqrestore() instead of
nrspin_lock_irq()/nrspin_unlock_irq().
> > @@ -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.
The XSM check still happens first in do_console_io() via
xsm_console_io(XSM_OTHER, current->domain, cmd), which validates that
the domain has permission to use console_io hypercalls. The focus check
is an additional restriction that only allows reading when the domain
has focus: it doesn't grant new permissions. Dom0less domains with
input_allowed = true are already permitted by XSM policy to use
console_io; the focus mechanism just ensures only one domain can read
input at a time (similar to vpl011 behavior). Additionally, this is also
allowed for dom0less domains which are quite special and manually
configured at boot by the user. There are no arbitrary unknown dom0less
domains that can be started later.
> > {
> > + 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 ...
Good idea.
> > 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.
OK
> > 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?
Done
