cifs: check kzalloc return

Message ID 1545150439-26055-1-git-send-email-hofrat@osadl.org
State New
Headers show
Series
  • cifs: check kzalloc return
Related show

Commit Message

Nicholas Mc Guire Dec. 18, 2018, 4:27 p.m.
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(-)

Comments

Joe Perches Dec. 18, 2018, 5:33 p.m. | #1
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;
 }
Nicholas Mc Guire Dec. 19, 2018, 7:22 a.m. | #2
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;
>  }
>  
>
Joe Perches Dec. 19, 2018, 10:57 a.m. | #3
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;
> >  }
> >
Steve French Dec. 21, 2018, 5:53 a.m. | #4
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;
> > >  }
> > >
>
>
>

Patch

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