[PATCH 0/2] xen: two fixes related to event channels

[PATCH 0/2] xen: two fixes related to event channels

[Juergen Gross]

Two patches fixing one seen problem and another potential one. Both
have been introduced in the 6.7 kernel.

Juergen Gross (2):
  xen/evtchn: avoid WARN() when unbinding an event channel
  xen/events: increment refcnt only if event channel is refcounted

 drivers/xen/events/events_base.c | 22 +++++++++++++---------
 drivers/xen/evtchn.c             |  6 ++++++
 2 files changed, 19 insertions(+), 9 deletions(-)

-- 
2.35.3

[PATCH 1/2] xen/evtchn: avoid WARN() when unbinding an event channel

[Juergen Gross]

When unbinding a user event channel, the related handler might be
called a last time in case the kernel was built with
CONFIG_DEBUG_SHIRQ. This might cause a WARN() in the handler.

Avoid that by adding an "unbinding" flag to struct user_event which
will short circuit the handler.

Fixes: 9e90e58c11b7 ("xen: evtchn: Allow shared registration of IRQ handers")
Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Tested-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/evtchn.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c
index 59717628ca42..f6a2216c2c87 100644
--- a/drivers/xen/evtchn.c
+++ b/drivers/xen/evtchn.c
@@ -85,6 +85,7 @@ struct user_evtchn {
 	struct per_user_data *user;
 	evtchn_port_t port;
 	bool enabled;
+	bool unbinding;
 };
 
 static void evtchn_free_ring(evtchn_port_t *ring)
@@ -164,6 +165,10 @@ static irqreturn_t evtchn_interrupt(int irq, void *data)
 	struct per_user_data *u = evtchn->user;
 	unsigned int prod, cons;
 
+	/* Handler might be called when tearing down the IRQ. */
+	if (evtchn->unbinding)
+		return IRQ_HANDLED;
+
 	WARN(!evtchn->enabled,
 	     "Interrupt for port %u, but apparently not enabled; per-user %p\n",
 	     evtchn->port, u);
@@ -421,6 +426,7 @@ static void evtchn_unbind_from_user(struct per_user_data *u,
 
 	BUG_ON(irq < 0);
 
+	evtchn->unbinding = true;
 	unbind_from_irqhandler(irq, evtchn);
 
 	del_evtchn(u, evtchn);
-- 
2.35.3

Re: [PATCH 1/2] xen/evtchn: avoid WARN() when unbinding an event channel

[Oleksandr Tyshchenko]

On 13.03.24 09:14, Juergen Gross wrote:

Hello Juergen


> When unbinding a user event channel, the related handler might be
> called a last time in case the kernel was built with
> CONFIG_DEBUG_SHIRQ. This might cause a WARN() in the handler.
> 
> Avoid that by adding an "unbinding" flag to struct user_event which
> will short circuit the handler.
> 
> Fixes: 9e90e58c11b7 ("xen: evtchn: Allow shared registration of IRQ handers")
> Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Tested-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>


Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


> ---
>   drivers/xen/evtchn.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c
> index 59717628ca42..f6a2216c2c87 100644
> --- a/drivers/xen/evtchn.c
> +++ b/drivers/xen/evtchn.c
> @@ -85,6 +85,7 @@ struct user_evtchn {
>   	struct per_user_data *user;
>   	evtchn_port_t port;
>   	bool enabled;
> +	bool unbinding;
>   };
>   
>   static void evtchn_free_ring(evtchn_port_t *ring)
> @@ -164,6 +165,10 @@ static irqreturn_t evtchn_interrupt(int irq, void *data)
>   	struct per_user_data *u = evtchn->user;
>   	unsigned int prod, cons;
>   
> +	/* Handler might be called when tearing down the IRQ. */
> +	if (evtchn->unbinding)
> +		return IRQ_HANDLED;
> +
>   	WARN(!evtchn->enabled,
>   	     "Interrupt for port %u, but apparently not enabled; per-user %p\n",
>   	     evtchn->port, u);
> @@ -421,6 +426,7 @@ static void evtchn_unbind_from_user(struct per_user_data *u,
>   
>   	BUG_ON(irq < 0);
>   
> +	evtchn->unbinding = true;
>   	unbind_from_irqhandler(irq, evtchn);
>   
>   	del_evtchn(u, evtchn);

[PATCH 2/2] xen/events: increment refcnt only if event channel is refcounted

[Juergen Gross]

In bind_evtchn_to_irq_chip() don't increment the refcnt of the event
channel blindly. In case the event channel is NOT refcounted, issue a
warning instead.

Add an additional safety net by doing the refcnt increment only if the
caller has specified IRQF_SHARED in the irqflags parameter.

Fixes: 9e90e58c11b7 ("xen: evtchn: Allow shared registration of IRQ handers")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/events/events_base.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 2faa4bf78c7a..81effbd53dc5 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -1190,7 +1190,7 @@ int xen_pirq_from_irq(unsigned irq)
 EXPORT_SYMBOL_GPL(xen_pirq_from_irq);
 
 static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
-				   struct xenbus_device *dev)
+				   struct xenbus_device *dev, bool shared)
 {
 	int ret = -ENOMEM;
 	struct irq_info *info;
@@ -1224,7 +1224,8 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
 		 */
 		bind_evtchn_to_cpu(info, 0, false);
 	} else if (!WARN_ON(info->type != IRQT_EVTCHN)) {
-		info->refcnt++;
+		if (shared && !WARN_ON(info->refcnt < 0))
+			info->refcnt++;
 	}
 
 	ret = info->irq;
