linux-kernel - Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

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]

Date:	Fri, 4 Oct 2013 11:05:37 -0600
From:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>
To:	Michal Simek <monstr@...str.eu>
Cc:	Michal Simek <michal.simek@...inx.com>,
	linux-kernel@...r.kernel.org, Alan Tull <atull@...era.com>,
	Pavel Machek <pavel@....cz>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Dinh Nguyen <dinguyen@...era.com>,
	Philip Balister <philip@...ister.org>,
	Alessandro Rubini <rubini@...dd.com>,
	Steffen Trumtrar <s.trumtrar@...gutronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Jason Cooper <jason@...edaemon.net>,
	Yves Vandervennet <rocket.yvanderv@...il.com>,
	Kyle Teske <kyle.teske@...com>,
	Josh Cartwright <joshc@....teric.us>,
	Rob Landley <rob@...dley.net>,
	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Cesar Eduardo Barros <cesarb@...arb.net>,
	Joe Perches <joe@...ches.com>,
	"David S. Miller" <davem@...emloft.net>,
	David Brown <davidb@...eaurora.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Nicolas Pitre <nico@...aro.org>,
	Mark Langsdorf <mark.langsdorf@...xeda.com>,
	Felipe Balbi <balbi@...com>, linux-doc@...r.kernel.org
Subject: Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

On Fri, Oct 04, 2013 at 06:28:23PM +0200, Michal Simek wrote:
> > I strongly encourage you to use text strings to indicate the state of
> > the configuration FSM, and I *really* think you should rework things
> > to have an explicit configuration FSM rather than trying to bodge one
> > together with a bunch of bit flags.
> 
> Any favourite names for states?
> Or ready, write_init, write_complete is enough for now?

Doesnt really matter to me, don't forget error states. Transisionts to
ready, write_init and write_complete can all fail.

> > I wonder if this is right, it seems like a strange way to make a class
> > subsystem, usually the struct fpga_manager would contain the 'struct
> > device' (not a pointer, so you can use container_of) and drvdata would
> > be reserved for something else.
> 
> I am not following you here. mrg structure is connected with the driver
> it means when driver is removed then this structure is freed.

You've got your mgr structure and then later you allocate a struct
device as well, the mgr can be free'd before the struct device is
released, due to the way ref counting works. You are not doing
anything to compensate for that.

> > This seems to create lifetime issues since the devm above will be
> > free'd when the platform driver is released, but the class device will
> > live on with the stray pointer. Better to allocate everything against
> > the class device below.
> 
> device in unregistered before this structure is freed.
> fpga_mgr_unregister() is called in the platform driver in remove function.

Which is the problem, device_unregister dosen't delete 'mgr->dev', and it
doesn't mean the sysfs call backs are uncallable, so you have a free'd
dangling pointer in mgr->dev, and an object lifetime issue.

> > What happens when userspace is holding one of the sysfs files open and
> > you unload the module? Looks like bad things?
> 
> I didn't test this but feel free to check it.

You should fix these problems before your driver reaches Linus.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/