[PATCH 0/2] Fix to latest syzkaller bugs in 9p area

[PATCH 0/2] Fix to latest syzkaller bugs in 9p area

[Leon Romanovsky]

This is followup to our discussion https://lore.kernel.org/all/YzVIGuISTnIFSIs9@codewreck.org/
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(-)

-- 
2.37.3

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

[Leon Romanovsky]

Rely on proper unwind order.

This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.

Reported-by: syzbot+67d13108d855f451cafc@syzkaller.appspotmail.com
Signed-off-by: Leon Romanovsky <leon@kernel.org>
---
 net/9p/client.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index bfa80f01992e..aaa37b07e30a 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -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);
-- 
2.37.3

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

[Dan Carpenter]

On Thu, Sep 29, 2022 at 12:37:55PM +0300, Leon Romanovsky wrote:
> Rely on proper unwind order.
> 
> This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.
> 
> Reported-by: syzbot+67d13108d855f451cafc@syzkaller.appspotmail.com
> Signed-off-by: Leon Romanovsky <leon@kernel.org>

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

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

[Dominique Martinet]

Dan Carpenter wrote on Tue, Oct 04, 2022 at 04:10:44PM +0300:
> On Thu, Sep 29, 2022 at 12:37:55PM +0300, Leon Romanovsky wrote:
> > Rely on proper unwind order.
> > 
> > This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.
> > 
> > Reported-by: syzbot+67d13108d855f451cafc@syzkaller.appspotmail.com
> > Signed-off-by: Leon Romanovsky <leon@kernel.org>
> 
> 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/20220928221923.1751130-1-asmadeus@codewreck.org/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...
-- 
Dominique

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

[Leon Romanovsky]

On Wed, Oct 05, 2022 at 06:01:39AM +0900, Dominique Martinet wrote:
> Dan Carpenter wrote on Tue, Oct 04, 2022 at 04:10:44PM +0300:
> > On Thu, Sep 29, 2022 at 12:37:55PM +0300, Leon Romanovsky wrote:
> > > Rely on proper unwind order.
> > > 
> > > This reverts commit 3ff51294a05529d0baf6d4b2517e561d12efb9f9.
> > > 
> > > Reported-by: syzbot+67d13108d855f451cafc@syzkaller.appspotmail.com
> > > Signed-off-by: Leon Romanovsky <leon@kernel.org>
> > 
> > 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/20220928221923.1751130-1-asmadeus@codewreck.org/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...

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

Thanks

> -- 
> Dominique

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

[Leon Romanovsky]

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

Reported-by: syzbot+de52531662ebb8823b26@syzkaller.appspotmail.com
Signed-off-by: Leon Romanovsky <leon@kernel.org>
---
 net/9p/client.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index aaa37b07e30a..8277e33506e7 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -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);
-- 
2.37.3

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

[Dominique Martinet]

Leon Romanovsky wrote on Thu, Sep 29, 2022 at 12:37:56PM +0300:
> 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

> 
> Reported-by: syzbot+de52531662ebb8823b26@syzkaller.appspotmail.com

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.


> Signed-off-by: Leon Romanovsky <leon@kernel.org>
> ---
>  net/9p/client.c | 37 ++++++++++++-------------------------
>  1 file changed, 12 insertions(+), 25 deletions(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index aaa37b07e30a..8277e33506e7 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -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)...


>  			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.)


>  		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);

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

>  	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);

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.

> +	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

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

[Leon Romanovsky]

On Thu, Sep 29, 2022 at 07:29:33PM +0900, Dominique Martinet wrote:
> Leon Romanovsky wrote on Thu, Sep 29, 2022 at 12:37:56PM +0300:
> > 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
> 
> > 
> > Reported-by: syzbot+de52531662ebb8823b26@syzkaller.appspotmail.com
> 
> 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.
> 
> 
> > Signed-off-by: Leon Romanovsky <leon@kernel.org>
> > ---
> >  net/9p/client.c | 37 ++++++++++++-------------------------
> >  1 file changed, 12 insertions(+), 25 deletions(-)
> > 
> > diff --git a/net/9p/client.c b/net/9p/client.c
> > index aaa37b07e30a..8277e33506e7 100644
> > --- a/net/9p/client.c
> > +++ b/net/9p/client.c
> > @@ -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)...

It is pity, you are right.

Thanks

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

[Christian Schoenebeck]

On Donnerstag, 29. September 2022 12:53:27 CEST Leon Romanovsky wrote:
> On Thu, Sep 29, 2022 at 07:29:33PM +0900, Dominique Martinet wrote:
> > Leon Romanovsky wrote on Thu, Sep 29, 2022 at 12:37:56PM +0300:
> > > 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
> > 
> > > Reported-by: syzbot+de52531662ebb8823b26@syzkaller.appspotmail.com
> > 
> > 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.
> > 
> > > Signed-off-by: Leon Romanovsky <leon@kernel.org>
> > > ---
> > > 
> > >  net/9p/client.c | 37 ++++++++++++-------------------------
> > >  1 file changed, 12 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/net/9p/client.c b/net/9p/client.c
> > > index aaa37b07e30a..8277e33506e7 100644
> > > --- a/net/9p/client.c
> > > +++ b/net/9p/client.c
> > > @@ -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)...
> 
> 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