[PATCH] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[PATCH] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

kzalloc() could fail so add a check and return -ENOMEM if it does that gets
propogated to the pci layer

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
 drivers/staging/dgnc/dgnc_tty.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 61d5a8e..23337da 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -311,6 +311,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
 			 * interrupt context, and there are no locks held.
 			 */
 			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
+			if (!brd->channels[i])
+				return -ENOMEM;
 		}
 	}
 
-- 
2.3.5

Re: [PATCH] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Sudip Mukherjee]

On Tue, Apr 07, 2015 at 01:26:32PM +0300, Giedrius Statkevičius wrote:
> kzalloc() could fail so add a check and return -ENOMEM if it does that gets
> propogated to the pci layer
> 
> Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> ---
>  drivers/staging/dgnc/dgnc_tty.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
> index 61d5a8e..23337da 100644
> --- a/drivers/staging/dgnc/dgnc_tty.c
> +++ b/drivers/staging/dgnc/dgnc_tty.c
> @@ -311,6 +311,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
>  			 * interrupt context, and there are no locks held.
>  			 */
>  			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
> +			if (!brd->channels[i])
> +				return -ENOMEM;
won't this create memory leak ?
suppose you have brd->nasync = 3 
and kzalloc fails when i=2, and you return -ENOMEM,
then what happens to the memory already allocated to brd->channels[0]
and brd->channels[1] ?

regards
sudip

Re: [PATCH] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1316 bytes --]

On Tue, 7 Apr 2015, Sudip Mukherjee wrote:

> On Tue, Apr 07, 2015 at 01:26:32PM +0300, Giedrius Statkevičius wrote:
> > kzalloc() could fail so add a check and return -ENOMEM if it does that gets
> > propogated to the pci layer
> > 
> > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > ---
> >  drivers/staging/dgnc/dgnc_tty.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
> > index 61d5a8e..23337da 100644
> > --- a/drivers/staging/dgnc/dgnc_tty.c
> > +++ b/drivers/staging/dgnc/dgnc_tty.c
> > @@ -311,6 +311,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
> >  			 * interrupt context, and there are no locks held.
> >  			 */
> >  			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
> > +			if (!brd->channels[i])
> > +				return -ENOMEM;
> won't this create memory leak ?
> suppose you have brd->nasync = 3 
> and kzalloc fails when i=2, and you return -ENOMEM,
> then what happens to the memory already allocated to brd->channels[0]
> and brd->channels[1] ?
> 
> regards
> sudip
> 
Didn't think about that, sorry. It will cause a memory leak indeed. I'll send a
v2 that creates a label that frees all successful kzalloc() before returning
-ENOMEM.

Su pagarba / Regards,
Giedrius

[PATCH v2] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

If one of the allocations of memory for storing a channel information struct
fails then free all the successful allocations and return -ENOMEM that gets
propogated to the pci layer.  Also, remove a bogus skipping in the next part of
the initiation if a previous memory allocation failed because we won't execute
that if any of the allocations failed.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_tty.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..60d7e49 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -316,6 +316,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
 			 * interrupt context, and there are no locks held.
 			 */
 			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
+			if (!brd->channels[i])
+				goto err_free_channels;
 		}
 	}
 
@@ -324,10 +326,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	/* Set up channel variables */
 	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
-
-		if (!brd->channels[i])
-			continue;
-
 		spin_lock_init(&ch->ch_lock);
 
 		/* Store all our magic numbers */
@@ -375,6 +373,11 @@ int dgnc_tty_init(struct dgnc_board *brd)
 	}
 
 	return 0;
+
+err_free_channels:
+	for (i = i - 1; i >= 0; --i)
+		kfree(brd->channels[i]);
+	return -ENOMEM;
 }
 
 
-- 
2.3.5

Re: [PATCH v2] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Dan Carpenter]

On Tue, Apr 07, 2015 at 03:40:17PM +0300, Giedrius Statkevičius wrote:
> If one of the allocations of memory for storing a channel information struct
> fails then free all the successful allocations and return -ENOMEM that gets
> propogated to the pci layer.  Also, remove a bogus skipping in the next part of
> the initiation if a previous memory allocation failed because we won't execute
> that if any of the allocations failed.
> 
> Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> ---
> v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
> spotted by Sudip so create a new label that frees all successfully allocated
> stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
> next loop.
> 
>  drivers/staging/dgnc/dgnc_tty.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
> index ce4187f..60d7e49 100644
> --- a/drivers/staging/dgnc/dgnc_tty.c
> +++ b/drivers/staging/dgnc/dgnc_tty.c
> @@ -316,6 +316,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
>  			 * interrupt context, and there are no locks held.
>  			 */
>  			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
> +			if (!brd->channels[i])
> +				goto err_free_channels;

