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

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

[Roger Pau Monne]

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>
---
Changes since v1:
 - Properly initialize the introduced lock in all paths.
---
 drivers/tty/hvc/hvc_xen.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 7c23112dc923..e63c1761a361 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -43,6 +43,7 @@ struct xencons_info {
 	int irq;
 	int vtermno;
 	grant_ref_t gntref;
+	spinlock_t ring_lock;
 };
 
 static LIST_HEAD(xenconsoles);
@@ -84,12 +85,15 @@ static int __write_console(struct xencons_info *xencons,
 	XENCONS_RING_IDX cons, prod;
 	struct xencons_interface *intf = xencons->intf;
 	int sent = 0;
+	unsigned long flags;
 
+	spin_lock_irqsave(&xencons->ring_lock, flags);
 	cons = intf->out_cons;
 	prod = intf->out_prod;
 	mb();			/* update queue values before going on */
 
 	if ((prod - cons) > sizeof(intf->out)) {
+		spin_unlock_irqrestore(&xencons->ring_lock, flags);
 		pr_err_once("xencons: Illegal ring page indices");
 		return -EINVAL;
 	}
@@ -99,6 +103,7 @@ static int __write_console(struct xencons_info *xencons,
 
 	wmb();			/* write ring before updating pointer */
 	intf->out_prod = prod;
+	spin_unlock_irqrestore(&xencons->ring_lock, flags);
 
 	if (sent)
 		notify_daemon(xencons);
@@ -141,16 +146,19 @@ static int domU_read_console(uint32_t vtermno, char *buf, int len)
 	int recv = 0;
 	struct xencons_info *xencons = vtermno_to_xencons(vtermno);
 	unsigned int eoiflag = 0;
+	unsigned long flags;
 
 	if (xencons == NULL)
 		return -EINVAL;
 	intf = xencons->intf;
 
+	spin_lock_irqsave(&xencons->ring_lock, flags);
 	cons = intf->in_cons;
 	prod = intf->in_prod;
 	mb();			/* get pointers before reading ring */
 
 	if ((prod - cons) > sizeof(intf->in)) {
+		spin_unlock_irqrestore(&xencons->ring_lock, flags);
 		pr_err_once("xencons: Illegal ring page indices");
 		return -EINVAL;
 	}
@@ -174,10 +182,13 @@ static int domU_read_console(uint32_t vtermno, char *buf, int len)
 		xencons->out_cons = intf->out_cons;
 		xencons->out_cons_same = 0;
 	}
+	if (!recv && xencons->out_cons_same++ > 1) {
+		eoiflag = XEN_EOI_FLAG_SPURIOUS;
+	}
+	spin_unlock_irqrestore(&xencons->ring_lock, flags);
+
 	if (recv) {
 		notify_daemon(xencons);
-	} else if (xencons->out_cons_same++ > 1) {
-		eoiflag = XEN_EOI_FLAG_SPURIOUS;
 	}
 
 	xen_irq_lateeoi(xencons->irq, eoiflag);
@@ -234,6 +245,7 @@ static int xen_hvm_console_init(void)
 		info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL);
 		if (!info)
 			return -ENOMEM;
+		spin_lock_init(&info->ring_lock);
 	} else if (info->intf != NULL) {
 		/* already configured */
 		return 0;
@@ -270,6 +282,7 @@ static int xen_hvm_console_init(void)
 
 static int xencons_info_pv_init(struct xencons_info *info, int vtermno)
 {
+	spin_lock_init(&info->ring_lock);
 	info->evtchn = xen_start_info->console.domU.evtchn;
 	/* GFN == MFN for PV guest */
 	info->intf = gfn_to_virt(xen_start_info->console.domU.mfn);
@@ -318,6 +331,7 @@ static int xen_initial_domain_console_init(void)
 		info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL);
 		if (!info)
 			return -ENOMEM;
+		spin_lock_init(&info->ring_lock);
 	}
 
 	info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0, false);
@@ -472,6 +486,7 @@ static int xencons_probe(struct xenbus_device *dev,
 	info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
+	spin_lock_init(&info->ring_lock);
 	dev_set_drvdata(&dev->dev, info);
 	info->xbdev = dev;
 	info->vtermno = xenbus_devid_to_vtermno(devid);
-- 
2.37.3

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5349 bytes --]

On Wed, 30 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.

For domU_read_console: I don't mean to block this patch but we need to
be sure about the semantics of hv_ops.get_chars. Either it is expected
to be already locked, then we definitely shouldn't add another lock to
domU_read_console. Or it is not expected to be already locked, then we
should add the lock.

