Re: [PATCH 2/8] afs: free the super info after a grace period

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] afs: TOCTOU race in afs_atcell_get_link() causes NULL dereference
--

commit 7e87d75fc751930aad8ea9a45c609797c83f1e6b
Author: Christian Brauner <brauner@kernel.org>

afs: free the super info after a grace period

afs_atcell_get_link() resolves the @cell and .@cell symlinks of a
dynamic root in rcu pathwalk mode. Before it looks at the dentry it gets
the afs_net through afs_i2net(), which reads ->net_ns from the
afs_super_info in sb->s_fs_info. The @cell dentries are kept around when
unused and the dynamic root has no ->d_revalidate(), so a lookup that
finds @cell in the dcache gets to ->get_link() in rcu mode.

afs_kill_super() frees the afs_super_info with a plain kfree() in
afs_destroy_sbi(). Right now a pathwalk can't still be in
afs_atcell_get_link() at that point, but only by accident. umount(2)
waits for a grace period in namespace_unlock() before it drops the
mount. And a mount whose last reference goes away elsewhere, like the
chdir out of a lazily unmounted directory, still gets a grace period
from bdi_unregister(). afs_fill_super() sets up a private bdi, so
generic_shutdown_super() unregisters it from kill_anon_super() and
bdi_remove_from_list() waits in synchronize_rcu_expedited() before
afs_destroy_sbi() runs. That grace period protects the rcu readers of
bdi_list in the writeback code. It has nothing to do with pathwalk.

Don't rely on it and free the afs_super_info after a grace period.

Fixes: 30bca65bbbae ("afs: Make /afs/@cell and /afs/.@cell symlinks")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

[ ... ]

> diff --git a/fs/afs/super.c b/fs/afs/super.c
> index 82bb713825a0d..33b195821e36a 100644
> --- a/fs/afs/super.c
> +++ b/fs/afs/super.c
> @@ -523,7 +523,7 @@ static void afs_destroy_sbi(struct afs_super_info *as)
>  		afs_put_volume(as->volume, afs_volume_trace_put_destroy_sbi);
>  		afs_unuse_cell(as->cell, afs_cell_trace_unuse_sbi);
>  		put_net(as->net_ns);
> -		kfree(as);
> +		kfree_rcu(as, rcu);

[Severity: High]
Does this regression introduce a TOCTOU race in afs_atcell_get_link() that
can cause a NULL dereference?

By leaving put_net() executing before the RCU grace period, afs_net_exit() can
tear down the namespace while pathwalks are still active. If cleanup_net()
calls afs_cell_purge(), it replaces net->ws_cell with NULL.

Concurrently, a pathwalk in afs_atcell_get_link() can verify net->ws_cell is
non-NULL via rcu_access_pointer(), but the subsequent rcu_dereference() may
return NULL:

fs/afs/dynroot.c:afs_atcell_get_link() {
    ...
	if (!rcu_access_pointer(net->ws_cell))
		return ERR_PTR(-ENOENT);

	if (!dentry) {
		/* We're in RCU-pathwalk. */
		cell = rcu_dereference(net->ws_cell);
		if (dotted)
			name = cell->name - 1;
    ...
}

If the thread is preempted and afs_cell_purge() runs, could cell be NULL here?

The same appears to happen in the non-RCU pathwalk:

fs/afs/dynroot.c:afs_atcell_get_link() {
    ...
	down_read(&net->cells_lock);

	cell = rcu_dereference_protected(net->ws_cell,
					 lockdep_is_held(&net->cells_lock));
	if (dotted)
		name = cell->name - 1;
	else
		name = cell->name;
    ...
}

Can net->ws_cell be cleared to NULL before the lock is acquired, causing a
NULL dereference here as well?

>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-work-fixes-rcu-v1-0-7446d2d335e6@kernel.org?part=2