linux-kernel - Re: [PATCH 3/8] drivers/pcmcia: remove unnecessary kzalloc

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: <20091121155135.GB13956@n2100.arm.linux.org.uk>
Date:	Sat, 21 Nov 2009 15:51:35 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Julia Lawall <julia@...u.dk>
Cc:	walter harms <wharms@....de>, linux-pcmcia@...ts.infradead.org,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 3/8] drivers/pcmcia: remove unnecessary kzalloc

On Sat, Nov 21, 2009 at 04:12:59PM +0100, Julia Lawall wrote:
> On Sat, 21 Nov 2009, walter harms wrote:
> 
> > 
> > 
> > Julia Lawall schrieb:
> > > From: Julia Lawall <julia@...u.dk>
> > > 
> > > The result of calling kzalloc is never used or freed.
> > > 
> > > The semantic match that finds this problem is as follows:
> > > (http://www.emn.fr/x-info/coccinelle/)
> > > 
> > > // <smpl>
> > > @r exists@
> > > local idexpression x;
> > > statement S;
> > > expression E;
> > > identifier f,f1,l;
> > > position p1,p2;
> > > expression *ptr != NULL;
> > > @@
> > > 
> > > x@p1 = \(kmalloc\|kzalloc\|kcalloc\)(...);
> > > ...
> > > if (x == NULL) S
> > > <... when != x
> > >      when != if (...) { <+...x...+> }
> > > (
> > > x->f1 = E
> > > |
> > >  (x->f1 == NULL || ...)
> > > |
> > >  f(...,x->f1,...)
> > > )
> > > ...>
> > > (
> > >  return \(0\|<+...x...+>\|ptr\);
> > > |
> > >  return@p2 ...;
> > > )
> > > 
> > > @script:python@
> > > p1 << r.p1;
> > > p2 << r.p2;
> > > @@
> > > 
> > > print "* file: %s kmalloc %s return %s" % (p1[0].file,p1[0].line,p2[0].line)
> > > // </smpl>
> > > 
> > > Signed-off-by: Julia Lawall <julia@...u.dk>
> > > ---
> > >  drivers/pcmcia/sa1111_generic.c     |    4 ----
> > >  1 files changed, 0 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/pcmcia/sa1111_generic.c b/drivers/pcmcia/sa1111_generic.c
> > > index deb5036..de6bc33 100644
> > > --- a/drivers/pcmcia/sa1111_generic.c
> > > +++ b/drivers/pcmcia/sa1111_generic.c
> > > @@ -127,10 +127,6 @@ int sa1111_pcmcia_add(struct sa1111_dev *dev, struct pcmcia_low_level *ops,
> > >  	ops->socket_state = sa1111_pcmcia_socket_state;
> > >  	ops->socket_suspend = sa1111_pcmcia_socket_suspend;
> > >  
> > > -	s = kzalloc(sizeof(*s) * ops->nr, GFP_KERNEL);
> > > -	if (!s)
> > > -		return -ENODEV;
> > > -
> > >  	for (i = 0; i < ops->nr; i++) {
> > >  		s = kzalloc(sizeof(*s), GFP_KERNEL);
> > >  		if (!s)
> > 
> > 
> > 
> > mmmh, perhaps the original idea was to have an array
> > and then he moved to a linked list ?
> > 
> > I thing the array idea is better (1. it fails on low mem propperly, 2. no need free()
> > 3. less zmalloc stuff) but requieres that pcmcia_remove()
> > will release that array propperly
> > 
> > the bug is strange,
> 
> Both kzallocs were added at the same time, when the function was added in 
> commit 701a5dc05ad99a06958b3f97cb69d99b47cebee3.  I have added the author 
> to the CC list.

Thanks for the additional info which allows me to track which patch it
corresponds with.  As an aside, it's really not nice to git pull and
then edit the commits afterwards - that's much worse than rebasing.
When trees are pulled, the act of merging it conveys sufficent "sign-off".

Just remove the first kzalloc and don't convert it to an array; that's the
safest option.  I don't remember if there's a reason why I switched to a
linked list - however, what I will say is that the way all the sa11xx
and pxa stuff interact is not plainly obvious.

There may be a corner case I found with the original patches which meant
a linked list was better than an array.
--
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/