linux-kernel - Re: [v2] afs: Fix memory leak in afs_put

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: <1358845.1591037529@warthog.procyon.org.uk>
Date:   Mon, 01 Jun 2020 19:52:09 +0100
From:   David Howells <dhowells@...hat.com>
To:     Markus Elfring <Markus.Elfring@....de>
Cc:     dhowells@...hat.com, linux-afs@...ts.infradead.org,
        linux-fsdevel@...r.kernel.org,
        Zhihao Cheng <chengzhihao1@...wei.com>,
        Yi Zhang <yi.zhang@...wei.com>, linux-kernel@...r.kernel.org
Subject: Re: [v2] afs: Fix memory leak in afs_put_sysnames()

Markus Elfring <Markus.Elfring@....de> wrote:

> > 	Fix afs_put_sysnames() to actually free the specified afs_sysnames
> > 	object after its reference count has been decreased to zero and its
> > 	contents have been released.
> 
> * How do you think about to omit the word "Fix" because of the provided tag?

Quite often I might put introductory paragraphs before that, so I prefer to
begin the paragraph that states a fix with that verb.  There may also be
auxiliary changes associated with it that aren't directly fixes but need to be
made because of the fix change.

> * Is freeing and releasing an item a duplicate operation anyhow?

You're missing the point.  afs_put_sysnames() does release the things the
object points to (ie. the content), but not the object itself.

> >> Will it matter to mention the size of the data structure "afs_sysnames"?
> >
> > Why is it necessary to do so?
> 
> I suggest to express the impact of the missed function call "kfree".

I would hope that anyone reading the patch could work the impact out for
themselves.  Just specifying the size of a struct isn't all that useful - it
may be wildly variable by arch (eg. 32/64) and config option (eg. lockdep)
anyway.  Add to that rounding and packing details from the memory subsys,
along with the pinning effect of something you can't get rid of.

Of more use would be specifying the frequency or likelyhood of such a leak but
unless it's especially high, it's probably not worth mentioning.

David