linux-kernel - Re: [PATCH 5/5] OF: Introduce utility helper functions

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: <20131115062729.09E4FC4073F@trevor.secretlab.ca>
Date:	Fri, 15 Nov 2013 15:27:29 +0900
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Pantelis Antoniou <panto@...oniou-consulting.com>
Cc:	Rob Herring <robherring2@...il.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Matt Porter <matt.porter@...aro.org>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Alison Chaiken <Alison_Chaiken@...tor.com>,
	Dinh Nguyen <dinh.linux@...il.com>,
	Jan Lubbe <jluebbe@...net.de>,
	Alexander Sverdlin <alexander.sverdlin@....com>,
	Michael Stickel <ms@...able.de>,
	Guenter Roeck <linux@...ck-us.net>,
	Dirk Behme <dirk.behme@...il.com>,
	Alan Tull <delicious.quinoa@...il.com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Michael Bohan <mbohan@...eaurora.org>,
	Ionut Nicu <ioan.nicu.ext@....com>,
	Michal Simek <monstr@...str.eu>,
	Matt Ranostay <mranostay@...il.com>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] OF: Introduce utility helper functions

On Thu, 14 Nov 2013 10:51:05 +0100, Pantelis Antoniou <panto@...oniou-consulting.com> wrote:
> Hi Grant,
> 
> On Nov 14, 2013, at 2:44 AM, Grant Likely wrote:
> 
> > On Wed, 13 Nov 2013 10:03:37 +0100, Pantelis Antoniou <panto@...oniou-consulting.com> wrote:
> >> On Nov 13, 2013, at 2:34 AM, Grant Likely wrote:
> >>> On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@...oniou-consulting.com> wrote:
> >>>>> On Tue,  5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@...oniou-consulting.com> wrote:
> >>>>>> +	} else {
> >>>>>> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
> >>>>>> +				__func__, node);
> >>>>>> +	}
> >>>>>> +}
> >>>>> 
> >>>>> All of the above is potentially dangerous. There is no way to determine
> >>>>> if anything still holds a reference to a node. The proper way to handle
> >>>>> removal of properties is to have a release method when the last
> >>>>> of_node_put is called.
> >>>>> 
> >>>> 
> >>>> This is safe, and expected to be called only on a dynamically created tree,
> >>>> that's what all the checks against OF_DYNAMIC guard against.
> >>>> 
> >>>> It is not ever meant to be called on an arbitrary tree, created by unflattening
> >>>> a blob.
> >>> 
> >>> I am talking about when being used on a dynamic tree. The problem is
> >>> when a driver or other code holds a reference to a dynamic nodes, but
> >>> doesn't release it correctly. The memory must not be freed until all of
> >>> the references are relased. OF_DYNAMIC doesn't actually help in that
> >>> case, and it is the reason for of_node_get()/of_node_put()
> >>> 
> >> 
> >> I know, but even that is not enough. of_node_get()/of_node_put() handles the
> >> case of references to the nodes, but not what happens with references to
> >> properties. deadprops is mitigating the problem somewhat, but if we're going
> >> to go to all the trouble of kobjectification let's do the props as well.
> >> 
> >> of_get_property could be modified to return a devm_kmalloced copy of the real
> >> property and that would deal with most of the callers. Of course for
> >> the small sized scalar data we can avoid the copy.
> >> 
> >> By using the devm_* interface we also avoid having to mess too much with the callers.
> >> 
> >> I.e. what about something like devm_of_get_property()?
> > 
> > Reference counting is already a horrible pain to keep correct. I don't
> > see a better way to handle it in the dynamic case, so we're stuck with
> > it, but I don't want to make it any harder. Adding ref counting to
> > properties will make it harder than it already is to get the code right.
> > I'm absolutely fine with a little bit of wasted memory in the form of
> > deadprops when the alternative is so horrible. References at the node
> > level is enough granularity.
> > 
> > I don't think kduping the property is the solution either. I strongly
> > suspect that will be far more expensive than the deadprop solution.
> > 
> 
> As long as we can live with deadprops all is fine. Perhaps a devm_of_get_property()
> makes sense for new drivers though? What do you think? Perhaps copying to a
> user supplied buffer as well?

I still don't think it is necessary. The device lifetime should always
be shorter than the node lifetime.

> It's a kind of drag. That means you get handed a device_node pointer you are not
> able to free it without having the blob along with the container/accessor of it.
> I.e. For the normal case where the blob comes from a request_firmware() call
> You have to keep the firmware structure around.
> 
> Depending on what other method you're going to use tends to make the code a little
> bit messier. 

Understood. Stick with keeping the blob around for now. It can be
reworkd in the future if necessary since there are no associated
userspace ABI issues.

g.
--
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/