From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965781AbdAKMAx (ORCPT ); Wed, 11 Jan 2017 07:00:53 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:57493 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964792AbdAKMAv (ORCPT ); Wed, 11 Jan 2017 07:00:51 -0500 Date: Wed, 11 Jan 2017 13:00:24 +0100 (CET) From: Thomas Gleixner To: Dave Jiang cc: mingo@redhat.com, hpa@zytor.com, keescook@chromium.org, bhe@redhat.com, linux-nvdimm@ml01.01.org, x86@kernel.org, david@fromorbit.com, linux-kernel@vger.kernel.org, dan.j.williams@intel.com Subject: Re: [PATCH v6] x86: fix kaslr and memmap collision In-Reply-To: <148407086864.34638.4228724339196415751.stgit@djiang5-desk3.ch.intel.com> Message-ID: References: <148407086864.34638.4228724339196415751.stgit@djiang5-desk3.ch.intel.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 10 Jan 2017, Dave Jiang wrote: > +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base); > +long simple_strtol(const char *cp, char **endp, unsigned int base); What are those functions for? They are not used in that patch at all. > +static void mem_avoid_memmap(void) > +{ > + char arg[128]; > + int rc = 0; What's the point of defining this variable here and zero initializing it? > + /* see if we have any memmap areas */ Sentences start with an upper case letter. Everywhere! > + if (cmdline_find_option("memmap", arg, sizeof(arg)) > 0) { You can spare an indentation level by just returning when the search fails. > + int i = 0; > + char *str = arg; > + > + while (str && (i < MAX_MEMMAP_REGIONS)) { for (i = 0; str && (i < MAX_MEMMAP_REGIONS); i++) { Please. > + unsigned long long start, size; > + char *k = strchr(str, ','); > + > + if (k) > + *k++ = 0; > + > + rc = parse_memmap(str, &start, &size); > + if (rc < 0) > + break; > + str = k; > + /* a usable region that should not be skipped */ > + if (size == 0) > + continue; > + > + mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start; > + mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size; So this makes no sense. You parse start/size as unsigned long long and then store it in an unsigned long. Works on 64bit, but on 32bit this is just wrong: Assume a memmap above 4G, which then will create a avoid region below 4G due to truncation to unsigned long. Thanks, tglx