linux-kernel - Re: [RFC PATCH 2/3] futex: Add basic infrastructure for local task local hash.

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <87cyjl4u1h.ffs@tglx>
Date: Sun, 27 Oct 2024 13:19:54 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 linux-kernel@...r.kernel.org
Cc: André Almeida <andrealmeid@...lia.com>, Darren Hart
 <dvhart@...radead.org>, Davidlohr Bueso <dave@...olabs.net>, Ingo Molnar
 <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>, Peter Zijlstra
 <peterz@...radead.org>, Valentin Schneider <vschneid@...hat.com>, Waiman
 Long <longman@...hat.com>, Sebastian Andrzej Siewior
 <bigeasy@...utronix.de>
Subject: Re: [RFC PATCH 2/3] futex: Add basic infrastructure for local task
 local hash.

On Sun, Oct 27 2024 at 00:34, Sebastian Andrzej Siewior wrote:
>  static void futex_cleanup(struct task_struct *tsk)
>  {
> +	struct futex_hash_table *fht;
> +	bool need_free = false;
> +
>  	if (unlikely(tsk->robust_list)) {
>  		exit_robust_list(tsk);
>  		tsk->robust_list = NULL;
> @@ -1054,6 +1064,23 @@ static void futex_cleanup(struct task_struct *tsk)
>  
>  	if (unlikely(!list_empty(&tsk->pi_state_list)))
>  		exit_pi_state_list(tsk);
> +
> +	rcu_read_lock();
> +	fht = rcu_dereference(current->futex_hash_table);
> +	if (fht) {
> +
> +		spin_lock(&fht->lock);

I don't think you need a spin lock for this.

> +		fht->users--;
> +		WARN_ON_ONCE(fht->users < 0);
> +		if (fht->users == 0)
> +			need_free = true;
> +		spin_unlock(&fht->lock);
> +		rcu_assign_pointer(current->futex_hash_table, NULL);
> +	}
> +	rcu_read_unlock();

	scoped_guard (rcu) {
        	fht = rcu_dereference(current->futex_hash_table);
                if (fht && rcuref_put_rcusafe(&fht->refcnt)
			rcu_assign_pointer(current->futex_hash_table, NULL);
                else
                        fht = NULL;
	}
	kfree_rcu_mightsleep(fht);

Hmm? But see below

> +static int futex_hash_allocate(unsigned long arg3, unsigned long arg4,
> +			       unsigned long arg5)
> +{
> +	unsigned int hash_slots = arg3;
> +	struct futex_hash_table *fht;
> +	size_t struct_size;
> +	int i;
> +
> +	if (hash_slots == 0)
> +		hash_slots = 4;
> +	if (hash_slots < 2)
> +		hash_slots = 2;
> +	if (hash_slots > 16)
> +		hash_slots = 16;
> +	if (!is_power_of_2(hash_slots))
> +		hash_slots = rounddown_pow_of_two(hash_slots);
> +
> +	if (current->futex_hash_table)
> +		return -EALREADY;

You also need to check whether a hash table exists already in the
process. The table must be process shared to make sense. So it should
live in current->signal, which is a valid pointer in the context of
current.

So this needs some more thoughts especially vs. automatic creation and
sharing.

The first question is whether the hash table might have been already
created when the application reaches main(). If so then this call will
fail.

To make this work correctly, this needs proper serialization as well.

Assume a situation where the application does not allocate a table
upfront and the first futex use happens late when threads are running
already.

CPU0                            CPU1
T1                              T2        
sys_futex()                     sys_futex()
  create_hash()                   create_hash()

So T1 and T2 create their local hash and the subsequent usage will fail
because they operate on different hashs. You have the same problem
vs. your allocation scheme when two threads do prctl(ALLOC). We really
want to make this as simple as possible.

sys_futex()
   ...
   if (private)
      hash = private_hash(current);
   else
      hash = &global_hash;

private_hash()
   hash = READ_ONCE(current->signal->futex_hash);
   if (hash)
      return hash;

   guard(spinlock_irq)(&task->sighand->siglock);
   hash = current->signal->futex_hash;
   if (hash))
   	 return hash;

   hash = alloc_hash();
   WRITE_ONCE(current->signal->futex_hash, hash);
   return hash;

alloc_hash()
   if (!local_hash_enabled)
   	return &global_hash;
   hash = alloc();
   return hash ?: &global_hash;

futex_cleanup_hash()
   scoped_guard(spinlock_irq, &current->sighand->siglock) {
       	hash = current->signal->futex_hash;
        if (!hash || current->signal->nr_threads > 1)
           return;

        current->signal->futex_hash = NULL;
	if (hash == &global_hash)
           return;
   }

   kfree_rcu_mightsleep(hash);

Or something like that.

Thanks,

        tglx