destroy code would incorrectly call close() if trans_mod exists after some
hasty code cleanup: we need to make sure we only call close after create
The new bool added to track this has been added in a hole of the struct
and will not increase p9_client's size.
It might be possible to do better with a bit more work, but that will
have to do for now.
Link: https://lkml.kernel.org/r/[email protected]
Reported-by: [email protected]
Reported-by: Leon Romanovsky <[email protected]>
Fixes: 3ff51294a055 ("9p: p9_client_create: use p9_client_destroy on failure")
Signed-off-by: Dominique Martinet <[email protected]>
---
v4: start over from scratch with a pool to properly track initialization
state instead of kludging over clnt->trans itself which is "private" to
the trans
include/net/9p/client.h | 2 ++
net/9p/client.c | 3 ++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index 78ebcf782ce5..3f7f473da05a 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -89,6 +89,7 @@ struct p9_req_t {
* @lock: protect @fids and @reqs
* @msize: maximum data size negotiated by protocol
* @proto_version: 9P protocol version to use
+ * @trans_initialized: Whether transport's close() is safe to call
* @trans_mod: module API instantiated with this client
* @status: connection state
* @trans: tranport instance state and API
@@ -103,6 +104,7 @@ struct p9_client {
spinlock_t lock;
unsigned int msize;
unsigned char proto_version;
+ bool trans_initialized;
struct p9_trans_module *trans_mod;
enum p9_trans_status status;
void *trans;
diff --git a/net/9p/client.c b/net/9p/client.c
index bfa80f01992e..cf2d5b60b61b 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -992,6 +992,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 out;
+ clnt->trans_initialized = true;
if (clnt->msize > clnt->trans_mod->maxsize) {
clnt->msize = clnt->trans_mod->maxsize;
@@ -1036,7 +1037,7 @@ void p9_client_destroy(struct p9_client *clnt)
p9_debug(P9_DEBUG_MUX, "clnt %p\n", clnt);
- if (clnt->trans_mod)
+ if (clnt->trans_mod && clnt->trans_initialized)
clnt->trans_mod->close(clnt);
v9fs_put_trans(clnt->trans_mod);
--
2.35.1
Dominique Martinet wrote on Wed, Oct 05, 2022 at 06:51:13AM +0900:
> destroy code would incorrectly call close() if trans_mod exists after some
> hasty code cleanup: we need to make sure we only call close after create
>
> The new bool added to track this has been added in a hole of the struct
> and will not increase p9_client's size.
> It might be possible to do better with a bit more work, but that will
> have to do for now.
>
> Link: https://lkml.kernel.org/r/[email protected]
> Reported-by: [email protected]
> Reported-by: Leon Romanovsky <[email protected]>
> Fixes: 3ff51294a055 ("9p: p9_client_create: use p9_client_destroy on failure")
Oh... Fixing tags for stable made me notice this actually wasn't merged
into 5.19 unlike what I thought, so we only have the original bug of
potentially freeing the idr with tags still in it.
That's a much smaller bug and I'll just remove the first
p9_client_destroy on failure patch for this merge window (small leak
that requires root on error) ; then we can take time to properly fix
this one way or another for next cycle.
--
Dominique