From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935741AbcJFWoD (ORCPT ); Thu, 6 Oct 2016 18:44:03 -0400 Received: from mail-pa0-f65.google.com ([209.85.220.65]:34385 "EHLO mail-pa0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935210AbcJFWnz (ORCPT ); Thu, 6 Oct 2016 18:43:55 -0400 Date: Thu, 6 Oct 2016 15:43:51 -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: <20161006224351.GA25887@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 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(). Thanks. -- Dmitry From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Torokhov Subject: Re: [PATCH 2/2] cgroup: Add a allow_attach policy for Android Date: Thu, 6 Oct 2016 15:43:51 -0700 Message-ID: <20161006224351.GA25887@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> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=HFmWwAKCpdcqfQezvEXUZ7HpKz2eju+bnTKs94Jf5as=; b=Z7w6ztGcJAOccWbv8vzHi3FrRqBX1T7KuODp2K8CPWXTiVk4zippckvxJABmNZveSl BfAJv2LeH/tc21GnQTq+gflGv8pmA9fArVeU/MCeJVBmEpYAMNmLTEeBQxGA29D/WRo0 xWlvuYiTrRv0fJfNJvZR+dXcZdRAHwUUud0YyAfses7U89AFCWsoxeE5dwK/B8tMMSv/ bF8hLCGXdHLwaQkBLvVixBNGMS+If3DeOtXZRrfmRa7MkC1awWMxYhD33PA2x8KkmGcr Wc6G4v7MVnP4qASGTX9TyRM2pa1hrrvwNe5wl5EBoBxOvoRNIRvE6eLokYWTrmtv8/d7 V5Pg== Content-Disposition: inline In-Reply-To: Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: John Stultz Cc: lkml , Rom Lemarchand , Tejun Heo , Li Zefan , Jonathan Corbet , cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Android Kernel Team , Colin Cross , Dmitry Shmidt , Todd Kjos , Christian Poetzsch , Amit Pundir , Ricky Zhou 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(). Thanks. -- Dmitry