9p/xen: fix release of IRQ

[PATCH] 9p/xen: fix release of IRQ

Posted by Alexander Merritt 1 month ago

From: Alex Zenla <alex@edera.dev>

Kernel logs indicate an IRQ was double-freed.

Pass correct device ID during IRQ release.

Fixes: 71ebd71921e45 ("xen/9pfs: connect to the backend")
Signed-off-by: Alex Zenla <alex@edera.dev>
Signed-off-by: Alexander Merritt <alexander@edera.dev>
Signed-off-by: Ariadne Conill <ariadne@ariadne.space>
---
 net/9p/trans_xen.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index dfdbe1ca5338..198d46d79d84 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -286,7 +286,8 @@ static void xen_9pfs_front_free(struct xen_9pfs_front_priv *priv)
 		if (!priv->rings[i].intf)
 			break;
 		if (priv->rings[i].irq > 0)
-			unbind_from_irqhandler(priv->rings[i].irq, priv->dev);
+			unbind_from_irqhandler(priv->rings[i].irq, ring);
+		priv->rings[i].evtchn = priv->rings[i].irq = 0;
 		if (priv->rings[i].data.in) {
 			for (j = 0;
 			     j < (1 << priv->rings[i].intf->ring_order);
-- 
2.43.0

Re: [PATCH] 9p/xen: fix release of IRQ

Posted by Jürgen Groß 1 month ago

On 21.11.24 23:51, Alexander Merritt wrote:
> From: Alex Zenla <alex@edera.dev>
> 
> Kernel logs indicate an IRQ was double-freed.
> 
> Pass correct device ID during IRQ release.
> 
> Fixes: 71ebd71921e45 ("xen/9pfs: connect to the backend")
> Signed-off-by: Alex Zenla <alex@edera.dev>
> Signed-off-by: Alexander Merritt <alexander@edera.dev>
> Signed-off-by: Ariadne Conill <ariadne@ariadne.space>

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


Juergen

Re: [PATCH] 9p/xen: fix release of IRQ

Posted by Dominique Martinet 1 month ago

Alexander Merritt wrote on Thu, Nov 21, 2024 at 10:51:00PM +0000:
> From: Alex Zenla <alex@edera.dev>
> 
> Kernel logs indicate an IRQ was double-freed.

Nit: if you still have the log it'd be great to include it in the commit
message, rather than paragraphing it.

The rationale is that someone with the same problem will likely just
search for the error as is first, and having it in the commit log will
be an easy hit.

(This alone wouldn't need a resend, I can add it if you just reply to
the mail with it; it's also fine if you no longer have the log, that'll
be a remark for the next patch)


> 
> Pass correct device ID during IRQ release.
> 
> Fixes: 71ebd71921e45 ("xen/9pfs: connect to the backend")
> Signed-off-by: Alex Zenla <alex@edera.dev>
> Signed-off-by: Alexander Merritt <alexander@edera.dev>
> Signed-off-by: Ariadne Conill <ariadne@ariadne.space>



> ---
>  net/9p/trans_xen.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> index dfdbe1ca5338..198d46d79d84 100644
> --- a/net/9p/trans_xen.c
> +++ b/net/9p/trans_xen.c
> @@ -286,7 +286,8 @@ static void xen_9pfs_front_free(struct xen_9pfs_front_priv *priv)
>  		if (!priv->rings[i].intf)
>  			break;
>  		if (priv->rings[i].irq > 0)
> -			unbind_from_irqhandler(priv->rings[i].irq, priv->dev);
> +			unbind_from_irqhandler(priv->rings[i].irq, ring);
> +		priv->rings[i].evtchn = priv->rings[i].irq = 0;

(style) I don't recall seeing much `a = b = 0` in the kernel, and
looking at it checkpatch seems to complain:
CHECK: multiple assignments should be avoided
#114: FILE: net/9p/trans_xen.c:290:
+		priv->rings[i].evtchn = priv->rings[i].irq = 0;

Please run checkpatch on the patches you send (b4 can do it for you if
you want to start using it)


code-wise,
I also don't see where unbinf_from_irqhandler would free the evtchn, so
is it leaking here, or is it implicit from something else?
We only free it explicitly on error binding the irq.



Thanks,
-- 
Dominique Martinet | Asmadeus

Re: [PATCH] 9p/xen: fix release of IRQ

Posted by Jürgen Groß 1 month ago

On 22.11.24 00:44, Dominique Martinet wrote:
> Alexander Merritt wrote on Thu, Nov 21, 2024 at 10:51:00PM +0000:
>> From: Alex Zenla <alex@edera.dev>
>>
>> Kernel logs indicate an IRQ was double-freed.
> 
> Nit: if you still have the log it'd be great to include it in the commit
> message, rather than paragraphing it.
> 
> The rationale is that someone with the same problem will likely just
> search for the error as is first, and having it in the commit log will
> be an easy hit.
> 
> (This alone wouldn't need a resend, I can add it if you just reply to
> the mail with it; it's also fine if you no longer have the log, that'll
> be a remark for the next patch)
> 
> 
>>
>> Pass correct device ID during IRQ release.
>>
>> Fixes: 71ebd71921e45 ("xen/9pfs: connect to the backend")
>> Signed-off-by: Alex Zenla <alex@edera.dev>
>> Signed-off-by: Alexander Merritt <alexander@edera.dev>
>> Signed-off-by: Ariadne Conill <ariadne@ariadne.space>
> 
> 
> 
>> ---
>>   net/9p/trans_xen.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
>> index dfdbe1ca5338..198d46d79d84 100644
>> --- a/net/9p/trans_xen.c
>> +++ b/net/9p/trans_xen.c
>> @@ -286,7 +286,8 @@ static void xen_9pfs_front_free(struct xen_9pfs_front_priv *priv)
>>   		if (!priv->rings[i].intf)
>>   			break;
>>   		if (priv->rings[i].irq > 0)
>> -			unbind_from_irqhandler(priv->rings[i].irq, priv->dev);
>> +			unbind_from_irqhandler(priv->rings[i].irq, ring);
>> +		priv->rings[i].evtchn = priv->rings[i].irq = 0;
> 
> (style) I don't recall seeing much `a = b = 0` in the kernel, and
> looking at it checkpatch seems to complain:
> CHECK: multiple assignments should be avoided
> #114: FILE: net/9p/trans_xen.c:290:
> +		priv->rings[i].evtchn = priv->rings[i].irq = 0;
> 
> Please run checkpatch on the patches you send (b4 can do it for you if
> you want to start using it)
> 
> 
> code-wise,
> I also don't see where unbinf_from_irqhandler would free the evtchn, so
> is it leaking here, or is it implicit from something else?
> We only free it explicitly on error binding the irq.

unbind_from_irqhandler()
   unbind_from_irq()
     __unbind_from_irq()
       close_evtchn()


Juergen

Re: [PATCH] 9p/xen: fix release of IRQ

Posted by Dominique Martinet 1 month ago

Jürgen Groß wrote on Fri, Nov 22, 2024 at 02:54:06PM +0100:
> > (style) I don't recall seeing much `a = b = 0` in the kernel, and
> > looking at it checkpatch seems to complain:
> > CHECK: multiple assignments should be avoided
> > #114: FILE: net/9p/trans_xen.c:290:
> > +		priv->rings[i].evtchn = priv->rings[i].irq = 0;
> > 
> > Please run checkpatch on the patches you send (b4 can do it for you if
> > you want to start using it)
> > 
> > 
> > code-wise,
> > I also don't see where unbinf_from_irqhandler would free the evtchn, so
> > is it leaking here, or is it implicit from something else?
> > We only free it explicitly on error binding the irq.
> 
> unbind_from_irqhandler()
>   unbind_from_irq()
>     __unbind_from_irq()
>       close_evtchn()

Thank you, I didn't go far enough.

And also, bah; I just spent 30 minutes thinking why would setting irq to
zero prevent anything, but the bulk of the patch was using the correct
device for unbind (as the commit correctly says, I just saw double-free
and setting something to 0 after free as being related)
I'll just remove this darned line, as the free function can't walk a
ring twice anyway.


Also this made me notice xen_9pfs_front_init calls xen_9pfs_front_free()
on error, but that init is part of a front_changed call and I'd bet
xen_9pfs_front_remove() will still be called afterwards.
If init failure ought to free then it probably should unset drvdata
first like remove, and remove (and possibly many other dev_get_drvdata
calls) should check for null; otherwise it's probably best to leave it
to remove to call free exactly once...

-- 
Dominique Martinet | Asmadeus