Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935364AbcJFWxC (ORCPT ); Thu, 6 Oct 2016 18:53:02 -0400 Received: from mail-pa0-f67.google.com ([209.85.220.67]:35889 "EHLO mail-pa0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934752AbcJFWwx (ORCPT ); Thu, 6 Oct 2016 18:52:53 -0400 Date: Thu, 6 Oct 2016 15:52:49 -0700 From: Dmitry Torokhov To: John Stultz Cc: lkml , Rom Lemarchand , Tejun Heo , Li Zefan , Jonathan Corbet , cgroups@vger.kernel.org, Android Kernel Team , Colin Cross , Dmitry Shmidt , Todd Kjos , Christian Poetzsch , Amit Pundir , Ricky Zhou Subject: Re: [PATCH 2/2] cgroup: Add a allow_attach policy for Android Message-ID: <20161006225249.GB25887@dtor-ws> References: <1475556090-6278-1-git-send-email-john.stultz@linaro.org> <1475556090-6278-3-git-send-email-john.stultz@linaro.org> <20161005191056.GB31873@dtor-ws> <20161006224351.GA25887@dtor-ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161006224351.GA25887@dtor-ws> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1611 Lines: 42 On Thu, Oct 06, 2016 at 03:43:51PM -0700, Dmitry Torokhov wrote: > On Wed, Oct 05, 2016 at 12:18:17PM -0700, John Stultz wrote: > > On Wed, Oct 5, 2016 at 12:10 PM, Dmitry Torokhov > > wrote: > > > On Mon, Oct 03, 2016 at 09:41:30PM -0700, John Stultz wrote: > > >> +#ifdef CONFIG_CGROUP_NICE_ATTACH > > >> +int cgroup_nice_allow_attach(struct cgroup_taskset *tset) > > >> +{ > > >> + const struct cred *cred = current_cred(), *tcred; > > >> + struct task_struct *task; > > >> + struct cgroup_subsys_state *css; > > >> + > > >> + if (capable(CAP_SYS_NICE)) > > >> + return 0; > > >> + > > >> + cgroup_taskset_for_each(task, css, tset) { > > >> + tcred = __task_cred(task); > > > > > > __task_cred() requires RCU lock (courtesy Ricky Z). > > > > Again, hopefully this isn't an issue with the new approach, but for > > the short term I'll see if we can get this fixed in the android tree. > > > > Actually, it should all be simply removed from there right away, as this > ends up being basically noop (but with all the locking violations and > races): > > cgroup_taskset_for_each() needs tasks to be placed on cset->mg_tasks > list, but nobody does this in the ->allow_access() code path, so this > loops always executes exactly 0 times and the whole thing is exactly > equivalent of doing > > return capable(CAP_SYS_NICE) ? 0 : -EACESS; > > which can be done right in cgroup_procs_write_permission(). Umm, sorry, no, it actually always returns 0, regardless even of capabilities. Permissions are indeed being relaxed ;) -- Dmitry