The comments here say that sometimes brd->channels[] are allocated
earlier.  If that's true then the error handling is not correct.  But
I don't think it is true...  Could you investigate and delete the
comments and unnecessary "if (!brd->channels[i])" NULL check.

regards,
dan carpenter

Re: [PATCH v2] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1983 bytes --]

On Tue, 7 Apr 2015, Dan Carpenter wrote:

> On Tue, Apr 07, 2015 at 03:40:17PM +0300, Giedrius Statkevičius wrote:
> > If one of the allocations of memory for storing a channel information struct
> > fails then free all the successful allocations and return -ENOMEM that gets
> > propogated to the pci layer.  Also, remove a bogus skipping in the next part of
> > the initiation if a previous memory allocation failed because we won't execute
> > that if any of the allocations failed.
> > 
> > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > ---
> > v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
> > spotted by Sudip so create a new label that frees all successfully allocated
> > stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
> > next loop.
> > 
> >  drivers/staging/dgnc/dgnc_tty.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
> > index ce4187f..60d7e49 100644
> > --- a/drivers/staging/dgnc/dgnc_tty.c
> > +++ b/drivers/staging/dgnc/dgnc_tty.c
> > @@ -316,6 +316,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
> >  			 * interrupt context, and there are no locks held.
> >  			 */
> >  			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
> > +			if (!brd->channels[i])
> > +				goto err_free_channels;
> 
> The comments here say that sometimes brd->channels[] are allocated
> earlier.  If that's true then the error handling is not correct.  But
> I don't think it is true...  Could you investigate and delete the
> comments and unnecessary "if (!brd->channels[i])" NULL check.
> 
> regards,
> dan carpenter
> 

I've checked this earlier and now looked over again and I didn't find any other
place where this is allocated. My thought was that deleting that comment and
that check could be too much for one patch. I'll send a v3.

Su pagarba / Regards,
Giedrius

[PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

If one of the allocations of memory for storing a channel information struct
fails then free all the successful allocations and return -ENOMEM that gets
propogated to the pci layer.  Also, remove a bogus skipping in the next part of
the initiation if a previous memory allocation failed because we won't execute
that if any of the allocations failed. Next, remove the misleading comment that
allocation could happen elsewhere. Finally, remove all (except in the ioctl
which can try to get information about a board that failed to probe) checks if
->channels[foo] is NULL or not because probe failing if we can't allocate enough
memory means that this scenario isn't possible.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v3: Remove the wrong comment at dgnc_tty_init() that says the allocation could
happen somewhere else before this and remove the check if (!brd->channels[i]).
Also, remove all checks where ->channels[i] is checked if it's NULL or not
because if probe fails then that means that this scenario isn't possible (except
in the ioctl).

v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_cls.c |  4 +---
 drivers/staging/dgnc/dgnc_neo.c |  2 +-
 drivers/staging/dgnc/dgnc_tty.c | 29 +++++++++++++----------------
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index e3564d2..a629a78 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
@@ -714,8 +714,6 @@ static void cls_tasklet(unsigned long data)
 		/* Loop on each port */
 		for (i = 0; i < ports; i++) {
 			ch = bd->channels[i];
-			if (!ch)
-				continue;
 
 			/*
 			 * NOTE: Remember you CANNOT hold any channel
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index f5a4d36..1e583c2 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..d1a2c0f 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -304,19 +304,15 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	brd->nasync = brd->maxports;
 
-	/*
-	 * Allocate channel memory that might not have been allocated
-	 * when the driver was first loaded.
-	 */
 	for (i = 0; i < brd->nasync; i++) {
-		if (!brd->channels[i]) {
-
-			/*
-			 * Okay to malloc with GFP_KERNEL, we are not at
-			 * interrupt context, and there are no locks held.
-			 */
-			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
-		}
+		/*
+		 * Okay to malloc with GFP_KERNEL, we are not at
+		 * interrupt context, and there are no locks held.
+		 */
+		brd->channels[i] = kzalloc(sizeof(*brd->channels[i]),
+					   GFP_KERNEL);
+		if (!brd->channels[i])
+			goto err_free_channels;
 	}
 
 	ch = brd->channels[0];
@@ -324,10 +320,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	/* Set up channel variables */
 	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
-
-		if (!brd->channels[i])
-			continue;
-
 		spin_lock_init(&ch->ch_lock);
 
 		/* Store all our magic numbers */
@@ -375,6 +367,11 @@ int dgnc_tty_init(struct dgnc_board *brd)
 	}
 
 	return 0;
