Re: [PATCH] Robust-futex wakes up the waiters will be missed

[Thomas Gleixner <tglx@linutronix.de>]

Yi,

On Wed, 6 Nov 2019, Yi Wang wrote:

thanks for addressing Peters comments. Just a few nitpicks.

> Subject: [PATCH] Robust-futex wakes up the waiters will be missed

When you send a new version of a patch then please say so in the subject
line, i.e.

 [PATCH v2] ....

Ideally you also add a short change notice _after_ the '---' seperator
which follows the Signed-off-by lines, i.e.

  Signed-off-by: ......

  ---
  v2: Added a proper comment in handle_futex_death() (Peter)

That's helpful for reviewers to see what you changed, so they know where
they should look for. You obviously do not have to document every tiny
change, but the ones which address review comments and those which change
the implementation. So for a follow up on a large set of comments it's
sufficient to mention classes of changes, e.g.

      - Correct the code flow in foo() (Reviewer 1)
      - Fixup coding style (Reviewer 2)
      - Use inlines instead of macros (Reviewer 1)

Not applicaple here, but you get the idea.

  v1: https://lore.kernel.org/r/1572573789-16557-1-git-send-email-wang.yi59@zte.com.cn

A link to previous versions is helpful for people who were not involved in
the V1 discussion. It makes it easy to follow the history via the archives.
The part after '...org/r/' is the message id of your v1 mail.

This section is not part of the changelog when the patch is applied in a
maintainer tree as the text between the '---' seperator and the start of
the patch is discarded along with the diffstat below.

---
  diffstat ...


Also all subjects should start with a subsystem prefix, in this case
'futex:', i.e.

 [PATCH v2] futex: ....

The easy way to figure the right prefix out is with git:

 git log --oneline kernel/futex.c

That lists the subjects of the commits which touched that file. The most
used one is 'futex:' which makes a lot of sense obviously.

There are a few other prefixes as well due to treewide changes of
infrastructure, but it's again pretty obvious that 'hrtimer:', 'treewide:',
'mm/gup:' etc. are not the right ones.

> We found two scenarios:
> (1)When the owner of a mutex lock with robust attribute and no_pi
> will release the lock and executes pthread_mutex_unlock(),it is
> killed after setting mutex->__data.__lock = 0 and before wake
> others. It will go through the robust process, but it will not wake
> up watiers because mutex->__data.__lock = 0.

...

Just for curiosity. How did you manage to trigger these races?

> We don't need to set "mutex->__data.__count"(in mutex structure),
> which will not affect repeated lock holding.

This is interesting in the context of pthread_mutexes, but irrelevant for
the kernel because the kernel does not know at all how the users space data
structure which contains the futex value looks like. The kernel really does
not care and while the problem happened with pthread_mutex it's the same
for any other implementation which uses the robust futex mechanism.

> Signed-off-by: Yang Tao <yang.tao172@zte.com.cn>

This is missing your Signed-off-by: As you are sending the patch and not
the author you have to add your Signed-off-by after the authors to document
the handling chain. i.e.

  Signed-off-by: Yang Tao <yang.tao172@zte.com.cn>
  Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>

Ok?

> -static int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int pi)
> +static int handle_futex_death(u32 __user *uaddr,
> +			      struct task_struct *curr, int pi,
> +			      bool pending)

If you break the existing line after 'curr,' then you spare an extra one.

>  {
>  	u32 uval, uninitialized_var(nval), mval;
>  	int err;
> @@ -3469,6 +3471,35 @@ static int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int p
>  	if (get_user(uval, uaddr))
>  		return -1;
>  
...

> +	 *	1) task->robust_list->list_op_pending != NULL
> +	 *	2) mutex->__data.__lock = 0 (uval = 0)
> +	 *	3) no_pi
> +	 */
> +	if (pending && !pi && uval == 0)
> +		futex_wake(uaddr, 1, 1, FUTEX_BITSET_MATCH_ANY);

This really wants an explict return here. Yes, the check below returns the
correct value, but it's not obvious at all. The futex code is complex
enough already, no need to introduce code which forces people to look
twice.

Please keep that in mind for future patches. No need to resend this one as
I already picked it up and fixed up the tiny issues already.

Thanks for the great detective work and profund analysis!

       tglx