From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f74.google.com (mail-wr1-f74.google.com [209.85.221.74]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3E2DC800 for ; Fri, 5 Dec 2025 18:17:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.74 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764958634; cv=none; b=grqO9ytzPzMJ2LaV8/T0s4Ihr3kbyf1YwJy9MLO7kjTD5yJpmp3nEH3kBKwzVIE+mn15abb3FtaITvPgguLqyQFSUte6Ou7YTsagDNFS1ihkItoIN7CDLUCCKB4eRgE36xi8bn5WPH9lmL8BgbT5x3z4qbB9EMx+X3UfYS+YkiI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764958634; c=relaxed/simple; bh=kfOxdGTe7tI9bBSpavpRUT6P4uC27C+LWQwk0BcUY+Q=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=HjJwGBUoqHaU9ZMvxP/8hAXR2+9oGmG1l7w/wqqN5DytzFy4/sJEc7pwE6MimPBcC2NdZLG/MPZJ+I5Q+XA+LvXrVudIMJKBOgM4JrP553kufLfZOi68tgszogxOiVHBM+YJsa0AUjFrrOIpi7DEd3lXL1Pia58tG5WeS+0Z/LE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=Jg7xaLFb; arc=none smtp.client-ip=209.85.221.74 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--aliceryhl.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="Jg7xaLFb" Received: by mail-wr1-f74.google.com with SMTP id ffacd0b85a97d-42e2d105358so1620118f8f.3 for ; Fri, 05 Dec 2025 10:17:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1764958630; x=1765563430; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=gYtQdVewYPYK+nwYYWgxTT03HxE0FT7b0fetIYX5WBk=; b=Jg7xaLFbz97p998djwI2IY634HCAu0I13+HXfQ3Y1RTx00g8Pr1ZVEhRzt6f6UGTN/ 6OxT/0VrFTpE8cI5j0qrg93JALhOgP5vBlVcPoyci4eIqVuIuuBDdJS3wI+9vRYHYQJP 25x9VeNGJK27cE7YTb+VUhQ1mtlLKHyd/06E6BGaw5hK7XO9j7h0yB+fBz8GdTJZWhiK ICZLNYDmc2SMdl/3IVKN2ACW4O83evtYk5GHQBQe+8Q3xyP83CR3+Ej1B+ui6n222Abd Csmp8vvXsBM3feyge0nzW10cCCG3HmpWVYP0AFoyf5kxK7Toi4EM2YER3YuFc5mogVF1 PeKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764958631; x=1765563431; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=gYtQdVewYPYK+nwYYWgxTT03HxE0FT7b0fetIYX5WBk=; b=qFMjPixU6fsEla1wA3tWR1JlBm/EzukHB3MaPZZzCbyScxhb6MUJwu7dR4rBTVWT6Y +J9fCxj0x1AsvgMg8KJvK4nZSjUdA5OaGSHwBNEE6DpaEa/xZt3GzaD7ncrXpMg/pe4N SNMrli+j1wj4naM88rhcRy7+x6+Hfq+JEHF/7F3S+xw+1yW10ou5aHqc6YTf7vdKiqGD BD/O2Ew7huqVxNJ8l7UvFm5Qw62MrCAEHWOAnNvUxB4wBKD1T05sED+qriOVb0w8uXk0 +yMQuKZmWwUIedaaxMErZZWhorZb1SxGj09pZCCjbVIu7CGoGjrvn5+eCenA9D0LajUz yLNQ== X-Forwarded-Encrypted: i=1; AJvYcCVOIVtmpt86CiYmsP4m5rVss/6vWblvWlRu3vFG2Pmaobd4ENmfVAPwjcsKSXds3JEzzzwsXPbuomW0ZUQ=@vger.kernel.org X-Gm-Message-State: AOJu0YxCuID8xekkxhUjR+B31x13ANyspex99HhGzLtDKqRyeipZQFC5 sZxndUl0Ln+oMfC7AdJMJPDQVkSTWxoV8ZjsX439cOvLU1cRIy/7tB4oRurjDjDSHPmahbuLzqK J8V35Xf/rUs2Dss218w== X-Google-Smtp-Source: AGHT+IFbgI6+zFr2WThM/jHji6RSillc9y0ALmMhNF3P25uuiyAh3pxG84bb+AyWqmj9XeWywvftwi9tJFedKmY= X-Received: from wruq1.prod.google.com ([2002:a5d:6581:0:b0:429:8ccf:4ae7]) (user=aliceryhl job=prod-delivery.src-stubby-dispatcher) by 2002:a05:6000:608:b0:42b:396e:2817 with SMTP id ffacd0b85a97d-42f89f48530mr105514f8f.40.1764958630611; Fri, 05 Dec 2025 10:17:10 -0800 (PST) Date: Fri, 5 Dec 2025 18:17:09 +0000 In-Reply-To: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: Message-ID: Subject: Re: rust: wrong SAFETY comments in group_leader() and pid() + questions From: Alice Ryhl To: Oleg Nesterov Cc: Christian Brauner , Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , "=?utf-8?B?QmrDtnJu?= Roy Baron" , Benno Lossin , Andreas Hindborg , Trevor Gross , Danilo Krummrich , Panagiotis Foliadis , Shankari Anand , FUJITA Tomonori , Alexey Gladkov , rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="utf-8" On Fri, Dec 05, 2025 at 06:21:46PM +0100, Oleg Nesterov wrote: > On 12/05, Alice Ryhl wrote: > > > > pub fn group_leader(&self) -> &Task { > > // SAFETY: The lifetime of the returned task reference is tied to > > // the lifetime of `self`, and given that a task has a reference to > > // its group leader, we know it must be valid for the lifetime of > > // the returned task reference. > > unsafe { &*bindings::task_group_leader(self.as_ptr()).cast::() } > > } > > Thanks again Alice, but the comment still looks misleading to me... > OK, quite possibly this is because I don't understand what does the > "lifetime of the returned task reference" actually mean in the rust code. > Does it mean "lifetime of task_struct" of "lifetime of the process/thread" ? To start with, it's likely that this comment is not the right choice for this function, given our discussion. Most likely group_leader() needs to be moved to `impl CurrentTask {}` and the safety comment needs to explain why being the current task ensures that the returned &Task lives for long enough. I just took the safety comment from the code we have today. To explain "lifetime of the returned task reference" it may be useful to show you the long form of this function signature: fn group_leader<'a>(&'a self) -> &'a Task; By "lifetime of the returned task reference" I am referring to 'a, which is some region of code in the caller. For example, 'a might be: * The region of code until I put the task I called group_leader() on. * The region of code between a rcu_read_lock() / rcu_read_unlock() pair. * The region of code until I release a mutex. * The region of code inside a scope or function. So for example, with the code as it is today, this example: let leader = task.group_leader(); drop(task); // put refcount do_something(leader); // COMPILE ERROR Rust will see that `task` may no longer be valid after the second line, so 'a becomes the region of code until drop(task), and the use of leader after the end of 'a is a compilation error. Now, the above example is not really valid. It works today, but once we fix group_leader() to require the task to be the current task, then the example might look like this: let leader; { let current = current!(); leader = current.leader(); do_something(leader); // OK } do_something(leader); // COMPILE ERROR And ok, this example isn't great because the last line is actually OK. However, the way that I designed current!() to work is that it gives you an `&'a CurrentTask` where 'a is the duration of the current scope. So any use of `leader` after the end of the current scope is a compile error. > Let me provide the artificial example. Suppose we have something like > > struct task_struct *TASK = NULL; > > void stupid_example(void) > { > TASK = get_task_struct(current); > > do_exit(0); > } > > and a non-leader task calls stupid_example(). > > After that the global TASK pointer is still valid, it is safe to > dereference it, task_struct itself can't go away. This would not compile in Rust. The &CurrentTask obtained using current!() can't leave the scope it is created in. > But! Right after that TASK->group_leader can point to nowhere (to the freed memory) > if another thread does do_group_exit() or sys_execve(). > > So. Perhaps the the comment should say something like > > SAFETY: The lifetime of the returned task reference is tied to > the lifetime of the THREAD represented by `self` > > ? I think the safety comment should make some reference to the fact that we know it was obtained from the current task. After all, that's why we know it's ok. impl CurrentTask { fn group_leader(&self) -> &Task { // SAFETY: This is the current task, so the task must be alive. // Therefore the group leader cannot change, and thus it will // stay valid as long as self is the current task. unsafe { &*bindings::task_group_leader(self.as_ptr()).cast::() } } } Alice