+
+err_free_channels:
+	for (i = i - 1; i >= 0; --i)
+		kfree(brd->channels[i]);
+	return -ENOMEM;
 }
 
 
-- 
2.3.5

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Sudip Mukherjee]

On Tue, Apr 07, 2015 at 05:11:15PM +0300, Giedrius Statkevičius wrote:
> If one of the allocations of memory for storing a channel information struct
> fails then free all the successful allocations and return -ENOMEM that gets
> propogated to the pci layer.  Also, remove a bogus skipping in the next part of
> the initiation if a previous memory allocation failed because we won't execute
> that if any of the allocations failed. Next, remove the misleading comment that
> allocation could happen elsewhere. Finally, remove all (except in the ioctl
> which can try to get information about a board that failed to probe) checks if
> ->channels[foo] is NULL or not because probe failing if we can't allocate enough
> memory means that this scenario isn't possible.

i think now it became too many changes for a single patch..

regards
sudip


> 
> 

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1212 bytes --]

On Tue, 7 Apr 2015, Sudip Mukherjee wrote:

> On Tue, Apr 07, 2015 at 05:11:15PM +0300, Giedrius Statkevičius wrote:
> > If one of the allocations of memory for storing a channel information struct
> > fails then free all the successful allocations and return -ENOMEM that gets
> > propogated to the pci layer.  Also, remove a bogus skipping in the next part of
> > the initiation if a previous memory allocation failed because we won't execute
> > that if any of the allocations failed. Next, remove the misleading comment that
> > allocation could happen elsewhere. Finally, remove all (except in the ioctl
> > which can try to get information about a board that failed to probe) checks if
> > ->channels[foo] is NULL or not because probe failing if we can't allocate enough
> > memory means that this scenario isn't possible.
> 
> i think now it became too many changes for a single patch..
> 
> regards
> sudip

If I split this into two patches then they both would have to be applied and
Greg or someone else could miss that one patch depended on another and leave the
kernel in a partial state so I think it's best to keep it in one. Lets see what
other people have to say.

Su pagarba / Regards,
Giedrius

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Dan Carpenter]

If you send a patch series then it's ok that later patches depend on the
earlier patches.

regards,
dan carpenter

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Dan Carpenter]

You will need to update the subject to reflect the new patch.

The original code did check for kzalloc() failure but it had lots of
checks scattered around instead nicely at the point where the memory
was allocated.

The old code and the new code are both buggy though and will crash in
dgnc_tty_uninit().  dgnc_found_board() does "One Err" style error
handling so it's obviously buggy like the underside of a rock.
https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

It's becoming a difficult thing to fix this because every time we look
there are more things which don't make sense.

I believe that if you do:

> +err_free_channels:
> +	for (i = i - 1; i >= 0; --i) {
> +		kfree(brd->channels[i]);
		brd->channels[i] = NULL;
	}
> +	return -ENOMEM;
>  }

And add some NULL checks in dgnc_tty_uninit() to see if ->channels[i] is
NULL before doing:

	dgnc_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);

and
	dgnc_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);

Then it will fix the bug.

Eventually we will want to clean up dgnc_found_board() error handling
and get rid of brd->dgnc_Major_TransparentPrint_Registered etc.

TODO: dgnc: cleanup dgnc_found_board().

> diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
> index e3564d2..a629a78 100644
> --- a/drivers/staging/dgnc/dgnc_cls.c
> +++ b/drivers/staging/dgnc/dgnc_cls.c
> @@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, uint port)
>  		return;
>  
>  	ch = brd->channels[port];
> -	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
> +	if (ch->magic != DGNC_CHANNEL_MAGIC)
>  		return;
>  
>  	/* Here we try to figure out what caused the interrupt to happen */
> @@ -714,8 +714,6 @@ static void cls_tasklet(unsigned long data)
>  		/* Loop on each port */
>  		for (i = 0; i < ports; i++) {
>  			ch = bd->channels[i];
> -			if (!ch)
> -				continue;
>  
>  			/*
>  			 * NOTE: Remember you CANNOT hold any channel
> diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
> index f5a4d36..1e583c2 100644
> --- a/drivers/staging/dgnc/dgnc_neo.c
> +++ b/drivers/staging/dgnc/dgnc_neo.c
> @@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, uint port)
>  		return;
>  
>  	ch = brd->channels[port];
> -	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
> +	if (ch->magic != DGNC_CHANNEL_MAGIC)
>  		return;
>  

Do these in a separate patch.  I'm looking for ways we can make this
patch minimal.  Deleting the comments and the NULL check in
dgnc_tty_init() is essential for the patch because otherwise the cleanup
doesn't make sense.

regards,
dan carpenter

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

On Tue, 7 Apr 2015, Dan Carpenter wrote:

> You will need to update the subject to reflect the new patch.
> 
> The original code did check for kzalloc() failure but it had lots of
> checks scattered around instead nicely at the point where the memory
> was allocated.
> 

There are a lot missing too. For example in dgnc_sysfs.c there are no checks on
any _show() methods if ->channels[i] is NULL or not. And in some other places.

> The old code and the new code are both buggy though and will crash in
> dgnc_tty_uninit().  dgnc_found_board() does "One Err" style error
> handling so it's obviously buggy like the underside of a rock.
> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ
> 
> It's becoming a difficult thing to fix this because every time we look
> there are more things which don't make sense.
> 
> I believe that if you do:
> 
> > +err_free_channels:
> > +	for (i = i - 1; i >= 0; --i) {
> > +		kfree(brd->channels[i]);
> 		brd->channels[i] = NULL;
> 	}
> > +	return -ENOMEM;
> >  }
> 
> And add some NULL checks in dgnc_tty_uninit() to see if ->channels[i] is
> NULL before doing:
> 
> 	dgnc_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
> 
> and
> 	dgnc_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
> 
> Then it will fix the bug.
> 

Missed this. Yep, I agree. 

> Do these in a separate patch.  I'm looking for ways we can make this
> patch minimal.  Deleting the comments and the NULL check in
> dgnc_tty_init() is essential for the patch because otherwise the cleanup
> doesn't make sense.

Well, the point of the patch is to put alloc and checks in one place to make the
code less error and bug prone and fix some of bugs where ->channels[i] isn't
checked. Ok, I'll send a v5 that's split into two patches.

> 
> regards,
> dan carpenter
> 
> 

Su pagarba / Regards,
Giedrius

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Dan Carpenter]

On Tue, Apr 07, 2015 at 06:35:51PM +0300, Giedrius Statkevičius wrote:
> Well, the point of the patch is to put alloc and checks in one place to make the
> code less error and bug prone and fix some of bugs where ->channels[i] isn't
> checked.

Fine, fine.  I can accept that.  One patch is also ok.

regards,
dan carpenter

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Dan Carpenter]

On Tue, Apr 07, 2015 at 06:35:51PM +0300, Giedrius Statkevičius wrote:
> Well, the point of the patch is to put alloc and checks in one place to make the
> code less error and bug prone and fix some of bugs where ->channels[i] isn't
> checked.

Fine, fine.  I can accept that.  One patch is also ok.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v4 1/3] staging: dgnc: clean up allocation of ->channels[i]

[Giedrius Statkevičius]

Check if kzalloc fails in dgnc_tty_init() and if it does then free all
previously allocated ->channels[i] and set them to NULL. This makes the code
less error/bug prone because instead of needing programmers attention to add
checks everywhere we do that in one place. Also, remove a bogus comment and
check in the same loop because ->channels[i] isn't allocated anywhere else.
Finally, remove a unnecessary check if ->channels[i] is NULL in the next loop
because it can't be.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v4: Make this patch only for dgnc_tty.c and only for this thing. Split into 3
patches (1 patch fixes a bug reported by Dan Carpenter)

v3: Remove the wrong comment at dgnc_tty_init() that says the allocation could
happen somewhere else before this and remove the check if (!brd->channels[i]).
Also, remove all checks where ->channels[i] is checked if it's NULL or not
because if probe fails then that means that this scenario isn't possible (except
in the ioctl).

v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_tty.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..0e48f95 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -304,19 +304,15 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	brd->nasync = brd->maxports;
 
-	/*
-	 * Allocate channel memory that might not have been allocated
-	 * when the driver was first loaded.
-	 */
 	for (i = 0; i < brd->nasync; i++) {
-		if (!brd->channels[i]) {
-
-			/*
-			 * Okay to malloc with GFP_KERNEL, we are not at
-			 * interrupt context, and there are no locks held.
-			 */
-			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
-		}
+		/*
+		 * Okay to malloc with GFP_KERNEL, we are not at
+		 * interrupt context, and there are no locks held.
+		 */
+		brd->channels[i] = kzalloc(sizeof(*brd->channels[i]),
+					   GFP_KERNEL);
+		if (!brd->channels[i])
+			goto err_free_channels;
 	}
 
 	ch = brd->channels[0];
@@ -324,10 +320,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	/* Set up channel variables */
 	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
-
-		if (!brd->channels[i])
-			continue;
-
 		spin_lock_init(&ch->ch_lock);
 
 		/* Store all our magic numbers */
@@ -375,6 +367,13 @@ int dgnc_tty_init(struct dgnc_board *brd)
 	}
 
 	return 0;
