From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753145AbdBMNHA (ORCPT ); Mon, 13 Feb 2017 08:07:00 -0500 Received: from mail-eopbgr20128.outbound.protection.outlook.com ([40.107.2.128]:47795 "EHLO EUR02-VE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752787AbdBMNGz (ORCPT ); Mon, 13 Feb 2017 08:06:55 -0500 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=dsafonov@virtuozzo.com; Subject: Re: [PATCHv4 2/5] x86/mm: introduce mmap{,_legacy}_base To: Thomas Gleixner References: <20170130120432.6716-1-dsafonov@virtuozzo.com> <20170130120432.6716-3-dsafonov@virtuozzo.com> CC: , <0x7f454c46@gmail.com>, Ingo Molnar , "H. Peter Anvin" , Andy Lutomirski , Borislav Petkov , , From: Dmitry Safonov Message-ID: <696975bb-8846-24b8-e166-b2569d49cec8@virtuozzo.com> Date: Mon, 13 Feb 2017 16:02:57 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [195.214.232.6] X-ClientProxiedBy: HE1PR09CA0063.eurprd09.prod.outlook.com (10.174.50.31) To DB6PR0801MB1735.eurprd08.prod.outlook.com (10.169.226.150) X-MS-Office365-Filtering-Correlation-Id: 2c5e0da9-326d-4687-cca6-08d454112720 X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001);SRVR:DB6PR0801MB1735; X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1735;3:ClgfXCrCt5fE+1zEqFbzHrWqcC7nduZxdasTXMaXzXtoqLQhwXxNZH/YsTpo2XOmHr/mu5iKNcMETlfSFPkhPpLpih1lFkp4oz8rhvoPgQ4kFqFyDWGRKBO4cpjMW98JK2oj1Qr4hv80r7YjehpzJCREcWWoRsDsti+mtYHeiUIhd9qAWGoQYp8gYnxKrMKcrwb4wd6AOmgxiFJdl1dq/4+xITb8nONRfREXf7Fcs2WDI4mfHYOfY3o/iz4mrEO1QS27aDkUTPjfJz5i5KIDOQ==;25:CbrrndIAMMpetQz0tScpubrDWJAK+7Xi4g9xy21ysrcwepHjnZi8s0rwFbbT9qbEhZrd3lUxjagI8H0JuHYeAsYIpMWPJhJFvxE6b55O5NSTYFaMxmZHeL8LCrCmbdk6eru9oqqKvjBvy31kzgLvF1enb9ZPjBJ6Vp/ZxlLj86xmqCT/qbY/cD4dgw7NAAYwC3sR4nlSoxwWM0PKX4cUvkAR59JZZEza9A2tW+YwYjHiJmvbnRzInDqGlhC/xNQXMXJlbGF7h72Gpyce9hyWYYAPFEzjgDnvU+l/2dbF8T643pDbTA09U89oLd2uzmpPWBY2CYL2js4izhdQnpftF7hwHZH+wNXZ4D5+WZiM+bR/1gE4hyWGNzYYimG3Ep4qiD37FvpM/8je/rKvDKAdeBZRDcUk+eEABt+E6BpA6yAkZmh/0ffbdJBSyPttj2lDj5jn88BD9ydGGZPRVpynxg== X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1735;31:u3DVFBF1PgWT9YjLvAUZA7G6ncsZtHYUvzEpNUq1BEp/UPmjGpoCNC3AqIh8AuCUYAgeHwI5j6hfAtthQOX+wGo/MDN2XxvKtns9MS4ejftwT5vgx/nbcQGlRH2mjIBdGgULSTKe/JEdRiBE2pmi/XXl634x1ZtD+UQ8FFDSqgrPdPWEbW9Ff0nN6RJNbWSnxJ1bB/bLzGNXiRY+qUgqMoHP5uZFIVuoBrvRhsNLYIDarZZXAQKtluoLcWvNHkgW;20:r5pWE/wW3Eq+tAkBbNVsOcNrovD9VDDvBEzg/TEwMMZJ5y0nRqge23tyaUdLmo1qaXrIHzgMnwyRlECkeh9rPe6ARkUi5NcC7wbPa+NTzmAyc1a2bIs4M0F30X5aoN3sKiKe2kIC+9akJeQCaqdiHBHdXItaeyJQ+QhMc/80BmTjzXyYWsMYH5dVDN8fOUBrL5PwUabc/esDR3nqS2+Z1oapRz6bVRzlT9em0NRVVBjLPUQ9u84LutSHoAGdn3+0ZSi6IU9ev62mrLLIJ3oJy5mVR3259yiZh9Ph/9cwcEys8o3ZbkVckxHz6/TIN3Zk8+Dc67s4fCg/5/xL4Ibz1iYL2JYjJCVMnERIueyiNrO6CkzW386oDnZ4xog0HMf+r4S5bi+FSOvAOyRV6VuZl79UrNc8q9347TfQQ3Zfm6s= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040375)(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046)(6041248)(20161123555025)(20161123558025)(20161123564025)(20161123562025)(20161123560025)(6072148);SRVR:DB6PR0801MB1735;BCL:0;PCL:0;RULEID:;SRVR:DB6PR0801MB1735; X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1735;4:RTsh2IRlz50o2n9jmmLu9UhVOJ7TcWaJD0HcbFQlDY5xUIyLrfzAs4BSEtxlrDKJKPGHNu8oNcmrc6ri8P4pOQ27f6Zoi7jzBqf8PN2ZVbhRquPDyShLVziXmrtGZ38uaWnR9wT6TkRdlrYewFLjhyOQ5G5v5RBBn28bCTDRc59aJU4jIvXsdRExZbgw+RBtAaXIVk/i9W32kryzM17cGUUTh9MyNjw0nJvAzpZ/9+Z/ZultI2U+1zS35+2JiI6AaXtehz5W0+C2msU0eC/oSw0QFm8xP4uAsLWkUeXA7qLlh1/sZj0U8/IBrQK7ICTmb2uVlIXTom5EQO9LqfJMBSwzXsli+tq7hAwwAc93VM2FBWi272iw/EQ9c7hIjiHUEsKIUXUjrs7aT40e89DlLCd39FWMr6LkxuK6fCcPN/8+0rIWV2dhv08KjvImhOLqifNxI6mXHe+o6AfSxgEmYQNH4AEZqHpZQ8vnFkweq5QFxWnMvZV9ycrQmqG602Y5/fHN/VFDywE/8NEeGAk3QP0MaXAGltoZjWaIZFx4ZTAtFKFKN7f7yvTFRtuBOhBhhhVwNRCjk+gyR2NtGj2s+FQgQMi/CBgBZe7JiejVoRM= X-Forefront-PRVS: 02176E2458 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6009001)(6049001)(7916002)(39830400002)(39410400002)(39450400003)(24454002)(377454003)(43784003)(199003)(189002)(54534003)(8676002)(42186005)(25786008)(53546003)(6246003)(31686004)(6916009)(101416001)(81166006)(81156014)(230700001)(6666003)(2950100002)(229853002)(33646002)(106356001)(54356999)(76176999)(50986999)(105586002)(90366009)(5660300001)(6486002)(77096006)(68736007)(64126003)(54906002)(39060400001)(53936002)(65826007)(31696002)(97736004)(65956001)(47776003)(4326007)(65806001)(66066001)(36756003)(110136004)(38730400002)(83506001)(189998001)(2906002)(4001350100001)(6116002)(3846002)(86362001)(92566002)(305945005)(23746002)(50466002)(7736002);DIR:OUT;SFP:1102;SCL:1;SRVR:DB6PR0801MB1735;H:[172.16.25.13];FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?Windows-1252?Q?1;DB6PR0801MB1735;23:dkvIwo/ZOJqOWYmuGhB0fiDDbZEvP9iR/xU?= =?Windows-1252?Q?SjatxKhuX9899aVTD1cd3sKV4AdwIO3F8ESwiNo2LmSaPavCcIdoEUJM?= =?Windows-1252?Q?bw0Hq3kpLgeeh0QXR56eIyXdvEowFtaygeyxirdW8fHQiCN8HZpS5y2k?= =?Windows-1252?Q?iTAH4FFwv/4Hxb5kizDPvEbXnZylSH+FmeGuJt7FYdXPyPI+58D3jurJ?= =?Windows-1252?Q?DTRRmuEYDM4a/5d4QO+bOvupfQiEcCSFGU0EtjYIkZe5Pj1Sh1yZGTld?= =?Windows-1252?Q?wleRMKUQLeDbgkKxURhzbV8obXIUfO7Wbu/i5bw8SqcCuYtzrtJA9cng?= =?Windows-1252?Q?gAn1OEXWDAQURl/VO5GOBU4A/uwUOMfkg2QNSh2NEgW0yL+aR6wAm5k2?= =?Windows-1252?Q?UuOQcL85TK27DFyPkIuWoja8KGCq85gMqJZSN8Fw1heJ06YE9is/ML4l?= =?Windows-1252?Q?EnpHM+cNlIB0ZtWDACvhln5o73T6zE2652F1H7go76Flst+X5gSxhcVz?= =?Windows-1252?Q?CibxViPwA5mFbJEWN269styfKTlhbDdXdfSPpBvPSd89qyNMrp07hO/+?= =?Windows-1252?Q?n28nNIDdeJeNl+fSY2qgedUkQCDRkg2q03H9J3yr8MdMm1z+YX9542RK?= =?Windows-1252?Q?cB+WqXeF9L+zAMsYIrPlvpnvhQgOGrAyziLn4KvmqoYP+t9qQ83hn4mK?= =?Windows-1252?Q?x+L8pBU8bMCLubhC0AMiefvdp6cnus3x8EerknwfLmv/r70T7hIMszKg?= =?Windows-1252?Q?DyCQM3uTtBb1waaeogASlub80vLTLgwDX02MEwVcpLDovG2k+OPV4eUj?= =?Windows-1252?Q?vkaYc/Pf+zqYvRgClLonTOQcc+8+gp4FhgwcEiImXI+6GLeq8qSE+GRZ?= =?Windows-1252?Q?d1iysCf7Q/a/t01+1DXKrBM5fMGs8jrda3yCxAlUttoVSBtU/97Hq9QI?= =?Windows-1252?Q?/CGr98LDMs9EUuU09eXZ+W7qxMrSsb5mlXcddacN8eElKMVywSeMKLkw?= =?Windows-1252?Q?r2AbUgIEo3n8UxoNLcpfZ8f5O7kYSw9BlHY1PO0KXgHEWkBWR61/nrEz?= =?Windows-1252?Q?HkgKjtGrVeXhmDNBvstB40COWMegByh8hXg6uHMP50krdvF+WtgYlhiP?= =?Windows-1252?Q?W5S1ZpGOCsRIdY5tUft8R+Nip9hrqn6RT3sTTTusRjI6pLjmfjV1F2H0?= =?Windows-1252?Q?O/XHFWPSQknS9BiSy0kWiYiEC6dBLoqsOqJmhUV2Crt5e6ZC7vRulrgZ?= =?Windows-1252?Q?8BdMgOsBdy3DR6sDAfzPxsH586a0ev0HeIChrivGYxEV5JGfSwnvZO8x?= =?Windows-1252?Q?noZFs7RrpjfNqhJo1AogR0/rBKAZ/UJoXwOzykwnEEhzjFsJP04A+D8S?= =?Windows-1252?Q?xBR8FssafLPL8K8D7aF/b26i/1cjHMQ5kvUhAi+jy0teegd8EgJa+sLI?= =?Windows-1252?Q?8aOrL5pmHTs6ap1ESooRfj8z+2sRT/GP3+7XrFoowlKNurCm5HX9G68c?= =?Windows-1252?Q?CtW71/FtzUE6/m1D5VAybQL1oI+NvLdbBQQBg6uwmOYJgl+1ofyZQ3j0?= =?Windows-1252?Q?Pt+sw+hJ+clR7MBR6uwKnkTdYqCbNdtNdl3G2RrGmKdHsp1bE8hwJ+qj?= =?Windows-1252?Q?TvjYJ+MZjULLyhyIcZmNtIQIvwpA65lr8FQq9jeZYLMmg5VZNJtYtQke?= =?Windows-1252?Q?XzBpv3zoSzQ=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1735;6:FP20JdNuP08kK/iKrPvTkIADglSdJT2Mg5c0eSVDEq8F6myJRS1xiS1IlmbrW5v5D4aoS2brOW7HMc32A7ls5CW6q84iMFW5aoYTzWIPOXtNVYd2fS10VHr8lRicWHQ857c70qUM9lFOVp5JpoycHOecqSOYTzPV+/dxLDdyVUlGA43TEoolWLJbjD9E8qybRkC49IsFkKRgVZ7AnaaCd6OEl9+eLx/MfMa0vOS5ox7MkAhC83zMg3l0YwaeZX6XVmjVmHn7Gra37apUTY8Q+le7+yzYMRHJcFPmdB5XUg7YJYhPOiZ8CHyCRNtNd5CkSUixZB/gnkCILbtbDn672AxifQrHF5PghMenerib4djS9modNoWE1vpeYtKiXA0Kz/2lfNGcLohNSmAYMZkOjw==;5:WS8WaPsW9JjeLLIOJ7t06baBQtkM3OZPiK8JRbyDSG63oEuT8EyKeNRNQmP3j4zcfrGrLmn2119hd+7G8UFCJdigrgGaYMlRpofO1Bcae59Ma3fR6PV2SI6rw6V7Sg7iSiS67dNGXfZhkNw+kVbegHjg6XIiV6/YRit8EwvULZ4=;24:WiKTxOpEUWaZjzk0qwGqpX5VvgHn2W+TcM79/2IJa0LWgY+NLxBhrpHxq0UEgIPo8bz5lLZ/JXu2QA9aCF5Hb3VOFwNFYCRVQ1FckU5SxEg= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;DB6PR0801MB1735;7:uS0/tCT3MKirvaewD4FqiTLfMOCaI1FeW5KIVgQwysIGv6nW2IwZgvmRN8J3L58Fx568mPauZBFEuugZ8ussyMi1VxChFuA4z/g2cLOSuJ8ZyACkhf7uJY4kaOYDWvqPQ0SX6EnKzs+duPsEJniRWKWDYw00/xdclvZzSfV/CtnddsC6sEOffBUFt6gGHLv0z3p0PHyU07dLCPjNtHrvKBw+TvpHXnN559Tlfo8Z1GYPYfdb/bJgTwL+aHkywoIim/Lk857T7Sm615XSk5Ze3tXu7Mf/h/iC3G1cIbWOImUKJjVyVaopWazzDiph0kvWOqiMKSElunBDC3vglncPEQpOpsRgMlfqq/1oFyVepcFMSQfcZoHrQl1Bzn6HO00nDRJQzTGq8gU6BYq9sX7LvHe9X7Lzk8mCV06QiiS6YBVJnXRyna3rrh0+vdMU+Wq7278ezxJWCoC3bAXGhAme1TdmivdS8p65fJTgulqsjDgK//4FhHA8+Fo21+LJuSJromluxIi1/UuvD/oZn7KJ8Q==;20:N23SjJWho67wHgv+4Ll2eYq47N5zambDhXJgM4s6PNNBYNxgwWABFXqiZSN97dTkjPq97//5tyEf6xAaQcGyXQzVBS8h2fl7F4qrFxjsGSLJOG6AlP4cvvxoamKmUmIW2R7tvi/XDbtXlgy/+uH9qyvv7PW7FmqRfJeHHb/sruDxTXNri6bx5aSQckB9QkfkAosKdqMBmo9hmipObhviEzPO2ZzKs9NWPMHEK6dkJndKPhgl8j64bLE9C8qvVjRG X-OriginatorOrg: virtuozzo.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Feb 2017 13:06:40.1940 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0801MB1735 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/11/2017 05:13 PM, Thomas Gleixner wrote: > On Mon, 30 Jan 2017, Dmitry Safonov wrote: > >> In the following patch they will be used to compute: >> - mmap{,_legacy}_base for 64-bit mmap() >> - mmap_compat{,_legacy}_base for 32-bit mmap() >> >> This patch makes it possible to calculate mmap bases for any specified >> task_size, which is needed to correctly choose the base address for mmap >> in 32-bit syscalls and 64-bit syscalls. > > Please rework your changelogs so they follow the requirements in > Documentation.... Look for 'this patch' there. Sure, I'll rewrite the changelogs. > > Also order it in a way which makes it clear what the problem is and how > it's solved. > >> +#define STACK_RND_MASK_MODE(native) (0x7ff) >> #define STACK_RND_MASK (0x7ff) >> >> #define ARCH_DLINFO ARCH_DLINFO_IA32 >> @@ -295,7 +296,8 @@ do { \ >> #else /* CONFIG_X86_32 */ >> >> /* 1GB for 64bit, 8MB for 32bit */ >> -#define STACK_RND_MASK (test_thread_flag(TIF_ADDR32) ? 0x7ff : 0x3fffff) >> +#define STACK_RND_MASK_MODE(native) ((native) ? 0x3fffff : 0x7ff) >> +#define STACK_RND_MASK STACK_RND_MASK_MODE(!test_thread_flag(TIF_ADDR32)) > > I had to look twice what MODE means. The macro name suggests that it > returns the mode, while it actually returns the mask. That's confusing at > best. And 'native' is not intuitive either. > > #define __STACK_RND_MASK(is64bit) ((is64bit) ? 0x3fffff : 0x7ff) > #define STACK_RND_MASK __STACK_RND_MASK(!mmap_is_ia32()) > > That is pretty much clear and it uses mmap_is_ia32() as we do in other > places. As a consequemce you can use the same macros for 64 and 32 bit. Thanks, that's more readable. I was tempting to name it more self-descriptive, but two underscores looks enough. I'll resend with `is32bit' parameter - this way I'll also get rid of negaion in a call. I'll also need to move it down in header to omit forward-declaration of mmap_is_ia32(). > >> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h >> index 1be64da0384e..52086e65b422 100644 >> --- a/arch/x86/include/asm/processor.h >> +++ b/arch/x86/include/asm/processor.h >> @@ -862,7 +862,8 @@ extern void start_thread(struct pt_regs *regs, unsigned long new_ip, >> * This decides where the kernel will search for a free chunk of vm >> * space during mmap's. >> */ >> -#define TASK_UNMAPPED_BASE (PAGE_ALIGN(TASK_SIZE / 3)) >> +#define _TASK_UNMAPPED_BASE(task_size) (PAGE_ALIGN(task_size / 3)) >> +#define TASK_UNMAPPED_BASE _TASK_UNMAPPED_BASE(TASK_SIZE) > > Please use two underscores and align the macros in tabular fashion. > > #define __TASK_UNMAPPED_BASE(task_size) (PAGE_ALIGN(task_size / 3)) > #define TASK_UNMAPPED_BASE __TASK_UNMAPPED_BASE(TASK_SIZE) > > That way stuff becomes easy to read. Ok > >> #define KSTK_EIP(task) (task_pt_regs(task)->ip) >> >> diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c >> index 42063e787717..98be520fd270 100644 >> --- a/arch/x86/mm/mmap.c >> +++ b/arch/x86/mm/mmap.c >> @@ -35,12 +35,14 @@ struct va_alignment __read_mostly va_align = { >> .flags = -1, >> }; >> >> -static unsigned long stack_maxrandom_size(void) >> +static unsigned long stack_maxrandom_size(unsigned long task_size) >> { >> unsigned long max = 0; >> if ((current->flags & PF_RANDOMIZE) && >> !(current->personality & ADDR_NO_RANDOMIZE)) { >> - max = ((-1UL) & STACK_RND_MASK) << PAGE_SHIFT; >> + max = (-1UL); >> + max &= STACK_RND_MASK_MODE(task_size == TASK_SIZE_MAX); > > That just makes me barf, really. I have to go and lookup how TASK_SIZE_MAX > is defined in order to read that code. TASK_SIZE_MAX as is does not give a > hint at all that it means TASK_SIZE_MAX of 64 bit tasks. > > You just explained me that you want stuff proper for clarity reasons. So > what's so wrong with adding a helper inline tasksize_64bit() or such? I thought about that, but I'll need to redefine it under ifdefs :-/ I mean, for 32-bit native code. Hmm, I think, if I use is32bit parameter for __STACK_RND_MASK(), will it be more readable if I just compare to IA32_PAGE_OFFSET here? Or does it makes sence to introduce tasksize_32bit()? > >> + max <<= PAGE_SHIFT; >> } >> >> return max; >> @@ -51,8 +53,8 @@ static unsigned long stack_maxrandom_size(void) >> * >> * Leave an at least ~128 MB hole with possible stack randomization. >> */ >> -#define MIN_GAP (128*1024*1024UL + stack_maxrandom_size()) >> -#define MAX_GAP (TASK_SIZE/6*5) >> +#define MIN_GAP(task_size) (128*1024*1024UL + stack_maxrandom_size(task_size)) >> +#define MAX_GAP(task_size) (task_size/6*5) > > Eew. Yes it's existing macro mess, but there is not point in proliferating > that. That macro crap is only used in mmap_base() and there is no > justification for these unreadable macros at all. It just makes stuff > obfuscated for no reason. Just blindly making it more obfuscated does not > make it any better. > >> static int mmap_is_legacy(void) >> { >> @@ -88,16 +90,22 @@ unsigned long arch_mmap_rnd(void) >> return arch_native_rnd(); >> } > > #define SIZE_128M (128 * 1024 * 1024UL) > >> -static unsigned long mmap_base(unsigned long rnd) >> +static unsigned long mmap_base(unsigned long rnd, unsigned long task_size) >> { >> unsigned long gap = rlimit(RLIMIT_STACK); > unsigned long gap_min, gap_max; > > /* Add comment what this means */ > gap_min = SIZE_128M + stack_maxrandom_size(task_size); > /* Explain that ' /6 * 5' magic */ > gap_max = (task_size / 6) * 5; > > and use gap_min/gap_max below. That would be too intuitive and readable, > right? Haha, will do, thanks again. > >> - if (gap < MIN_GAP) >> - gap = MIN_GAP; >> - else if (gap > MAX_GAP) >> - gap = MAX_GAP; >> + if (gap < MIN_GAP(task_size)) >> + gap = MIN_GAP(task_size); >> + else if (gap > MAX_GAP(task_size)) >> + gap = MAX_GAP(task_size); >> >> - return PAGE_ALIGN(TASK_SIZE - gap - rnd); >> + return PAGE_ALIGN(task_size - gap - rnd); >> +} >> + >> +static unsigned long mmap_legacy_base(unsigned long rnd, >> + unsigned long task_size) > > Please align the argument in the second line with the argument in the first > one > > static unsigned long mmap_legacy_base(unsigned long rnd, > unsigned long task_size) Ok > > Thanks, > > tglx > -- Dmitry From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-f197.google.com (mail-qt0-f197.google.com [209.85.216.197]) by kanga.kvack.org (Postfix) with ESMTP id C1AE76B0038 for ; Mon, 13 Feb 2017 08:06:45 -0500 (EST) Received: by mail-qt0-f197.google.com with SMTP id q3so99387937qtf.4 for ; Mon, 13 Feb 2017 05:06:45 -0800 (PST) Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0090.outbound.protection.outlook.com. [104.47.1.90]) by mx.google.com with ESMTPS id e8si10021602pgp.204.2017.02.13.05.06.44 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 13 Feb 2017 05:06:44 -0800 (PST) Subject: Re: [PATCHv4 2/5] x86/mm: introduce mmap{,_legacy}_base References: <20170130120432.6716-1-dsafonov@virtuozzo.com> <20170130120432.6716-3-dsafonov@virtuozzo.com> From: Dmitry Safonov Message-ID: <696975bb-8846-24b8-e166-b2569d49cec8@virtuozzo.com> Date: Mon, 13 Feb 2017 16:02:57 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Thomas Gleixner Cc: linux-kernel@vger.kernel.org, 0x7f454c46@gmail.com, Ingo Molnar , "H. Peter Anvin" , Andy Lutomirski , Borislav Petkov , x86@kernel.org, linux-mm@kvack.org On 02/11/2017 05:13 PM, Thomas Gleixner wrote: > On Mon, 30 Jan 2017, Dmitry Safonov wrote: > >> In the following patch they will be used to compute: >> - mmap{,_legacy}_base for 64-bit mmap() >> - mmap_compat{,_legacy}_base for 32-bit mmap() >> >> This patch makes it possible to calculate mmap bases for any specified >> task_size, which is needed to correctly choose the base address for mmap >> in 32-bit syscalls and 64-bit syscalls. > > Please rework your changelogs so they follow the requirements in > Documentation.... Look for 'this patch' there. Sure, I'll rewrite the changelogs. > > Also order it in a way which makes it clear what the problem is and how > it's solved. > >> +#define STACK_RND_MASK_MODE(native) (0x7ff) >> #define STACK_RND_MASK (0x7ff) >> >> #define ARCH_DLINFO ARCH_DLINFO_IA32 >> @@ -295,7 +296,8 @@ do { \ >> #else /* CONFIG_X86_32 */ >> >> /* 1GB for 64bit, 8MB for 32bit */ >> -#define STACK_RND_MASK (test_thread_flag(TIF_ADDR32) ? 0x7ff : 0x3fffff) >> +#define STACK_RND_MASK_MODE(native) ((native) ? 0x3fffff : 0x7ff) >> +#define STACK_RND_MASK STACK_RND_MASK_MODE(!test_thread_flag(TIF_ADDR32)) > > I had to look twice what MODE means. The macro name suggests that it > returns the mode, while it actually returns the mask. That's confusing at > best. And 'native' is not intuitive either. > > #define __STACK_RND_MASK(is64bit) ((is64bit) ? 0x3fffff : 0x7ff) > #define STACK_RND_MASK __STACK_RND_MASK(!mmap_is_ia32()) > > That is pretty much clear and it uses mmap_is_ia32() as we do in other > places. As a consequemce you can use the same macros for 64 and 32 bit. Thanks, that's more readable. I was tempting to name it more self-descriptive, but two underscores looks enough. I'll resend with `is32bit' parameter - this way I'll also get rid of negaion in a call. I'll also need to move it down in header to omit forward-declaration of mmap_is_ia32(). > >> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h >> index 1be64da0384e..52086e65b422 100644 >> --- a/arch/x86/include/asm/processor.h >> +++ b/arch/x86/include/asm/processor.h >> @@ -862,7 +862,8 @@ extern void start_thread(struct pt_regs *regs, unsigned long new_ip, >> * This decides where the kernel will search for a free chunk of vm >> * space during mmap's. >> */ >> -#define TASK_UNMAPPED_BASE (PAGE_ALIGN(TASK_SIZE / 3)) >> +#define _TASK_UNMAPPED_BASE(task_size) (PAGE_ALIGN(task_size / 3)) >> +#define TASK_UNMAPPED_BASE _TASK_UNMAPPED_BASE(TASK_SIZE) > > Please use two underscores and align the macros in tabular fashion. > > #define __TASK_UNMAPPED_BASE(task_size) (PAGE_ALIGN(task_size / 3)) > #define TASK_UNMAPPED_BASE __TASK_UNMAPPED_BASE(TASK_SIZE) > > That way stuff becomes easy to read. Ok > >> #define KSTK_EIP(task) (task_pt_regs(task)->ip) >> >> diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c >> index 42063e787717..98be520fd270 100644 >> --- a/arch/x86/mm/mmap.c >> +++ b/arch/x86/mm/mmap.c >> @@ -35,12 +35,14 @@ struct va_alignment __read_mostly va_align = { >> .flags = -1, >> }; >> >> -static unsigned long stack_maxrandom_size(void) >> +static unsigned long stack_maxrandom_size(unsigned long task_size) >> { >> unsigned long max = 0; >> if ((current->flags & PF_RANDOMIZE) && >> !(current->personality & ADDR_NO_RANDOMIZE)) { >> - max = ((-1UL) & STACK_RND_MASK) << PAGE_SHIFT; >> + max = (-1UL); >> + max &= STACK_RND_MASK_MODE(task_size == TASK_SIZE_MAX); > > That just makes me barf, really. I have to go and lookup how TASK_SIZE_MAX > is defined in order to read that code. TASK_SIZE_MAX as is does not give a > hint at all that it means TASK_SIZE_MAX of 64 bit tasks. > > You just explained me that you want stuff proper for clarity reasons. So > what's so wrong with adding a helper inline tasksize_64bit() or such? I thought about that, but I'll need to redefine it under ifdefs :-/ I mean, for 32-bit native code. Hmm, I think, if I use is32bit parameter for __STACK_RND_MASK(), will it be more readable if I just compare to IA32_PAGE_OFFSET here? Or does it makes sence to introduce tasksize_32bit()? > >> + max <<= PAGE_SHIFT; >> } >> >> return max; >> @@ -51,8 +53,8 @@ static unsigned long stack_maxrandom_size(void) >> * >> * Leave an at least ~128 MB hole with possible stack randomization. >> */ >> -#define MIN_GAP (128*1024*1024UL + stack_maxrandom_size()) >> -#define MAX_GAP (TASK_SIZE/6*5) >> +#define MIN_GAP(task_size) (128*1024*1024UL + stack_maxrandom_size(task_size)) >> +#define MAX_GAP(task_size) (task_size/6*5) > > Eew. Yes it's existing macro mess, but there is not point in proliferating > that. That macro crap is only used in mmap_base() and there is no > justification for these unreadable macros at all. It just makes stuff > obfuscated for no reason. Just blindly making it more obfuscated does not > make it any better. > >> static int mmap_is_legacy(void) >> { >> @@ -88,16 +90,22 @@ unsigned long arch_mmap_rnd(void) >> return arch_native_rnd(); >> } > > #define SIZE_128M (128 * 1024 * 1024UL) > >> -static unsigned long mmap_base(unsigned long rnd) >> +static unsigned long mmap_base(unsigned long rnd, unsigned long task_size) >> { >> unsigned long gap = rlimit(RLIMIT_STACK); > unsigned long gap_min, gap_max; > > /* Add comment what this means */ > gap_min = SIZE_128M + stack_maxrandom_size(task_size); > /* Explain that ' /6 * 5' magic */ > gap_max = (task_size / 6) * 5; > > and use gap_min/gap_max below. That would be too intuitive and readable, > right? Haha, will do, thanks again. > >> - if (gap < MIN_GAP) >> - gap = MIN_GAP; >> - else if (gap > MAX_GAP) >> - gap = MAX_GAP; >> + if (gap < MIN_GAP(task_size)) >> + gap = MIN_GAP(task_size); >> + else if (gap > MAX_GAP(task_size)) >> + gap = MAX_GAP(task_size); >> >> - return PAGE_ALIGN(TASK_SIZE - gap - rnd); >> + return PAGE_ALIGN(task_size - gap - rnd); >> +} >> + >> +static unsigned long mmap_legacy_base(unsigned long rnd, >> + unsigned long task_size) > > Please align the argument in the second line with the argument in the first > one > > static unsigned long mmap_legacy_base(unsigned long rnd, > unsigned long task_size) Ok > > Thanks, > > tglx > -- Dmitry -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org