From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 27C73375F65 for ; Mon, 1 Jun 2026 12:12:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780315980; cv=none; b=IyrkLSvyxvajac1Z7tcmDhoKXFIJAY3XWZDjDarV8I2Ti3bZkZ99jbcv5uyg2+cYeEh+6yt7LFCftn7lQyrVY3asqBb8ntSi2VABSv0Mgz3L4HL9d2mv4tunHE6RU3QMw6tDDdpoQcLcGyfj1jMIyHWI4fJjmcn7eRQ7micFN2M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780315980; c=relaxed/simple; bh=uh+zlNJ+glvOGUyIFOQ+aXuenNzd48MEGrqHWzaUJoA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=me1uFoIoaUxJROVa8PqHzlvPJVKqxYVQaWBz1+V2X3ClxLuzJX1XqN+PjB4mwwRuIRafxvMaxFzOawFI+E1fFh6N5F4rCKX2ITt5Fl3AwZAyCPq3itDv6Dwn08wieZMdarkA0GsznmocTxI99R4Wf/lvCwKtNbyuBMQA7V7oT0U= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=cFLXOi7n; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="cFLXOi7n" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 46DFA1F00893; Mon, 1 Jun 2026 12:12:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1780315978; bh=KQXASZfUusEYAftFy8BR8pLrhMKSi9CEQMMeeezJgJU=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=cFLXOi7nRfYzrRywx2F0As87A2rvawHElB2xDQKhZd3N0312VlH1jAbPqlTa7THzo pmP+gzufeQzffCn/VzZnAPEG8leEu6XO14UoXXHBEtqqwiv6MhaP2PxmJdWojpiz3z ZB7STxUQGlPWETODA7ZYvNLiZs2mRJObUhojbJuDsLovRxS/+z6Hy0CJP/tiGdcl7o idnbBOBxAghycby2uKbVLG2jlHzVWj59AEoO3eVZhDxNzb5HQRioWG6x0QNpcs5Qb5 RyN1NPsQtN10Ec5u+rwHJ7ARQXplIQwPO+w1fm+eJRvec39UsdOwP+NWIPQkhRoCx6 QJBC6aCOP4VaA== Date: Mon, 1 Jun 2026 13:12:54 +0100 From: Lorenzo Stoakes To: Oleg Nesterov Cc: Andrew Morton , Christian Brauner , David Hildenbrand , Jann Horn , Kees Cook , Michal Hocko , linux-kernel@vger.kernel.org, linux-mm@kvack.org, "Liam R. Howlett" Subject: Re: [PATCH v2] mm_access: simplify the security checks Message-ID: References: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: +cc Liam for mm lifecycle stuff :) The subject here seems not quite right - you're adding complexity here in that now there's a racey fast path. On Sat, May 30, 2026 at 04:12:32PM +0200, Oleg Nesterov wrote: > 1. Shift the fast-path "mm == current->mm" check from may_access_mm() > to mm_access(), and do it locklessly. > > task->mm is not stable but we do not care. We can race with exec, > but in this case we pin/return current->mm. This doesn't differ > from the case where the target execs after we drop exec_update_lock. Well it does differ in that previously we increment a reference counter on the mm with the exec lock held, and now we don't? Also how often are we invoking this where mm == current->mm? I think the reasoning here is more so that current is guaranteed to hold a reference on current->mm (de_thread() will have issued a fatal signal not yet processed). So the commit message should say that I think. One behavioural change here though is that down_read_killable() was used previously, so such a situation would return -EINTR, but now would instead succeed. > > All we need for correctness is READ_ONCE() to ensure the compiler > won't reload task->mm. This is not enough for KCSAN, but we already I'm not sure 'this is not enough for KCSAN' is really reassuring :) > have other lockless ->mm LOAD's. We should probably change exec_mmap/ > exit_mm to use WRITE_ONCE(). > > 2. With the change above, may_access_mm() doesn't need the "mm" argument, > so we do not need to call get_task_mm() beforehand. We can call it > only if may_access_mm() succeeds. > > Signed-off-by: Oleg Nesterov > --- It's useful to put a revision history (ideally with links to prior revisions) below the --- line to explain how vN differs from v(N-1). > kernel/fork.c | 30 ++++++++++++++++-------------- > 1 file changed, 16 insertions(+), 14 deletions(-) > > diff --git a/kernel/fork.c b/kernel/fork.c > index b8b651abce8b..3239380ab93b 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1381,10 +1381,8 @@ struct mm_struct *get_task_mm(struct task_struct *task) > } > EXPORT_SYMBOL_GPL(get_task_mm); > > -static bool may_access_mm(struct mm_struct *mm, struct task_struct *task, unsigned int mode) > +static bool may_access_mm(struct task_struct *task, unsigned int mode) > { > - if (mm == current->mm) > - return true; > if (ptrace_may_access(task, mode)) > return true; > if ((mode & PTRACE_MODE_READ) && perfmon_capable()) > @@ -1394,20 +1392,24 @@ static bool may_access_mm(struct mm_struct *mm, struct task_struct *task, unsign > > struct mm_struct *mm_access(struct task_struct *task, unsigned int mode) > { > - struct mm_struct *mm; > - int err; > + struct mm_struct *mm = READ_ONCE(task->mm); > > - err = down_read_killable(&task->signal->exec_update_lock); > - if (err) > - return ERR_PTR(err); > + if (!mm || (task->flags & PF_KTHREAD)) > + return ERR_PTR(-ESRCH); This really needs a comment to explain your reasoning for while READ_ONCE() suffices here. > > - mm = get_task_mm(task); > - if (!mm) { > - mm = ERR_PTR(-ESRCH); > - } else if (!may_access_mm(mm, task, mode)) { > - mmput(mm); > - mm = ERR_PTR(-EACCES); > + if (mm == current->mm) { > + mmget(mm); > + return mm; > } > + > + if (down_read_killable(&task->signal->exec_update_lock)) > + return ERR_PTR(-EINTR); > + > + if (may_access_mm(task, mode)) > + mm = get_task_mm(task) ?: ERR_PTR(-ESRCH); > + else > + mm = ERR_PTR(-EACCES); > + > up_read(&task->signal->exec_update_lock); > > return mm; > -- > 2.52.0 > > (Side-note - we should really have these functions (and anything else mm-related) in mm files.) Overall I'm not really convinced about this patch - this isn't simplifying things, it's introducing subtle assumptions and I don't really see the benefit? So I think it's a no unless you can provide a really solid justification. And if it's a performance thing - how often are we actually calling mm_access() for current->mm? mm lifecycle is a very horrible part of mm and I think we should only make changes when really necessary. Thanks, Lorenzo