My impression is that it is expected to be already locked, but I think
we need Greg or Jiri to confirm one way or the other.

Aside from that the rest looks fine.



> Fixes: b536b4b96230 ('xen: use the hvc console infrastructure for Xen console')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - Properly initialize the introduced lock in all paths.
> ---
>  drivers/tty/hvc/hvc_xen.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 7c23112dc923..e63c1761a361 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -43,6 +43,7 @@ struct xencons_info {
>  	int irq;
>  	int vtermno;
>  	grant_ref_t gntref;
> +	spinlock_t ring_lock;
>  };
>  
>  static LIST_HEAD(xenconsoles);
> @@ -84,12 +85,15 @@ static int __write_console(struct xencons_info *xencons,
>  	XENCONS_RING_IDX cons, prod;
>  	struct xencons_interface *intf = xencons->intf;
>  	int sent = 0;
> +	unsigned long flags;
>  
> +	spin_lock_irqsave(&xencons->ring_lock, flags);
>  	cons = intf->out_cons;
>  	prod = intf->out_prod;
>  	mb();			/* update queue values before going on */
>  
>  	if ((prod - cons) > sizeof(intf->out)) {
> +		spin_unlock_irqrestore(&xencons->ring_lock, flags);
>  		pr_err_once("xencons: Illegal ring page indices");
>  		return -EINVAL;
>  	}
> @@ -99,6 +103,7 @@ static int __write_console(struct xencons_info *xencons,
>  
>  	wmb();			/* write ring before updating pointer */
>  	intf->out_prod = prod;
> +	spin_unlock_irqrestore(&xencons->ring_lock, flags);
>  
>  	if (sent)
>  		notify_daemon(xencons);
> @@ -141,16 +146,19 @@ static int domU_read_console(uint32_t vtermno, char *buf, int len)
>  	int recv = 0;
>  	struct xencons_info *xencons = vtermno_to_xencons(vtermno);
>  	unsigned int eoiflag = 0;
> +	unsigned long flags;
>  
>  	if (xencons == NULL)
>  		return -EINVAL;
>  	intf = xencons->intf;
>  
> +	spin_lock_irqsave(&xencons->ring_lock, flags);
>  	cons = intf->in_cons;
>  	prod = intf->in_prod;
>  	mb();			/* get pointers before reading ring */
>  
>  	if ((prod - cons) > sizeof(intf->in)) {
> +		spin_unlock_irqrestore(&xencons->ring_lock, flags);
>  		pr_err_once("xencons: Illegal ring page indices");
>  		return -EINVAL;
>  	}
> @@ -174,10 +182,13 @@ static int domU_read_console(uint32_t vtermno, char *buf, int len)
>  		xencons->out_cons = intf->out_cons;
>  		xencons->out_cons_same = 0;
>  	}
> +	if (!recv && xencons->out_cons_same++ > 1) {
> +		eoiflag = XEN_EOI_FLAG_SPURIOUS;
> +	}
> +	spin_unlock_irqrestore(&xencons->ring_lock, flags);
> +
>  	if (recv) {
>  		notify_daemon(xencons);
> -	} else if (xencons->out_cons_same++ > 1) {
> -		eoiflag = XEN_EOI_FLAG_SPURIOUS;
>  	}
>  
>  	xen_irq_lateeoi(xencons->irq, eoiflag);
> @@ -234,6 +245,7 @@ static int xen_hvm_console_init(void)
>  		info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL);
>  		if (!info)
>  			return -ENOMEM;
> +		spin_lock_init(&info->ring_lock);
>  	} else if (info->intf != NULL) {
>  		/* already configured */
>  		return 0;
> @@ -270,6 +282,7 @@ static int xen_hvm_console_init(void)
>  
>  static int xencons_info_pv_init(struct xencons_info *info, int vtermno)
>  {
> +	spin_lock_init(&info->ring_lock);
>  	info->evtchn = xen_start_info->console.domU.evtchn;
>  	/* GFN == MFN for PV guest */
>  	info->intf = gfn_to_virt(xen_start_info->console.domU.mfn);
> @@ -318,6 +331,7 @@ static int xen_initial_domain_console_init(void)
>  		info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL);
>  		if (!info)
>  			return -ENOMEM;
> +		spin_lock_init(&info->ring_lock);
>  	}
>  
>  	info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0, false);
> @@ -472,6 +486,7 @@ static int xencons_probe(struct xenbus_device *dev,
>  	info = kzalloc(sizeof(struct xencons_info), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
> +	spin_lock_init(&info->ring_lock);
>  	dev_set_drvdata(&dev->dev, info);
>  	info->xbdev = dev;
>  	info->vtermno = xenbus_devid_to_vtermno(devid);
> -- 
> 2.37.3
> 

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

[Roger Pau Monné]

On Wed, Nov 30, 2022 at 05:08:06PM -0800, Stefano Stabellini wrote:
> On Wed, 30 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.
> 
> For domU_read_console: I don't mean to block this patch but we need to
> be sure about the semantics of hv_ops.get_chars. Either it is expected
> to be already locked, then we definitely shouldn't add another lock to
> domU_read_console. Or it is not expected to be already locked, then we
> should add the lock.
> 
> My impression is that it is expected to be already locked, but I think
> we need Greg or Jiri to confirm one way or the other.

Let me move both to the 'To:' field then.

My main concern is the usage of hv_ops.get_chars hook in
hvc_poll_get_char(), as it's not obvious to me that callers of
tty->poll_get_char hook as returned by tty_find_polling_driver() will
always do so with the tty lock held (in fact the only user right now
doesn't seem to hold the tty lock).

> Aside from that the rest looks fine.

Thanks for the review, Roger.

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

[Roger Pau Monné]

Hello,

Gentle ping regarding the locking question below.

Thanks, Roger.

On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné wrote:
> On Wed, Nov 30, 2022 at 05:08:06PM -0800, Stefano Stabellini wrote:
> > On Wed, 30 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.
> > 
> > For domU_read_console: I don't mean to block this patch but we need to
> > be sure about the semantics of hv_ops.get_chars. Either it is expected
> > to be already locked, then we definitely shouldn't add another lock to
> > domU_read_console. Or it is not expected to be already locked, then we
> > should add the lock.
> > 
> > My impression is that it is expected to be already locked, but I think
> > we need Greg or Jiri to confirm one way or the other.
> 
> Let me move both to the 'To:' field then.
> 
> My main concern is the usage of hv_ops.get_chars hook in
> hvc_poll_get_char(), as it's not obvious to me that callers of
> tty->poll_get_char hook as returned by tty_find_polling_driver() will
> always do so with the tty lock held (in fact the only user right now
> doesn't seem to hold the tty lock).
> 
> > Aside from that the rest looks fine.
> 
> Thanks for the review, Roger.
> 

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

[Roger Pau Monné]

Hello,

It's been 3 months and no reply.

On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote:
> Hello,
> 
> Gentle ping regarding the locking question below.
> 
> Thanks, Roger.
> 
> On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné wrote:
> > On Wed, Nov 30, 2022 at 05:08:06PM -0800, Stefano Stabellini wrote:
> > > On Wed, 30 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.
> > > 
> > > For domU_read_console: I don't mean to block this patch but we need to
> > > be sure about the semantics of hv_ops.get_chars. Either it is expected
> > > to be already locked, then we definitely shouldn't add another lock to
> > > domU_read_console. Or it is not expected to be already locked, then we
> > > should add the lock.
> > > 
> > > My impression is that it is expected to be already locked, but I think
> > > we need Greg or Jiri to confirm one way or the other.
> > 
> > Let me move both to the 'To:' field then.
> > 
> > My main concern is the usage of hv_ops.get_chars hook in
> > hvc_poll_get_char(), as it's not obvious to me that callers of
> > tty->poll_get_char hook as returned by tty_find_polling_driver() will
> > always do so with the tty lock held (in fact the only user right now
> > doesn't seem to hold the tty lock).
> > 
> > > Aside from that the rest looks fine.

I guess I could reluctantly remove the lock in the get_chars hook,
albeit I'm not convinced at all the lock is not needed there.

Roger.

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

[Michael Ellerman]

Roger Pau Monné <roger.pau@citrix.com> writes:
> On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote:
>> On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné wrote:
>> > On Wed, Nov 30, 2022 at 05:08:06PM -0800, Stefano Stabellini wrote:
>> > > On Wed, 30 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.
>> > >
>> > > For domU_read_console: I don't mean to block this patch but we need to
>> > > be sure about the semantics of hv_ops.get_chars. Either it is expected
>> > > to be already locked, then we definitely shouldn't add another lock to
>> > > domU_read_console. Or it is not expected to be already locked, then we
>> > > should add the lock.
>> > >
>> > > My impression is that it is expected to be already locked, but I think
>> > > we need Greg or Jiri to confirm one way or the other.
>> >
>> > Let me move both to the 'To:' field then.
>> >
>> > My main concern is the usage of hv_ops.get_chars hook in
>> > hvc_poll_get_char(), as it's not obvious to me that callers of
>> > tty->poll_get_char hook as returned by tty_find_polling_driver() will
>> > always do so with the tty lock held (in fact the only user right now
>> > doesn't seem to hold the tty lock).
>> >
>> > > Aside from that the rest looks fine.
>
> I guess I could reluctantly remove the lock in the get_chars hook,
> albeit I'm not convinced at all the lock is not needed there.

I don't know the xen driver, but other HVC backends have a lock around
their private state in their get_chars() implementations.

See eg. hvterm_raw_get_chars().

cheers

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

[Roger Pau Monné]

On Fri, Mar 10, 2023 at 10:01:39AM +1100, Michael Ellerman wrote:
> Roger Pau Monné <roger.pau@citrix.com> writes:
> > On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote:
> >> On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné wrote:
> >> > On Wed, Nov 30, 2022 at 05:08:06PM -0800, Stefano Stabellini wrote:
> >> > > On Wed, 30 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.
> >> > >
> >> > > For domU_read_console: I don't mean to block this patch but we need to
> >> > > be sure about the semantics of hv_ops.get_chars. Either it is expected
> >> > > to be already locked, then we definitely shouldn't add another lock to
> >> > > domU_read_console. Or it is not expected to be already locked, then we
> >> > > should add the lock.
> >> > >
> >> > > My impression is that it is expected to be already locked, but I think
> >> > > we need Greg or Jiri to confirm one way or the other.
> >> >
> >> > Let me move both to the 'To:' field then.
> >> >
> >> > My main concern is the usage of hv_ops.get_chars hook in
> >> > hvc_poll_get_char(), as it's not obvious to me that callers of
> >> > tty->poll_get_char hook as returned by tty_find_polling_driver() will
> >> > always do so with the tty lock held (in fact the only user right now
> >> > doesn't seem to hold the tty lock).
> >> >
> >> > > Aside from that the rest looks fine.
> >
> > I guess I could reluctantly remove the lock in the get_chars hook,
> > albeit I'm not convinced at all the lock is not needed there.
> 
> I don't know the xen driver, but other HVC backends have a lock around
> their private state in their get_chars() implementations.
> 
> See eg. hvterm_raw_get_chars().

Yes, that was one of the motivation for adding the lock also here, and
it has already been mentioned.  The other is the usage of the hooks by
callers of hvc_poll_get_char().

Thanks, Roger.

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

[Roger Pau Monné]

On Fri, Mar 10, 2023 at 10:18:32AM +0100, Roger Pau Monné wrote:
> On Fri, Mar 10, 2023 at 10:01:39AM +1100, Michael Ellerman wrote:
> > Roger Pau Monné <roger.pau@citrix.com> writes:
> > > On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote:
> > >> On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné wrote:
> > >> > On Wed, Nov 30, 2022 at 05:08:06PM -0800, Stefano Stabellini wrote:
> > >> > > On Wed, 30 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.
> > >> > >
> > >> > > For domU_read_console: I don't mean to block this patch but we need to
> > >> > > be sure about the semantics of hv_ops.get_chars. Either it is expected
> > >> > > to be already locked, then we definitely shouldn't add another lock to
> > >> > > domU_read_console. Or it is not expected to be already locked, then we
> > >> > > should add the lock.
> > >> > >
> > >> > > My impression is that it is expected to be already locked, but I think
> > >> > > we need Greg or Jiri to confirm one way or the other.
> > >> >
> > >> > Let me move both to the 'To:' field then.
> > >> >
> > >> > My main concern is the usage of hv_ops.get_chars hook in
> > >> > hvc_poll_get_char(), as it's not obvious to me that callers of
> > >> > tty->poll_get_char hook as returned by tty_find_polling_driver() will
> > >> > always do so with the tty lock held (in fact the only user right now
> > >> > doesn't seem to hold the tty lock).
> > >> >
> > >> > > Aside from that the rest looks fine.
> > >
> > > I guess I could reluctantly remove the lock in the get_chars hook,
> > > albeit I'm not convinced at all the lock is not needed there.
> > 
> > I don't know the xen driver, but other HVC backends have a lock around
> > their private state in their get_chars() implementations.
> > 
> > See eg. hvterm_raw_get_chars().
> 
> Yes, that was one of the motivation for adding the lock also here, and
> it has already been mentioned.  The other is the usage of the hooks by
> callers of hvc_poll_get_char().

Ping?

Thanks, Roger.

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1199 bytes --]

On 30.11.22 16:09, 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@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]