Re: linux-next: manual merge of the tip-core tree with Linus' tree

[Darren Hart]

Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the tip-core tree got a conflict in
> kernel/futex.c between commit 64d1304a64477629cb16b75491a77bafe6f86963
> ("futex: setup writeable mapping for futex ops which modify user space
> data") from Linus' tree and a couple of commits from the tip-core tree.
>
> I fixed it up (see below - but please check in particular, I have no idea
> if the call to get_futex_key() in futex_wait_requeue_pi() should take
> VERIFY_READ or VERIFY_WRITE).
>
> I can carry this fixup as necessary.

I suspect Thomas will be catching that up in the next day or so in tip
(or I can).  If you need something now, the uaddr is VERIFY_READ and
uaddr2 is VERIFY_WRITE.  See below...

> --
> Cheers,
> Stephen Rothwell                    sfr@xxxxxxxxxxxxxxxx
>
> diff --cc kernel/futex.c
> index d546b2d,0c406a3..0000000
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@@ -813,13 -1092,43 +1094,43 @@@ static int futex_requeue(u32 __user *ua
>   	struct futex_hash_bucket *hb1, *hb2;
>   	struct plist_head *head1;
>   	struct futex_q *this, *next;
> - 	int ret, drop_count = 0;
> + 	u32 curval2;
> + 
> + 	if (requeue_pi) {
> + 		/*
> + 		 * requeue_pi requires a pi_state, try to allocate it now
> + 		 * without any locks in case it fails.
> + 		 */
> + 		if (refill_pi_state_cache())
> + 			return -ENOMEM;
> + 		/*
> + 		 * requeue_pi must wake as many tasks as it can, up to nr_wake
> + 		 * + nr_requeue, since it acquires the rt_mutex prior to
> + 		 * returning to userspace, so as to not leave the rt_mutex with
> + 		 * waiters and no owner.  However, second and third wake-ups
> + 		 * cannot be predicted as they involve race conditions with the
> + 		 * first wake and a fault while looking up the pi_state.  Both
> + 		 * pthread_cond_signal() and pthread_cond_broadcast() should
> + 		 * use nr_wake=1.
> + 		 */
> + 		if (nr_wake != 1)
> + 			return -EINVAL;
> + 	}
>   
>   retry:
> + 	if (pi_state != NULL) {
> + 		/*
> + 		 * We will have to lookup the pi_state again, so free this one
> + 		 * to keep the accounting correct.
> + 		 */
> + 		free_pi_state(pi_state);
> + 		pi_state = NULL;
> + 	}
> + 
>  -	ret = get_futex_key(uaddr1, fshared, &key1);
>  +	ret = get_futex_key(uaddr1, fshared, &key1, VERIFY_READ);
>   	if (unlikely(ret != 0))
>   		goto out;
>  -	ret = get_futex_key(uaddr2, fshared, &key2);
>  +	ret = get_futex_key(uaddr2, fshared, &key2, VERIFY_READ);

This one should be VERIFY_WRITE in the case of requeue_pi==1 as we
attempt to take this futex on behalf of the waiting task (either setting
the FUTEX_HAS_WAITERS flag or setting it as the owner).  So maybe
something like:

+	ret = get_futex_key(uaddr2, fshared, &key2,
			    requeue_pi == 1 ? VERIFY_WRITE : VERIFY_READ);

Alternatively just put it in an if/else block to avoid the inline
tertiary operator.

> + static int futex_wait_requeue_pi(u32 __user *uaddr, int fshared,
> + 				 u32 val, ktime_t *abs_time, u32 bitset,
> + 				 int clockrt, u32 __user *uaddr2)
> + {
> + 	struct hrtimer_sleeper timeout, *to = NULL;
> + 	struct rt_mutex_waiter rt_waiter;
> + 	struct rt_mutex *pi_mutex = NULL;
> + 	DECLARE_WAITQUEUE(wait, current);
> + 	struct restart_block *restart;
> + 	struct futex_hash_bucket *hb;
> + 	union futex_key key2;
> + 	struct futex_q q;
> + 	int res, ret;
> + 	u32 uval;
> + 
> + 	if (!bitset)
> + 		return -EINVAL;
> + 
> + 	if (abs_time) {
> + 		to = &timeout;
> + 		hrtimer_init_on_stack(&to->timer, clockrt ? CLOCK_REALTIME :
> + 				      CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> + 		hrtimer_init_sleeper(to, current);
> + 		hrtimer_set_expires_range_ns(&to->timer, *abs_time,
> + 					     current->timer_slack_ns);
> + 	}
> + 
> + 	/*
> + 	 * The waiter is allocated on our stack, manipulated by the requeue
> + 	 * code while we sleep on uaddr.
> + 	 */
> + 	debug_rt_mutex_init_waiter(&rt_waiter);
> + 	rt_waiter.task = NULL;
> + 
> + 	q.pi_state = NULL;
> + 	q.bitset = bitset;
> + 	q.rt_waiter = &rt_waiter;
> + 
> + 	key2 = FUTEX_KEY_INIT;
>  -	ret = get_futex_key(uaddr2, fshared, &key2);
> ++	ret = get_futex_key(uaddr2, fshared, &key2, VERIFY_READ);

This one should be VERIFY_WRITE (it's the pi futex, so we set it's value
here in the kernel).

Thanks,

--
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/