From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Cyrus-Session-Id: sloti22d1t05-2643914-1526391057-2-16287055996355158867 X-Sieve: CMU Sieve 3.0 X-Spam-known-sender: no X-Spam-charsets: plain='iso-8859-1' X-Resolved-to: linux@kroah.com X-Delivered-to: linux@kroah.com X-Mail-from: linux-arch-owner@vger.kernel.org ARC-Seal: i=1; a=rsa-sha256; cv=none; d=messagingengine.com; s=fm2; t= 1526391057; b=GzUqYFWagVX/tICA5PUXIF84v2e9sLVneKaaRCpWk+vsJ6FMy/ 4YNwsG+e/8QYLmLVqZwSwYswECekat8jh5wll3zSRQJeFnqTUM2cFdj1G0TBPQ/5 TWTTwGT/BO1UC+fJNmCgfpu0IQWLaJU7MEld5eOdH+X0AxZo4FUc9Om9jj9e4Uoi 4y561B0XRo42XcXTECnMLvW8Aw/RBpz0PVBJovLt24G2XzlsiFYTwtRxN5CEiXx3 dUSVdmX6fwSRtKxjzP/6e711KWztlPJPNCVu7k7otGLhm2f4TsLcyDZZXq35QSXm b2zQxJZEFS2jdUzZFJsJVUV16W9pU0AGWv6w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=date:from:to:cc:subject:message-id :references:mime-version:content-type:content-transfer-encoding :in-reply-to:sender:list-id; s=fm2; t=1526391057; bh=xVEbqQ8URNO WHMc3TdNjiHI69guux2oBM19IsGT55U8=; b=XfCeEgRd0N8Hl74j6I4o3HMlEs1 FR+320qYCngVWMvXUtc/AuOk7PiOfDqTWi7Tp7/O+FjZp+S9PCYAWAa4Ak9mTFpk htwdjUXhCZslaM697iTFtxmLUtpMDc7im/OE2oHJVdMRjmISgyR9I2P5O4NFCFmz DfwQ4RaPVEyr+jLFnmIuOoXICu2F8cX4FbBt/uzjQLYhrCwGrSpKs/pt1EpQVc5k fLZjupOl3urBHzDAvlM2YAk7/gO/jU9IFxbx4Ud1rPoBFHC+OfhpinXk2qmG9/jn swguTa3w1xel3im2q0YxQLxXMVGccuOCbMSzeAkdVXdx2cRlrbWumXv6I+A== ARC-Authentication-Results: i=1; mx5.messagingengine.com; arc=none (no signatures found); dkim=none (no signatures found); dmarc=none (p=none,has-list-id=yes,d=none) header.from=arm.com; iprev=pass policy.iprev=209.132.180.67 (vger.kernel.org); spf=none smtp.mailfrom=linux-arch-owner@vger.kernel.org smtp.helo=vger.kernel.org; x-aligned-from=fail; x-cm=none score=0; x-ptr=pass x-ptr-helo=vger.kernel.org x-ptr-lookup=vger.kernel.org; x-return-mx=pass smtp.domain=vger.kernel.org smtp.result=pass smtp_org.domain=kernel.org smtp_org.result=pass smtp_is_org_domain=no header.domain=arm.com header.result=pass header_is_org_domain=yes; x-vs=clean score=-100 state=0 Authentication-Results: mx5.messagingengine.com; arc=none (no signatures found); dkim=none (no signatures found); dmarc=none (p=none,has-list-id=yes,d=none) header.from=arm.com; iprev=pass policy.iprev=209.132.180.67 (vger.kernel.org); spf=none smtp.mailfrom=linux-arch-owner@vger.kernel.org smtp.helo=vger.kernel.org; x-aligned-from=fail; x-cm=none score=0; x-ptr=pass x-ptr-helo=vger.kernel.org x-ptr-lookup=vger.kernel.org; x-return-mx=pass smtp.domain=vger.kernel.org smtp.result=pass smtp_org.domain=kernel.org smtp_org.result=pass smtp_is_org_domain=no header.domain=arm.com header.result=pass header_is_org_domain=yes; x-vs=clean score=-100 state=0 X-ME-VSCategory: clean X-CM-Envelope: MS4wfAIl9v3BT5lIQgccuOXTU952Pl7Xsz8QhRuP/T0tcDTAw7rD97EFzGexT3GSpJDxPADZF4faKSYuBpWXbjvn1+ELr9NtMC7eHfkCleNIj/BfR5cUokLT gb/+HOrwI9Yar8ZCxohN+2kz5sg6bHWC8z+TQdw/v3kwZ1sNRncg/moa1j7RfhXAOIq1X1CHH0M/KDLD2Q7LWCB787jE0FWH0htbs3+11cUzuoKS6M3KV2QU X-CM-Analysis: v=2.3 cv=NPP7BXyg c=1 sm=1 tr=0 a=UK1r566ZdBxH71SXbqIOeA==:117 a=UK1r566ZdBxH71SXbqIOeA==:17 a=8nJEP1OIZ-IA:10 a=VUJBJC2UJ8kA:10 a=7CQSdrXTAAAA:8 a=ygxfXX2ePW69jj8DKUwA:9 a=gtHO2Lxe_6lQNkid:21 a=Y0hdbjRbOZBIHLCX:21 a=wPNLvfGTeEIA:10 a=a-qgeE7W1pNrGK8U0ZQC:22 X-ME-CMScore: 0 X-ME-CMCategory: none Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753049AbeEONaz (ORCPT ); Tue, 15 May 2018 09:30:55 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:60596 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752863AbeEONay (ORCPT ); Tue, 15 May 2018 09:30:54 -0400 Date: Tue, 15 May 2018 14:30:47 +0100 From: Dave Martin To: Kees Cook Cc: LKML , linux-arch , Andrew Morton , Benjamin Herrenschmidt , Catalin Marinas , Fenghua Yu , "H. Peter Anvin" , Ingo Molnar , Ivan Kokshaysky , James Hogan , Matt Turner , Michael Ellerman , Paul Mackerras , Ralf Baechle , Richard Henderson , Rich Felker , Thomas Gleixner , Tony Luck , Will Deacon , X86 ML , Yoshinori Sato Subject: Re: [RFC PATCH 00/11] prctl: Modernise wiring for optional prctl() calls Message-ID: <20180515133047.GP7753@e103592.cambridge.arm.com> References: <1526318067-4964-1-git-send-email-Dave.Martin@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-arch-owner@vger.kernel.org X-Mailing-List: linux-arch@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On Mon, May 14, 2018 at 07:28:11PM +0100, Kees Cook wrote: > On Mon, May 14, 2018 at 10:14 AM, Dave Martin wrote: > > [Reviewer note: this is a cross-arch series. To reduce spam, I have > > tried not to Cc people on patches they aren't likely to care about. > > The complete series can be found in the LKML or linux-arch archives.] > > > > The core framework for the prctl() syscall is unloved and looking > > rather crusty these days. It also relies on defining ancillary > > boilerplate macros for each prctl() in order to control conditional > > compilation of the different prctl calls. We have better ways to > > do this now. > > This is a nice clean-up series, thanks! Some thoughts/comments below... > > > This series attempts to modernise the code by defining a couple of new > > Kconfig variables HAVE_PRCTL_ARCH and HAVE_ARCH_PR_SET_GET_UNALIGN to > > allow architectures to provide hooks for arch-dependent prctls. > > > > > > For now this series has had minimal testing: some basic testing of > > arch-specific prctls using PR_SVE_SET_VL on arm64; build-tested on > > x86; otherwise untested. > > > > This is not polished yet... but I'm interested to know what people > > think about the approach. > > I'm not entirely convinced the removal of the "task not current" > interface is very useful as I've found myself needing to adjust other > interfaces like this to _add_ the "task not current" logic, but ... I > can't really argue very hard for keeping the task args since nothing > is using them... meh. For operations that are shared with ptrace it does make sense to have a task argument, but in the end I found it equally natural to put that only in the backend rather than having it invoked directly with an explicit argument by the prctl demux. In the case of PR_SVE_SET_VL the interface has to be massaged in different ways depending on whether it's being invokved via prctl or ptrace anyway, so I guess there was no temptation to keep the task argument on the prctl side in this case. Ptrace differs from prctl in that in the former case we can assume the task is stopped and in the latter we can't. This results in some conditional local_bh_disable()..local_bh_enable() in sve_set_vector_length(), which is a bit gross. Maybe it's best not to encourage ptrace and prctl to be given the same backend functions without thinking about it. (See the different callers of sve_set_vector_length() under arch/arm64/.) Others' views may differ though, so I'm happy to be overruled on this if people object. > I dislike this being named "prctl_arch" when everything else like this > is "arch_foo...", but I do see the collision with the x86-specific > "arch_prctl" syscall. Perhaps it might be better to wire things up Agreed. Actually, it was originally called arch_prctl(), until I discovered that it broke um. x86 seems happy enough, since its existing arch_prctl() functions all have do_/sys_/etc. prefixes. > differently, since x86's arch_prctl is actually "sys_arch_prctl" so I > don't think there would be a symbol collision, and maybe the 9 options > could be added to the global prctl list, with the x86 syscall getting > called at the end of the new "arch_prctl"? Then the syscall could get > deprecated in favor of just using prctl directly? > > Your new x86 arch_prctl (ne้ prctl_arch) could be something like: > > int arch_prctl(int option, unsigned long arg2, unsigned long arg3, > unsigned long arg4, unsigned long arg5) > { > ... existing stuff in your series ... > case ARCH_SET_GS: > case ARCH_SET_FS: > case ARCH_GET_GS: > case ARCH_GET_FS: > case ARCH_MAP_VDSO_X32: > case ARCH_MAP_VDSO_32: > case ARCH_MAP_VDSO_64: > if (arg3 || arg4 || arg5) > return -EINVAL; > return do_arch_prctl_64(current, option, arg2); > case ARCH_GET_CPUID: > case ARCH_SET_CPUID: > if (arg3 || arg4 || arg5) > return -EINVAL; > return do_arch_prctl_common(current, option, arg2); > default: > return -EINVAL; > } > } So, you mean we merge x86's arch_prctl() syscall with the generic one, and have both syscalls from userland invoke the common backend? That does look kind of feasible, given the compatible ABI and the fact that there does not appear to be a namespace clash in the option values. > Or maybe all the logic could get ripped out of > arch/x86/kernel/process*.c and put in arch/x86/kernel/sys.c directly? > > Perhaps this is all overkill, but I find it rather annoying that one > arch's weird syscall would block "expected" naming of a cross-arch > interface... I didn't really like overloading the name "arch_prctl" at all, since this is established as the name of the existing x86 syscall. Even if we arrange things in the kernel so that there is no linkage namespace clash, there seemed to be high risk of confusion. If we can merge the two (as suggested above) then that might be acceptable, but I wasn't confident that there were no subtle gotchas with such an approach... Thoughts? Cheers ---Dave