linux-kernel - Re: [RESEND PATCH v7 1/8] kernfs: Introduce interface to access global kernfs_open_file

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: <0dfe1056-3dc5-4d31-698e-e2c075ffd6ee@oracle.com>
Date:   Wed, 6 Apr 2022 14:54:19 +1000
From:   Imran Khan <imran.f.khan@...cle.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     tj@...nel.org, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH v7 1/8] kernfs: Introduce interface to access
 global kernfs_open_file_mutex.

Hello Al,

On 6/4/22 12:24 am, Al Viro wrote:
[...]
> 
> What for?  Again, have kernfs_drain_open_files() do this:
> {
>         struct kernfs_open_node *on;
> 	struct kernfs_open_file *of;
> 
> 	if (!(kn->flags & (KERNFS_HAS_MMAP | KERNFS_HAS_RELEASE)))
> 		return;
> 	if (rcu_dereference(kn->attr.open) == NULL)
> 		return;
> 	mutex_lock(&kernfs_open_file_mutex);
> 	// now ->attr.open is stable (all stores are under kernfs_open_file_mutex)
> 	on = rcu_dereference(kn->attr.open);
> 	if (!on) {
> 		mutex_unlock(&kernfs_open_file_mutex);
> 		return;
> 	}
> 	// on->files contents is stable
> 	list_for_each_entry(of, &on->files, list) {
> 		struct inode *inode = file_inode(of->file);
> 
> 		if (kn->flags & KERNFS_HAS_MMAP)
> 			unmap_mapping_range(inode->i_mapping, 0, 0, 1);
> 
> 		if (kn->flags & KERNFS_HAS_RELEASE)
> 			kernfs_release_file(kn, of);
> 	}
> 	mutex_unlock(&kernfs_open_file_mutex);
> }
> 

I did something similar in in [1], except that I was traversing
on->files under rcu_read_lock and this was a source of confusion.

> What's the problem?  The caller has already guaranteed that no additions will
> happen.  Once we'd grabbed kernfs_open_file_mutex, we know that
> 	* kn->attr.open value won't change until we drop the mutex
> 	* nothing gets removed from kn->attr.open->files until we drop the mutex
> so we can bloody well walk that list, blocking as much as we want.
> 
> We don't need rcu_read_lock() there - we are already holding the mutex used
> by writers for exclusion among themselves.  RCU *allows* lockless readers,
> it doesn't require all readers to be such.  kernfs_notify() can be made
> lockless, this one can't and that's fine.
> 

Thanks for explaining this. I missed the exclusiveness being provided by
kernfs_open_file_mutex in this case.

> BTW, speaking of kernfs_notify() - can calls of that come from NMI handlers?
> If not, I'd consider using llist for kernfs_notify_list...

I see it gets invoked from 3 places only: cgroup_file_notify,
sysfs_notify and sysfs_notify_dirent. So kernfs_notify should not be
getting invoked in NMI context. I will make the llist transition in next
version.

Thanks,
-- Imran

[1]
https://lore.kernel.org/lkml/20220324103040.584491-3-imran.f.khan@oracle.com/