linux-kernel - Re: [PATCH v2 3/4] rcu/kvfree: Move need_offload

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: <20221129233833.GA154809@paulmck-ThinkPad-P17-Gen-1>
Date:   Tue, 29 Nov 2022 15:38:33 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     "Uladzislau Rezki (Sony)" <urezki@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, RCU <rcu@...r.kernel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Neeraj Upadhyay <neeraj.iitr10@...il.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Oleksiy Avramchenko <oleksiy.avramchenko@...y.com>
Subject: Re: [PATCH v2 3/4] rcu/kvfree: Move need_offload_krc() out of
 krcp->lock

On Tue, Nov 29, 2022 at 04:58:21PM +0100, Uladzislau Rezki (Sony) wrote:
> Currently a need_offload_krc() function requires the krcp->lock
> to be held because krcp->head can not be checked concurrently.
> 
> Fix it by updating the krcp->head using WRITE_ONCE() macro so
> it becomes lock-free and safe for readers to see a valid data
> without any locking.

Don't we also need to use READ_ONCE() for the code loading this krcp->head
pointer?  Or do the remaining plain C-language accesses somehow avoid
running concurrently with those new WRITE_ONCE() invocations?

						Thanx, Paul

> Signed-off-by: Uladzislau Rezki (Sony) <urezki@...il.com>
> ---
>  kernel/rcu/tree.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 445f8c11a9a3..c94c17194299 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3058,7 +3058,7 @@ static void kfree_rcu_monitor(struct work_struct *work)
>  			// objects queued on the linked list.
>  			if (!krwp->head_free) {
>  				krwp->head_free = krcp->head;
> -				krcp->head = NULL;
> +				WRITE_ONCE(krcp->head, NULL);
>  			}
>  
>  			WRITE_ONCE(krcp->count, 0);
> @@ -3072,6 +3072,8 @@ static void kfree_rcu_monitor(struct work_struct *work)
>  		}
>  	}
>  
> +	raw_spin_unlock_irqrestore(&krcp->lock, flags);
> +
>  	// If there is nothing to detach, it means that our job is
>  	// successfully done here. In case of having at least one
>  	// of the channels that is still busy we should rearm the
> @@ -3079,8 +3081,6 @@ static void kfree_rcu_monitor(struct work_struct *work)
>  	// still in progress.
>  	if (need_offload_krc(krcp))
>  		schedule_delayed_monitor_work(krcp);
> -
> -	raw_spin_unlock_irqrestore(&krcp->lock, flags);
>  }
>  
>  static enum hrtimer_restart
> @@ -3250,7 +3250,7 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr)
>  
>  		head->func = ptr;
>  		head->next = krcp->head;
> -		krcp->head = head;
> +		WRITE_ONCE(krcp->head, head);
>  		success = true;
>  	}
>  
> @@ -3327,15 +3327,12 @@ static struct shrinker kfree_rcu_shrinker = {
>  void __init kfree_rcu_scheduler_running(void)
>  {
>  	int cpu;
> -	unsigned long flags;
>  
>  	for_each_possible_cpu(cpu) {
>  		struct kfree_rcu_cpu *krcp = per_cpu_ptr(&krc, cpu);
>  
> -		raw_spin_lock_irqsave(&krcp->lock, flags);
>  		if (need_offload_krc(krcp))
>  			schedule_delayed_monitor_work(krcp);
> -		raw_spin_unlock_irqrestore(&krcp->lock, flags);
>  	}
>  }
>  
> -- 
> 2.30.2
>