linux-kernel - Re: Freeing of dev->p

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: <20140110152402.GC22533@kroah.com>
Date:	Fri, 10 Jan 2014 07:24:02 -0800
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Jean Delvare <khali@...ux-fr.org>
Cc:	linux-kernel@...r.kernel.org, Peter Wu <lekensteyn@...il.com>
Subject: Re: Freeing of dev->p

On Fri, Jan 10, 2014 at 03:39:07PM +0100, Jean Delvare wrote:
> + * @driver_data: Private pointer for driver specific info.  Will turn into a
> + *		list soon.

Ah, this comment reminds me of why I originally did this.  I was working
on moving for a way to have multiple drivers bound to the same device,
as people needed that type of thing for something that I can't remember
at the moment.

As it's been years now with no real movement forward on that idea, I
guess it's not going to happen :)

>   * @power:	For device power management.
>   * 		See Documentation/power/devices.txt for details.
>   * @pm_domain:	Provide callbacks that are executed during system suspend,
> @@ -737,6 +739,8 @@ struct device {
>  					   device */
>  	void		*platform_data;	/* Platform specific data, device
>  					   core doesn't touch it */
> +	void		*driver_data;	/* Driver data, set and get with
> +					   dev_set/get_drvdata */
>  	struct dev_pm_info	power;
>  	struct dev_pm_domain	*pm_domain;
>  
> 
> For performance I'd even question the point of the dev check in
> dev_get_drvdata(), especially when there is no such check in
> dev_set_drvdata() which presumably is always called first.

It's nice to not oops if a NULL pointer is passed in :)

> Plus dev_set_drvdata() can no longer fail (something only 3 drivers in
> the whole kernel tree were checking for anyway) so it could return
> void instead of int.

True.

> Then I suppose we could inline both functions
> again, for performance. Well, put in short, really revering
> b4028437876866aba4747a655ede00f892089e14 would be the way to go IMHO.

Almost, the copyright lines should stay :)

> Really, while I understand your envy to protect driver core internals
> from unwanted access, the cost here was simply too high IMHO, both in
> terms of getting things right and performance. Some drivers are calling
> dev_get_drvdata() directly or indirectly repeatedly at run-time. They
> had no reason not to as this used to be so fast, and now it is no
> longer an inline function, it has conditionals and a double pointer
> indirection...
> 
> Plus, I can't think of anything really bad that could result from
> accessing driver_data directly, contrary to the other members of struct
> device_private.

See first response above for why I did this, it wasn't to just make
things "harder" to mess up, I actually had a reason to do it (imagine
that!)

Thanks for the detailed response, I think I'll just revert most of that
patch and see if it's still workable.

greg k-h
--
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/