Fix to latest syzkaller bugs in 9p area

PATCH In review Leon Romanovsky 4 participants 9 messages Sep 29, 2022 – Oct 6, 2022

Subsystems (3)

Networking Network drivers 9P file system

lore

Share & tools

lore

Leon Romanovsky

author

2022-09-29 09:38 UTC #0 lore raw

cover Fix to latest syzkaller bugs in 9p area

This is followup to our discussion https://lore.kernel.org/all/[email protected]/
with fixes to two syzkaller bugs in 9p.

It is based on linux-next.

Leon Romanovsky (2):
Revert "9p: p9_client_create: use p9_client_destroy on failure" 9p: destroy client in symmetric order

 net/9p/client.c | 47 +++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

Signature · 1 line

2.37.3

Leon Romanovsky

author

2022-09-29 09:38 UTC #1 lore raw

PATCH 1/2 Revert "9p: p9_client_create: use p9_client_destroy on failure"

Rely on proper unwind order.

This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.

Reported-by [email protected]

Signed-off-by Leon Romanovsky <[email protected]>

---
 net/9p/client.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

net/9p/client.c +16 -8

index bfa80f01992e..aaa37b07e30a 100644
@@ -961,10 +961,14 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 	char *client_id;
 
 	err = 0;
-	clnt = kzalloc(sizeof(*clnt), GFP_KERNEL);
+	clnt = kmalloc(sizeof(*clnt), GFP_KERNEL);
 	if (!clnt)
 		return ERR_PTR(-ENOMEM);
 
+	clnt->trans_mod = NULL;
+	clnt->trans = NULL;
+	clnt->fcall_cache = NULL;
+
 	client_id = utsname()->nodename;
 	memcpy(clnt->name, client_id, strlen(client_id) + 1);

@@ -974,7 +978,7 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 
 	err = parse_opts(options, clnt);
 	if (err < 0)
-		goto out;
+		goto free_client;
 
 	if (!clnt->trans_mod)
 		clnt->trans_mod = v9fs_get_default_trans();

@@ -983,7 +987,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 out;
+		goto free_client;
 	}
 
 	p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n",

@@ -991,7 +995,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;
+		goto put_trans;
 
 	if (clnt->msize > clnt->trans_mod->maxsize) {
 		clnt->msize = clnt->trans_mod->maxsize;

@@ -1005,12 +1009,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 out;
+		goto close_trans;
 	}
 
 	err = p9_client_version(clnt);
 	if (err)
