Re: [[PATCH v2] 1/2] futex: Add check_robust_futex to verify process usage of robust_futex

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, Aug 01 2025 at 23:36, zhongjinji@honor.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