linux-kernel - Re: [PATCH 3/3 v4] mm/vmalloc: Cache the vmalloc memory info

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: <20150824151114.18743.qmail@ns.horizon.com>
Date:	24 Aug 2015 11:11:14 -0400
From:	"George Spelvin" <linux@...izon.com>
To:	john@...ffel.org, mingo@...nel.org
Cc:	dave@...1.net, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	linux@...izon.com, linux@...musvillemoes.dk, peterz@...radead.org,
	riel@...hat.com, rientjes@...gle.com, torvalds@...ux-foundation.org
Subject: Re: [PATCH 3/3 v4] mm/vmalloc: Cache the vmalloc memory info

John Stoffel <john@...ffel.org> wrote:
>> vmap_info_gen should be initialized to 1 to force an initial
>> cache update.

> Blech, it should be initialized with a proper #define
> VMAP_CACHE_NEEDS_UPDATE 1, instead of more magic numbers.

Er... this is a joke, right?

First, this number is used exactly once, and it's not part of a collection
of similar numbers.  And the definition would be adjacent to the use.

We have easier ways of accomplishing that, called "comments".

Second, your proposed name is misleading.  "needs update" is defined
as vmap_info_gen != vmap_info_cache_gen.  There is no particular value
of either that has this meaning.

For example, initializing vmap_info_cache_gen to -1 would do just as well.
(I actually considered that before deciding that +1 was "simpler" than -1.)

For some versions of the code, an *arbitrary* difference is okay.
You could set one ot 0xDEADBEEF and the other to 0xFEEDFACE.

For other versions, the magnitude matters, but not *too* much.
Initializing it to 42 would be perfectly correct, but waste time doing
42 cache updates before settling down.

Singling out the value 1 as VMAP_CACHE_NEEDS_UPDATE is actively misleading.

> This will help keep bugs like this out in the future... I hope!

And this is the punchline, right?

The problem was not realizing that non-default initialization was required;
what we *call* the non-default value is irrelevant.

I doubt it would ever have been a real (i.e. noticeable) bug, actually;
the first bit of vmap activity in very early boot would have invalidated
the cache.

(John, my apologies if I went over the top and am contributing to LKML's
reputation for flaming.  I *did* actually laugh, and *do* think it's a
dumb idea, but my annoyance is really directed at unpleasant memories of
mindless application of coding style guidelines.  In this case, I suspect
you just posted before reading carefully enough to see the subtle logic.)
--
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/