[PATCH] 9p: p9_client_create: use p9_client_destroy on failure

From: Dominique Martinet
Date: Wed Aug 17 2022 - 01:49:29 EST


If trans was connected it's somehow possible to fail with requests in
flight that could still be accessed after free if we just free the clnt
on failure.
Just use p9_client_destroy instead that has proper safeguards.

Reported-by: syzbot+de52531662ebb8823b26@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>

diff --git a/net/9p/client.c b/net/9p/client.c
index 5bf4dfef0c70..da5d43848600 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -948,7 +948,7 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)

err = parse_opts(options, clnt);
if (err < 0)
- goto free_client;
+ goto out;

if (!clnt->trans_mod)
clnt->trans_mod = v9fs_get_default_trans();
@@ -957,7 +957,7 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
err = -EPROTONOSUPPORT;
p9_debug(P9_DEBUG_ERROR,
"No transport defined or default transport\n");
- goto free_client;
+ goto out;
}

p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n",
@@ -965,7 +965,7 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)

err = clnt->trans_mod->create(clnt, dev_name, options);
if (err)
- goto put_trans;
+ goto out;

if (clnt->msize > clnt->trans_mod->maxsize) {
clnt->msize = clnt->trans_mod->maxsize;
@@ -979,12 +979,12 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
p9_debug(P9_DEBUG_ERROR,
"Please specify a msize of at least 4k\n");
err = -EINVAL;
- goto close_trans;
+ goto out;
}

err = p9_client_version(clnt);
if (err)
- goto close_trans;
+ goto out;

/* P9_HDRSZ + 4 is the smallest packet header we can have that is
* followed by data accessed from userspace by read
@@ -997,12 +997,8 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)

return clnt;

-close_trans:
- clnt->trans_mod->close(clnt);
-put_trans:
- v9fs_put_trans(clnt->trans_mod);
-free_client:
- kfree(clnt);
+out:
+ p9_client_destroy(clnt);
return ERR_PTR(err);
}
EXPORT_SYMBOL(p9_client_create);
-----

I'll test and submit to Linus over the next few weeks.

I had a quick look at the other new syzbot warnings and:
- 'possible deadlock in p9_req_put' is clear enough, we can just drop
the lock before running through the cancel list and I don't think
that'll cause any problem as everything has been moved to a local list
and that lock is abused by trans fd for its local stuff. I'll also send
that after quick testing.
----