-		goto out;
+		goto close_trans;
 
 	/* P9_HDRSZ + 4 is the smallest packet header we can have that is
 	 * followed by data accessed from userspace by read

@@ -1023,8 +1027,12 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 
 	return clnt;
 
-out:
-	p9_client_destroy(clnt);
+close_trans:
+	clnt->trans_mod->close(clnt);
+put_trans:
+	v9fs_put_trans(clnt->trans_mod);
+free_client:
+	kfree(clnt);
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL(p9_client_create);

Signature · 1 line

2.37.3

Dan Carpenter

2022-10-04 13:11 UTC #2 lore raw

Re: [PATCH 1/2] Revert "9p: p9_client_create: use p9_client_destroy on failure"

Quoted 6 lines · Leon Romanovsky

Rely on proper unwind order.
This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.
Reported-by [email protected]
Signed-off-by Leon Romanovsky <[email protected]>

The commit message doesn't really say what the problem is to the user. Is this just to make the next patch easier?

regards,
dan carpenter

Dominique Martinet

maintainer

2022-10-04 21:02 UTC #3 lore raw

Re: [PATCH 1/2] Revert "9p: p9_client_create: use p9_client_destroy on failure"

Dan Carpenter wrote on Tue, Oct 04, 2022 at 04:10:44PM +0300:

Quoted 10 lines · Leon Romanovsky

Quoted 6 lines
Rely on proper unwind order.
This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.
Reported-by [email protected]
Signed-off-by Leon Romanovsky <[email protected]>
The commit message doesn't really say what the problem is to the user. Is this just to make the next patch easier?

Yes (and perhaps a bit of spite from the previous discussion), and the next patch was not useable so I am not applying this as is.

The next patch was meant as an alternative implementation to this fix:
https://lore.kernel.org/all/[email protected]/T/#u

At this point I have the original fix in my -next branch but it hasn't had any positive review (and well, I myself agree it's ugly), so unless Leon sends a v2 I'll need to think of a better way of tracking if clnt->trans_mod->create has been successfully called.
I'm starting to think that since we don't have so many clnt I can probably just fit a bool/bitfield in one of the holes of the struct and just keep track of it; that'll be less error-prone than relying on clnt->trans (which -is- initialized in create() most of the time, but not in a way we can rely on) or reworking create() to return it as I originally wanted to do (rdma still needs to populate clnt->trans behind the scenees before create() returns, so the abstraction is also quite ugly)

The current breakage is actually quite bad so I'll try to send that today or tomorrow for merging next week unless Leon resends something we can work with... Conceptually won't be different than the patch currently in next so hopefully can pretend it's had a couple of weeks of testing...

Signature · 1 line

Dominique

Leon Romanovsky

author

2022-10-06 16:19 UTC #4 lore raw

Re: [PATCH 1/2] Revert "9p: p9_client_create: use p9_client_destroy on failure"

Quoted 31 lines · Dominique Martinet

Dan Carpenter wrote on Tue, Oct 04, 2022 at 04:10:44PM +0300:
Quoted 10 lines · Leon Romanovsky
Quoted 6 lines
Rely on proper unwind order.
This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.
Reported-by [email protected]
Signed-off-by Leon Romanovsky <[email protected]>
The commit message doesn't really say what the problem is to the user. Is this just to make the next patch easier?
Yes (and perhaps a bit of spite from the previous discussion), and the next patch was not useable so I am not applying this as is.
The next patch was meant as an alternative implementation to this fix:
https://lore.kernel.org/all/[email protected]/T/#u
At this point I have the original fix in my -next branch but it hasn't had any positive review (and well, I myself agree it's ugly), so unless Leon sends a v2 I'll need to think of a better way of tracking if clnt->trans_mod->create has been successfully called.
I'm starting to think that since we don't have so many clnt I can probably just fit a bool/bitfield in one of the holes of the struct and just keep track of it; that'll be less error-prone than relying on clnt->trans (which -is- initialized in create() most of the time, but not in a way we can rely on) or reworking create() to return it as I originally wanted to do (rdma still needs to populate clnt->trans behind the scenees before create() returns, so the abstraction is also quite ugly)

Quoted 5 lines

The current breakage is actually quite bad so I'll try to send that today or tomorrow for merging next week unless Leon resends something we can work with... Conceptually won't be different than the patch currently in next so hopefully can pretend it's had a couple of weeks of testing...

I can't resend anything in near future and most of the time have very limited access to computer due to holidays here.

Thanks

Quoted 2 lines

Signature · 1 line
Dominique

Leon Romanovsky

author

2022-09-29 09:38 UTC #5 lore raw

PATCH 2/2 9p: destroy client in symmetric order

In reply to Leon Romanovsky

Make sure that all variables are initialized and released in correct order.

Reported-by [email protected]

Signed-off-by Leon Romanovsky <[email protected]>

---
 net/9p/client.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

net/9p/client.c +12 ~~-25~~

index aaa37b07e30a..8277e33506e7 100644
@@ -179,7 +179,6 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 				goto free_and_return;
 			}
 
-			v9fs_put_trans(clnt->trans_mod);
 			clnt->trans_mod = v9fs_get_trans_by_name(s);
 			if (!clnt->trans_mod) {
 				pr_info("Could not find request transport: %s\n",

@@ -187,7 +186,7 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 				ret = -EINVAL;
 			}
 			kfree(s);
-			break;
+			goto free_and_return;
 		case Opt_legacy:
 			clnt->proto_version = p9_proto_legacy;
 			break;

@@ -211,9 +210,14 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 		}
 	}
 
+	clnt->trans_mod = v9fs_get_default_trans();
+	if (!clnt->trans_mod) {
+		ret = -EPROTONOSUPPORT;
+		p9_debug(P9_DEBUG_ERROR,
+			 "No transport defined or default transport\n");
+	}
+
 free_and_return:
-	if (ret)
-		v9fs_put_trans(clnt->trans_mod);
 	kfree(tmp_options);
 	return ret;
 }

@@ -956,19 +960,14 @@ static int p9_client_version(struct p9_client *c)
 
 struct p9_client *p9_client_create(const char *dev_name, char *options)
 {
-	int err;
 	struct p9_client *clnt;
 	char *client_id;
+	int err = 0;
 
-	err = 0;
-	clnt = kmalloc(sizeof(*clnt), GFP_KERNEL);
+	clnt = kzalloc(sizeof(*clnt), GFP_KERNEL);
 	if (!clnt)
 		return ERR_PTR(-ENOMEM);
 
-	clnt->trans_mod = NULL;
-	clnt->trans = NULL;
-	clnt->fcall_cache = NULL;
-
 	client_id = utsname()->nodename;
 	memcpy(clnt->name, client_id, strlen(client_id) + 1);

@@ -980,16 +979,6 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 	if (err < 0)
 		goto free_client;
 
-	if (!clnt->trans_mod)
-		clnt->trans_mod = v9fs_get_default_trans();
-
-	if (!clnt->trans_mod) {
-		err = -EPROTONOSUPPORT;
-		p9_debug(P9_DEBUG_ERROR,
-			 "No transport defined or default transport\n");
-		goto free_client;
-	}
-
 	p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n",
 		 clnt, clnt->trans_mod, clnt->msize, clnt->proto_version);

@@ -1044,9 +1033,8 @@ void p9_client_destroy(struct p9_client *clnt)
 
 	p9_debug(P9_DEBUG_MUX, "clnt %p\n", clnt);
 
-	if (clnt->trans_mod)
-		clnt->trans_mod->close(clnt);
-
+	kmem_cache_destroy(clnt->fcall_cache);
+	clnt->trans_mod->close(clnt);
 	v9fs_put_trans(clnt->trans_mod);
 
 	idr_for_each_entry(&clnt->fids, fid, id) {

@@ -1056,7 +1044,6 @@ void p9_client_destroy(struct p9_client *clnt)
 
 	p9_tag_cleanup(clnt);
 
-	kmem_cache_destroy(clnt->fcall_cache);
 	kfree(clnt);
 }
 EXPORT_SYMBOL(p9_client_destroy);

Signature · 1 line

2.37.3

Dominique Martinet

maintainer

2022-09-29 10:30 UTC #6 lore raw

Re: [PATCH 2/2] 9p: destroy client in symmetric order

Leon Romanovsky wrote on Thu, Sep 29, 2022 at 12:37:56PM +0300:

Quoted 2 lines

Make sure that all variables are initialized and released in correct order.

Haven't tried running or compiling, comments out of my head that might be wrong below

Quoted 2 lines

Reported-by [email protected]

You're adding this report tag but I don't see how you fix that failure. What you need is p9_tag_cleanup(clnt) from p9_client_destroy -- I assume this isn't possible for any fid to be allocated at this point so the fid destroying loop is -probably- optional.

I would assume it is needed from p9_client_version failures.

Quoted 10 lines

Signed-off-by Leon Romanovsky <[email protected]>
---
 net/9p/client.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)
net/9p/client.c +0 -0
index aaa37b07e30a..8277e33506e7 100644

net/9p/client.c static int parse_opts(char *opts, struct p9_client *clnt) · line 182

Quoted 4 lines

net/9p/client.c +0 -1

@@ -179,7 +179,6 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 				goto free_and_return;
 			}
 
-			v9fs_put_trans(clnt->trans_mod);

Pretty sure you'll be "leaking transports" if someone tries to pass trans=foo multiple times; this can't be removed...(continues below)...

net/9p/client.c static int parse_opts(char *opts, struct p9_client *clnt) · line 189

Quoted 9 lines

net/9p/client.c +1 -1

 			clnt->trans_mod = v9fs_get_trans_by_name(s);
 			if (!clnt->trans_mod) {
 				pr_info("Could not find request transport: %s\n",

@@ -187,7 +186,7 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 				ret = -EINVAL;
 			}
 			kfree(s);
-			break;
+			goto free_and_return;

... unless you also break the whole parsing, with this asking for -o trans=virtio,msize=1M will just ignore the msize argument. (or anything else that follows)

I appreciate that you're trying to avoid the get_default_trans below, but unless you just remember the last string and try to get it get/put trans is the most straight forward way to go.

(Note you just got me to try weird parsing and my old version was double-puting these modules because of the put_trans below in this function echoes with the client destroy. That'd just require removing the put here though (or nulling after put), yet another invariant I had wrongly assumed... Anyway.)

Quoted 7 lines

net/9p/client.c +0 -0

 		case Opt_legacy:
 			clnt->proto_version = p9_proto_legacy;
 			break;

@@ -211,9 +210,14 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 		}
 	}

net/9p/client.c static int parse_opts(char *opts, struct p9_client *clnt) · line 216

Quoted 10 lines

net/9p/client.c +7 -2

+	clnt->trans_mod = v9fs_get_default_trans();
+	if (!clnt->trans_mod) {
+		ret = -EPROTONOSUPPORT;
+		p9_debug(P9_DEBUG_ERROR,
+			 "No transport defined or default transport\n");
+	}
+
 free_and_return:
-	if (ret)
-		v9fs_put_trans(clnt->trans_mod);

(oh, and if you parse options after trans= you'll need some sort of escape hatch here...)

Quoted 43 lines

net/9p/client.c +2 ~~-17~~

 	kfree(tmp_options);
 	return ret;
 }

@@ -956,19 +960,14 @@ static int p9_client_version(struct p9_client *c)
 
 struct p9_client *p9_client_create(const char *dev_name, char *options)
 {
-	int err;
 	struct p9_client *clnt;
 	char *client_id;
+	int err = 0;
 
-	err = 0;
-	clnt = kmalloc(sizeof(*clnt), GFP_KERNEL);
+	clnt = kzalloc(sizeof(*clnt), GFP_KERNEL);
 	if (!clnt)
 		return ERR_PTR(-ENOMEM);
 
-	clnt->trans_mod = NULL;
-	clnt->trans = NULL;
-	clnt->fcall_cache = NULL;
-
 	client_id = utsname()->nodename;
 	memcpy(clnt->name, client_id, strlen(client_id) + 1);

@@ -980,16 +979,6 @@ struct p9_client *p9_client_create(const char *dev_name, char *options)
 	if (err < 0)
 		goto free_client;
 
-	if (!clnt->trans_mod)
-		clnt->trans_mod = v9fs_get_default_trans();
-
-	if (!clnt->trans_mod) {
-		err = -EPROTONOSUPPORT;
-		p9_debug(P9_DEBUG_ERROR,
-			 "No transport defined or default transport\n");
-		goto free_client;
-	}
-
 	p9_debug(P9_DEBUG_MUX, "clnt %p trans %p msize %d protocol %d\n",
 		 clnt, clnt->trans_mod, clnt->msize, clnt->proto_version);

net/9p/client.c void p9_client_destroy(struct p9_client *clnt) · line 1036

Quoted 7 lines

net/9p/client.c +1 -3

@@ -1044,9 +1033,8 @@ void p9_client_destroy(struct p9_client *clnt)
 
 	p9_debug(P9_DEBUG_MUX, "clnt %p\n", clnt);
 
-	if (clnt->trans_mod)
-		clnt->trans_mod->close(clnt);
-
+	kmem_cache_destroy(clnt->fcall_cache);

Pretty sure kmem_cache used to issue a warning when we did that (hence me trying to move it up on allocation) -- at this point there can still be in flight requests we need to finish freeing before we can destroy the cache.

net/9p/client.c void p9_client_destroy(struct p9_client *clnt) · line 1049

Quoted 12 lines

net/9p/client.c +1 -1

+	clnt->trans_mod->close(clnt);
 	v9fs_put_trans(clnt->trans_mod);
 
 	idr_for_each_entry(&clnt->fids, fid, id) {

@@ -1056,7 +1044,6 @@ void p9_client_destroy(struct p9_client *clnt)
 
 	p9_tag_cleanup(clnt);
 
-	kmem_cache_destroy(clnt->fcall_cache);
 	kfree(clnt);
 }
 EXPORT_SYMBOL(p9_client_destroy);

--
Dominique

Leon Romanovsky

author

2022-09-29 10:53 UTC #7 lore raw

Re: [PATCH 2/2] 9p: destroy client in symmetric order

Quoted 31 lines · Dominique Martinet

Leon Romanovsky wrote on Thu, Sep 29, 2022 at 12:37:56PM +0300:
Quoted 2 lines
Make sure that all variables are initialized and released in correct order.
Haven't tried running or compiling, comments out of my head that might be wrong below
Quoted 2 lines
Reported-by [email protected]
You're adding this report tag but I don't see how you fix that failure. What you need is p9_tag_cleanup(clnt) from p9_client_destroy -- I assume this isn't possible for any fid to be allocated at this point so the fid destroying loop is -probably- optional.
I would assume it is needed from p9_client_version failures.
Quoted 13 lines
Signed-off-by Leon Romanovsky <[email protected]>
---
 net/9p/client.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)
net/9p/client.c +0 -0
index aaa37b07e30a..8277e33506e7 100644
@@ -179,7 +179,6 @@ static int parse_opts(char *opts, struct p9_client *clnt)
 				goto free_and_return;
 			}
 

Quoted 4 lines

net/9p/client.c static int parse_opts(char *opts, struct p9_client *clnt) · line 182
Quoted 1 line
net/9p/client.c +0 -1
-			v9fs_put_trans(clnt->trans_mod);
Pretty sure you'll be "leaking transports" if someone tries to pass trans=foo multiple times; this can't be removed...(continues below)...

It is pity, you are right.

Thanks

Christian Schoenebeck

reviewer

2022-10-04 13:04 UTC #8 lore raw

Re: [PATCH 2/2] 9p: destroy client in symmetric order

Quoted 33 lines · On Donnerstag, 29. September 2022 12:53:27 CEST Leon Romanovsky wrote:

Quoted 32 lines · Dominique Martinet
Leon Romanovsky wrote on Thu, Sep 29, 2022 at 12:37:56PM +0300:
Quoted 2 lines
Make sure that all variables are initialized and released in correct order.
Haven't tried running or compiling, comments out of my head that might be wrong below
Quoted 1 line
Reported-by [email protected]
You're adding this report tag but I don't see how you fix that failure. What you need is p9_tag_cleanup(clnt) from p9_client_destroy -- I assume this isn't possible for any fid to be allocated at this point so the fid destroying loop is -probably- optional.
I would assume it is needed from p9_client_version failures.
Quoted 16 lines
Signed-off-by Leon Romanovsky <[email protected]>
---
 net/9p/client.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)
net/9p/client.c +0 -0
index aaa37b07e30a..8277e33506e7 100644
@@ -179,7 +179,6 @@ static int parse_opts(char *opts, struct p9_client
 *clnt)> > 
 				goto free_and_return;
 
 			}
 

Quoted 8 lines

Quoted 4 lines
net/9p/client.c static int parse_opts(char *opts, struct p9_client · line 184
Quoted 1 line
net/9p/client.c +0 -1
-			v9fs_put_trans(clnt->trans_mod);
Pretty sure you'll be "leaking transports" if someone tries to pass trans=foo multiple times; this can't be removed...(continues below)...
It is pity, you are right.
Thanks

Hi Leon,

have you planned a v2 on this? Just asking, so that we know whether to go forward.

Best regards,
Christian Schoenebeck