[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Y4csGBQ6NXBC7vIO@Air-de-Roger>
Date: Wed, 30 Nov 2022 11:10:32 +0100
From: Roger Pau Monné <roger.pau@...rix.com>
To: Jan Beulich <jbeulich@...e.com>
Cc: linux-kernel@...r.kernel.org, xen-devel@...ts.xenproject.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>,
Juergen Gross <jgross@...e.com>,
Boris Ostrovsky <boris.ostrovsky@...cle.com>,
Olof Johansson <olof@...om.net>, Ingo Molnar <mingo@...e.hu>,
Chris Wright <chrisw@...s-sol.org>,
linuxppc-dev@...ts.ozlabs.org,
Stefano Stabellini <sstabellini@...nel.org>
Subject: 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@...rix.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.
Powered by blists - more mailing lists