| 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
| ||
|
Message-ID: <20140208150958.GK17045@pengutronix.de>
Date: Sat, 8 Feb 2014 16:09:58 +0100
From: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Yann Droneaud <ydroneaud@...eya.com>, linux-kernel@...r.kernel.org,
Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [PATCHv4] driver core/platform: don't leak memory allocated for
dma_mask
On Fri, Feb 07, 2014 at 03:20:05PM -0800, Greg Kroah-Hartman wrote:
> On Mon, Jan 27, 2014 at 11:05:52AM +0100, Yann Droneaud wrote:
> > Since commit 01dcc60a7cb8, platform_device_register_full() is
> > available to allocate and register a platform device.
> >
> > If a dma_mask is provided as part of platform_device_info,
> > platform_device_register_full() allocate memory for a u64 using
> > kmalloc().
> >
> > A comment in the code state that "[t]his memory isn't freed when the
> > device is put".
> >
> > It's never a good thing to leak memory, but there's only very few
> > users of platform_device_info's dma_mask, and those are mostly
> > "static" devices that are not going to be plugged/unplugged often.
> >
> > So memory leak is not really an issue, but allocating 8 bytes through
> > kmalloc() seems overkill.
> >
> > And it's a pity to have to allocate 8 bytes for the dma_mask while up
> > to 7 bytes are wasted at the end of struct platform_object in the form
> > of padding after name field: unfortunately this padding is not used
> > when allocating the memory to hold the name.
> >
> > To address theses issues, this patch adds dma_mask to platform_object
> > struct so that it is always allocated for all struct platform_device
> > allocated through platform_device_alloc() or platform_device_register_full().
> > And since it's within struct platform_object, dma_mask won't be leaked
> > anymore when struct platform_device got released. Storage for dma_mask
> > is added almost for free by removing the padding at the end of struct
> > platform_object.
How does the padding of name change? The only thing that I see changing
for .name is that it's a char[] now instead of a char[1]. As it was
used as a flexible array already before the padding (which only applies
to a stand alone struct platform_object) doesn't matter.
I guess that is a tool problem that still some padding changes are
reported?
Other than that the patch looks good.
Uwe
> > Padding at the end of the structure is removed by making name a C99
> > flexible array member (eg. a zero sized array). To handle this change,
> > memory allocation is updated to take care of allocating an additional
> > byte for the NUL terminating character.
> >
> > No more memory leak, no small allocation, no byte wasted and
> > a slight reduction in code size.
> >
> > Built on Fedora 20, using GCC 4.8, for ARM, i386, x86_64 and PPC64
> > architectures, the size of the object file and the data structure layout
> > are updated as follow, produced with commands:
> >
> > $ size drivers/base/platform.o
> > $ pahole drivers/base/platform.o \
> > --recursive \
> > --class_name device,pdev_archdata,platform_device,platform_object
> >
> > --- size+pahole.next-20140124
> > +++ size+pahole.next-20140124+
> > @@ -1,5 +1,5 @@
> > text data bss dec hex filename
> > - 5616 472 32 6120 17e8 obj-arm/drivers/base/platform.o
> > + 5572 472 32 6076 17bc obj-arm/drivers/base/platform.o
> > struct device {
> > struct device * parent; /* 0 4 */
> > struct device_private * p; /* 4 4 */
> > @@ -77,15 +77,15 @@ struct platform_object {
> > /* XXX last struct has 4 bytes of padding */
> >
> > /* --- cacheline 6 boundary (384 bytes) --- */
> > - char name[1]; /* 384 1 */
> > + u64 dma_mask; /* 384 8 */
> > + char name[0]; /* 392 0 */
> >
> > - /* size: 392, cachelines: 7, members: 2 */
> > - /* padding: 7 */
> > + /* size: 392, cachelines: 7, members: 3 */
> > /* paddings: 1, sum paddings: 4 */
> > /* last cacheline: 8 bytes */
> > };
> >
> > text data bss dec hex filename
> > - 6007 532 32 6571 19ab obj-i386/drivers/base/platform.o
> > + 5943 532 32 6507 196b obj-i386/drivers/base/platform.o
> > struct device {
> > struct device * parent; /* 0 4 */
> > struct device_private * p; /* 4 4 */
> > @@ -161,14 +161,14 @@ struct platform_device {
> > struct platform_object {
> > struct platform_device pdev; /* 0 392 */
> > /* --- cacheline 6 boundary (384 bytes) was 8 bytes ago --- */
> > - char name[1]; /* 392 1 */
> > + u64 dma_mask; /* 392 8 */
> > + char name[0]; /* 400 0 */
> >
> > - /* size: 396, cachelines: 7, members: 2 */
> > - /* padding: 3 */
> > - /* last cacheline: 12 bytes */
> > + /* size: 400, cachelines: 7, members: 3 */
> > + /* last cacheline: 16 bytes */
> > };
> >
> > text data bss dec hex filename
> > - 8851 2112 48 11011 2b03 obj-ppc64/drivers/base/platform.o
> > + 8787 2104 48 10939 2abb obj-ppc64/drivers/base/platform.o
> > struct device {
> > struct device * parent; /* 0 8 */
> > struct device_private * p; /* 8 8 */
> > @@ -256,14 +256,14 @@ struct platform_device {
> > struct platform_object {
> > struct platform_device pdev; /* 0 728 */
> > /* --- cacheline 11 boundary (704 bytes) was 24 bytes ago --- */
> > - char name[1]; /* 728 1 */
> > + u64 dma_mask; /* 728 8 */
> > + char name[0]; /* 736 0 */
> >
> > - /* size: 736, cachelines: 12, members: 2 */
> > - /* padding: 7 */
> > + /* size: 736, cachelines: 12, members: 3 */
> > /* last cacheline: 32 bytes */
> > };
> >
> > text data bss dec hex filename
> > - 7100 960 48 8108 1fac obj-x86_64/drivers/base/platform.o
> > + 7020 960 48 8028 1f5c obj-x86_64/drivers/base/platform.o
> > struct device {
> > struct device * parent; /* 0 8 */
> > struct device_private * p; /* 8 8 */
> > @@ -350,9 +350,9 @@ struct platform_device {
> > struct platform_object {
> > struct platform_device pdev; /* 0 712 */
> > /* --- cacheline 11 boundary (704 bytes) was 8 bytes ago --- */
> > - char name[1]; /* 712 1 */
> > + u64 dma_mask; /* 712 8 */
> > + char name[0]; /* 720 0 */
> >
> > - /* size: 720, cachelines: 12, members: 2 */
> > - /* padding: 7 */
> > + /* size: 720, cachelines: 12, members: 3 */
> > /* last cacheline: 16 bytes */
> > };
> >
> > Changes from v3 [1]:
> > - fixed commit message so that git am doesn't fail.
> >
> > Changes from v2 [2]:
> > - move 'dma_mask' to platform_object so that it's always
> > allocated and won't leak on release; remove all previously
> > added support functions.
> > - use C99 flexible array member for 'name' to remove padding
> > at the end of platform_object.
> >
> > Changes from v1 [3]:
> > - remove unneeded kfree() from error path
> > - add reference to author/commit adding allocation of dmamask
> >
> > Changes from v0 [4]:
> > - small rewrite to squeeze the patch to a bare minimal
> >
> > [1] http://lkml.kernel.org/r/1390771138-28348-1-git-send-email-ydroneaud@opteya.com
> > https://patchwork.kernel.org/patch/3540081/
> >
> > [2] http://lkml.kernel.org/r/1389683909-17495-1-git-send-email-ydroneaud@opteya.com
> > https://patchwork.kernel.org/patch/3484411/
> >
> > [3] http://lkml.kernel.org/r/1389649085-7365-1-git-send-email-ydroneaud@opteya.com
> > https://patchwork.kernel.org/patch/3480961/
> >
> > [4] http://lkml.kernel.org/r/1386886207-2735-1-git-send-email-ydroneaud@opteya.com
> >
> > Cc: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> > Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Signed-off-by: Yann Droneaud <ydroneaud@...eya.com>
> > ---
> > Hi,
> >
> > I've fixed the commit message to move the diff on pahole
> > outside of the scope of git am.
>
> Uwe, can I get your review of this?
>
> thanks,
>
> greg k-h
>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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/
Powered by blists - more mailing lists