[PATCH] cifs: check kzalloc return

[PATCH] cifs: check kzalloc return

[Nicholas Mc Guire]

kzalloc can return NULL so a check is needed. While there is a
check for ret_buf there is no check for the allocation of
ret_buf->crfid.fid - this check is thus added. Both call-sites
of tconInfoAlloc() check for NULL return of tconInfoAlloc()
so returning NULL on failure of kzalloc() here seems appropriate.
As the kzalloc() is the only thing here that can fail it is
moved to the beginning so as not to initialize other resources
on failure of kzalloc.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: 3d4ef9a15343 ("smb3: fix redundant opens on root")
---

Problem located with an experimental coccinelle script

While at it make checkpatch happy by using *ret_buf->crfid.fid
rather than struct cifs_fid.

Patch was compile tested with: x86_64_defconfig + CIFS=m
(with some unrelated smatch warnings and some pending cocci fixes)

Patch is against v4.20-rc7 (localversion-next is next-20181218)

 fs/cifs/misc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 8a41f4e..59efffe 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -113,6 +113,13 @@ tconInfoAlloc(void)
 	struct cifs_tcon *ret_buf;
 	ret_buf = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
 	if (ret_buf) {
+		ret_buf->crfid.fid = kzalloc(sizeof(*ret_buf->crfid.fid),
+					     GFP_KERNEL);
+		if (!ret_buf->crfid.fid) {
+			kfree(ret_buf);
+			return NULL;
+		}
+
 		atomic_inc(&tconInfoAllocCount);
 		ret_buf->tidStatus = CifsNew;
 		++ret_buf->tc_count;
@@ -120,8 +127,6 @@ tconInfoAlloc(void)
 		INIT_LIST_HEAD(&ret_buf->tcon_list);
 		spin_lock_init(&ret_buf->open_file_lock);
 		mutex_init(&ret_buf->crfid.fid_mutex);
-		ret_buf->crfid.fid = kzalloc(sizeof(struct cifs_fid),
-					     GFP_KERNEL);
 		spin_lock_init(&ret_buf->stat_lock);
 		atomic_set(&ret_buf->num_local_opens, 0);
 		atomic_set(&ret_buf->num_remote_opens, 0);
-- 
2.1.4

Re: [PATCH] cifs: check kzalloc return

[Joe Perches]

On Tue, 2018-12-18 at 17:27 +0100, Nicholas Mc Guire wrote:
> kzalloc can return NULL so a check is needed. While there is a
> check for ret_buf there is no check for the allocation of
> ret_buf->crfid.fid - this check is thus added. Both call-sites
> of tconInfoAlloc() check for NULL return of tconInfoAlloc()
> so returning NULL on failure of kzalloc() here seems appropriate.
> As the kzalloc() is the only thing here that can fail it is
> moved to the beginning so as not to initialize other resources
> on failure of kzalloc.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: 3d4ef9a15343 ("smb3: fix redundant opens on root")
> ---
> 
> Problem located with an experimental coccinelle script
> 
> While at it make checkpatch happy by using *ret_buf->crfid.fid
> rather than struct cifs_fid.
> 
> Patch was compile tested with: x86_64_defconfig + CIFS=m
> (with some unrelated smatch warnings and some pending cocci fixes)
> 
> Patch is against v4.20-rc7 (localversion-next is next-20181218)
[]
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
[]
> @@ -113,6 +113,13 @@ tconInfoAlloc(void)
>  	struct cifs_tcon *ret_buf;
>  	ret_buf = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
>  	if (ret_buf) {
> +		ret_buf->crfid.fid = kzalloc(sizeof(*ret_buf->crfid.fid),
> +					     GFP_KERNEL);
> +		if (!ret_buf->crfid.fid) {
> +			kfree(ret_buf);
> +			return NULL;
> +		}
> +
>  		atomic_inc(&tconInfoAllocCount);
>  		ret_buf->tidStatus = CifsNew;
>  		++ret_buf->tc_count;
> @@ -120,8 +127,6 @@ tconInfoAlloc(void)
>  		INIT_LIST_HEAD(&ret_buf->tcon_list);
>  		spin_lock_init(&ret_buf->open_file_lock);
>  		mutex_init(&ret_buf->crfid.fid_mutex);
> -		ret_buf->crfid.fid = kzalloc(sizeof(struct cifs_fid),
> -					     GFP_KERNEL);
>  		spin_lock_init(&ret_buf->stat_lock);
>  		atomic_set(&ret_buf->num_local_opens, 0);
>  		atomic_set(&ret_buf->num_remote_opens, 0);

Perhaps use a more common style by returning early on the
first possible failure too so the block can be unindented.

Maybe as a separate cleanup patch.
---
 fs/cifs/misc.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 113980dba4d8..bee203055b30 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -111,21 +111,27 @@ struct cifs_tcon *
 tconInfoAlloc(void)
 {
 	struct cifs_tcon *ret_buf;
-	ret_buf = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
-	if (ret_buf) {
-		atomic_inc(&tconInfoAllocCount);
-		ret_buf->tidStatus = CifsNew;
-		++ret_buf->tc_count;
-		INIT_LIST_HEAD(&ret_buf->openFileList);
-		INIT_LIST_HEAD(&ret_buf->tcon_list);
-		spin_lock_init(&ret_buf->open_file_lock);
-		mutex_init(&ret_buf->crfid.fid_mutex);
-		ret_buf->crfid.fid = kzalloc(sizeof(struct cifs_fid),
-					     GFP_KERNEL);
-		spin_lock_init(&ret_buf->stat_lock);
-		atomic_set(&ret_buf->num_local_opens, 0);
-		atomic_set(&ret_buf->num_remote_opens, 0);
+
+	ret_buf = kzalloc(sizeof(*ret_buf), GFP_KERNEL);
+	if (!ret_buf)
+		return NULL;
+	ret_buf->crfid.fid = kzalloc(sizeof(*ret_buf->crfid.fid), GFP_KERNEL);
+	if (!ret_buf->crfid.fid) {
+		kfree(ret_buf);
+		return NULL;
 	}
+
+	atomic_inc(&tconInfoAllocCount);
+	ret_buf->tidStatus = CifsNew;
+	++ret_buf->tc_count;
+	INIT_LIST_HEAD(&ret_buf->openFileList);
+	INIT_LIST_HEAD(&ret_buf->tcon_list);
+	spin_lock_init(&ret_buf->open_file_lock);
+	mutex_init(&ret_buf->crfid.fid_mutex);
+	spin_lock_init(&ret_buf->stat_lock);
+	atomic_set(&ret_buf->num_local_opens, 0);
+	atomic_set(&ret_buf->num_remote_opens, 0);
+
 	return ret_buf;
 }
 

Re: [PATCH] cifs: check kzalloc return

[Nicholas Mc Guire]

On Tue, Dec 18, 2018 at 09:33:58AM -0800, Joe Perches wrote:
> On Tue, 2018-12-18 at 17:27 +0100, Nicholas Mc Guire wrote:
> > kzalloc can return NULL so a check is needed. While there is a
> > check for ret_buf there is no check for the allocation of
> > ret_buf->crfid.fid - this check is thus added. Both call-sites
> > of tconInfoAlloc() check for NULL return of tconInfoAlloc()
> > so returning NULL on failure of kzalloc() here seems appropriate.
> > As the kzalloc() is the only thing here that can fail it is
> > moved to the beginning so as not to initialize other resources
> > on failure of kzalloc.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > Fixes: 3d4ef9a15343 ("smb3: fix redundant opens on root")
> > ---
> > 
> > Problem located with an experimental coccinelle script
> > 
> > While at it make checkpatch happy by using *ret_buf->crfid.fid
> > rather than struct cifs_fid.
> > 
> > Patch was compile tested with: x86_64_defconfig + CIFS=m
> > (with some unrelated smatch warnings and some pending cocci fixes)
> > 
> > Patch is against v4.20-rc7 (localversion-next is next-20181218)
> []
> > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> []
> > @@ -113,6 +113,13 @@ tconInfoAlloc(void)
> >  	struct cifs_tcon *ret_buf;
> >  	ret_buf = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
> >  	if (ret_buf) {
> > +		ret_buf->crfid.fid = kzalloc(sizeof(*ret_buf->crfid.fid),
> > +					     GFP_KERNEL);
> > +		if (!ret_buf->crfid.fid) {
> > +			kfree(ret_buf);
> > +			return NULL;
> > +		}
> > +
> >  		atomic_inc(&tconInfoAllocCount);
> >  		ret_buf->tidStatus = CifsNew;
> >  		++ret_buf->tc_count;
> > @@ -120,8 +127,6 @@ tconInfoAlloc(void)
> >  		INIT_LIST_HEAD(&ret_buf->tcon_list);
> >  		spin_lock_init(&ret_buf->open_file_lock);
> >  		mutex_init(&ret_buf->crfid.fid_mutex);
> > -		ret_buf->crfid.fid = kzalloc(sizeof(struct cifs_fid),
> > -					     GFP_KERNEL);
> >  		spin_lock_init(&ret_buf->stat_lock);
> >  		atomic_set(&ret_buf->num_local_opens, 0);
> >  		atomic_set(&ret_buf->num_remote_opens, 0);
> 
> Perhaps use a more common style by returning early on the
> first possible failure too so the block can be unindented.
>

yup the restructured cleanup would be the better way to go

rather than making this two patches I guess it would be the
best to just skip the intermediate kzalloc only cleanup -
atleast I see little value in that intermediat step - so
could you take care of the kzalloc() in your refactoring 
please ?

thx!
hofrat
 
> Maybe as a separate cleanup patch.
> ---
>  fs/cifs/misc.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 113980dba4d8..bee203055b30 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -111,21 +111,27 @@ struct cifs_tcon *
>  tconInfoAlloc(void)
>  {
>  	struct cifs_tcon *ret_buf;
> -	ret_buf = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
> -	if (ret_buf) {
> -		atomic_inc(&tconInfoAllocCount);
> -		ret_buf->tidStatus = CifsNew;
> -		++ret_buf->tc_count;
> -		INIT_LIST_HEAD(&ret_buf->openFileList);
> -		INIT_LIST_HEAD(&ret_buf->tcon_list);
> -		spin_lock_init(&ret_buf->open_file_lock);
> -		mutex_init(&ret_buf->crfid.fid_mutex);
> -		ret_buf->crfid.fid = kzalloc(sizeof(struct cifs_fid),
> -					     GFP_KERNEL);
> -		spin_lock_init(&ret_buf->stat_lock);
> -		atomic_set(&ret_buf->num_local_opens, 0);
> -		atomic_set(&ret_buf->num_remote_opens, 0);
> +
> +	ret_buf = kzalloc(sizeof(*ret_buf), GFP_KERNEL);
> +	if (!ret_buf)
> +		return NULL;
> +	ret_buf->crfid.fid = kzalloc(sizeof(*ret_buf->crfid.fid), GFP_KERNEL);
> +	if (!ret_buf->crfid.fid) {
> +		kfree(ret_buf);
> +		return NULL;
>  	}
> +
> +	atomic_inc(&tconInfoAllocCount);
> +	ret_buf->tidStatus = CifsNew;
> +	++ret_buf->tc_count;
> +	INIT_LIST_HEAD(&ret_buf->openFileList);
> +	INIT_LIST_HEAD(&ret_buf->tcon_list);
> +	spin_lock_init(&ret_buf->open_file_lock);
> +	mutex_init(&ret_buf->crfid.fid_mutex);
> +	spin_lock_init(&ret_buf->stat_lock);
> +	atomic_set(&ret_buf->num_local_opens, 0);
> +	atomic_set(&ret_buf->num_remote_opens, 0);
> +
>  	return ret_buf;
>  }
>  
> 

Re: [PATCH] cifs: check kzalloc return

[Joe Perches]

On Wed, 2018-12-19 at 08:22 +0100, Nicholas Mc Guire wrote:
> On Tue, Dec 18, 2018 at 09:33:58AM -0800, Joe Perches wrote:
> > On Tue, 2018-12-18 at 17:27 +0100, Nicholas Mc Guire wrote:
> > > kzalloc can return NULL so a check is needed. While there is a
> > > check for ret_buf there is no check for the allocation of
> > > ret_buf->crfid.fid - this check is thus added. Both call-sites
> > > of tconInfoAlloc() check for NULL return of tconInfoAlloc()
> > > so returning NULL on failure of kzalloc() here seems appropriate.
> > > As the kzalloc() is the only thing here that can fail it is
> > > moved to the beginning so as not to initialize other resources
> > > on failure of kzalloc.
> > > 
> > Perhaps use a more common style by returning early on the
> > first possible failure too so the block can be unindented.
[]
> > yup the restructured cleanup would be the better way to go
> 
> rather than making this two patches I guess it would be the
> best to just skip the intermediate kzalloc only cleanup -
> atleast I see little value in that intermediat step - so
> could you take care of the kzalloc() in your refactoring 
> please ?

I did that in the patch I proposed, which is trivial.
If you want to take it, please do.

If you want a sign-off:

Signed-off-by: Joe Perches <joe@perches.com>

> > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
[]
> > @@ -111,21 +111,27 @@ struct cifs_tcon *
> >  tconInfoAlloc(void)
> >  {
> >  	struct cifs_tcon *ret_buf;
> > -	ret_buf = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
> > -	if (ret_buf) {
> > -		atomic_inc(&tconInfoAllocCount);
> > -		ret_buf->tidStatus = CifsNew;
> > -		++ret_buf->tc_count;
> > -		INIT_LIST_HEAD(&ret_buf->openFileList);
> > -		INIT_LIST_HEAD(&ret_buf->tcon_list);
> > -		spin_lock_init(&ret_buf->open_file_lock);
> > -		mutex_init(&ret_buf->crfid.fid_mutex);
> > -		ret_buf->crfid.fid = kzalloc(sizeof(struct cifs_fid),
> > -					     GFP_KERNEL);
> > -		spin_lock_init(&ret_buf->stat_lock);
> > -		atomic_set(&ret_buf->num_local_opens, 0);
> > -		atomic_set(&ret_buf->num_remote_opens, 0);
> > +
> > +	ret_buf = kzalloc(sizeof(*ret_buf), GFP_KERNEL);
> > +	if (!ret_buf)
> > +		return NULL;
> > +	ret_buf->crfid.fid = kzalloc(sizeof(*ret_buf->crfid.fid), GFP_KERNEL);
> > +	if (!ret_buf->crfid.fid) {
> > +		kfree(ret_buf);
> > +		return NULL;
> >  	}
> > +
> > +	atomic_inc(&tconInfoAllocCount);
> > +	ret_buf->tidStatus = CifsNew;
> > +	++ret_buf->tc_count;
> > +	INIT_LIST_HEAD(&ret_buf->openFileList);
> > +	INIT_LIST_HEAD(&ret_buf->tcon_list);
> > +	spin_lock_init(&ret_buf->open_file_lock);
> > +	mutex_init(&ret_buf->crfid.fid_mutex);
> > +	spin_lock_init(&ret_buf->stat_lock);
> > +	atomic_set(&ret_buf->num_local_opens, 0);
> > +	atomic_set(&ret_buf->num_remote_opens, 0);
> > +
> >  	return ret_buf;
> >  }
> >  

Re: [PATCH] cifs: check kzalloc return

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

Updated the patch with Joe's signed off and Nicholas's reviewed-by and
pushed to cifs-2.6.git for-next

See attached.

On Wed, Dec 19, 2018 at 4:58 AM Joe Perches via samba-technical
<samba-technical@lists.samba.org> wrote:
>
> On Wed, 2018-12-19 at 08:22 +0100, Nicholas Mc Guire wrote:
> > On Tue, Dec 18, 2018 at 09:33:58AM -0800, Joe Perches wrote:
> > > On Tue, 2018-12-18 at 17:27 +0100, Nicholas Mc Guire wrote:
> > > > kzalloc can return NULL so a check is needed. While there is a
> > > > check for ret_buf there is no check for the allocation of
> > > > ret_buf->crfid.fid - this check is thus added. Both call-sites
> > > > of tconInfoAlloc() check for NULL return of tconInfoAlloc()
> > > > so returning NULL on failure of kzalloc() here seems appropriate.
> > > > As the kzalloc() is the only thing here that can fail it is
> > > > moved to the beginning so as not to initialize other resources
> > > > on failure of kzalloc.
> > > >
> > > Perhaps use a more common style by returning early on the
> > > first possible failure too so the block can be unindented.
> []
> > > yup the restructured cleanup would be the better way to go
> >
> > rather than making this two patches I guess it would be the
> > best to just skip the intermediate kzalloc only cleanup -
> > atleast I see little value in that intermediat step - so
> > could you take care of the kzalloc() in your refactoring
> > please ?
>
> I did that in the patch I proposed, which is trivial.
> If you want to take it, please do.
>
> If you want a sign-off:
>
> Signed-off-by: Joe Perches <joe@perches.com>
>
> > > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> []
> > > @@ -111,21 +111,27 @@ struct cifs_tcon *
> > >  tconInfoAlloc(void)
> > >  {
> > >     struct cifs_tcon *ret_buf;
> > > -   ret_buf = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
> > > -   if (ret_buf) {
> > > -           atomic_inc(&tconInfoAllocCount);
> > > -           ret_buf->tidStatus = CifsNew;
> > > -           ++ret_buf->tc_count;
> > > -           INIT_LIST_HEAD(&ret_buf->openFileList);
> > > -           INIT_LIST_HEAD(&ret_buf->tcon_list);
> > > -           spin_lock_init(&ret_buf->open_file_lock);
> > > -           mutex_init(&ret_buf->crfid.fid_mutex);
> > > -           ret_buf->crfid.fid = kzalloc(sizeof(struct cifs_fid),
> > > -                                        GFP_KERNEL);
> > > -           spin_lock_init(&ret_buf->stat_lock);
> > > -           atomic_set(&ret_buf->num_local_opens, 0);
> > > -           atomic_set(&ret_buf->num_remote_opens, 0);
> > > +
> > > +   ret_buf = kzalloc(sizeof(*ret_buf), GFP_KERNEL);
> > > +   if (!ret_buf)
> > > +           return NULL;
> > > +   ret_buf->crfid.fid = kzalloc(sizeof(*ret_buf->crfid.fid), GFP_KERNEL);
> > > +   if (!ret_buf->crfid.fid) {
> > > +           kfree(ret_buf);
> > > +           return NULL;
> > >     }
> > > +
> > > +   atomic_inc(&tconInfoAllocCount);
> > > +   ret_buf->tidStatus = CifsNew;
> > > +   ++ret_buf->tc_count;
> > > +   INIT_LIST_HEAD(&ret_buf->openFileList);
> > > +   INIT_LIST_HEAD(&ret_buf->tcon_list);
> > > +   spin_lock_init(&ret_buf->open_file_lock);
> > > +   mutex_init(&ret_buf->crfid.fid_mutex);
> > > +   spin_lock_init(&ret_buf->stat_lock);
> > > +   atomic_set(&ret_buf->num_local_opens, 0);
> > > +   atomic_set(&ret_buf->num_remote_opens, 0);
> > > +
> > >     return ret_buf;
> > >  }
> > >
>
>
>


-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-check-kzalloc-return.patch --]
[-- Type: text/x-patch, Size: 2549 bytes --]

From 687f983f49d80037bee0125f431f8e0243c7ab77 Mon Sep 17 00:00:00 2001
From: Joe Perches <joe@perches.com>
Date: Thu, 20 Dec 2018 23:50:48 -0600
Subject: [PATCH] cifs: check kzalloc return

kzalloc can return NULL so an additional check is needed. While there
is a check for ret_buf there is no check for the allocation of
ret_buf->crfid.fid - this check is thus added. Both call-sites
of tconInfoAlloc() check for NULL return of tconInfoAlloc()
so returning NULL on failure of kzalloc() here seems appropriate.
As the kzalloc() is the only thing here that can fail it is
moved to the beginning so as not to initialize other resources
on failure of kzalloc.

Fixes: 3d4ef9a15343 ("smb3: fix redundant opens on root")
---

Problem located with an experimental coccinelle script

While at it make checkpatch happy by using *ret_buf->crfid.fid
rather than struct cifs_fid.

Signed-off-by: Joe Perches <joe@perches.com>
Reported-by: Nicholas Mc Guire <hofrat@osadl.org>
Reviewed-by: Nicholas Mc Guire <hofrat@osadl.org>
---
 fs/cifs/misc.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 113980dba4d8..bee203055b30 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -111,21 +111,27 @@ struct cifs_tcon *
 tconInfoAlloc(void)
 {
 	struct cifs_tcon *ret_buf;
-	ret_buf = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
-	if (ret_buf) {
-		atomic_inc(&tconInfoAllocCount);
-		ret_buf->tidStatus = CifsNew;
-		++ret_buf->tc_count;
-		INIT_LIST_HEAD(&ret_buf->openFileList);
-		INIT_LIST_HEAD(&ret_buf->tcon_list);
-		spin_lock_init(&ret_buf->open_file_lock);
-		mutex_init(&ret_buf->crfid.fid_mutex);
-		ret_buf->crfid.fid = kzalloc(sizeof(struct cifs_fid),
-					     GFP_KERNEL);
-		spin_lock_init(&ret_buf->stat_lock);
-		atomic_set(&ret_buf->num_local_opens, 0);
-		atomic_set(&ret_buf->num_remote_opens, 0);
+
+	ret_buf = kzalloc(sizeof(*ret_buf), GFP_KERNEL);
+	if (!ret_buf)
+		return NULL;
+	ret_buf->crfid.fid = kzalloc(sizeof(*ret_buf->crfid.fid), GFP_KERNEL);
+	if (!ret_buf->crfid.fid) {
+		kfree(ret_buf);
+		return NULL;
 	}
+
+	atomic_inc(&tconInfoAllocCount);
+	ret_buf->tidStatus = CifsNew;
+	++ret_buf->tc_count;
+	INIT_LIST_HEAD(&ret_buf->openFileList);
+	INIT_LIST_HEAD(&ret_buf->tcon_list);
+	spin_lock_init(&ret_buf->open_file_lock);
+	mutex_init(&ret_buf->crfid.fid_mutex);
+	spin_lock_init(&ret_buf->stat_lock);
+	atomic_set(&ret_buf->num_local_opens, 0);
+	atomic_set(&ret_buf->num_remote_opens, 0);
+
 	return ret_buf;
 }
 
-- 
2.17.1