diff mbox series

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

Message ID 20221130150919.13935-1-roger.pau@citrix.com (mailing list archive)
State New
Headers show
Series [v2] hvc/xen: prevent concurrent accesses to the shared ring | expand

Commit Message

Roger Pau Monne Nov. 30, 2022, 3:09 p.m. UTC
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(-)

Comments

Stefano Stabellini Dec. 1, 2022, 1:08 a.m. UTC | #1
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
>
Roger Pau Monne Dec. 2, 2022, 11:40 a.m. UTC | #2
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.
Roger Pau Monne Dec. 12, 2022, 12:36 p.m. UTC | #3
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.
>
diff mbox series

Patch

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);