linux-kernel - Re: [PATCH v8 02/10] kernfs: make ->attr.open RCU protected.

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: <YmLVrfI4UN8ajoUM@slm.duckdns.org>
Date:   Fri, 22 Apr 2022 06:19:57 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Imran Khan <imran.f.khan@...cle.com>
Cc:     viro@...iv.linux.org.uk, gregkh@...uxfoundation.org,
        ebiederm@...ssion.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 02/10] kernfs: make ->attr.open RCU protected.

On Sun, Apr 10, 2022 at 12:37:11PM +1000, Imran Khan wrote:
>  static int kernfs_seq_show(struct seq_file *sf, void *v)
>  {
>  	struct kernfs_open_file *of = sf->private;
> +	struct kernfs_open_node *on = rcu_dereference_raw(of->kn->attr.open);

I suppose raw deref is safe because @on can't go away while @of is alive,
right? If that's the case, please factor out of -> on dereferencing into a
helper and put a comment there explaining why the raw deref is safe.

...
> @@ -201,7 +203,8 @@ static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  		goto out_free;
>  	}
>  
> -	of->event = atomic_read(&of->kn->attr.open->event);
> +	on = rcu_dereference_raw(of->kn->attr.open);

cuz we don't wanna sprinkle raw derefs in multiple places without
explanation like this.

...
>  /**
> @@ -566,24 +567,30 @@ static int kernfs_get_open_node(struct kernfs_node *kn,
>  static void kernfs_put_open_node(struct kernfs_node *kn,
>  				 struct kernfs_open_file *of)
>  {
> -	struct kernfs_open_node *on = kn->attr.open;
> -	unsigned long flags;
> +	struct kernfs_open_node *on;
> +
> +	/* ->attr.open NULL means there are no more open files */
> +	if (rcu_dereference_raw(kn->attr.open) == NULL)
> +		return;

For pointer value check, what you want is rcu_access_pointer(). That said,
tho, why is this being called if no one is linked on it? Before removing the
refcnt, that'd be the same as trying to put a 0 ref. How does that happen?
Also, can you please rename it to unlink or something of the sort? It's
confusing to call it put when there's no refcnt.

>  
>  	mutex_lock(&kernfs_open_file_mutex);
> -	spin_lock_irqsave(&kernfs_open_node_lock, flags);
> +
> +	on = rcu_dereference_protected(kn->attr.open,
> +				       lockdep_is_held(&kernfs_open_file_mutex));

Again, a better way to do it would be defining a kn -> on accessor which
encodes the safe way to deref and use it. The deref rule is tied to the
deref itself not the callsite.

> +
> +	if (!on) {
> +		mutex_unlock(&kernfs_open_file_mutex);
> +		return;
> +	}
>  
>  	if (of)
>  		list_del(&of->list);
>  
> -	if (list_empty(&on->files))
> -		kn->attr.open = NULL;
> -	else
> -		on = NULL;
> -
> -	spin_unlock_irqrestore(&kernfs_open_node_lock, flags);
> +	if (list_empty(&on->files)) {
> +		rcu_assign_pointer(kn->attr.open, NULL);
> +		kfree_rcu(on, rcu_head);
> +	}
>  	mutex_unlock(&kernfs_open_file_mutex);
> -
> -	kfree(on);
>  }
>  
>  static int kernfs_fop_open(struct inode *inode, struct file *file)
> @@ -765,12 +772,13 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
>  	if (!(kn->flags & (KERNFS_HAS_MMAP | KERNFS_HAS_RELEASE)))
>  		return;
>  
> -	on = kn->attr.open;
> -	if (!on)
> +	if (rcu_dereference_raw(kn->attr.open) == NULL)
>  		return;

rcu_access_pointer again and you gotta explain why the lockless check is
safe.

>  	mutex_lock(&kernfs_open_file_mutex);
> -	if (!kn->attr.open) {
> +	on = rcu_dereference_check(kn->attr.open,
> +				   lockdep_is_held(&kernfs_open_file_mutex));
> +	if (!on) {
>  		mutex_unlock(&kernfs_open_file_mutex);
>  		return;
>  	}
...
> @@ -912,14 +920,13 @@ void kernfs_notify(struct kernfs_node *kn)
>  		return;
>  
>  	/* kick poll immediately */
> -	spin_lock_irqsave(&kernfs_open_node_lock, flags);
> -	on = kn->attr.open;
> +	rcu_read_lock();
> +	on = rcu_dereference(kn->attr.open);
>  	if (on) {
>  		atomic_inc(&on->event);
>  		wake_up_interruptible(&on->poll);
>  	}
> -	spin_unlock_irqrestore(&kernfs_open_node_lock, flags);
> -
> +	rcu_read_unlock();

An explanation of why this is safe in terms of event ordering would be great
here.

Thanks.

-- 
tejun