Re: [PATCH] hvc/xen: prevent concurrent accesses to the shared ring

[Roger Pau Monné]

On Wed, Nov 30, 2022 at 10:34:41AM +0100, Jan Beulich wrote:
> On 30.11.2022 10:26, Roger Pau Monné wrote:
> > On Tue, Nov 29, 2022 at 02:12:10PM -0800, Stefano Stabellini wrote:
> >> On Tue, 29 Nov 2022, Roger Pau Monne wrote:
> >>> 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@xxxxxxxxxx>
> >>> ---
> >>> 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.