@@ -1237,13 +1238,13 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
 
 int bind_evtchn_to_irq(evtchn_port_t evtchn)
 {
-	return bind_evtchn_to_irq_chip(evtchn, &xen_dynamic_chip, NULL);
+	return bind_evtchn_to_irq_chip(evtchn, &xen_dynamic_chip, NULL, false);
 }
 EXPORT_SYMBOL_GPL(bind_evtchn_to_irq);
 
 int bind_evtchn_to_irq_lateeoi(evtchn_port_t evtchn)
 {
-	return bind_evtchn_to_irq_chip(evtchn, &xen_lateeoi_chip, NULL);
+	return bind_evtchn_to_irq_chip(evtchn, &xen_lateeoi_chip, NULL, false);
 }
 EXPORT_SYMBOL_GPL(bind_evtchn_to_irq_lateeoi);
 
@@ -1295,7 +1296,8 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int cpu)
 
 static int bind_interdomain_evtchn_to_irq_chip(struct xenbus_device *dev,
 					       evtchn_port_t remote_port,
-					       struct irq_chip *chip)
+					       struct irq_chip *chip,
+					       bool shared)
 {
 	struct evtchn_bind_interdomain bind_interdomain;
 	int err;
@@ -1307,14 +1309,14 @@ static int bind_interdomain_evtchn_to_irq_chip(struct xenbus_device *dev,
 					  &bind_interdomain);
 
 	return err ? : bind_evtchn_to_irq_chip(bind_interdomain.local_port,
-					       chip, dev);
+					       chip, dev, shared);
 }
 
 int bind_interdomain_evtchn_to_irq_lateeoi(struct xenbus_device *dev,
 					   evtchn_port_t remote_port)
 {
 	return bind_interdomain_evtchn_to_irq_chip(dev, remote_port,
-						   &xen_lateeoi_chip);
+						   &xen_lateeoi_chip, false);
 }
 EXPORT_SYMBOL_GPL(bind_interdomain_evtchn_to_irq_lateeoi);
 
