[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] hvc/xen: prevent concurrent accesses to the shared ring
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.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |