linux-kernel - Re: [[PATCH v2] 1/2] futex: Add check_robust_futex to verify process usage of robust

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250812132103.9910-1-zhongjinji@honor.com>
Date: Tue, 12 Aug 2025 21:21:03 +0800
From: zhongjinji <zhongjinji@...or.com>
To: <tglx@...utronix.de>
CC: <akpm@...ux-foundation.org>, <andrealmeid@...lia.com>,
	<dave@...olabs.net>, <dvhart@...radead.org>, <feng.han@...or.com>,
	<linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>, <liulu.liu@...or.com>,
	<mhocko@...e.com>, <mingo@...hat.com>, <npache@...hat.com>,
	<peterz@...radead.org>, <rientjes@...gle.com>, <shakeel.butt@...ux.dev>,
	<zhongjinji@...or.com>
Subject: Re: [[PATCH v2] 1/2] futex: Add check_robust_futex to verify process usage of robust_futex

> Please use foo() notation for functions in subject and change log.
> 
> > The check_robust_futex function is added to detect whether a process uses
> > robust_futex.
> 
> Explain the problem first and do not start with what the patch is doing.
> 
> > According to the patch discussion
> > (https://lore.kernel.org/all/20220414144042.677008-1-npache@redhat.com/T/#u),
> 
> Can you properly describe what you are trying to solve as part of the
> change log? A link can be provided for further information, but not
> instead of a proper explanation.
> 
> > executing the OOM reaper too early on processes using robust_futex may cause
> > the lock holder to wait indefinitely.
> >
> > Therefore, this patch introduces check_robust_futex to identify such
> 
> # git grep 'This patch' Documentation/process/
> 
> See also:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> 
> > +bool __check_robust_futex(struct task_struct *p)
> > +{
> > +	struct task_struct *t;
> > +
> > +	for_each_thread(p, t) {
> > +		if (unlikely(t->robust_list))
> 
> This is a racy access as the thread might concurrently write to it. So
> it has to be annotated with data_race().
> 
> > +			return true;
> > +#ifdef CONFIG_COMPAT
> > +		if (unlikely(t->compat_robust_list))
> > +			return true;
> > +#endif
> > +	}
> > +	return false;
> > +}
> > +
> > +bool check_robust_futex(struct task_struct *p)
> 
> The name sucks. Public futex functions are prefixed with
> futex.
> 
> But this is about checking a process, no? So something like
> process_has_robust_futex() makes it clear what this is about.
> 
> > +{
> > +	bool has_robust;
> > +
> > +	rcu_read_lock();
> > +	has_robust = __check_robust_futex(p);
> > +	rcu_read_unlock();
> > +	return has_robust;
> > +}
> 
> Why do you need two functions here?
> 
> If the OOM killer is invoked, then saving a rcu_read_lock()/unlock() is
> just a pointless optimization with zero value. rcu_read_lock() nests
> nicely.
> 
> But I'm not convinced yet, that this is actually a sane approach.
> 
> Thanks,
> 
>         tglx

Thank you very much for your review. I will fix them in the next version.