@@ -1430,7 +1432,8 @@ static int bind_evtchn_to_irqhandler_chip(evtchn_port_t evtchn,
 {
 	int irq, retval;
 
-	irq = bind_evtchn_to_irq_chip(evtchn, chip, NULL);
+	irq = bind_evtchn_to_irq_chip(evtchn, chip, NULL,
+				      irqflags & IRQF_SHARED);
 	if (irq < 0)
 		return irq;
 	retval = request_irq(irq, handler, irqflags, devname, dev_id);
@@ -1471,7 +1474,8 @@ static int bind_interdomain_evtchn_to_irqhandler_chip(
 {
 	int irq, retval;
 
-	irq = bind_interdomain_evtchn_to_irq_chip(dev, remote_port, chip);
+	irq = bind_interdomain_evtchn_to_irq_chip(dev, remote_port, chip,
+						  irqflags & IRQF_SHARED);
 	if (irq < 0)
 		return irq;
 
-- 
2.35.3

Re: [PATCH 2/2] xen/events: increment refcnt only if event channel is refcounted

[Oleksandr Tyshchenko]

On 13.03.24 09:14, Juergen Gross wrote:


Hello Juergen

> In bind_evtchn_to_irq_chip() don't increment the refcnt of the event
> channel blindly. In case the event channel is NOT refcounted, issue a
> warning instead.
> 
> Add an additional safety net by doing the refcnt increment only if the
> caller has specified IRQF_SHARED in the irqflags parameter.
> 
> Fixes: 9e90e58c11b7 ("xen: evtchn: Allow shared registration of IRQ handers")
> Signed-off-by: Juergen Gross <jgross@suse.com>


Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


> ---
>   drivers/xen/events/events_base.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index 2faa4bf78c7a..81effbd53dc5 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -1190,7 +1190,7 @@ int xen_pirq_from_irq(unsigned irq)
>   EXPORT_SYMBOL_GPL(xen_pirq_from_irq);
>   
>   static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
> -				   struct xenbus_device *dev)
> +				   struct xenbus_device *dev, bool shared)
>   {
>   	int ret = -ENOMEM;
>   	struct irq_info *info;
> @@ -1224,7 +1224,8 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
>   		 */
>   		bind_evtchn_to_cpu(info, 0, false);
>   	} else if (!WARN_ON(info->type != IRQT_EVTCHN)) {
> -		info->refcnt++;
> +		if (shared && !WARN_ON(info->refcnt < 0))
> +			info->refcnt++;
>   	}
>   
>   	ret = info->irq;
> @@ -1237,13 +1238,13 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
>   
>   int bind_evtchn_to_irq(evtchn_port_t evtchn)
>   {
> -	return bind_evtchn_to_irq_chip(evtchn, &xen_dynamic_chip, NULL);
> +	return bind_evtchn_to_irq_chip(evtchn, &xen_dynamic_chip, NULL, false);
>   }
>   EXPORT_SYMBOL_GPL(bind_evtchn_to_irq);
>   
>   int bind_evtchn_to_irq_lateeoi(evtchn_port_t evtchn)
>   {
> -	return bind_evtchn_to_irq_chip(evtchn, &xen_lateeoi_chip, NULL);
> +	return bind_evtchn_to_irq_chip(evtchn, &xen_lateeoi_chip, NULL, false);
>   }
>   EXPORT_SYMBOL_GPL(bind_evtchn_to_irq_lateeoi);
>   
> @@ -1295,7 +1296,8 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int cpu)
>   
>   static int bind_interdomain_evtchn_to_irq_chip(struct xenbus_device *dev,
>   					       evtchn_port_t remote_port,
> -					       struct irq_chip *chip)
> +					       struct irq_chip *chip,
> +					       bool shared)
>   {
>   	struct evtchn_bind_interdomain bind_interdomain;
>   	int err;
> @@ -1307,14 +1309,14 @@ static int bind_interdomain_evtchn_to_irq_chip(struct xenbus_device *dev,
>   					  &bind_interdomain);
>   
>   	return err ? : bind_evtchn_to_irq_chip(bind_interdomain.local_port,
> -					       chip, dev);
> +					       chip, dev, shared);
>   }
>   
>   int bind_interdomain_evtchn_to_irq_lateeoi(struct xenbus_device *dev,
>   					   evtchn_port_t remote_port)
>   {
>   	return bind_interdomain_evtchn_to_irq_chip(dev, remote_port,
> -						   &xen_lateeoi_chip);
> +						   &xen_lateeoi_chip, false);
>   }
>   EXPORT_SYMBOL_GPL(bind_interdomain_evtchn_to_irq_lateeoi);
>   
> @@ -1430,7 +1432,8 @@ static int bind_evtchn_to_irqhandler_chip(evtchn_port_t evtchn,
>   {
>   	int irq, retval;
>   
> -	irq = bind_evtchn_to_irq_chip(evtchn, chip, NULL);
> +	irq = bind_evtchn_to_irq_chip(evtchn, chip, NULL,
> +				      irqflags & IRQF_SHARED);
>   	if (irq < 0)
>   		return irq;
>   	retval = request_irq(irq, handler, irqflags, devname, dev_id);
> @@ -1471,7 +1474,8 @@ static int bind_interdomain_evtchn_to_irqhandler_chip(
>   {
>   	int irq, retval;
>   
> -	irq = bind_interdomain_evtchn_to_irq_chip(dev, remote_port, chip);
> +	irq = bind_interdomain_evtchn_to_irq_chip(dev, remote_port, chip,
> +						  irqflags & IRQF_SHARED);
>   	if (irq < 0)
>   		return irq;
>