On Wed, Nov 30, 2022 at 10:34:41AM +0100, Jan Beulich wrote:

On 30.11.2022 10:26, Roger Pau Monné wrote:

quoted

On Tue, Nov 29, 2022 at 02:12:10PM -0800, Stefano Stabellini wrote:

quoted

On Tue, 29 Nov 2022, Roger Pau Monne wrote:

quoted

The hvc machinery registers both a console and a tty device based on
the hv ops provided by the specific implementation.  Those two
interfaces however have different locks, and there's no single locks
that's shared between the tty and the console implementations, hence
the driver needs to protect itself against concurrent accesses.
Otherwise concurrent calls using the split interfaces are likely to
corrupt the ring indexes, leaving the console unusable.

Introduce a lock to xencons_info to serialize accesses to the shared
ring.  This is only required when using the shared memory console,
concurrent accesses to the hypercall based console implementation are
not an issue.

Note the conditional logic in domU_read_console() is slightly modified
so the notify_daemon() call can be done outside of the locked region:
it's an hypercall and there's no need for it to be done with the lock
held.

Fixes: b536b4b96230 ('xen: use the hvc console infrastructure for Xen console')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
While the write handler (domU_write_console()) is used by both the
console and the tty ops, that's not the case for the read side
(domU_read_console()).  It's not obvious to me whether we could get
concurrent poll calls from the poll_get_char tty hook, hence stay on
the safe side also serialize read accesses in domU_read_console().

I think domU_read_console doesn't need it. struct hv_ops and struct
console are both already locked although independently locked.

I think we shouldn't add an unrequired lock there.

Not all accesses are done using the tty lock.  There's a path using
tty_find_polling_driver() in kgdboc.c that directly calls into the
->poll_get_char() hook without any locks apparently taken.

Simply by the name of the file I'm inclined to say that debugger code
not respecting locks may be kind of intentional (but would then need
to be accompanied by certain other precautions there).

I'm also confused because hvc_poll() which calls get_chars() does so
while holding an hvc lock, while hvc_poll_get_char() calls get_chars()
without holding any lock.  The call to get_chars() being done with a
lock held in hvc_poll() might just be a side-effect of the lock
being hold to keep consistency in the hvc_struct struct.

I also wonder whether new users of tty_find_polling_driver() and
->poll_get_char() could start appearing and assuming that the
underlying implementation would already take the necessary locks for
consistency.  Just looking at hvc_vio.c it does take a lock in
its get_chars() implementation to serialize accesses to the buffer.

Thanks, Roger.