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

[zhongjinji <zhongjinji@honor.com>]

> 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.