From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753784AbcBBHjX (ORCPT ); Tue, 2 Feb 2016 02:39:23 -0500 Received: from e06smtp06.uk.ibm.com ([195.75.94.102]:47028 "EHLO e06smtp06.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750913AbcBBHjV (ORCPT ); Tue, 2 Feb 2016 02:39:21 -0500 X-IBM-Helo: d06dlp02.portsmouth.uk.ibm.com X-IBM-MailFrom: heiko.carstens@de.ibm.com X-IBM-RcptTo: linux-arch@vger.kernel.org;linux-kernel@vger.kernel.org;linux-s390@vger.kernel.org Date: Tue, 2 Feb 2016 08:39:13 +0100 From: Heiko Carstens To: Yury Norov Cc: linux-arch@vger.kernel.org, linux-s390@vger.kernel.org, arnd@arndb.de, pinskia@gmail.com, Prasun.Kapoor@caviumnetworks.com, catalin.marinas@arm.com, Nathan_Lynch@mentor.com, linux-kernel@vger.kernel.org, agraf@suse.de, klimov.linux@gmail.com, broonie@kernel.org, joseph@codesourcery.com, schwidefsky@de.ibm.com, linux-arm-kernel@lists.infradead.org, christoph.muellner@theobroma-systems.com Subject: Re: [PATCH 1/5] all: s390: move wrapper infrastructure to generic headers Message-ID: <20160202073913.GB3990@osiris> References: <1453741047-5498-1-git-send-email-ynorov@caviumnetworks.com> <1453741047-5498-2-git-send-email-ynorov@caviumnetworks.com> <20160128121618.GB5418@osiris> <20160128163109.GA8529@yury-N73SV> <20160201114251.GB973@yury-N73SV> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160201114251.GB973@yury-N73SV> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16020207-0025-0000-0000-000005B82F2E Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Feb 01, 2016 at 02:42:51PM +0300, Yury Norov wrote: > Hi Heiko, > > I tried this idea, and I don't like what happened. > - Wrappers around safe syscalls does exist. We can remove it by > overcomplicating __SC_COMPAT_CAST, but I don't like it. > - We still need to declare numerous list of new compat syscalls. > And it becomes even bigger, as we need to declare all compat > syscall versions not declared in include/linux/compat.h already. > (Currently - only for unsafe syscalls.) > - 'Weak' trick doesn't work for the whole kernel, so we'd figure out > some new prefix for wrapped syscalls. Or declare all non-compat > syscalls explicitly with SYSCALL_COMPAT_DEFINE. So the list of > replacements grow. And for me, it's harder to explain why we are > wrapping safe syscalls. Or we introduce another bunch of useless > wrappers (with new prefix), and have to handle it in non-compat code. > - With all listed above, we move all wrapper logic to non-compat > 'include/linux/syscalls.h' header. Which is not a good idea, if it > doesn't benefit us much in return. > > > > No need to look up if a compat variant (or wrapper) exists or > > > sys_ should be used instead. Also no possibility for security > > > bugs that could creep in because SYSCALL_DEFINE has been used instead of > > > SYSCALL_DEFINE_WRAP. > > I thought again about it. With current version, it's very easy to > define whether we have wrapper or not - just by macro we use. Once > reviewed, this list is hardly to be changed frequently. If someone is > introducing new syscall, it will attract much attention, so security > risk is minimal. > > Maybe I missed some elegant implementation, and if so I'll be happy > if someone'll point me out. But with what I see, I'd vote for what we > have now. (Plus description in docs, plus renaming new macro.) Well, I'd like to have some proof by the compiler or linker that nothing went wrong. Which seems hard if only selected system call defines will be converted to the new defines. How can you tell that nothing has been forgotten? Also, what happens if the prototype of a system call get's changed shortly after it was merged. We might miss such changes and have bugs. Therefore, and to get to a solution, I think we should stick with your first idea, which only moves the compat_wrapper.c file. Before doing that I think you should actually revert this patch: my commit 7681df456f97 ("s390/compat: remove superfluous compat wrappers") probably wasn't a very bright idea :) This again allows me to use only compat system calls in s390's system call table (execpt for system calls without parameters, but that can be easily fixed). What I still don't like is that you need to add all the protoypes. Why are the system call tables actually written in C and not in asm? From mboxrd@z Thu Jan 1 00:00:00 1970 From: heiko.carstens@de.ibm.com (Heiko Carstens) Date: Tue, 2 Feb 2016 08:39:13 +0100 Subject: [PATCH 1/5] all: s390: move wrapper infrastructure to generic headers In-Reply-To: <20160201114251.GB973@yury-N73SV> References: <1453741047-5498-1-git-send-email-ynorov@caviumnetworks.com> <1453741047-5498-2-git-send-email-ynorov@caviumnetworks.com> <20160128121618.GB5418@osiris> <20160128163109.GA8529@yury-N73SV> <20160201114251.GB973@yury-N73SV> Message-ID: <20160202073913.GB3990@osiris> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Feb 01, 2016 at 02:42:51PM +0300, Yury Norov wrote: > Hi Heiko, > > I tried this idea, and I don't like what happened. > - Wrappers around safe syscalls does exist. We can remove it by > overcomplicating __SC_COMPAT_CAST, but I don't like it. > - We still need to declare numerous list of new compat syscalls. > And it becomes even bigger, as we need to declare all compat > syscall versions not declared in include/linux/compat.h already. > (Currently - only for unsafe syscalls.) > - 'Weak' trick doesn't work for the whole kernel, so we'd figure out > some new prefix for wrapped syscalls. Or declare all non-compat > syscalls explicitly with SYSCALL_COMPAT_DEFINE. So the list of > replacements grow. And for me, it's harder to explain why we are > wrapping safe syscalls. Or we introduce another bunch of useless > wrappers (with new prefix), and have to handle it in non-compat code. > - With all listed above, we move all wrapper logic to non-compat > 'include/linux/syscalls.h' header. Which is not a good idea, if it > doesn't benefit us much in return. > > > > No need to look up if a compat variant (or wrapper) exists or > > > sys_ should be used instead. Also no possibility for security > > > bugs that could creep in because SYSCALL_DEFINE has been used instead of > > > SYSCALL_DEFINE_WRAP. > > I thought again about it. With current version, it's very easy to > define whether we have wrapper or not - just by macro we use. Once > reviewed, this list is hardly to be changed frequently. If someone is > introducing new syscall, it will attract much attention, so security > risk is minimal. > > Maybe I missed some elegant implementation, and if so I'll be happy > if someone'll point me out. But with what I see, I'd vote for what we > have now. (Plus description in docs, plus renaming new macro.) Well, I'd like to have some proof by the compiler or linker that nothing went wrong. Which seems hard if only selected system call defines will be converted to the new defines. How can you tell that nothing has been forgotten? Also, what happens if the prototype of a system call get's changed shortly after it was merged. We might miss such changes and have bugs. Therefore, and to get to a solution, I think we should stick with your first idea, which only moves the compat_wrapper.c file. Before doing that I think you should actually revert this patch: my commit 7681df456f97 ("s390/compat: remove superfluous compat wrappers") probably wasn't a very bright idea :) This again allows me to use only compat system calls in s390's system call table (execpt for system calls without parameters, but that can be easily fixed). What I still don't like is that you need to add all the protoypes. Why are the system call tables actually written in C and not in asm?