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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
---
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
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
>
>
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
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
If you send a patch series then it's ok that later patches depend on the
earlier patches.
regards,
dan carpenter
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
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
---
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
Remove checks that are redundant since we don't have boards with partially
initialized ->channels[i].
Signed-off-by: Giedrius Statkevičius <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
---
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
Remove checks that are redundant since we don't have boards with partially
initialized ->channels[i].
Signed-off-by: Giedrius Statkevičius <[email protected]>
---
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
Please drop this one Giedrius spotted a mistake in it and sent a v5
patch.
regards,
dan carpenter