+
+err_free_channels:
+	for (i = i - 1; i >= 0; --i) {
+		kfree(brd->channels[i]);
+		brd->channels[i] = NULL;
+	}
+	return -ENOMEM;
 }
 
 
-- 
2.3.5

[PATCH v4 1/3] staging: dgnc: clean up allocation of ->channels[i]

[Giedrius Statkevičius]

Check if kzalloc fails in dgnc_tty_init() and if it does then free all
previously allocated ->channels[i] and set them to NULL. This makes the code
less error/bug prone because instead of needing programmers attention to add
checks everywhere we do that in one place. Also, remove a bogus comment and
check in the same loop because ->channels[i] isn't allocated anywhere else.
Finally, remove a unnecessary check if ->channels[i] is NULL in the next loop
because it can't be.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v4: Make this patch only for dgnc_tty.c and only for this thing. Split into 3
patches (1 patch fixes a bug reported by Dan Carpenter)

v3: Remove the wrong comment at dgnc_tty_init() that says the allocation could
happen somewhere else before this and remove the check if (!brd->channels[i]).
Also, remove all checks where ->channels[i] is checked if it's NULL or not
because if probe fails then that means that this scenario isn't possible (except
in the ioctl).

v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_tty.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..0e48f95 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -304,19 +304,15 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	brd->nasync = brd->maxports;
 
-	/*
-	 * Allocate channel memory that might not have been allocated
-	 * when the driver was first loaded.
-	 */
 	for (i = 0; i < brd->nasync; i++) {
-		if (!brd->channels[i]) {
-
-			/*
-			 * Okay to malloc with GFP_KERNEL, we are not at
-			 * interrupt context, and there are no locks held.
-			 */
-			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
-		}
+		/*
+		 * Okay to malloc with GFP_KERNEL, we are not at
+		 * interrupt context, and there are no locks held.
+		 */
+		brd->channels[i] = kzalloc(sizeof(*brd->channels[i]),
+					   GFP_KERNEL);
+		if (!brd->channels[i])
+			goto err_free_channels;
 	}
 
 	ch = brd->channels[0];
@@ -324,10 +320,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	/* Set up channel variables */
 	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
-
-		if (!brd->channels[i])
-			continue;
-
 		spin_lock_init(&ch->ch_lock);
 
 		/* Store all our magic numbers */
@@ -375,6 +367,13 @@ int dgnc_tty_init(struct dgnc_board *brd)
 	}
 
 	return 0;
+
+err_free_channels:
+	for (i = i - 1; i >= 0; --i) {
+		kfree(brd->channels[i]);
+		brd->channels[i] = NULL;
+	}
+	return -ENOMEM;
 }
 
 
-- 
2.3.5

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 1/3] staging: dgnc: clean up allocation of ->channels[i]

[Giedrius Statkevičius]

Check if kzalloc fails in dgnc_tty_init() and if it does then free all
previously allocated ->channels[i] and set them to NULL. This makes the code
less error/bug prone because instead of needing programmers attention to add
checks everywhere we do that in one place. Also, remove a bogus comment and
check in the same loop because ->channels[i] isn't allocated anywhere else.
Finally, remove a unnecessary check if ->channels[i] is NULL in the next loop
because it can't be.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v5: no change

v4: Make this patch only for dgnc_tty.c and only for this thing. Split into 3
patches (1 patch fixes a bug reported by Dan Carpenter)

