Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp2346006imu; Sat, 22 Dec 2018 19:53:12 -0800 (PST) X-Google-Smtp-Source: ALg8bN5Fs03x1iM4BPmHzZy5LxQPLl+4WG4H0n2FrOM5M275dMBjoc/LnuVrQFOm/qWlaZ1uaPqW X-Received: by 2002:a17:902:704b:: with SMTP id h11mr8587375plt.157.1545537192267; Sat, 22 Dec 2018 19:53:12 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1545537192; cv=none; d=google.com; s=arc-20160816; b=rUKa3BiOh0Aw4gcTBCZx6JBB2+vl9oxl72bZzW1SL0vBxZ5+BE2Vwy6qwbKcCwHruI LnYgtu6//0bRHn0xuDXivN0NyR0EOipNh76wRQic3M3DEs5QoSeZ77cgcyL3D+G+GU2v Gp47l0xSZxwg8XVtmrzbchnGNFzwi/5oS2FCPh40cxrRsUTeiWAjaek8ncbCSnfGpR1E 0uKjI9JuLEqye45l6/CRqqIldxMmrmOJBWcS8vs7YrlvOZiCKPXG2EiGvWwuCXd+n+1k Dh/5CnaGC8Lad6W1NRXczblR8/iE17au2valExsH5dB40CLpL6HpKndmoa12DD/Lct/+ YU6Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:dkim-signature; bh=jXFLTI8JHQDvVJR/RJzDh54rUI3Cr7wPyRKQV5m3vEg=; b=tqBWgIym7V4q3fbJMccmzurc+okiqy1kdrZUnzE3WPRpj82K5IbDfEk1cXk9F+wFAi mIDQytS6+kHfGPGTYY/1mwlmcSfUlI1oJ20FATJmOeE4QCnrsE55UBZGQJWCgDFMoYnT UllAHVXRJDG+I8YG8t3HKyWTagz6Jna1e4WgpCvncaqxPq7lF+endrGX4JI8qMvaEQ1R 0jzMbJKDn2FPVfRCWpWOlri8jDeHEeZxIyOHCt2Fh92cSYP7iebTtD1ZRgyjWDh4pgEM NPHr38ufsfzt9yRf4xa+TAtfBhBzxSvngUvEKIeKW9K+zL6izZLvHepeE+7eiNDHlIW0 Y7Kg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b="im/Mlm+c"; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id g33si25449976pgm.426.2018.12.22.19.52.48; Sat, 22 Dec 2018 19:53:12 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b="im/Mlm+c"; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2391547AbeLUTIA (ORCPT + 99 others); Fri, 21 Dec 2018 14:08:00 -0500 Received: from mail-lf1-f67.google.com ([209.85.167.67]:33478 "EHLO mail-lf1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2389409AbeLUTIA (ORCPT ); Fri, 21 Dec 2018 14:08:00 -0500 Received: by mail-lf1-f67.google.com with SMTP id i26so4655386lfc.0; Fri, 21 Dec 2018 11:07:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=jXFLTI8JHQDvVJR/RJzDh54rUI3Cr7wPyRKQV5m3vEg=; b=im/Mlm+c2j/AxAaz7es2VdN9/H1+kt9/iAqdOYY9SotYhfR2ToDieyP46bdUC3Yl/O 3W64atjFM8q9HOs+wuAyewAzX7Y0y6e4dOK5BeWWII9jxvBRPqMeGSnypIpxmDx95sTT QRc2T/nahGo1nT6u3PTXITy44drVil/FmR+csv4nouQzJawoJFaWnNQSnCotKUqViGBA I390nT8mONQu15dxkpUSCHTrpi+Yflw9YJv/e4kbxrOQ4R03d0HvfK4MhW/dbN5rdTxN YSzAEKVq1oKoYApV3QdNEY5wswtq+K8dvt1SuS4X0wN+Mz3QEafxAgxxQB3UNTBzOvxM IjDQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=jXFLTI8JHQDvVJR/RJzDh54rUI3Cr7wPyRKQV5m3vEg=; b=LPfyJBjSBBqghKfCl99GVhkyUri0dusHj4E0WrSPuZ0ImItBQnCT4x3+LIUV4zL1I8 s2KoZlvMNtX5iAgkO8LfYSHaFKq0evok+MNs3TooyltFtbOKOJqW+8SnIHoU3taV/jlg oOmySEJ/xdrxhEs1PxIcko1LUbNZ8AOMCMDtm+ADwjkEssucPoA74aoIdHZNrLbS223P yZbf9KpVd8MBHWXufkr7A3yiA5Ijqm0hDBzoL4zfSpdG1QYV81Vb/BcOXTduSpY6WhFA FTmvALjpcz8XQ4piz+stm/okC1dQaC77OPZND4GdgRnlyfKvevw7SVvucY3n1M66zVRE LjbA== X-Gm-Message-State: AA+aEWbMGFOVpVh49JoM6Br35E0tA2khy1ysXhZ9Pj50FoJvVABlIx6o TOtJmimd/EgjBTCicYOlPRQI4HqFPzs= X-Received: by 2002:a19:cfc6:: with SMTP id f189mr2249346lfg.102.1545419277138; Fri, 21 Dec 2018 11:07:57 -0800 (PST) Received: from ?IPv6:2001:14bb:51:a4c8:5c24:24d7:ca5f:e7d2? (dmhwpt3bffxn8z3-j6k-4.rev.dnainternet.fi. [2001:14bb:51:a4c8:5c24:24d7:ca5f:e7d2]) by smtp.gmail.com with ESMTPSA id u11sm5046544lfb.85.2018.12.21.11.07.55 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 21 Dec 2018 11:07:56 -0800 (PST) Subject: Re: [PATCH 03/12] __wr_after_init: generic functionality To: Matthew Wilcox Cc: Andy Lutomirski , Peter Zijlstra , Dave Hansen , Mimi Zohar , Thiago Jung Bauermann , igor.stoppa@huawei.com, Nadav Amit , Kees Cook , Ahmed Soliman , linux-integrity@vger.kernel.org, kernel-hardening@lists.openwall.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20181221181423.20455-1-igor.stoppa@huawei.com> <20181221181423.20455-4-igor.stoppa@huawei.com> <20181221184120.GG10600@bombadil.infradead.org> From: Igor Stoppa Message-ID: <14487401-dec3-6a7d-a0b1-e369e93aa9c4@gmail.com> Date: Fri, 21 Dec 2018 21:07:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20181221184120.GG10600@bombadil.infradead.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 21/12/2018 20:41, Matthew Wilcox wrote: > On Fri, Dec 21, 2018 at 08:14:14PM +0200, Igor Stoppa wrote: >> +static inline int memtst(void *p, int c, __kernel_size_t len) > > I don't understand why you're verifying that writes actually happen > in production code. Sure, write lib/test_wrmem.c or something, but > verifying every single rare write seems like a mistake to me. This is actually something I wrote more as a stop-gap. I have the feeling there should be already something similar available. And probably I could not find it. Unless it's so trivial that it doesn't deserve to become a function? But if there is really no existing alternative, I can put it in a separate file. > >> +#ifndef CONFIG_PRMEM > > So is this PRMEM or wr_mem? It's not obvious that CONFIG_PRMEM controls > wrmem. In my mind (maybe still clinging to the old implementation), PRMEM is the master toggle, for protected memory. Then there are various types and the first one being now implemented is write rare after init (because ro after init already exists). However, the same levels of protection should then follow for dynamically allocated memory (ye old pmalloc). PRMEM would then become the moniker for the whole shebang. >> +#define wr_assign(var, val) ((var) = (val)) > > The hamming distance between 'var' and 'val' is too small. The convention > in the line immediately below (p and v) is much more readable. ok, I'll fix it >> +#define wr_rcu_assign_pointer(p, v) rcu_assign_pointer(p, v) >> +#define wr_assign(var, val) ({ \ >> + typeof(var) tmp = (typeof(var))val; \ >> + \ >> + wr_memcpy(&var, &tmp, sizeof(var)); \ >> + var; \ >> +}) > > Doesn't wr_memcpy return 'var' anyway? It should return the destination, which is &var. But I wanted to return the actual value of the assignment, val Like if I do (a = 7) it evaluates to 7, similarly wr_assign(a, 7) would also evaluate to 7 The reason why i returned var instead of val is that it would allow to detect any error. >> +/** >> + * wr_memcpy() - copyes size bytes from q to p > > typo :-( thanks >> + * @p: beginning of the memory to write to >> + * @q: beginning of the memory to read from >> + * @size: amount of bytes to copy >> + * >> + * Returns pointer to the destination > >> + * The architecture code must provide: >> + * void __wr_enable(wr_state_t *state) >> + * void *__wr_addr(void *addr) >> + * void *__wr_memcpy(void *p, const void *q, __kernel_size_t size) >> + * void __wr_disable(wr_state_t *state) > > This section shouldn't be in the user documentation of wr_memcpy(). ok >> + */ >> +void *wr_memcpy(void *p, const void *q, __kernel_size_t size) >> +{ >> + wr_state_t wr_state; >> + void *wr_poking_addr = __wr_addr(p); >> + >> + if (WARN_ONCE(!wr_ready, "No writable mapping available") || > > Surely not. If somebody's called wr_memcpy() before wr_ready is set, > that means we can just call memcpy(). What I was trying to catch is the case where, after a failed init, the writable mapping doesn't exist. In that case wr_ready is also not set. The problem is that I just don't know what to do in a case where there has been such a major error which prevents he creation of hte alternate mapping. I understand that we still want to continue, to provide as much debug info as possible, but I am at a loss about finding the saner course of actions. -- igor