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 for Android: free password hash cracker in your pocket

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

Message-ID: <87cy99g3k6.ffs@tglx>
Date: Tue, 05 Aug 2025 18:02:01 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: zhongjinji@...or.com, linux-mm@...ck.org
Cc: akpm@...ux-foundation.org, mhocko@...e.com, rientjes@...gle.com,
 shakeel.butt@...ux.dev, npache@...hat.com, linux-kernel@...r.kernel.org,
 mingo@...hat.com, peterz@...radead.org, dvhart@...radead.org,
 dave@...olabs.net, andrealmeid@...lia.com, liulu.liu@...or.com,
 feng.han@...or.com
Subject: Re: [[PATCH v2] 1/2] futex: Add check_robust_futex to verify
 process usage of robust_futex

On Fri, Aug 01 2025 at 23:36, zhongjinji@...or.com wrote:

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