v3: Remove the wrong comment at dgnc_tty_init() that says the allocation could
happen somewhere else before this and remove the check if (!brd->channels[i]).
Also, remove all checks where ->channels[i] is checked if it's NULL or not
because if probe fails then that means that this scenario isn't possible (except
in the ioctl).

v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_tty.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..0e48f95 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -304,19 +304,15 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	brd->nasync = brd->maxports;
 
-	/*
-	 * Allocate channel memory that might not have been allocated
-	 * when the driver was first loaded.
-	 */
 	for (i = 0; i < brd->nasync; i++) {
-		if (!brd->channels[i]) {
-
-			/*
-			 * Okay to malloc with GFP_KERNEL, we are not at
-			 * interrupt context, and there are no locks held.
-			 */
-			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
-		}
+		/*
+		 * Okay to malloc with GFP_KERNEL, we are not at
+		 * interrupt context, and there are no locks held.
+		 */
+		brd->channels[i] = kzalloc(sizeof(*brd->channels[i]),
+					   GFP_KERNEL);
+		if (!brd->channels[i])
+			goto err_free_channels;
 	}
 
 	ch = brd->channels[0];
@@ -324,10 +320,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	/* Set up channel variables */
 	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
-
-		if (!brd->channels[i])
-			continue;
-
 		spin_lock_init(&ch->ch_lock);
 
 		/* Store all our magic numbers */
@@ -375,6 +367,13 @@ int dgnc_tty_init(struct dgnc_board *brd)
 	}
 
 	return 0;
+
+err_free_channels:
+	for (i = i - 1; i >= 0; --i) {
+		kfree(brd->channels[i]);
+		brd->channels[i] = NULL;
+	}
+	return -ENOMEM;
 }
 
 
-- 
2.3.5

[PATCH v5 1/3] staging: dgnc: clean up allocation of ->channels[i]

[Giedrius Statkevičius]

Check if kzalloc fails in dgnc_tty_init() and if it does then free all
previously allocated ->channels[i] and set them to NULL. This makes the code
less error/bug prone because instead of needing programmers attention to add
checks everywhere we do that in one place. Also, remove a bogus comment and
check in the same loop because ->channels[i] isn't allocated anywhere else.
Finally, remove a unnecessary check if ->channels[i] is NULL in the next loop
because it can't be.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v5: no change

v4: Make this patch only for dgnc_tty.c and only for this thing. Split into 3
patches (1 patch fixes a bug reported by Dan Carpenter)

v3: Remove the wrong comment at dgnc_tty_init() that says the allocation could
happen somewhere else before this and remove the check if (!brd->channels[i]).
Also, remove all checks where ->channels[i] is checked if it's NULL or not
because if probe fails then that means that this scenario isn't possible (except
in the ioctl).

v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_tty.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..0e48f95 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -304,19 +304,15 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	brd->nasync = brd->maxports;
 
-	/*
-	 * Allocate channel memory that might not have been allocated
-	 * when the driver was first loaded.
-	 */
 	for (i = 0; i < brd->nasync; i++) {
-		if (!brd->channels[i]) {
-
-			/*
-			 * Okay to malloc with GFP_KERNEL, we are not at
-			 * interrupt context, and there are no locks held.
-			 */
-			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
-		}
+		/*
+		 * Okay to malloc with GFP_KERNEL, we are not at
+		 * interrupt context, and there are no locks held.
+		 */
+		brd->channels[i] = kzalloc(sizeof(*brd->channels[i]),
+					   GFP_KERNEL);
+		if (!brd->channels[i])
+			goto err_free_channels;
 	}
 
 	ch = brd->channels[0];
@@ -324,10 +320,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	/* Set up channel variables */
 	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
-
-		if (!brd->channels[i])
-			continue;
-
 		spin_lock_init(&ch->ch_lock);
 
 		/* Store all our magic numbers */
@@ -375,6 +367,13 @@ int dgnc_tty_init(struct dgnc_board *brd)
 	}
 
 	return 0;
+
+err_free_channels:
+	for (i = i - 1; i >= 0; --i) {
+		kfree(brd->channels[i]);
+		brd->channels[i] = NULL;
+	}
+	return -ENOMEM;
 }
 
 
-- 
2.3.5

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 2/3] staging: dgnc: don't forget to check if ->channels[i] is NULL in dgnc_tty_uninit()

[Giedrius Statkevičius]

