linux-kernel - Re: [PATCH 2/3] vfs: Use the mounts_to_id array to do /proc/mounts and co.

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]

Date:   Mon, 15 Mar 2021 12:54:40 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     David Howells <dhowells@...hat.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Miklos Szeredi <miklos@...redi.hu>,
        Ian Kent <raven@...maw.net>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] vfs: Use the mounts_to_id array to do /proc/mounts
 and co.

On Mon, Mar 15, 2021 at 12:07:56PM +0000, David Howells wrote:
> Use the mounts_to_id xarray added to the mount namespace to perform

You called it mounts_by_id in the last patch ...

> Since it doesn't trawl a standard list_head, but rather uses xarray, this
> could be done under the RCU read lock only.  To do this, we would need to
> hide mounts that are in the process of being inserted into the tree by
> marking them in the xarray itself or using a mount flag.

>  /* iterator; we want it to have access to namespace_sem, thus here... */
>  static void *m_start(struct seq_file *m, loff_t *pos)
>  {
> -	struct proc_mounts *p = m->private;
> -	struct list_head *prev;
> +	struct proc_mounts *state = m->private;
> +	void *entry;
>  
>  	down_read(&namespace_sem);
> -	if (!*pos) {
> -		prev = &p->ns->list;
> -	} else {
> -		prev = &p->cursor.mnt_list;
> +	state->xas = (struct xa_state) __XA_STATE(&state->ns->mounts_by_id, *pos, 0, 0);
>  
> -		/* Read after we'd reached the end? */
> -		if (list_empty(prev))
> -			return NULL;
> -	}
> +	entry = xas_find(&state->xas, ULONG_MAX);

I know you haven't enabled enough debugging because this will assert
that either the RCU read lock or the xa_lock is held to prevent xa_nodes
from disappearing underneath us.

Why do you want to use an xa_state for this?  This is /proc, so efficiency
isn't the highest priority.  I'd just use xa_find(), and then you don't
need to care about an xa_state or locking -- it handles taking the rcu
read lock for you.

> +	while (entry && xas_invalid(entry))

I've never seen anybody make that mistake before.  Good one.  Not sure
if there's anything I can do to prevent it in future.

> +		entry = xas_next_entry(&state->xas, ULONG_MAX);