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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zz_F9wMda68xhvKa@codewreck.org>
Date: Fri, 22 Nov 2024 08:44:55 +0900
From: Dominique Martinet <asmadeus@...ewreck.org>
To: Alexander Merritt <alexander@...ra.dev>
Cc: v9fs@...ts.linux.dev, linux-kernel@...r.kernel.org,
	xen-devel@...ts.xenproject.org,
	Eric Van Hensbergen <ericvh@...nel.org>,
	Latchesar Ionkov <lucho@...kov.net>,
	Christian Schoenebeck <linux_oss@...debyte.com>,
	Juergen Gross <jgross@...e.com>,
	Stefano Stabellini <sstabellini@...nel.org>,
	Alex Zenla <alex@...ra.dev>, Ariadne Conill <ariadne@...adne.space>
Subject: Re: [PATCH] 9p/xen: fix release of IRQ

Alexander Merritt wrote on Thu, Nov 21, 2024 at 10:51:00PM +0000:
> From: Alex Zenla <alex@...ra.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@...ra.dev>
> Signed-off-by: Alexander Merritt <alexander@...ra.dev>
> Signed-off-by: Ariadne Conill <ariadne@...adne.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