Add a check if ->channels[i] is NULL because a NULL pointer may be dereferenced
in case one of the allocations failed

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v5: no change
v4: new patch that fixes a bug reported by Dan Carpenter

 drivers/staging/dgnc/dgnc_tty.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 0e48f95..785eb6c 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -403,7 +403,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
 		dgnc_BoardsByMajor[brd->SerialDriver.major] = NULL;
 		brd->dgnc_Serial_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
-			dgnc_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
+			if (brd->channels[i])
+				dgnc_remove_tty_sysfs(brd->channels[i]->
+						      ch_tun.un_sysfs);
 			tty_unregister_device(&brd->SerialDriver, i);
 		}
 		tty_unregister_driver(&brd->SerialDriver);
@@ -414,7 +416,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
 		dgnc_BoardsByMajor[brd->PrintDriver.major] = NULL;
 		brd->dgnc_TransparentPrint_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
-			dgnc_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
+			if (brd->channels[i])
+				dgnc_remove_tty_sysfs(brd->channels[i]->
+						      ch_pun.un_sysfs);
 			tty_unregister_device(&brd->PrintDriver, i);
 		}
 		tty_unregister_driver(&brd->PrintDriver);
-- 
2.3.5

[PATCH v5 2/3] staging: dgnc: don't forget to check if ->channels[i] is NULL in dgnc_tty_uninit()

[Giedrius Statkevičius]

Add a check if ->channels[i] is NULL because a NULL pointer may be dereferenced
in case one of the allocations failed

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v5: no change
v4: new patch that fixes a bug reported by Dan Carpenter

 drivers/staging/dgnc/dgnc_tty.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 0e48f95..785eb6c 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -403,7 +403,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
 		dgnc_BoardsByMajor[brd->SerialDriver.major] = NULL;
 		brd->dgnc_Serial_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
-			dgnc_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
+			if (brd->channels[i])
+				dgnc_remove_tty_sysfs(brd->channels[i]->
+						      ch_tun.un_sysfs);
 			tty_unregister_device(&brd->SerialDriver, i);
 		}
 		tty_unregister_driver(&brd->SerialDriver);
@@ -414,7 +416,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
 		dgnc_BoardsByMajor[brd->PrintDriver.major] = NULL;
 		brd->dgnc_TransparentPrint_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
-			dgnc_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
+			if (brd->channels[i])
+				dgnc_remove_tty_sysfs(brd->channels[i]->
+						      ch_pun.un_sysfs);
 			tty_unregister_device(&brd->PrintDriver, i);
 		}
 		tty_unregister_driver(&brd->PrintDriver);
-- 
2.3.5

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v5 3/3] staging: dgnc: remove redundant !ch checks

[Giedrius Statkevičius]

Remove checks that are redundant since we don't have boards with partially
initialized ->channels[i].

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v5: there was a mistake in v4 in which a wrong check was deleted in cls_tasklet
v4: splitted this from the one patch.

 drivers/staging/dgnc/dgnc_cls.c | 4 +---
 drivers/staging/dgnc/dgnc_neo.c | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index e3564d2..a629a78 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
