[PATCH] SUNRPC: serialize pipedir removal on the rpc_client_register() error path

[PATCH] SUNRPC: serialize pipedir removal on the rpc_client_register() error path

[Joas Antonio dos Santos]

When rpcauth_create() fails, rpc_client_register() calls
rpc_get_sb_net() and then __rpc_clnt_remove_pipedir() whether or not a
pipefs superblock was found.  If rpc_get_sb_net() returns NULL, the
removal runs without sn->pipefs_sb_lock, and the client is still on
sn->all_clients until rpc_unregister_client() a line later.  Pipefs
mount/umount notifications run under pipefs_sb_lock and walk that list,
so they can race with it:

 - a pipefs mount between rpc_get_sb_net() and rpc_unregister_client()
   gives the client a new directory, which the unlocked
   rpc_remove_client_dir() then removes while a concurrent umount of
   that superblock drops the same dentries in select_collect_umount();

 - a umount notification that picked the client up before it was
   unregistered keeps using it after rpc_new_client() has freed it.

syzbot hit the first one when racing rpc_pipefs mount/umount with
NFSv4 mounts (the krb5i rpcauth_create() fails without the gss mech):

  WARNING: fs/dcache.c:1063 at d_make_discardable+0x28e/0x2f0 fs/dcache.c:1063
   __simple_recursive_removal+0x276/0x5c0 fs/libfs.c:625
   rpc_remove_client_dir+0xf0/0x140 net/sunrpc/rpc_pipe.c:898
   __rpc_clnt_remove_pipedir net/sunrpc/clnt.c:100 [inline]
   rpc_client_register+0x605/0x6f0 net/sunrpc/clnt.c:333
   rpc_new_client+0x977/0x1300 net/sunrpc/clnt.c:437

With a test-only msleep() after rpc_get_sb_net() returns NULL, the
syzbot reproducer also triggers the second one:

  BUG: KASAN: slab-use-after-free in rpc_pipefs_event+0x568/0x57c
  Write of size 8 at addr fff000000ec53240 by task rpc_repro/107
   rpc_pipefs_event+0x568/0x57c
   notifier_call_chain+0xfc/0x384
   blocking_notifier_call_chain+0x68/0x9c
   rpc_kill_sb+0xbc/0x1a8
   deactivate_locked_super+0x98/0x11c
  Allocated by task 111:
   rpc_new_client+0x144/0xf04
  Freed by task 111:
   kfree+0x1dc/0x5c8
   rpc_new_client+0x9f8/0xf04

Do what rpc_free_client() does instead: unregister the client first, so
no later notification can find it, then remove the directory with
rpc_clnt_remove_pipedir(), which takes pipefs_sb_lock (waiting for a
notification that is still running) and only removes it if the client's
superblock is the current one.

Fixes: eeee245268c9 ("SUNRPC: Fix a deadlock in rpc_client_register()")
Reported-by: syzbot+2e3c70a51d4ac0f6bdd9@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2e3c70a51d4ac0f6bdd9
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
Tested on Linus' master c339f4010dea (arm64, QEMU, generic KASAN,
PROVE_LOCKING, NFSv4 without RPCSEC_GSS_KRB5) with the syzbot C
reproducer.  The race window is narrow: the plain reproducer did not
hit it here in 6 runs, with or without this patch.  To widen it, a
test-only msleep(20) was added on the err_auth path (after
rpc_get_sb_net() returns NULL in the unpatched code, and between
rpc_unregister_client() and the removal with this patch):

  unpatched + delay:  2 of 2 boots, 16-22 WARNINGs each (mostly
                      d_make_discardable, plus fast_dput, and umount_check /
                      generic_shutdown_super once) and one KASAN
                      slab-use-after-free each (rpc_pipefs_event in one boot,
                      rpc_create_client_dir in the other)
  patched + delay:    3 of 3 boots (9 runs), no WARNING, KASAN or lockdep
                      report
  patched, no delay:  2 of 2 boots clean

net/sunrpc/clnt.o builds cleanly with W=1.

 net/sunrpc/clnt.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 6cedc82..16aff2a 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -328,12 +328,18 @@ static int rpc_client_register(struct rpc_clnt *clnt,
 	}
 	return 0;
 err_auth:
-	pipefs_sb = rpc_get_sb_net(net);
+	/*
+	 * Unregister first so that a concurrent pipefs mount cannot give the
+	 * client a new directory, then remove the directory under
+	 * pipefs_sb_lock like rpc_free_client() does.
+	 */
 	rpc_unregister_client(clnt);
-	__rpc_clnt_remove_pipedir(clnt);
+	rpc_clnt_remove_pipedir(clnt);
+	goto out_destroy;
 out:
 	if (pipefs_sb)
 		rpc_put_sb_net(net);
+out_destroy:
 	rpc_sysfs_client_destroy(clnt);
 	rpc_clnt_debugfs_unregister(clnt);
 	return err;
-- 
2.39.5