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] [day] [month] [year] [list]

Message-ID: <Z0CWyXuMiifOv7HO@codewreck.org>
Date: Fri, 22 Nov 2024 23:35:53 +0900
From: Dominique Martinet <asmadeus@...ewreck.org>
To: Jürgen Groß <jgross@...e.com>
Cc: Alexander Merritt <alexander@...ra.dev>, 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>,
	Stefano Stabellini <sstabellini@...nel.org>,
	Alex Zenla <alex@...ra.dev>, Ariadne Conill <ariadne@...adne.space>
Subject: Re: [PATCH] 9p/xen: fix release of IRQ

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