@@ -714,8 +714,6 @@ static void cls_tasklet(unsigned long data)
 		/* Loop on each port */
 		for (i = 0; i < ports; i++) {
 			ch = bd->channels[i];
-			if (!ch)
-				continue;
 
 			/*
 			 * NOTE: Remember you CANNOT hold any channel
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index f5a4d36..1e583c2 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
-- 
2.3.5

[PATCH v5 3/3] staging: dgnc: remove redundant !ch checks

[Giedrius Statkevičius]

Remove checks that are redundant since we don't have boards with partially
initialized ->channels[i].

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v5: there was a mistake in v4 in which a wrong check was deleted in cls_tasklet
v4: splitted this from the one patch.

 drivers/staging/dgnc/dgnc_cls.c | 4 +---
 drivers/staging/dgnc/dgnc_neo.c | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index e3564d2..a629a78 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
@@ -714,8 +714,6 @@ static void cls_tasklet(unsigned long data)
 		/* Loop on each port */
 		for (i = 0; i < ports; i++) {
 			ch = bd->channels[i];
-			if (!ch)
-				continue;
 
 			/*
 			 * NOTE: Remember you CANNOT hold any channel
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index f5a4d36..1e583c2 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
-- 
2.3.5

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v4 2/3] staging: dgnc: don't forget to check if ->channels[i] is NULL in dgnc_tty_uninit()

[Giedrius Statkevičius]

Add a check if ->channels[i] is NULL because a NULL pointer may be dereferenced
in case one of the allocations failed

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v4: new patch that fixes a bug reported by Dan Carpenter

 drivers/staging/dgnc/dgnc_tty.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 0e48f95..785eb6c 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -403,7 +403,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
 		dgnc_BoardsByMajor[brd->SerialDriver.major] = NULL;
 		brd->dgnc_Serial_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
-			dgnc_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
+			if (brd->channels[i])
+				dgnc_remove_tty_sysfs(brd->channels[i]->
+						      ch_tun.un_sysfs);
 			tty_unregister_device(&brd->SerialDriver, i);
 		}
 		tty_unregister_driver(&brd->SerialDriver);
@@ -414,7 +416,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
 		dgnc_BoardsByMajor[brd->PrintDriver.major] = NULL;
 		brd->dgnc_TransparentPrint_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
-			dgnc_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
+			if (brd->channels[i])
+				dgnc_remove_tty_sysfs(brd->channels[i]->
+						      ch_pun.un_sysfs);
 			tty_unregister_device(&brd->PrintDriver, i);
 		}
 		tty_unregister_driver(&brd->PrintDriver);
-- 
2.3.5

[PATCH v4 2/3] staging: dgnc: don't forget to check if ->channels[i] is NULL in dgnc_tty_uninit()

[Giedrius Statkevičius]

Add a check if ->channels[i] is NULL because a NULL pointer may be dereferenced
in case one of the allocations failed

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v4: new patch that fixes a bug reported by Dan Carpenter

 drivers/staging/dgnc/dgnc_tty.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 0e48f95..785eb6c 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -403,7 +403,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
 		dgnc_BoardsByMajor[brd->SerialDriver.major] = NULL;
 		brd->dgnc_Serial_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
-			dgnc_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
+			if (brd->channels[i])
+				dgnc_remove_tty_sysfs(brd->channels[i]->
+						      ch_tun.un_sysfs);
 			tty_unregister_device(&brd->SerialDriver, i);
 		}
 		tty_unregister_driver(&brd->SerialDriver);
@@ -414,7 +416,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
 		dgnc_BoardsByMajor[brd->PrintDriver.major] = NULL;
 		brd->dgnc_TransparentPrint_Major = 0;
 		for (i = 0; i < brd->nasync; i++) {
-			dgnc_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
+			if (brd->channels[i])
+				dgnc_remove_tty_sysfs(brd->channels[i]->
+						      ch_pun.un_sysfs);
 			tty_unregister_device(&brd->PrintDriver, i);
 		}
 		tty_unregister_driver(&brd->PrintDriver);
-- 
2.3.5

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v4 3/3] staging: dgnc: remove redundant !ch checks

[Giedrius Statkevičius]

Remove checks that are redundant since we don't have boards with partially
initialized ->channels[i].

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v4: splitted this from the one patch.

 drivers/staging/dgnc/dgnc_cls.c | 4 ++--
 drivers/staging/dgnc/dgnc_neo.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index e3564d2..82e8680 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
@@ -691,7 +691,7 @@ static void cls_tasklet(unsigned long data)
 	int state = 0;
 	int ports = 0;
 
-	if (!bd || bd->magic != DGNC_BOARD_MAGIC)
+	if (bd->magic != DGNC_BOARD_MAGIC)
 		return;
 
 	/* Cache a couple board values */
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index f5a4d36..1e583c2 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
-- 
2.3.5

[PATCH v4 3/3] staging: dgnc: remove redundant !ch checks

[Giedrius Statkevičius]

Remove checks that are redundant since we don't have boards with partially
initialized ->channels[i].

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
v4: splitted this from the one patch.

 drivers/staging/dgnc/dgnc_cls.c | 4 ++--
 drivers/staging/dgnc/dgnc_neo.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index e3564d2..82e8680 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
@@ -691,7 +691,7 @@ static void cls_tasklet(unsigned long data)
 	int state = 0;
 	int ports = 0;
 
-	if (!bd || bd->magic != DGNC_BOARD_MAGIC)
+	if (bd->magic != DGNC_BOARD_MAGIC)
 		return;
 
 	/* Cache a couple board values */
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index f5a4d36..1e583c2 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
-- 
2.3.5

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 3/3] staging: dgnc: remove redundant !ch checks

[Dan Carpenter]

Please drop this one Giedrius spotted a mistake in it and sent a v5
patch.

regards,
dan carpenter