Re: [PATCH] [v2] nvdimm-btt: fix several memleaks
> dinghao.liu@ wrote: > > > Ira Weiny wrote: > > > > Dinghao Liu wrote: [snip] > > > > > > > > This does not quite work. > > > > > > > > free_arenas() is used in the error paths of create_arenas() and > > > > discover_arenas(). In those cases devm_kfree() is probably a better way > > > > to clean up this. > > > > Here I'm a little confused about when devm_kfree() should be used. > > Over all it should be used whenever memory is allocated for the lifetime > of the device. > > > Code in btt_init() implies that resources allocated by devm_* could be > > auto freed in both error and success paths of probe/attach (e.g., btt > > allocated by devm_kzalloc is never freed by devm_kfree). > > Using devm_kfree() in free_arenas() is ok for me, but I want to know > > whether not using devm_kfree() constitutes a bug. > > Unfortunately I'm not familiar enough with this code to know for sure. > > After my quick checks before I thought it was. But each time I look at it > I get confused. This is why I was thinking maybe not using devm_*() and > using no_free_ptr() may be a better solution to make sure things don't > leak without changing the success path (which is likely working fine > because no bugs have been found.) We have the same confusion here... I find a discussion about this problem, which implies that not using devm_kfree() may delay the release, but the memory will be freed later and no memory is leaked: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2009561.html > > > > > We might want to look at using no_free_ptr() in this code. See this > > > patch[1] for an example of how to inhibit the cleanup and pass the > > > pointer on when the function succeeds. > > > > > > [1] > > > https://lore.kernel.org/all/170261791914.1714654.6447680285357545638.st...@dwillia2-xfh.jf.intel.com/ > > > > > > Ira > > > > Thanks for this example. But it seems that no_free_ptr() is used to > > handle the scope based resource management. Changes in this patch does > > not introduce this feature. Do I understand this correctly? > > You do understand but I was thinking that perhaps using no_free_ptr() > rather than devm_*() might be an easier way to fix this bug without trying > to decode the lifetime of everything. > My concern is that no_free_ptr() may not be able to completely fix all memleaks because some of them are triggered in (part of) success paths (e.g., when btt_freelist_init succeeds but btt_rtt_init fails, discover_arenas still needs to clean up the memory allocated in btt_freelist_init). I checked the design of no_free_ptr(), and it seems that it will generate a new pointer on success and the memory still leaks in the above case. Therefore, I think using devm_*() is still the best solution for this bug. Regards, Dinghao
Re: [PATCH] [v2] nvdimm-btt: fix several memleaks
> Ira Weiny wrote:
> > Dinghao Liu wrote:
>
> [snip]
>
> -static int btt_freelist_init(struct arena_info *arena)
> +static int btt_freelist_init(struct device *dev, struct arena_info *arena)
>
> Both struct arena_info and struct btt contain references to struct nd_btt
> which is the device you are passing down this call stack.
>
> Just use the device in the arena/btt rather than passing a device
> structure. That makes the code easier to read and ensures that the device
> associated with this arena or btt is used.
Thanks for this suggestion! I will fix this in the v3 patch.
> [snip]
> > >
> > > -static int btt_maplocks_init(struct arena_info *arena)
> > > +static int btt_maplocks_init(struct device *dev, struct arena_info
> > > *arena)
> > > {
> > > u32 i;
> > >
> > > - arena->map_locks = kcalloc(arena->nfree, sizeof(struct aligned_lock),
> > > + arena->map_locks = devm_kcalloc(dev, arena->nfree, sizeof(struct
> > > aligned_lock),
> > > GFP_KERNEL);
> > > if (!arena->map_locks)
> > > return -ENOMEM;
> > > @@ -805,9 +805,6 @@ static void free_arenas(struct btt *btt)
> > >
> > > list_for_each_entry_safe(arena, next, >arena_list, list) {
> > > list_del(>list);
> > > - kfree(arena->rtt);
> > > - kfree(arena->map_locks);
> > > - kfree(arena->freelist);
> >
> > This does not quite work.
> >
> > free_arenas() is used in the error paths of create_arenas() and
> > discover_arenas(). In those cases devm_kfree() is probably a better way
> > to clean up this.
Here I'm a little confused about when devm_kfree() should be used.
Code in btt_init() implies that resources allocated by devm_* could be
auto freed in both error and success paths of probe/attach (e.g., btt
allocated by devm_kzalloc is never freed by devm_kfree).
Using devm_kfree() in free_arenas() is ok for me, but I want to know
whether not using devm_kfree() constitutes a bug.
> >
> > However...
> >
> > > debugfs_remove_recursive(arena->debugfs_dir);
> > > kfree(arena);
> >
> > Why can't arena be allocated with devm_*?
> >
> > We need to change this up a bit more to handle the error path vs regular
> > device shutdown free (automatic devm frees).
>
At first, I think the use of arena is correct. Therefore, allocating arena
with devm_* should be a code style optimization. However, I rechecked
discover_arenas and found arena might also be leaked (e.g., if alloc_arena()
fails in the second loop, the previously allocated resources in the loop is
leaked). The correct code could be found in create_arenas(), where free_arenas
is called on failure of alloc_arena().
To fix this issue, I think it's better to change the 'goto out_super' tag
to 'goto out'. We could also use devm_* for arena to simplify the error path
in discover_arenas().
> We might want to look at using no_free_ptr() in this code. See this
> patch[1] for an example of how to inhibit the cleanup and pass the pointer
> on when the function succeeds.
>
> [1]
> https://lore.kernel.org/all/170261791914.1714654.6447680285357545638.st...@dwillia2-xfh.jf.intel.com/
>
> Ira
Thanks for this example. But it seems that no_free_ptr() is used to handle
the scope based resource management. Changes in this patch does not introduce
this feature. Do I understand this correctly?
Regards,
Dinghao
[PATCH] [v2] nvdimm-btt: simplify code with the scope based resource management
Use the scope based resource management (defined in
linux/cleanup.h) to automate resource lifetime
control on struct btt_sb *super in discover_arenas().
Signed-off-by: Dinghao Liu
---
Changelog:
v2: Set the __free attribute before kzalloc.
---
drivers/nvdimm/btt.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index d5593b0dc700..32a9e2f543c5 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include "btt.h"
#include "nd.h"
@@ -847,23 +848,20 @@ static int discover_arenas(struct btt *btt)
{
int ret = 0;
struct arena_info *arena;
- struct btt_sb *super;
size_t remaining = btt->rawsize;
u64 cur_nlba = 0;
size_t cur_off = 0;
int num_arenas = 0;
- super = kzalloc(sizeof(*super), GFP_KERNEL);
+ struct btt_sb *super __free(kfree) = kzalloc(sizeof(*super),
GFP_KERNEL);
if (!super)
return -ENOMEM;
while (remaining) {
/* Alloc memory for arena */
arena = alloc_arena(btt, 0, 0, 0);
- if (!arena) {
- ret = -ENOMEM;
- goto out_super;
- }
+ if (!arena)
+ return -ENOMEM;
arena->infooff = cur_off;
ret = btt_info_read(arena, super);
@@ -919,14 +917,11 @@ static int discover_arenas(struct btt *btt)
btt->nlba = cur_nlba;
btt->init_state = INIT_READY;
- kfree(super);
return ret;
out:
kfree(arena);
free_arenas(btt);
- out_super:
- kfree(super);
return ret;
}
--
2.17.1
Re: [PATCH] nvdimm-btt: simplify code with the scope based resource management
> > It's a little strange that we do not check super immediately after
> > allocation.
> > How about this:
> >
> > static int discover_arenas(struct btt *btt)
> > {
> > int ret = 0;
> > struct arena_info *arena;
> > - struct btt_sb *super;
> > size_t remaining = btt->rawsize;
> > u64 cur_nlba = 0;
> > size_t cur_off = 0;
> > int num_arenas = 0;
> >
> > - super = kzalloc(sizeof(*super), GFP_KERNEL);
> > + struct btt_sb *super __free(kfree) =
> > + kzalloc(sizeof(*super), GFP_KERNEL);
> > if (!super)
> > return -ENOMEM;
> >
> > while (remaining) {
> >
>
> That's fine by me
I will resend a new patch soon, thanks!
Regards,
Dinghao
Re: [PATCH] nvdimm-btt: simplify code with the scope based resource management
>
> On 12/10/23 03:27, Dinghao Liu wrote:
> > Use the scope based resource management (defined in
> > linux/cleanup.h) to automate resource lifetime
> > control on struct btt_sb *super in discover_arenas().
> >
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/nvdimm/btt.c | 12
> > 1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> > index d5593b0dc700..ff42778b51de 100644
> > --- a/drivers/nvdimm/btt.c
> > +++ b/drivers/nvdimm/btt.c
> > @@ -16,6 +16,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "btt.h"
> > #include "nd.h"
> >
> > @@ -847,7 +848,7 @@ static int discover_arenas(struct btt *btt)
> > {
> > int ret = 0;
> > struct arena_info *arena;
> > - struct btt_sb *super;
> > + struct btt_sb *super __free(kfree) = NULL;
> > size_t remaining = btt->rawsize;
> > u64 cur_nlba = 0;
> > size_t cur_off = 0;
> > @@ -860,10 +861,8 @@ static int discover_arenas(struct btt *btt)
> > while (remaining) {
> > /* Alloc memory for arena */
> > arena = alloc_arena(btt, 0, 0, 0);
> > - if (!arena) {
> > - ret = -ENOMEM;
> > - goto out_super;
> > - }
> > + if (!arena)
> > + return -ENOMEM;
> >
> > arena->infooff = cur_off;
> > ret = btt_info_read(arena, super);
> > @@ -919,14 +918,11 @@ static int discover_arenas(struct btt *btt)
> > btt->nlba = cur_nlba;
> > btt->init_state = INIT_READY;
> >
> > - kfree(super);
> > return ret;
> >
> > out:
> > kfree(arena);
> > free_arenas(btt);
> > - out_super:
> > - kfree(super);
> > return ret;
> > }
> >
>
> I would do the allocation like something below for the first chunk. Otherwise
> the rest LGTM.
>
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index d5593b0dc700..143921e7f26c 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
> #include "btt.h"
> #include "nd.h"
>
> @@ -845,25 +846,23 @@ static void parse_arena_meta(struct arena_info *arena,
> struct btt_sb *super,
>
> static int discover_arenas(struct btt *btt)
> {
> + struct btt_sb *super __free(kfree) =
> + kzalloc(sizeof(*super), GFP_KERNEL);
> int ret = 0;
> struct arena_info *arena;
> - struct btt_sb *super;
> size_t remaining = btt->rawsize;
> u64 cur_nlba = 0;
> size_t cur_off = 0;
> int num_arenas = 0;
>
> - super = kzalloc(sizeof(*super), GFP_KERNEL);
> if (!super)
> return -ENOMEM;
>
> while (remaining) {
> /* Alloc memory for arena */
It's a little strange that we do not check super immediately after allocation.
How about this:
static int discover_arenas(struct btt *btt)
{
int ret = 0;
struct arena_info *arena;
- struct btt_sb *super;
size_t remaining = btt->rawsize;
u64 cur_nlba = 0;
size_t cur_off = 0;
int num_arenas = 0;
- super = kzalloc(sizeof(*super), GFP_KERNEL);
+ struct btt_sb *super __free(kfree) =
+ kzalloc(sizeof(*super), GFP_KERNEL);
if (!super)
return -ENOMEM;
while (remaining) {
[PATCH] nvdimm-btt: simplify code with the scope based resource management
Use the scope based resource management (defined in
linux/cleanup.h) to automate resource lifetime
control on struct btt_sb *super in discover_arenas().
Signed-off-by: Dinghao Liu
---
drivers/nvdimm/btt.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index d5593b0dc700..ff42778b51de 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include "btt.h"
#include "nd.h"
@@ -847,7 +848,7 @@ static int discover_arenas(struct btt *btt)
{
int ret = 0;
struct arena_info *arena;
- struct btt_sb *super;
+ struct btt_sb *super __free(kfree) = NULL;
size_t remaining = btt->rawsize;
u64 cur_nlba = 0;
size_t cur_off = 0;
@@ -860,10 +861,8 @@ static int discover_arenas(struct btt *btt)
while (remaining) {
/* Alloc memory for arena */
arena = alloc_arena(btt, 0, 0, 0);
- if (!arena) {
- ret = -ENOMEM;
- goto out_super;
- }
+ if (!arena)
+ return -ENOMEM;
arena->infooff = cur_off;
ret = btt_info_read(arena, super);
@@ -919,14 +918,11 @@ static int discover_arenas(struct btt *btt)
btt->nlba = cur_nlba;
btt->init_state = INIT_READY;
- kfree(super);
return ret;
out:
kfree(arena);
free_arenas(btt);
- out_super:
- kfree(super);
return ret;
}
--
2.17.1
[PATCH] [v2] nvdimm-btt: fix several memleaks
Resources allocated by kcalloc() in btt_freelist_init(),
btt_rtt_init(), and btt_maplocks_init() are not correctly
released in their callers when an error happens. For
example, when an error happens in btt_freelist_init(), its
caller discover_arenas() will directly free arena, which makes
arena->freelist a leaked memory. Fix these memleaks by using
devm_kcalloc() to make the memory auto-freed on driver detach.
Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
Signed-off-by: Dinghao Liu
---
Changelog:
v2: -Use devm_kcalloc() to fix the memleaks.
-Fix the potential leaked memory in btt_rtt_init()
and btt_maplocks_init().
---
drivers/nvdimm/btt.c | 35 ---
1 file changed, 16 insertions(+), 19 deletions(-)
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index d5593b0dc700..c55231f42617 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -531,13 +531,13 @@ static int arena_clear_freelist_error(struct arena_info
*arena, u32 lane)
return ret;
}
-static int btt_freelist_init(struct arena_info *arena)
+static int btt_freelist_init(struct device *dev, struct arena_info *arena)
{
int new, ret;
struct log_entry log_new;
u32 i, map_entry, log_oldmap, log_newmap;
- arena->freelist = kcalloc(arena->nfree, sizeof(struct free_entry),
+ arena->freelist = devm_kcalloc(dev, arena->nfree, sizeof(struct
free_entry),
GFP_KERNEL);
if (!arena->freelist)
return -ENOMEM;
@@ -718,20 +718,20 @@ static int log_set_indices(struct arena_info *arena)
return 0;
}
-static int btt_rtt_init(struct arena_info *arena)
+static int btt_rtt_init(struct device *dev, struct arena_info *arena)
{
- arena->rtt = kcalloc(arena->nfree, sizeof(u32), GFP_KERNEL);
+ arena->rtt = devm_kcalloc(dev, arena->nfree, sizeof(u32), GFP_KERNEL);
if (arena->rtt == NULL)
return -ENOMEM;
return 0;
}
-static int btt_maplocks_init(struct arena_info *arena)
+static int btt_maplocks_init(struct device *dev, struct arena_info *arena)
{
u32 i;
- arena->map_locks = kcalloc(arena->nfree, sizeof(struct aligned_lock),
+ arena->map_locks = devm_kcalloc(dev, arena->nfree, sizeof(struct
aligned_lock),
GFP_KERNEL);
if (!arena->map_locks)
return -ENOMEM;
@@ -805,9 +805,6 @@ static void free_arenas(struct btt *btt)
list_for_each_entry_safe(arena, next, >arena_list, list) {
list_del(>list);
- kfree(arena->rtt);
- kfree(arena->map_locks);
- kfree(arena->freelist);
debugfs_remove_recursive(arena->debugfs_dir);
kfree(arena);
}
@@ -843,7 +840,7 @@ static void parse_arena_meta(struct arena_info *arena,
struct btt_sb *super,
arena->flags = le32_to_cpu(super->flags);
}
-static int discover_arenas(struct btt *btt)
+static int discover_arenas(struct device *dev, struct btt *btt)
{
int ret = 0;
struct arena_info *arena;
@@ -893,15 +890,15 @@ static int discover_arenas(struct btt *btt)
goto out;
}
- ret = btt_freelist_init(arena);
+ ret = btt_freelist_init(dev, arena);
if (ret)
goto out;
- ret = btt_rtt_init(arena);
+ ret = btt_rtt_init(dev, arena);
if (ret)
goto out;
- ret = btt_maplocks_init(arena);
+ ret = btt_maplocks_init(dev, arena);
if (ret)
goto out;
@@ -1022,7 +1019,7 @@ static int btt_arena_write_layout(struct arena_info
*arena)
* This function completes the initialization for the BTT namespace
* such that it is ready to accept IOs
*/
-static int btt_meta_init(struct btt *btt)
+static int btt_meta_init(struct device *dev, struct btt *btt)
{
int ret = 0;
struct arena_info *arena;
@@ -1033,15 +1030,15 @@ static int btt_meta_init(struct btt *btt)
if (ret)
goto unlock;
- ret = btt_freelist_init(arena);
+ ret = btt_freelist_init(dev, arena);
if (ret)
goto unlock;
- ret = btt_rtt_init(arena);
+ ret = btt_rtt_init(dev, arena);
if (ret)
goto unlock;
- ret = btt_maplocks_init(arena);
+ ret = btt_maplocks_init(dev, arena);
if (ret)
goto unlock;
}
@@ -1584,7 +1581,7 @@ static struct btt *btt_init(struct nd_btt *nd_btt,
unsigned long long rawsize,
nsio = to_nd_namespace_io(_btt->ndns->dev);
btt->
Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init
> dinghao.liu@ wrote: > > > Dave Jiang wrote: > > [snip] > > > > That said, this patch does not completely fix freelist from leaking in the > > > following error path. > > > > > > discover_arenas() > > > btt_freelist_init() -> ok (memory allocated) > > > btt_rtt_init() -> fail > > > goto out; > > > (leak because arena is not yet on btt->arena_list) > > > OR > > > btt_maplocks_init() -> fail > > > goto out; > > > (leak because arena is not yet on btt->arena_list) > > > > > > > Thanks for pointing out this issue! I rechecked discover_arenas() and found > > that btt_rtt_init() may also trigger a memleak for the same reason as > > btt_freelist_init(). Also, I checked another call trace: > > > > btt_init() -> btt_meta_init() -> btt_maplocks_init() > > > > I think there is a memleak if btt_maplocks_init() succeeds but an error > > happens in btt_init() after btt_meta_init() (e.g., when btt_blk_init() > > returns an error). Therefore, we may need to fix three functions. > > Yea I think we need to trace this code better. This is why devm_ is nice for > memory allocated for the life of the device. > > > > > > This error could be fixed by adding to arena_list earlier but devm_*() > > > also takes care of this without having to worry about that logic. > > > > > > On normal operation all of this memory can be free'ed with the > > > corresponding devm_kfree() and/or devm_add_action_*() calls if arenas come > > > and go. I'm not sure off the top of my head. > > > > > > In addition, looking at this code. discover_arenas() could make use of > > > the scoped based management for struct btt_sb *super! > > > > > > Dinghao would you be willing to submit a series of 2 or 3 patches to fix > > > the above issues? > > > > > > > Sure. Currently I plan to send 2 patches as follows: > > 1. Using devm_kcalloc() to replace kcalloc() in btt_freelist_init(), > >btt_rtt_init(), and btt_maplocks_init(), and removing the corresponding > >kfree in free_arenas(). I checked some uses of devm_kcalloc() and it > >seems that we need not to call devm_kfree(). The memory is automatically > >freed on driver detach, right? > > On device put yes. So if these allocations are scoped to the life of the > device there would be no reason to call devm_kfree() on them at all. I was > not > sure if they got reallocated at some point or not. > > > 2. Using the scoped based management for struct btt_sb *super (not a bug, > >but it could improve the code). > > Good! > > > > > I'm not quite sure whether my understanding or bug fixing plan is correct. > > If there are any issues, please correct me, thanks! > > The above sounds right. > Ira Thanks for the review! I will send the patches soon. Regards, Dinghao
Re: [PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init
> Dave Jiang wrote:
> >
>
> [snip]
>
> First off thanks for the patch. This code seems to have a few things to
> clean up.
>
> >
> > On 12/6/23 20:43, Dinghao Liu wrote:
> > > When an error happens in btt_freelist_init(), its caller
> > > discover_arenas() will directly free arena, which makes
> > > arena->freelist allocated in btt_freelist_init() a leaked
> > > memory. Fix this by freeing arena->freelist in all error
> > > handling paths of btt_freelist_init().
> > >
> > > Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
> > > Signed-off-by: Dinghao Liu
> >
> > How about use the new scope based resource management and we can avoid the
> > goto mess altogether?
> > https://lwn.net/Articles/934679/
> >
>
> The freelist is returned as part of arena. I've not traced both paths of
> btt_freelist_init() completely but devm_kcalloc() looks like a better
> solution here because this memory needs to live past the function scope.
>
> That said, this patch does not completely fix freelist from leaking in the
> following error path.
>
> discover_arenas()
> btt_freelist_init() -> ok (memory allocated)
> btt_rtt_init() -> fail
> goto out;
> (leak because arena is not yet on btt->arena_list)
> OR
> btt_maplocks_init() -> fail
> goto out;
> (leak because arena is not yet on btt->arena_list)
>
Thanks for pointing out this issue! I rechecked discover_arenas() and found
that btt_rtt_init() may also trigger a memleak for the same reason as
btt_freelist_init(). Also, I checked another call trace:
btt_init() -> btt_meta_init() -> btt_maplocks_init()
I think there is a memleak if btt_maplocks_init() succeeds but an error
happens in btt_init() after btt_meta_init() (e.g., when btt_blk_init()
returns an error). Therefore, we may need to fix three functions.
> This error could be fixed by adding to arena_list earlier but devm_*()
> also takes care of this without having to worry about that logic.
>
> On normal operation all of this memory can be free'ed with the
> corresponding devm_kfree() and/or devm_add_action_*() calls if arenas come
> and go. I'm not sure off the top of my head.
>
> In addition, looking at this code. discover_arenas() could make use of
> the scoped based management for struct btt_sb *super!
>
> Dinghao would you be willing to submit a series of 2 or 3 patches to fix
> the above issues?
>
Sure. Currently I plan to send 2 patches as follows:
1. Using devm_kcalloc() to replace kcalloc() in btt_freelist_init(),
btt_rtt_init(), and btt_maplocks_init(), and removing the corresponding
kfree in free_arenas(). I checked some uses of devm_kcalloc() and it
seems that we need not to call devm_kfree(). The memory is automatically
freed on driver detach, right?
2. Using the scoped based management for struct btt_sb *super (not a bug,
but it could improve the code).
I'm not quite sure whether my understanding or bug fixing plan is correct.
If there are any issues, please correct me, thanks!
Regards,
Dinghao
[PATCH] nvdimm-btt: fix a potential memleak in btt_freelist_init
When an error happens in btt_freelist_init(), its caller
discover_arenas() will directly free arena, which makes
arena->freelist allocated in btt_freelist_init() a leaked
memory. Fix this by freeing arena->freelist in all error
handling paths of btt_freelist_init().
Fixes: 5212e11fde4d ("nd_btt: atomic sector updates")
Signed-off-by: Dinghao Liu
---
drivers/nvdimm/btt.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index d5593b0dc700..d8c4ba8bfdda 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -544,8 +544,10 @@ static int btt_freelist_init(struct arena_info *arena)
for (i = 0; i < arena->nfree; i++) {
new = btt_log_read(arena, i, _new, LOG_NEW_ENT);
- if (new < 0)
- return new;
+ if (new < 0) {
+ ret = new;
+ goto out_free;
+ }
/* old and new map entries with any flags stripped out */
log_oldmap = ent_lba(le32_to_cpu(log_new.old_map));
@@ -577,7 +579,7 @@ static int btt_freelist_init(struct arena_info *arena)
ret = btt_map_read(arena, le32_to_cpu(log_new.lba), _entry,
NULL, NULL, 0);
if (ret)
- return ret;
+ goto out_free;
/*
* The map_entry from btt_read_map is stripped of any flag bits,
@@ -594,11 +596,16 @@ static int btt_freelist_init(struct arena_info *arena)
ret = btt_map_write(arena, le32_to_cpu(log_new.lba),
le32_to_cpu(log_new.new_map), 0, 0, 0);
if (ret)
- return ret;
+ goto out_free;
}
}
return 0;
+
+out_free:
+ kfree(arena->freelist);
+ arena->freelist = NULL;
+ return ret;
}
static bool ent_is_padding(struct log_entry *ent)
--
2.17.1
[PATCH] [v4] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
If of_clk_add_provider() fails in ca8210_register_ext_clock(),
it calls clk_unregister() to release priv->clk and returns an
error. However, the caller ca8210_probe() then calls ca8210_remove(),
where priv->clk is freed again in ca8210_unregister_ext_clock(). In
this case, a use-after-free may happen in the second time we call
clk_unregister().
Fix this by removing the first clk_unregister(). Also, priv->clk could
be an error code on failure of clk_register_fixed_rate(). Use
IS_ERR_OR_NULL to catch this case in ca8210_unregister_ext_clock().
Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
Signed-off-by: Dinghao Liu
---
Changelog:
v2: -Remove the first clk_unregister() instead of nulling priv->clk.
v3: -Simplify ca8210_register_ext_clock().
-Add a ';' after return in ca8210_unregister_ext_clock().
v4: -Remove an unused variable 'ret'.
---
drivers/net/ieee802154/ca8210.c | 17 +++--
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index aebb19f1b3a4..4ec0dab38872 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -2740,7 +2740,6 @@ static int ca8210_register_ext_clock(struct spi_device
*spi)
struct device_node *np = spi->dev.of_node;
struct ca8210_priv *priv = spi_get_drvdata(spi);
struct ca8210_platform_data *pdata = spi->dev.platform_data;
- int ret = 0;
if (!np)
return -EFAULT;
@@ -2757,18 +2756,8 @@ static int ca8210_register_ext_clock(struct spi_device
*spi)
dev_crit(>dev, "Failed to register external clk\n");
return PTR_ERR(priv->clk);
}
- ret = of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
- if (ret) {
- clk_unregister(priv->clk);
- dev_crit(
- >dev,
- "Failed to register external clock as clock provider\n"
- );
- } else {
- dev_info(>dev, "External clock set as clock provider\n");
- }
- return ret;
+ return of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
}
/**
@@ -2780,8 +2769,8 @@ static void ca8210_unregister_ext_clock(struct spi_device
*spi)
{
struct ca8210_priv *priv = spi_get_drvdata(spi);
- if (!priv->clk)
- return
+ if (IS_ERR_OR_NULL(priv->clk))
+ return;
of_clk_del_provider(spi->dev.of_node);
clk_unregister(priv->clk);
--
2.17.1
Re: [PATCH] [v3] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
> Hello Dinghao, > > > On 01.10.23 09:19, kernel test robot wrote: > > Hi Dinghao, > > > > kernel test robot noticed the following build warnings: > > > > [auto build test WARNING on linus/master] > > [also build test WARNING on v6.6-rc3 next-20230929] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > url: > > https://github.com/intel-lab-lkp/linux/commits/Dinghao-Liu/ieee802154-ca8210-Fix-a-potential-UAF-in-ca8210_probe/20231001-135130 > > base: linus/master > > patch link: > > https://lore.kernel.org/r/20231001054949.14624-1-dinghao.liu%40zju.edu.cn > > patch subject: [PATCH] [v3] ieee802154: ca8210: Fix a potential UAF in > > ca8210_probe > > config: m68k-allyesconfig > > (https://download.01.org/0day-ci/archive/20231001/202310011548.qyqmuodi-...@intel.com/config) > > compiler: m68k-linux-gcc (GCC) 13.2.0 > > reproduce (this is a W=1 build): > > (https://download.01.org/0day-ci/archive/20231001/202310011548.qyqmuodi-...@intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new > > version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot > > | Closes: > > https://lore.kernel.org/oe-kbuild-all/202310011548.qyqmuodi-...@intel.com/ > > > > All warnings (new ones prefixed by >>): > > > > drivers/net/ieee802154/ca8210.c: In function > > 'ca8210_register_ext_clock': > >>> drivers/net/ieee802154/ca8210.c:2743:13: warning: unused variable 'ret' > >>> [-Wunused-variable] > > 2743 | int ret = 0; > > | ^~~ > > > > Please take care of this now unused variable after your re-factor. > With this fixed and send out as v4 I am happy to get this applied to the > wpan tree. I will resend the v4 patch soon, thanks! Regards, Dinghao
[PATCH] [v3] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
If of_clk_add_provider() fails in ca8210_register_ext_clock(),
it calls clk_unregister() to release priv->clk and returns an
error. However, the caller ca8210_probe() then calls ca8210_remove(),
where priv->clk is freed again in ca8210_unregister_ext_clock(). In
this case, a use-after-free may happen in the second time we call
clk_unregister().
Fix this by removing the first clk_unregister(). Also, priv->clk could
be an error code on failure of clk_register_fixed_rate(). Use
IS_ERR_OR_NULL to catch this case in ca8210_unregister_ext_clock().
Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
Signed-off-by: Dinghao Liu
---
Changelog:
v2: -Remove the first clk_unregister() instead of nulling priv->clk.
v3: -Simplify ca8210_register_ext_clock().
-Add a ';' after return in ca8210_unregister_ext_clock().
---
drivers/net/ieee802154/ca8210.c | 16 +++-
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index aebb19f1b3a4..ae44a9133937 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -2757,18 +2757,8 @@ static int ca8210_register_ext_clock(struct spi_device
*spi)
dev_crit(>dev, "Failed to register external clk\n");
return PTR_ERR(priv->clk);
}
- ret = of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
- if (ret) {
- clk_unregister(priv->clk);
- dev_crit(
- >dev,
- "Failed to register external clock as clock provider\n"
- );
- } else {
- dev_info(>dev, "External clock set as clock provider\n");
- }
- return ret;
+ return of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
}
/**
@@ -2780,8 +2770,8 @@ static void ca8210_unregister_ext_clock(struct spi_device
*spi)
{
struct ca8210_priv *priv = spi_get_drvdata(spi);
- if (!priv->clk)
- return
+ if (IS_ERR_OR_NULL(priv->clk))
+ return;
of_clk_del_provider(spi->dev.of_node);
clk_unregister(priv->clk);
--
2.17.1
Re: [PATCH] [v2] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
> Missing Cc stable, this needs to be backported.
I will cc stable (sta...@vger.kernel.org) for the next version, thanks!
> > diff --git a/drivers/net/ieee802154/ca8210.c
> > b/drivers/net/ieee802154/ca8210.c
> > index aebb19f1b3a4..b35c6f59bd1a 100644
> > --- a/drivers/net/ieee802154/ca8210.c
> > +++ b/drivers/net/ieee802154/ca8210.c
> > @@ -2759,7 +2759,6 @@ static int ca8210_register_ext_clock(struct
> > spi_device *spi)
> > }
> > ret = of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
> > if (ret) {
> > - clk_unregister(priv->clk);
> > dev_crit(
> > >dev,
> > "Failed to register external clock as clock provider\n"
>
> I was hoping you would simplify this function a bit more.
I understand. In the next patch version, I will just return
of_clk_add_provider().
>
> > @@ -2780,7 +2779,7 @@ static void ca8210_unregister_ext_clock(struct
> > spi_device *spi)
> > {
> > struct ca8210_priv *priv = spi_get_drvdata(spi);
> >
> > - if (!priv->clk)
> > + if (IS_ERR_OR_NULL(priv->clk))
> > return
> >
> > of_clk_del_provider(spi->dev.of_node);
>
> Alex, Stefan, who handles wpan and wpan/next this release?
>
Is there any problem I need to handle in the next patch?
Regards,
Dinghao
[PATCH] [v2] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
If of_clk_add_provider() fails in ca8210_register_ext_clock(),
it calls clk_unregister() to release priv->clk and returns an
error. However, the caller ca8210_probe() then calls ca8210_remove(),
where priv->clk is freed again in ca8210_unregister_ext_clock(). In
this case, a use-after-free may happen in the second time we call
clk_unregister().
Fix this by removing the first clk_unregister(). Also, priv->clk could
be an error code on failure of clk_register_fixed_rate(). Use
IS_ERR_OR_NULL to catch this case in ca8210_unregister_ext_clock().
Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
Signed-off-by: Dinghao Liu
---
Changelog:
v2: -Remove the first clk_unregister() instead of nulling priv->clk.
---
drivers/net/ieee802154/ca8210.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index aebb19f1b3a4..b35c6f59bd1a 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -2759,7 +2759,6 @@ static int ca8210_register_ext_clock(struct spi_device
*spi)
}
ret = of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
if (ret) {
- clk_unregister(priv->clk);
dev_crit(
>dev,
"Failed to register external clock as clock provider\n"
@@ -2780,7 +2779,7 @@ static void ca8210_unregister_ext_clock(struct spi_device
*spi)
{
struct ca8210_priv *priv = spi_get_drvdata(spi);
- if (!priv->clk)
+ if (IS_ERR_OR_NULL(priv->clk))
return
of_clk_del_provider(spi->dev.of_node);
--
2.17.1
Re: [PATCH] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
Hi Miquèl,
> > index aebb19f1b3a4..1d545879c000 100644
> > --- a/drivers/net/ieee802154/ca8210.c
> > +++ b/drivers/net/ieee802154/ca8210.c
> > @@ -2760,6 +2760,7 @@ static int ca8210_register_ext_clock(struct
> > spi_device *spi)
> > ret = of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
> > if (ret) {
> > clk_unregister(priv->clk);
> > + priv->clk = NULL;
>
> This function is a bit convoluted. You could just return the result of
> of_clk_add_provider() (keep the printk's if you want, they don't seem
> very useful) and let ca8210_unregister_ext_clock() do the cleanup.
Thanks for your advice! I will resend a new patch as suggested.
>
> > dev_crit(
> > >dev,
> > "Failed to register external clock as clock provider\n"
> > @@ -2780,7 +2781,7 @@ static void ca8210_unregister_ext_clock(struct
> > spi_device *spi)
> > {
> > struct ca8210_priv *priv = spi_get_drvdata(spi);
> >
> > - if (!priv->clk)
> > + if (IS_ERR_OR_NULL(priv->clk))
>
> Does not look useful as you are enforcing priv->clock to be valid or
> NULL, it cannot be an error code.
I find that ca8210_register_ext_clock() uses IS_ERR to check priv->clk
after calling clk_register_fixed_rate(). So I think priv->clk could be
a non-null pointer even on failure. And a null pointer check may miss
this case in ca8210_unregister_ext_clock().
Regards,
Dinghao
[PATCH] ieee802154: ca8210: Fix a potential UAF in ca8210_probe
If of_clk_add_provider() fails in ca8210_register_ext_clock(),
it calls clk_unregister() to release priv->clk and returns an
error. However, the caller ca8210_probe() then calls ca8210_remove(),
where priv->clk is freed again in ca8210_unregister_ext_clock(). In
this case, a use-after-free may happen in the second time we call
clk_unregister().
Fix this by nulling priv->clk after the first clk_unregister(). Also
refine the pointer checking in ca8210_unregister_ext_clock().
Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
Signed-off-by: Dinghao Liu
---
drivers/net/ieee802154/ca8210.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index aebb19f1b3a4..1d545879c000 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -2760,6 +2760,7 @@ static int ca8210_register_ext_clock(struct spi_device
*spi)
ret = of_clk_add_provider(np, of_clk_src_simple_get, priv->clk);
if (ret) {
clk_unregister(priv->clk);
+ priv->clk = NULL;
dev_crit(
>dev,
"Failed to register external clock as clock provider\n"
@@ -2780,7 +2781,7 @@ static void ca8210_unregister_ext_clock(struct spi_device
*spi)
{
struct ca8210_priv *priv = spi_get_drvdata(spi);
- if (!priv->clk)
+ if (IS_ERR_OR_NULL(priv->clk))
return
of_clk_del_provider(spi->dev.of_node);
--
2.17.1
[PATCH] [v4] spi: spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe
There is a PM usage counter decrement after zynqmp_qspi_init_hw()
without any refcount increment, which leads to refcount leak.Add
a refcount increment to balance the refcount. Also set
auto_runtime_pm to resume suspended spi controller.
Fixes: 9e3a000362aec ("spi: zynqmp: Add pm runtime support")
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Add a refcount increment to fix refcout leak instead of the
refcount decrement on error.
Set ctlr->auto_runtime_pm = true.
v3: - Add fix tag.
Add a return value check against pm_runtime_get_sync().
Move pm_runtime_{mark_last_busy & put_autosuspend} to the
end of current function.
v4: - Add error message on failure of pm_runtime_get_sync().
---
drivers/spi/spi-zynqmp-gqspi.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index c8fa6ee18ae7..38f3ddd3ea7c 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -1160,11 +1160,16 @@ static int zynqmp_qspi_probe(struct platform_device
*pdev)
pm_runtime_set_autosuspend_delay(>dev, SPI_AUTOSUSPEND_TIMEOUT);
pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
+
+ ret = pm_runtime_get_sync(>dev);
+ if (ret < 0) {
+ dev_err(>dev, "Failed to pm_runtime_get_sync: %d\n", ret);
+ goto clk_dis_all;
+ }
+
/* QSPI controller initializations */
zynqmp_qspi_init_hw(xqspi);
- pm_runtime_mark_last_busy(>dev);
- pm_runtime_put_autosuspend(>dev);
xqspi->irq = platform_get_irq(pdev, 0);
if (xqspi->irq <= 0) {
ret = -ENXIO;
@@ -1187,6 +1192,7 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
SPI_TX_DUAL | SPI_TX_QUAD;
ctlr->dev.of_node = np;
+ ctlr->auto_runtime_pm = true;
ret = devm_spi_register_controller(>dev, ctlr);
if (ret) {
@@ -1194,9 +1200,13 @@ static int zynqmp_qspi_probe(struct platform_device
*pdev)
goto clk_dis_all;
}
+ pm_runtime_mark_last_busy(>dev);
+ pm_runtime_put_autosuspend(>dev);
+
return 0;
clk_dis_all:
+ pm_runtime_put_sync(>dev);
pm_runtime_set_suspended(>dev);
pm_runtime_disable(>dev);
clk_disable_unprepare(xqspi->refclk);
--
2.17.1
[PATCH] [v3] clk: renesas: rcar-usb2-clock-sel: Fix error handling in rcar_usb2_clock_sel_probe
The error handling paths after pm_runtime_get_sync() has no
refcount decrement, which leads to refcount leak.
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Move the position of pm_runtime_enable,_get_sync().
Use devm_clk_register() to simplify error handling.
v2: - Use devm_clk_hw_register() instead of devm_clk_register().
---
drivers/clk/renesas/rcar-usb2-clock-sel.c | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/renesas/rcar-usb2-clock-sel.c
b/drivers/clk/renesas/rcar-usb2-clock-sel.c
index 3abafd78f7c8..a6f82a5a6335 100644
--- a/drivers/clk/renesas/rcar-usb2-clock-sel.c
+++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
@@ -131,7 +131,6 @@ static int rcar_usb2_clock_sel_remove(struct
platform_device *pdev)
struct usb2_clock_sel_priv *priv = platform_get_drvdata(pdev);
of_clk_del_provider(dev->of_node);
- clk_hw_unregister(>hw);
pm_runtime_put(dev);
pm_runtime_disable(dev);
@@ -164,9 +163,6 @@ static int rcar_usb2_clock_sel_probe(struct platform_device
*pdev)
if (IS_ERR(priv->rsts))
return PTR_ERR(priv->rsts);
- pm_runtime_enable(dev);
- pm_runtime_get_sync(dev);
-
clk = devm_clk_get(dev, "usb_extal");
if (!IS_ERR(clk) && !clk_prepare_enable(clk)) {
priv->extal = !!clk_get_rate(clk);
@@ -183,6 +179,8 @@ static int rcar_usb2_clock_sel_probe(struct platform_device
*pdev)
return -ENOENT;
}
+ pm_runtime_enable(dev);
+ pm_runtime_get_sync(dev);
platform_set_drvdata(pdev, priv);
dev_set_drvdata(dev, priv);
@@ -193,11 +191,20 @@ static int rcar_usb2_clock_sel_probe(struct
platform_device *pdev)
init.num_parents = 0;
priv->hw.init =
- clk = clk_register(NULL, >hw);
- if (IS_ERR(clk))
- return PTR_ERR(clk);
+ ret = devm_clk_hw_register(NULL, >hw);
+ if (ret)
+ goto pm_put;
+
+ ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, >hw);
+ if (ret)
+ goto pm_put;
+
+ return 0;
- return of_clk_add_hw_provider(np, of_clk_hw_simple_get, >hw);
+pm_put:
+ pm_runtime_put(dev);
+ pm_runtime_disable(dev);
+ return ret;
}
static const struct dev_pm_ops rcar_usb2_clock_sel_pm_ops = {
--
2.17.1
[PATCH] [v3] spi: spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe
There is a PM usage counter decrement after zynqmp_qspi_init_hw()
without any refcount increment, which leads to refcount leak.Add
a refcount increment to balance the refcount. Also set
auto_runtime_pm to resume suspended spi controller.
Fixes: 9e3a000362aec ("spi: zynqmp: Add pm runtime support")
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Add a refcount increment to fix refcout leak instead of the
refcount decrement on error.
Set ctlr->auto_runtime_pm = true.
v3: - Add fix tag.
Add a return value check against pm_runtime_get_sync().
Move pm_runtime_{mark_last_busy & put_autosuspend} to the
end of current function.
---
drivers/spi/spi-zynqmp-gqspi.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index c8fa6ee18ae7..781ef3fc76e2 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -1160,11 +1160,14 @@ static int zynqmp_qspi_probe(struct platform_device
*pdev)
pm_runtime_set_autosuspend_delay(>dev, SPI_AUTOSUSPEND_TIMEOUT);
pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
+
+ ret = pm_runtime_get_sync(>dev);
+ if (ret < 0)
+ goto clk_dis_all;
+
/* QSPI controller initializations */
zynqmp_qspi_init_hw(xqspi);
- pm_runtime_mark_last_busy(>dev);
- pm_runtime_put_autosuspend(>dev);
xqspi->irq = platform_get_irq(pdev, 0);
if (xqspi->irq <= 0) {
ret = -ENXIO;
@@ -1187,6 +1190,7 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
SPI_TX_DUAL | SPI_TX_QUAD;
ctlr->dev.of_node = np;
+ ctlr->auto_runtime_pm = true;
ret = devm_spi_register_controller(>dev, ctlr);
if (ret) {
@@ -1194,9 +1198,13 @@ static int zynqmp_qspi_probe(struct platform_device
*pdev)
goto clk_dis_all;
}
+ pm_runtime_mark_last_busy(>dev);
+ pm_runtime_put_autosuspend(>dev);
+
return 0;
clk_dis_all:
+ pm_runtime_put_sync(>dev);
pm_runtime_set_suspended(>dev);
pm_runtime_disable(>dev);
clk_disable_unprepare(xqspi->refclk);
--
2.17.1
Re: Re: [PATCH] [v2] spi: spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe
> Hi Dinghao,
> On Mon, Apr 12, 2021 at 03:31:54PM +0800, Dinghao Liu wrote:
> > There is a PM usage counter decrement after zynqmp_qspi_init_hw()
> > without any refcount increment, which leads to refcount leak.Add
> > a refcount increment to balance the refcount. Also set
> > auto_runtime_pm to resume suspended spi controller.
> >
> > Signed-off-by: Dinghao Liu
> > ---
> > changelog:
> >
> > v2: - Add a refcount increment to fix refcout leak instead of the
> > refcount decrement on error.
> > Set ctlr->auto_runtime_pm = true.
> > ---
> > drivers/spi/spi-zynqmp-gqspi.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
> > index c8fa6ee18ae7..8b21c7b0e7eb 100644
> > --- a/drivers/spi/spi-zynqmp-gqspi.c
> > +++ b/drivers/spi/spi-zynqmp-gqspi.c
> > @@ -1160,6 +1160,7 @@ static int zynqmp_qspi_probe(struct platform_device
> > *pdev)
> > pm_runtime_set_autosuspend_delay(>dev, SPI_AUTOSUSPEND_TIMEOUT);
> > pm_runtime_set_active(>dev);
> > pm_runtime_enable(>dev);
> > + pm_runtime_get_sync(>dev);
> Please check the return value here, if ret is "< 0", goto error label,
> and a pm_runtime_put_sync is needed in error label
> > /* QSPI controller initializations */
> > zynqmp_qspi_init_hw(xqspi);
> >
> > @@ -1187,6 +1188,7 @@ static int zynqmp_qspi_probe(struct platform_device
> > *pdev)
> > ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
> > SPI_TX_DUAL | SPI_TX_QUAD;
> > ctlr->dev.of_node = np;
> > + ctlr->auto_runtime_pm = true;
> >
> > ret = devm_spi_register_controller(>dev, ctlr);
> > if (ret) {
> These 2 function
> pm_runtime_mark_last_busy(>dev);
> pm_runtime_put_autosuspend(>dev);
> are the last operations in probe function since if they runs,
> spi_controller will enter suspend state and disable clks after 3s
> passing. So please move them just before "return 0".
>
> And would you please cc me when you send V3? I am preparing to send a patch
> series
> to fix clk and suspend/resume issues which bases on the pm_runtime issue.
>
Thanks for your advice and I will send a new patch soon.
Regards,
Dinghao
[PATCH] clk: renesas: rcar-usb2-clock-sel: Fix error handling in rcar_usb2_clock_sel_probe
When clk_get_rate() fails, a pairing PM usage counter decrement
and disable is required to prevent refcount leak. It's the same
for the subsequent error paths. When of_clk_add_hw_provider()
fails, we need to unregister clk_hw.
Signed-off-by: Dinghao Liu
---
drivers/clk/renesas/rcar-usb2-clock-sel.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/renesas/rcar-usb2-clock-sel.c
b/drivers/clk/renesas/rcar-usb2-clock-sel.c
index 3abafd78f7c8..ad7bd50b9d1b 100644
--- a/drivers/clk/renesas/rcar-usb2-clock-sel.c
+++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
@@ -180,7 +180,8 @@ static int rcar_usb2_clock_sel_probe(struct platform_device
*pdev)
if (!priv->extal && !priv->xtal) {
dev_err(dev, "This driver needs usb_extal or usb_xtal\n");
- return -ENOENT;
+ ret = -ENOENT;
+ goto pm_put;
}
platform_set_drvdata(pdev, priv);
@@ -194,10 +195,23 @@ static int rcar_usb2_clock_sel_probe(struct
platform_device *pdev)
priv->hw.init =
clk = clk_register(NULL, >hw);
- if (IS_ERR(clk))
- return PTR_ERR(clk);
+ if (IS_ERR(clk)) {
+ ret = PTR_ERR(clk);
+ goto pm_put;
+ }
+
+ ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, >hw);
+ if (ret)
+ goto clk_unregister;
+
+ return 0;
- return of_clk_add_hw_provider(np, of_clk_hw_simple_get, >hw);
+clk_unregister:
+ clk_hw_unregister(>hw);
+pm_put:
+ pm_runtime_put(dev);
+ pm_runtime_disable(dev);
+ return ret;
}
static const struct dev_pm_ops rcar_usb2_clock_sel_pm_ops = {
--
2.17.1
[PATCH] [v2] spi: spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe
There is a PM usage counter decrement after zynqmp_qspi_init_hw()
without any refcount increment, which leads to refcount leak.Add
a refcount increment to balance the refcount. Also set
auto_runtime_pm to resume suspended spi controller.
Signed-off-by: Dinghao Liu
---
changelog:
v2: - Add a refcount increment to fix refcout leak instead of the
refcount decrement on error.
Set ctlr->auto_runtime_pm = true.
---
drivers/spi/spi-zynqmp-gqspi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index c8fa6ee18ae7..8b21c7b0e7eb 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -1160,6 +1160,7 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
pm_runtime_set_autosuspend_delay(>dev, SPI_AUTOSUSPEND_TIMEOUT);
pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
+ pm_runtime_get_sync(>dev);
/* QSPI controller initializations */
zynqmp_qspi_init_hw(xqspi);
@@ -1187,6 +1188,7 @@ static int zynqmp_qspi_probe(struct platform_device *pdev)
ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
SPI_TX_DUAL | SPI_TX_QUAD;
ctlr->dev.of_node = np;
+ ctlr->auto_runtime_pm = true;
ret = devm_spi_register_controller(>dev, ctlr);
if (ret) {
--
2.17.1
[PATCH] [v2] usb: cdns3: Fix runtime PM imbalance on error
When cdns3_gadget_start() fails, a pairing PM usage counter
decrement is needed to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Use pm_runtime_put_sync() to decrease refcount.
---
drivers/usb/cdns3/cdns3-gadget.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index 582bfeceedb4..a49fc68ec2ef 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -3255,8 +3255,10 @@ static int __cdns3_gadget_init(struct cdns *cdns)
pm_runtime_get_sync(cdns->dev);
ret = cdns3_gadget_start(cdns);
- if (ret)
+ if (ret) {
+ pm_runtime_put_sync(cdns->dev);
return ret;
+ }
/*
* Because interrupt line can be shared with other components in
--
2.17.1
Re: Re: [PATCH] usb: cdns3: Fix rumtime PM imbalance on error
> On 21-04-07 13:22:26, Dinghao Liu wrote:
> > When cdns3_gadget_start() fails, a pairing PM usage counter
> > decrement is needed to keep the counter balanced.
> >
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/usb/cdns3/cdns3-gadget.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/cdns3/cdns3-gadget.c
> > b/drivers/usb/cdns3/cdns3-gadget.c
> > index 582bfeceedb4..ad891a108aed 100644
> > --- a/drivers/usb/cdns3/cdns3-gadget.c
> > +++ b/drivers/usb/cdns3/cdns3-gadget.c
> > @@ -3255,8 +3255,11 @@ static int __cdns3_gadget_init(struct cdns *cdns)
> > pm_runtime_get_sync(cdns->dev);
> >
> > ret = cdns3_gadget_start(cdns);
> > - if (ret)
> > + if (ret) {
> > + pm_runtime_mark_last_busy(cdns->dev);
> > + pm_runtime_put_autosuspend(cdns->dev);
> > return ret;
>
> It doesn't need to delay entering runtime suspend, I prefer using
> pm_runtime_put_sync directly.
>
Sounds reasonable, thanks! I will send a new patch soon.
Regards,
Dinghao
[PATCH] [v2] iio: proximity: pulsedlight: Fix rumtime PM imbalance on error
When lidar_write_control() fails, a pairing PM usage counter
decrement is needed to keep the counter balanced.
Fixes: 4ac4e086fd8c5 ("iio: pulsedlight-lidar-lite: add runtime PM")
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Add the fix tag.
---
drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
index c685f10b5ae4..cc206bfa09c7 100644
--- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
+++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
@@ -160,6 +160,7 @@ static int lidar_get_measurement(struct lidar_data *data,
u16 *reg)
ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
if (ret < 0) {
dev_err(>dev, "cannot send start measurement command");
+ pm_runtime_put_noidle(>dev);
return ret;
}
--
2.17.1
[PATCH] [v2] dmaengine: tegra20: Fix runtime PM imbalance on error
pm_runtime_get_sync() will increase the runtime PM counter
even it returns an error. Thus a pairing decrement is needed
to prevent refcount leak. Fix this by replacing this API with
pm_runtime_resume_and_get(), which will not change the runtime
PM counter on error.
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Fix another similar case in tegra_dma_synchronize().
---
drivers/dma/tegra20-apb-dma.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 71827d9b0aa1..b7260749e8ee 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -723,7 +723,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
goto end;
}
if (!tdc->busy) {
- err = pm_runtime_get_sync(tdc->tdma->dev);
+ err = pm_runtime_resume_and_get(tdc->tdma->dev);
if (err < 0) {
dev_err(tdc2dev(tdc), "Failed to enable DMA\n");
goto end;
@@ -818,7 +818,7 @@ static void tegra_dma_synchronize(struct dma_chan *dc)
struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
int err;
- err = pm_runtime_get_sync(tdc->tdma->dev);
+ err = pm_runtime_resume_and_get(tdc->tdma->dev);
if (err < 0) {
dev_err(tdc2dev(tdc), "Failed to synchronize DMA: %d\n", err);
return;
--
2.17.1
Re: Re: [PATCH] dmaengine: tegra20: Fix runtime PM imbalance in tegra_dma_issue_pending
> On 08/04/2021 08:11, Dinghao Liu wrote:
> > pm_runtime_get_sync() will increase the rumtime PM counter
> > even it returns an error. Thus a pairing decrement is needed
> > to prevent refcount leak. Fix this by replacing this API with
> > pm_runtime_resume_and_get(), which will not change the runtime
> > PM counter on error.
> >
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/dma/tegra20-apb-dma.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> > index 71827d9b0aa1..73178afaf4c2 100644
> > --- a/drivers/dma/tegra20-apb-dma.c
> > +++ b/drivers/dma/tegra20-apb-dma.c
> > @@ -723,7 +723,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
> > goto end;
> > }
> > if (!tdc->busy) {
> > - err = pm_runtime_get_sync(tdc->tdma->dev);
> > + err = pm_runtime_resume_and_get(tdc->tdma->dev);
> > if (err < 0) {
> > dev_err(tdc2dev(tdc), "Failed to enable DMA\n");
> > goto end;
> >
>
>
> Thanks! Looks like there are two instances of this that need fixing.
>
Thanks for pointing out this! I will fix this and send a new patch soon.
Regards,
Dinghao
[PATCH] [v2] media: imx: imx7-mipi-csis: Fix runtime PM imbalance in mipi_csis_s_stream
When v4l2_subdev_call() fails, a pairing PM usage counter decrement is needed to keep the counter balanced. It's the same for the following error paths in case 'enable' is on. Signed-off-by: Dinghao Liu --- Changelog: v2: - Use pm_runtime_put() to balance the refcount. --- drivers/staging/media/imx/imx7-mipi-csis.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c index a01a7364b4b9..6f3e8a15e7c4 100644 --- a/drivers/staging/media/imx/imx7-mipi-csis.c +++ b/drivers/staging/media/imx/imx7-mipi-csis.c @@ -628,7 +628,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable) } ret = v4l2_subdev_call(state->src_sd, core, s_power, 1); if (ret < 0) - return ret; + goto pm_put; } mutex_lock(>lock); @@ -657,7 +657,8 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable) unlock: mutex_unlock(>lock); - if (!enable) +pm_put: + if (!enable || (ret < 0)) pm_runtime_put(>pdev->dev); return ret; -- 2.17.1
Re: Re: [PATCH] media: imx: imx7-mipi-csis: Fix runtime PM imbalance in mipi_csis_s_stream
> Hi Liu,
> Thanks for your patch.
>
> On Thu, Apr 08, 2021 at 05:08:27PM +0800, Dinghao Liu wrote:
> > When v4l2_subdev_call() fails, a pairing PM usage counter
> > decrement is needed to keep the counter balanced. It's the
> > same for the following error paths in case 'enable' is on.
> >
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/staging/media/imx/imx7-mipi-csis.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c
> > b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index a01a7364b4b9..2a3fff231a40 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -627,21 +627,26 @@ static int mipi_csis_s_stream(struct v4l2_subdev
> > *mipi_sd, int enable)
> > return ret;
> > }
> > ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + pm_runtime_put_noidle(>pdev->dev);
>
> I think here we should go completely pm_runtime_put to call the
> mipi_csis_pm_suspend down the line, right?
>
> > return ret;
> > + }
> > }
> >
> > mutex_lock(>lock);
> > if (enable) {
> > if (state->flags & ST_SUSPENDED) {
> > ret = -EBUSY;
> > + pm_runtime_put_noidle(>pdev->dev);
>
> since we are in ST_SUSPENDED state, for sure the pm counter was
> already 0.
>
> > goto unlock;
> > }
> >
> > mipi_csis_start_stream(state);
> > ret = v4l2_subdev_call(state->src_sd, video, s_stream, 1);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + pm_runtime_put_noidle(>pdev->dev);
>
> here also we need the pm_runtime_put, maybe just changing the unlock
> tag bellow from:
> if (!enable)
> pm_runtime_put(>pdev->dev);
>
> to
> if (!enable || (ret < 0))
> pm_runtime_put(>pdev->dev);
>
> will not hurt the first case and will complete the suspend routine
> afterward in the second case.
>
This is much clearer, thanks! I will fix this and send a new patch soon.
Regards,
Dinghao
Re: Re: [PATCH] spi: spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe
> Hi Dinghao, > > On 4/8/21 6:33 PM, Michal Simek wrote: > > ++ > > > > On 4/8/21 11:25 AM, Dinghao Liu wrote: > >> When platform_get_irq() fails, a pairing PM usage counter > >> increment is needed to keep the counter balanced. It's the > >> same for the following error paths. > >> > >> Signed-off-by: Dinghao Liu > >> --- > >> drivers/spi/spi-zynqmp-gqspi.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/spi/spi-zynqmp-gqspi.c > >> b/drivers/spi/spi-zynqmp-gqspi.c > >> index c8fa6ee18ae7..95963a2de64a 100644 > >> --- a/drivers/spi/spi-zynqmp-gqspi.c > >> +++ b/drivers/spi/spi-zynqmp-gqspi.c > >> @@ -1197,6 +1197,7 @@ static int zynqmp_qspi_probe(struct platform_device > >> *pdev) > >>return 0; > >> > >> clk_dis_all: > >> + pm_runtime_get_noresume(>dev); > >>pm_runtime_set_suspended(>dev); > >>pm_runtime_disable(>dev); > >>clk_disable_unprepare(xqspi->refclk); > >> > The imbalance is because pm_runtime_put_autosuspend is called to make > counter to be -1. > > It looks strange that there is no counter increament op before > pm_runtime_put_autosuspend. > > In my limited understanding, it should look like: > > .. > > pm_runtime_enable > > pm_runtime_get_sync //increase counter to one to resume device > > DO OPERATIONS HERE > > pm_runtime_mark_last_busy > pm_runtime_put_autosuspend //decrease counter to zero and trigger suspend > > return 0; > > error_path: > > pm_runtime_put_sync > > pm_runtime_disable > > return err; > > > Am I missing something? > Thanks for point out this! Usually there is an increment refcount in a _probe function and a decrement refcount in a _remove function. Sometimes the refcount decrement is in the _probe and the increment is in the _remove. But the refcount is balanced in both cases. So I think zynqmp_qspi_remove() needs a refcount increment to fix this bug. Regards, Dinghao
[PATCH] spi: spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe
When platform_get_irq() fails, a pairing PM usage counter increment is needed to keep the counter balanced. It's the same for the following error paths. Signed-off-by: Dinghao Liu --- drivers/spi/spi-zynqmp-gqspi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c index c8fa6ee18ae7..95963a2de64a 100644 --- a/drivers/spi/spi-zynqmp-gqspi.c +++ b/drivers/spi/spi-zynqmp-gqspi.c @@ -1197,6 +1197,7 @@ static int zynqmp_qspi_probe(struct platform_device *pdev) return 0; clk_dis_all: + pm_runtime_get_noresume(>dev); pm_runtime_set_suspended(>dev); pm_runtime_disable(>dev); clk_disable_unprepare(xqspi->refclk); -- 2.17.1
[PATCH] media: imx: imx7-mipi-csis: Fix runtime PM imbalance in mipi_csis_s_stream
When v4l2_subdev_call() fails, a pairing PM usage counter
decrement is needed to keep the counter balanced. It's the
same for the following error paths in case 'enable' is on.
Signed-off-by: Dinghao Liu
---
drivers/staging/media/imx/imx7-mipi-csis.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c
b/drivers/staging/media/imx/imx7-mipi-csis.c
index a01a7364b4b9..2a3fff231a40 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -627,21 +627,26 @@ static int mipi_csis_s_stream(struct v4l2_subdev
*mipi_sd, int enable)
return ret;
}
ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
- if (ret < 0)
+ if (ret < 0) {
+ pm_runtime_put_noidle(>pdev->dev);
return ret;
+ }
}
mutex_lock(>lock);
if (enable) {
if (state->flags & ST_SUSPENDED) {
ret = -EBUSY;
+ pm_runtime_put_noidle(>pdev->dev);
goto unlock;
}
mipi_csis_start_stream(state);
ret = v4l2_subdev_call(state->src_sd, video, s_stream, 1);
- if (ret < 0)
+ if (ret < 0) {
+ pm_runtime_put_noidle(>pdev->dev);
goto unlock;
+ }
mipi_csis_log_counters(state, true);
--
2.17.1
[PATCH] media: atomisp: Fix runtime PM imbalance in atomisp_pci_probe
When hmm_pool_register() fails, a pairing PM usage counter increment is needed to keep the counter balanced. It's the same for the following error paths. Signed-off-by: Dinghao Liu --- drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c index 0295e2e32d79..02f774ed80c8 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c @@ -1815,6 +1815,7 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i hmm_cleanup(); hmm_pool_unregister(HMM_POOL_TYPE_RESERVED); hmm_pool_fail: + pm_runtime_get_noresume(>dev); destroy_workqueue(isp->wdt_work_queue); wdt_work_queue_fail: atomisp_acc_cleanup(isp); -- 2.17.1
[PATCH] PCI: tegra: Fix runtime PM imbalance in pex_ep_event_pex_rst_deassert
pm_runtime_get_sync() will increase the runtime PM counter
even it returns an error. Thus a pairing decrement is needed
to prevent refcount leak. Fix this by replacing this API with
pm_runtime_resume_and_get(), which will not change the runtime
PM counter on error.
Signed-off-by: Dinghao Liu
---
drivers/pci/controller/dwc/pcie-tegra194.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
b/drivers/pci/controller/dwc/pcie-tegra194.c
index 6fa216e52d14..0e94190ca4e8 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1645,7 +1645,7 @@ static void pex_ep_event_pex_rst_deassert(struct
tegra_pcie_dw *pcie)
if (pcie->ep_state == EP_STATE_ENABLED)
return;
- ret = pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
if (ret < 0) {
dev_err(dev, "Failed to get runtime sync for PCIe dev: %d\n",
ret);
--
2.17.1
[PATCH] PCI: rcar: Fix runtime PM imbalance in rcar_pcie_ep_probe
pm_runtime_get_sync() will increase the runtime PM counter
even it returns an error. Thus a pairing decrement is needed
to prevent refcount leak. Fix this by replacing this API with
pm_runtime_resume_and_get(), which will not change the runtime
PM counter on error.
Signed-off-by: Dinghao Liu
---
drivers/pci/controller/pcie-rcar-ep.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/pcie-rcar-ep.c
b/drivers/pci/controller/pcie-rcar-ep.c
index b4a288e24aaf..c91d85b15129 100644
--- a/drivers/pci/controller/pcie-rcar-ep.c
+++ b/drivers/pci/controller/pcie-rcar-ep.c
@@ -492,9 +492,9 @@ static int rcar_pcie_ep_probe(struct platform_device *pdev)
pcie->dev = dev;
pm_runtime_enable(dev);
- err = pm_runtime_get_sync(dev);
+ err = pm_runtime_resume_and_get(dev);
if (err < 0) {
- dev_err(dev, "pm_runtime_get_sync failed\n");
+ dev_err(dev, "pm_runtime_resume_and_get failed\n");
goto err_pm_disable;
}
--
2.17.1
[PATCH] slimbus: qcom-ngd-ctrl: Fix runtime PM imbalance in qcom_slim_ngd_enable
When slim_register_controller() fails, a pairing PM usage counter
increment is needed to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
drivers/slimbus/qcom-ngd-ctrl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c
index c054e83ab636..99cf2ab3d862 100644
--- a/drivers/slimbus/qcom-ngd-ctrl.c
+++ b/drivers/slimbus/qcom-ngd-ctrl.c
@@ -1268,6 +1268,7 @@ static int qcom_slim_ngd_enable(struct qcom_slim_ngd_ctrl
*ctrl, bool enable)
ret = slim_register_controller(>ctrl);
if (ret) {
dev_err(ctrl->dev, "error adding slim controller\n");
+ pm_runtime_get_noresume(ctrl->dev);
return ret;
}
--
2.17.1
[PATCH] dmaengine: tegra20: Fix runtime PM imbalance in tegra_dma_issue_pending
pm_runtime_get_sync() will increase the rumtime PM counter
even it returns an error. Thus a pairing decrement is needed
to prevent refcount leak. Fix this by replacing this API with
pm_runtime_resume_and_get(), which will not change the runtime
PM counter on error.
Signed-off-by: Dinghao Liu
---
drivers/dma/tegra20-apb-dma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 71827d9b0aa1..73178afaf4c2 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -723,7 +723,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
goto end;
}
if (!tdc->busy) {
- err = pm_runtime_get_sync(tdc->tdma->dev);
+ err = pm_runtime_resume_and_get(tdc->tdma->dev);
if (err < 0) {
dev_err(tdc2dev(tdc), "Failed to enable DMA\n");
goto end;
--
2.17.1
[PATCH] media: ti-vpe: cal: Fix runtime PM imbalance in cal_probe
pm_runtime_get_sync() will increase the rumtime PM counter even it returns an error. Thus a pairing decrement is needed to prevent refcount leak. Fix this by replacing this API with pm_runtime_resume_and_get(), which will not change the runtime PM counter on error. Signed-off-by: Dinghao Liu --- drivers/media/platform/ti-vpe/cal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c index fa0931788040..ce4e53b9be10 100644 --- a/drivers/media/platform/ti-vpe/cal.c +++ b/drivers/media/platform/ti-vpe/cal.c @@ -1010,7 +1010,7 @@ static int cal_probe(struct platform_device *pdev) /* Read the revision and hardware info to verify hardware access. */ pm_runtime_enable(>dev); - ret = pm_runtime_get_sync(>dev); + ret = pm_runtime_resume_and_get(>dev); if (ret) goto error_pm_runtime; -- 2.17.1
[PATCH] media: atomisp: Fix error handling in atomisp_open
Some error paths in atomisp_open will execute PM runtime
decrement and unregister hmm pool even before we increase
the PM refcount and registration. Fix this by adjusting
jump labels on error.
Signed-off-by: Dinghao Liu
---
drivers/staging/media/atomisp/pci/atomisp_fops.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c
b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index 453bb6913550..8f552d6f1f19 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -837,7 +837,7 @@ static int atomisp_open(struct file *file)
ret = pm_runtime_get_sync(vdev->v4l2_dev->dev);
if (ret < 0) {
dev_err(isp->dev, "Failed to power on device\n");
- goto error;
+ goto pm_error;
}
if (dypool_enable) {
@@ -878,9 +878,10 @@ static int atomisp_open(struct file *file)
css_error:
atomisp_css_uninit(isp);
-error:
hmm_pool_unregister(HMM_POOL_TYPE_DYNAMIC);
+pm_error:
pm_runtime_put(vdev->v4l2_dev->dev);
+error:
rt_mutex_unlock(>mutex);
return ret;
}
--
2.17.1
[PATCH] [v2] ASoC: codecs: Fix runtime PM imbalance in tas2552_probe
There is a rumtime PM imbalance between the error handling path
after devm_snd_soc_register_component() and all other error
handling paths. Add a PM runtime increment to balance refcount.
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Add a PM runtime increment to fix it instead of moving
the PM related operations after the registration.
---
sound/soc/codecs/tas2552.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/tas2552.c b/sound/soc/codecs/tas2552.c
index bd00c35116cd..700baa6314aa 100644
--- a/sound/soc/codecs/tas2552.c
+++ b/sound/soc/codecs/tas2552.c
@@ -730,8 +730,10 @@ static int tas2552_probe(struct i2c_client *client,
ret = devm_snd_soc_register_component(>dev,
_component_dev_tas2552,
tas2552_dai, ARRAY_SIZE(tas2552_dai));
- if (ret < 0)
+ if (ret < 0) {
dev_err(>dev, "Failed to register component: %d\n",
ret);
+ pm_runtime_get_noresume(>dev);
+ }
return ret;
}
--
2.17.1
Re: Re: [PATCH] Input: cyapa - Fix rumtime PM imbalance on error
> Hi Dinghao,
>
> On Wed, Apr 07, 2021 at 12:07:38PM +0800, Dinghao Liu wrote:
> > When mutex_lock_interruptible() fails, a pairing PM usage
> > counter decrement is needed to keep the counter balanced.
>
> Thank you for the patch.
>
> >
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/input/mouse/cyapa.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> > index 77cc653edca2..e411ab45a218 100644
> > --- a/drivers/input/mouse/cyapa.c
> > +++ b/drivers/input/mouse/cyapa.c
> > @@ -904,8 +904,10 @@ static ssize_t cyapa_update_rt_suspend_scanrate(struct
> > device *dev,
> > pm_runtime_get_sync(dev);
> >
> > error = mutex_lock_interruptible(>state_sync_lock);
> > - if (error)
> > + if (error) {
> > + pm_runtime_put_noidle(dev);
>
> Why "noidle" and not pm_runtime_put_sync_autosuspend() like we do in
> case of normal flow?
>
pm_runtime_put_noidle() only decrease the refcount, while
pm_runtime_put_sync_autosuspend() will execute an extra
pm_runtime_autosuspend(). I'm not sure if the autosuspend is necessary
in this error handling path, so I only balance the counter.
Regards,
Dinghao
Re: Re: [PATCH] ASoC: codecs: Fix rumtime PM imbalance in tas2552_probe
> On Wed, Apr 07, 2021 at 02:54:00PM +0800, Dinghao Liu wrote: > > > - pm_runtime_set_active(>dev); > > - pm_runtime_set_autosuspend_delay(>dev, 1000); > > - pm_runtime_use_autosuspend(>dev); > > - pm_runtime_enable(>dev); > > - pm_runtime_mark_last_busy(>dev); > > - pm_runtime_put_sync_autosuspend(>dev); > > - > > dev_set_drvdata(>dev, data); > > > > ret = devm_snd_soc_register_component(>dev, > > @@ -733,6 +726,13 @@ static int tas2552_probe(struct i2c_client *client, > > if (ret < 0) > > dev_err(>dev, "Failed to register component: %d\n", > > ret); > > > > + pm_runtime_set_active(>dev); > > + pm_runtime_set_autosuspend_delay(>dev, 1000); > > + pm_runtime_use_autosuspend(>dev); > > It's not clear to me that just moving the operations after the > registration is a good fix - once the component is registered we could > start trying to do runtime PM operations with it which AFAIR won't count > references and so on properly if runtime PM isn't enabled so if we later > enable runtime PM we might have the rest of the code in a confused state > about what's going on. Thanks for your advice. I checked the use of devm_snd_soc_register_component() in the kernel and found sometimes runtime PM is enabled before registration and sometimes after registration. To be on the safe side, I will send a new patch to fix this in error handling path. Regards, Dinghao
[PATCH] ASoC: codecs: Fix rumtime PM imbalance in tas2552_probe
There is a rumtime PM imbalance between the error handling path after devm_snd_soc_register_component() and all other error handling paths. Fix this by moving PM runtime decrement to the end of the function. Signed-off-by: Dinghao Liu --- sound/soc/codecs/tas2552.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/sound/soc/codecs/tas2552.c b/sound/soc/codecs/tas2552.c index bd00c35116cd..52de6f7b4227 100644 --- a/sound/soc/codecs/tas2552.c +++ b/sound/soc/codecs/tas2552.c @@ -718,13 +718,6 @@ static int tas2552_probe(struct i2c_client *client, return ret; } - pm_runtime_set_active(>dev); - pm_runtime_set_autosuspend_delay(>dev, 1000); - pm_runtime_use_autosuspend(>dev); - pm_runtime_enable(>dev); - pm_runtime_mark_last_busy(>dev); - pm_runtime_put_sync_autosuspend(>dev); - dev_set_drvdata(>dev, data); ret = devm_snd_soc_register_component(>dev, @@ -733,6 +726,13 @@ static int tas2552_probe(struct i2c_client *client, if (ret < 0) dev_err(>dev, "Failed to register component: %d\n", ret); + pm_runtime_set_active(>dev); + pm_runtime_set_autosuspend_delay(>dev, 1000); + pm_runtime_use_autosuspend(>dev); + pm_runtime_enable(>dev); + pm_runtime_mark_last_busy(>dev); + pm_runtime_put_sync_autosuspend(>dev); + return ret; } -- 2.17.1
[PATCH] media: sun8i-di: Fix rumtime PM imbalance in deinterlace_start_streaming
pm_runtime_get_sync() will increase the rumtime PM counter
even it returns an error. Thus a pairing decrement is needed
to prevent refcount leak. Fix this by replacing this API with
pm_runtime_resume_and_get(), which will not change the runtime
PM counter on error.
Signed-off-by: Dinghao Liu
---
drivers/media/platform/sunxi/sun8i-di/sun8i-di.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/sunxi/sun8i-di/sun8i-di.c
b/drivers/media/platform/sunxi/sun8i-di/sun8i-di.c
index ed863bf5ea80..671e4a928993 100644
--- a/drivers/media/platform/sunxi/sun8i-di/sun8i-di.c
+++ b/drivers/media/platform/sunxi/sun8i-di/sun8i-di.c
@@ -589,7 +589,7 @@ static int deinterlace_start_streaming(struct vb2_queue
*vq, unsigned int count)
int ret;
if (V4L2_TYPE_IS_OUTPUT(vq->type)) {
- ret = pm_runtime_get_sync(dev);
+ ret = pm_runtime_resume_and_get(dev);
if (ret < 0) {
dev_err(dev, "Failed to enable module\n");
--
2.17.1
[PATCH] media: platform: sti: Fix rumtime PM imbalance in regs_show
pm_runtime_get_sync() will increase the rumtime PM counter
even it returns an error. Thus a pairing decrement is needed
to prevent refcount leak. Fix this by replacing this API with
pm_runtime_resume_and_get(), which will not change the runtime
PM counter on error.
Signed-off-by: Dinghao Liu
---
drivers/media/platform/sti/bdisp/bdisp-debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/sti/bdisp/bdisp-debug.c
b/drivers/media/platform/sti/bdisp/bdisp-debug.c
index 2b270093009c..a27f638df11c 100644
--- a/drivers/media/platform/sti/bdisp/bdisp-debug.c
+++ b/drivers/media/platform/sti/bdisp/bdisp-debug.c
@@ -480,7 +480,7 @@ static int regs_show(struct seq_file *s, void *data)
int ret;
unsigned int i;
- ret = pm_runtime_get_sync(bdisp->dev);
+ ret = pm_runtime_resume_and_get(bdisp->dev);
if (ret < 0) {
seq_puts(s, "Cannot wake up IP\n");
return 0;
--
2.17.1
[PATCH] dmaengine: stm32: Fix rumtime PM imbalance in stm32_dmamux_resume
pm_runtime_get_sync() will increase the rumtime PM counter even it returns an error. Thus a pairing decrement is needed to prevent refcount leak. Fix this by replacing this API with pm_runtime_resume_and_get(), which will not change the runtime PM counter on error. Signed-off-by: Dinghao Liu --- drivers/dma/stm32-dmamux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma/stm32-dmamux.c b/drivers/dma/stm32-dmamux.c index ef0d0555103d..f9258a63b9c3 100644 --- a/drivers/dma/stm32-dmamux.c +++ b/drivers/dma/stm32-dmamux.c @@ -361,7 +361,7 @@ static int stm32_dmamux_resume(struct device *dev) if (ret < 0) return ret; - ret = pm_runtime_get_sync(dev); + ret = pm_runtime_resume_and_get(dev); if (ret < 0) return ret; -- 2.17.1
[PATCH] usb: cdns3: Fix rumtime PM imbalance on error
When cdns3_gadget_start() fails, a pairing PM usage counter
decrement is needed to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
drivers/usb/cdns3/cdns3-gadget.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index 582bfeceedb4..ad891a108aed 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -3255,8 +3255,11 @@ static int __cdns3_gadget_init(struct cdns *cdns)
pm_runtime_get_sync(cdns->dev);
ret = cdns3_gadget_start(cdns);
- if (ret)
+ if (ret) {
+ pm_runtime_mark_last_busy(cdns->dev);
+ pm_runtime_put_autosuspend(cdns->dev);
return ret;
+ }
/*
* Because interrupt line can be shared with other components in
--
2.17.1
[PATCH] mfd: arizona: Fix rumtime PM imbalance on error
pm_runtime_get_sync() will increase the rumtime PM counter
even it returns an error. Thus a pairing decrement is needed
to prevent refcount leak. Fix this by replacing this API with
pm_runtime_resume_and_get(), which will not change the runtime
PM counter on error.
Signed-off-by: Dinghao Liu
---
drivers/mfd/arizona-irq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
index 077d9ab112b7..d919ae9691e2 100644
--- a/drivers/mfd/arizona-irq.c
+++ b/drivers/mfd/arizona-irq.c
@@ -100,7 +100,7 @@ static irqreturn_t arizona_irq_thread(int irq, void *data)
unsigned int val;
int ret;
- ret = pm_runtime_get_sync(arizona->dev);
+ ret = pm_runtime_resume_and_get(arizona->dev);
if (ret < 0) {
dev_err(arizona->dev, "Failed to resume device: %d\n", ret);
return IRQ_NONE;
--
2.17.1
[PATCH] iio: proximity: pulsedlight: Fix rumtime PM imbalance on error
When lidar_write_control() fails, a pairing PM usage counter
decrement is needed to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
drivers/iio/proximity/pulsedlight-lidar-lite-v2.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
index c685f10b5ae4..cc206bfa09c7 100644
--- a/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
+++ b/drivers/iio/proximity/pulsedlight-lidar-lite-v2.c
@@ -160,6 +160,7 @@ static int lidar_get_measurement(struct lidar_data *data,
u16 *reg)
ret = lidar_write_control(data, LIDAR_REG_CONTROL_ACQUIRE);
if (ret < 0) {
dev_err(>dev, "cannot send start measurement command");
+ pm_runtime_put_noidle(>dev);
return ret;
}
--
2.17.1
[PATCH] Input: cyapa - Fix rumtime PM imbalance on error
When mutex_lock_interruptible() fails, a pairing PM usage
counter decrement is needed to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
drivers/input/mouse/cyapa.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
index 77cc653edca2..e411ab45a218 100644
--- a/drivers/input/mouse/cyapa.c
+++ b/drivers/input/mouse/cyapa.c
@@ -904,8 +904,10 @@ static ssize_t cyapa_update_rt_suspend_scanrate(struct
device *dev,
pm_runtime_get_sync(dev);
error = mutex_lock_interruptible(>state_sync_lock);
- if (error)
+ if (error) {
+ pm_runtime_put_noidle(dev);
return error;
+ }
cyapa->runtime_suspend_sleep_time = min_t(u16, time, 1000);
cyapa->runtime_suspend_power_mode =
--
2.17.1
[PATCH] iio: light: gp2ap002: Fix rumtime PM imbalance on error
When devm_request_threaded_irq() fails, we should decrease the
runtime PM counter to keep the counter balanced. But when
iio_device_register() fails, we need not to decrease it because
we have already decreased it before.
Signed-off-by: Dinghao Liu
---
drivers/iio/light/gp2ap002.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/light/gp2ap002.c b/drivers/iio/light/gp2ap002.c
index 7ba7aa59437c..040d8429a6e0 100644
--- a/drivers/iio/light/gp2ap002.c
+++ b/drivers/iio/light/gp2ap002.c
@@ -583,7 +583,7 @@ static int gp2ap002_probe(struct i2c_client *client,
"gp2ap002", indio_dev);
if (ret) {
dev_err(dev, "unable to request IRQ\n");
- goto out_disable_vio;
+ goto out_put_pm;
}
gp2ap002->irq = client->irq;
@@ -613,8 +613,9 @@ static int gp2ap002_probe(struct i2c_client *client,
return 0;
-out_disable_pm:
+out_put_pm:
pm_runtime_put_noidle(dev);
+out_disable_pm:
pm_runtime_disable(dev);
out_disable_vio:
regulator_disable(gp2ap002->vio);
--
2.17.1
[PATCH] i2c: omap: Fix rumtime PM imbalance on error
pm_runtime_get_sync() will increase the rumtime PM counter even it returns an error. Thus a pairing decrement is needed to prevent refcount leak. Fix this by replacing this API with pm_runtime_resume_and_get(), which will not change the runtime PM counter on error. Signed-off-by: Dinghao Liu --- drivers/i2c/busses/i2c-omap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 12ac4212aded..c9ee0875a79d 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1404,7 +1404,7 @@ omap_i2c_probe(struct platform_device *pdev) pm_runtime_set_autosuspend_delay(omap->dev, OMAP_I2C_PM_TIMEOUT); pm_runtime_use_autosuspend(omap->dev); - r = pm_runtime_get_sync(omap->dev); + r = pm_runtime_resume_and_get(omap->dev); if (r < 0) goto err_free_mem; -- 2.17.1
[PATCH] hostap: Fix memleak in prism2_config
When prism2_hw_config() fails, we just return an error code without any resource release, which may lead to memleak. Signed-off-by: Dinghao Liu --- drivers/net/wireless/intersil/hostap/hostap_cs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/wireless/intersil/hostap/hostap_cs.c b/drivers/net/wireless/intersil/hostap/hostap_cs.c index ec7db2badc40..7dc16ab50ad6 100644 --- a/drivers/net/wireless/intersil/hostap/hostap_cs.c +++ b/drivers/net/wireless/intersil/hostap/hostap_cs.c @@ -536,10 +536,10 @@ static int prism2_config(struct pcmcia_device *link) sandisk_enable_wireless(dev); ret = prism2_hw_config(dev, 1); - if (!ret) - ret = hostap_hw_ready(dev); + if (ret) + goto failed; - return ret; + return hostap_hw_ready(dev);; failed: kfree(hw_priv); -- 2.17.1
[PATCH] scsi: aic7xxx: aic79xx: Add missing check in ahc_handle_seqint
ahc_lookup_scb() may return a null pointer and further lead to
null-pointer-dereference in case DATA_OVERRUN. Fix this by adding
a null check.
Signed-off-by: Dinghao Liu
---
drivers/scsi/aic7xxx/aic7xxx_core.c | 72 +++--
1 file changed, 37 insertions(+), 35 deletions(-)
diff --git a/drivers/scsi/aic7xxx/aic7xxx_core.c
b/drivers/scsi/aic7xxx/aic7xxx_core.c
index 4b04ab8908f8..3a1cd6a0334e 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_core.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_core.c
@@ -1382,43 +1382,45 @@ ahc_handle_seqint(struct ahc_softc *ahc, u_int intstat)
u_int i;
scb = ahc_lookup_scb(ahc, scbindex);
- for (i = 0; i < num_phases; i++) {
- if (lastphase == ahc_phase_table[i].phase)
- break;
- }
- ahc_print_path(ahc, scb);
- printk("data overrun detected %s."
- " Tag == 0x%x.\n",
- ahc_phase_table[i].phasemsg,
- scb->hscb->tag);
- ahc_print_path(ahc, scb);
- printk("%s seen Data Phase. Length = %ld. NumSGs = %d.\n",
- ahc_inb(ahc, SEQ_FLAGS) & DPHASE ? "Have" : "Haven't",
- ahc_get_transfer_length(scb), scb->sg_count);
- if (scb->sg_count > 0) {
- for (i = 0; i < scb->sg_count; i++) {
-
- printk("sg[%d] - Addr 0x%x%x : Length %d\n",
- i,
- (ahc_le32toh(scb->sg_list[i].len) >> 24
- & SG_HIGH_ADDR_BITS),
- ahc_le32toh(scb->sg_list[i].addr),
- ahc_le32toh(scb->sg_list[i].len)
- & AHC_SG_LEN_MASK);
+ if (scb != NULL) {
+ for (i = 0; i < num_phases; i++) {
+ if (lastphase == ahc_phase_table[i].phase)
+ break;
}
+ ahc_print_path(ahc, scb);
+ printk("data overrun detected %s."
+ " Tag == 0x%x.\n",
+ ahc_phase_table[i].phasemsg,
+ scb->hscb->tag);
+ ahc_print_path(ahc, scb);
+ printk("%s seen Data Phase. Length = %ld. NumSGs =
%d.\n",
+ ahc_inb(ahc, SEQ_FLAGS) & DPHASE ? "Have" :
"Haven't",
+ ahc_get_transfer_length(scb), scb->sg_count);
+ if (scb->sg_count > 0) {
+ for (i = 0; i < scb->sg_count; i++) {
+
+ printk("sg[%d] - Addr 0x%x%x : Length
%d\n",
+ i,
+
(ahc_le32toh(scb->sg_list[i].len) >> 24
+ & SG_HIGH_ADDR_BITS),
+
ahc_le32toh(scb->sg_list[i].addr),
+ ahc_le32toh(scb->sg_list[i].len)
+ & AHC_SG_LEN_MASK);
+ }
+ }
+ /*
+ * Set this and it will take effect when the
+ * target does a command complete.
+ */
+ ahc_freeze_devq(ahc, scb);
+ if ((scb->flags & SCB_SENSE) == 0) {
+ ahc_set_transaction_status(scb,
CAM_DATA_RUN_ERR);
+ } else {
+ scb->flags &= ~SCB_SENSE;
+ ahc_set_transaction_status(scb,
CAM_AUTOSENSE_FAIL);
+ }
+ ahc_freeze_scb(scb);
}
- /*
-* Set this and it will take effect when the
-* target does a command complete.
-*/
- ahc_freeze_devq(ahc, scb);
- if ((scb->flags & SCB_SENSE) == 0) {
- ahc_set_transaction_status(scb, CAM_DATA_RUN_ERR);
- } else {
- scb->flags &= ~SCB_SENSE;
- ahc_set_transaction_status(scb, CAM_AUTOSENSE_FAIL);
- }
- ahc_freeze_scb(scb);
if ((ahc->features & AHC_ULTRA2) != 0) {
/*
--
2.17.1
[PATCH] media: em28xx: Fix missing check in em28xx_capture_start
There are several em28xx_write_reg() and em28xx_write_reg_bits()
calls that we have caught their return values but lack further
handling. Check and return error on failure just like other calls
in em28xx_capture_start().
Signed-off-by: Dinghao Liu
---
drivers/media/usb/em28xx/em28xx-core.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/em28xx/em28xx-core.c
b/drivers/media/usb/em28xx/em28xx-core.c
index 584fa400cd7d..2563275fec8e 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -661,6 +661,8 @@ int em28xx_capture_start(struct em28xx *dev, int start)
EM2874_R5F_TS_ENABLE,
start ?
EM2874_TS2_CAPTURE_ENABLE : 0x00,
EM2874_TS2_CAPTURE_ENABLE |
EM2874_TS2_FILTER_ENABLE | EM2874_TS2_NULL_DISCARD);
+ if (rc < 0)
+ return rc;
} else {
/* FIXME: which is the best order? */
/* video registers are sampled by VREF */
@@ -670,8 +672,11 @@ int em28xx_capture_start(struct em28xx *dev, int start)
return rc;
if (start) {
- if (dev->is_webcam)
+ if (dev->is_webcam) {
rc = em28xx_write_reg(dev, 0x13, 0x0c);
+ if (rc < 0)
+ return rc;
+ }
/* Enable video capture */
rc = em28xx_write_reg(dev, 0x48, 0x00);
@@ -693,6 +698,8 @@ int em28xx_capture_start(struct em28xx *dev, int start)
} else {
/* disable video capture */
rc = em28xx_write_reg(dev, EM28XX_R12_VINENABLE, 0x27);
+ if (rc < 0)
+ return rc;
}
}
--
2.17.1
[PATCH] power: supply: axp20x_usb_power: Add missing check in axp20x_usb_power_probe
There are two regmap_update_bits() calls but only one of them has
return value check, which is odd. Add a return value check and
terminate the execution flow on failure just like the other call.
Signed-off-by: Dinghao Liu
---
drivers/power/supply/axp20x_usb_power.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/power/supply/axp20x_usb_power.c
b/drivers/power/supply/axp20x_usb_power.c
index 8933ae26c3d6..7ed76eef8417 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -614,8 +614,10 @@ static int axp20x_usb_power_probe(struct platform_device
*pdev)
if (power->axp20x_id == AXP813_ID) {
/* Enable USB Battery Charging specification detection */
- regmap_update_bits(axp20x->regmap, AXP288_BC_GLOBAL,
+ ret = regmap_update_bits(axp20x->regmap, AXP288_BC_GLOBAL,
AXP813_BC_EN, AXP813_BC_EN);
+ if (ret)
+ return ret;
}
psy_cfg.of_node = pdev->dev.of_node;
--
2.17.1
[PATCH] [v2] sata_dwc_460ex: Fix missing check in sata_dwc_isr
The return value of ata_qc_from_tag() is checked in the whole
kernel except for two calls in sata_dwc_isr(), which may lead
to null-pointer-dereference. Add return value checks to avoid
such case.
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Refine commit message.
Add return value check for another ata_qc_from_tag() call.
---
drivers/ata/sata_dwc_460ex.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 9dcef6ac643b..f0ef844428bb 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -543,6 +543,11 @@ static irqreturn_t sata_dwc_isr(int irq, void
*dev_instance)
hsdev->sactive_issued |= qcmd_tag_to_mask(tag);
qc = ata_qc_from_tag(ap, tag);
+ if (unlikely(!qc)) {
+ dev_err(ap->dev, "failed to get qc");
+ handled = 1;
+ goto DONE;
+ }
/*
* Start FP DMA for NCQ command. At this point the tag is the
* active tag. It is the tag that matches the command about to
@@ -658,6 +663,11 @@ static irqreturn_t sata_dwc_isr(int irq, void
*dev_instance)
tag_mask &= (~0x0001);
qc = ata_qc_from_tag(ap, tag);
+ if (unlikely(!qc)) {
+ dev_err(ap->dev, "failed to get qc");
+ handled = 1;
+ goto DONE;
+ }
/* To be picked up by completion functions */
qc->ap->link.active_tag = tag;
--
2.17.1
Re: Re: Re: Re: [PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr
> On Tue, Mar 2, 2021 at 9:34 AM wrote: > > > On Mon, Mar 1, 2021 at 1:20 PM wrote: > > > > > On Mon, Mar 1, 2021 at 9:44 AM Dinghao Liu > > > > > wrote: > > ... > > > > > This issue is reported by my static analysis tool, so I don't have the > > > > vulnerable input currently. > > > > > > Should we blindly follow everything that some (non-ideal) tool > > > reports? I don't think so. > > > For all my experiments with that hardware, I haven't heard about the > > > issue with NULL pointers. Useless checks make code harder to read and > > > CPU to waste cycles. It might be maintainers of this driver consider > > > otherwise, so not my call. > > > > > > > Thanks for your advice. I also checked all use of ata_qc_from_tag() in the > > whole kernel and found all of them had return value checks except for the > > calls in sata_dwc_isr(), which is odd. > > Thanks for this information, it makes sense to me. Perhaps you need to > put this into the commit message to justify the need of the change. > OK. I will fix this and send a new patch soon. Regards, Dinghao
Re: Re: Re: [PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr
> On Mon, Mar 1, 2021 at 1:20 PM wrote: > > > > > On Mon, Mar 1, 2021 at 9:44 AM Dinghao Liu wrote: > > > > > > > > ata_qc_from_tag() may return a null pointer and further lead to > > > > null-pointer-dereference. Add a return value check to avoid such case. > > > > > > Can you elaborate more on this? Is it a real case? > > > I have a hardware, how can I reproduce this? > > > > > > > In the branch 'if (intpr & SATA_DWC_INTPR_NEWFP)', we call ata_qc_from_tag() > > and access qc->ap->link.active_tag immediately. If ata_qc_from_tag() returns > > a null pointer, accessing qc->ap->link.active_tag may crash the system. > > Yes, I can see that. My question is how to get into the case when this > will be true. > I cannot answer this question immediately. I think it's possible to build a designed input to trigger this case for some professional attackers. > > This issue is reported by my static analysis tool, so I don't have the > > vulnerable input currently. > > Should we blindly follow everything that some (non-ideal) tool > reports? I don't think so. > For all my experiments with that hardware, I haven't heard about the > issue with NULL pointers. Useless checks make code harder to read and > CPU to waste cycles. It might be maintainers of this driver consider > otherwise, so not my call. > Thanks for your advice. I also checked all use of ata_qc_from_tag() in the whole kernel and found all of them had return value checks except for the calls in sata_dwc_isr(), which is odd. There is no issue currently does not mean it will never happen in the future. So I suggest the maintainer of function sata_dwc_isr() to fix this issue. Regards, Dinghao
[PATCH] drivers: misc: ad525x_dpot: Add missing check in dpot_read_spi
The use of dpot_read_r8d8() after checking dpot->uid is similar. However, we check the return value and return an error code only in one path, which is odd. Signed-off-by: Dinghao Liu --- drivers/misc/ad525x_dpot.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/misc/ad525x_dpot.c b/drivers/misc/ad525x_dpot.c index 6f164522b028..5d8f3f6a95f2 100644 --- a/drivers/misc/ad525x_dpot.c +++ b/drivers/misc/ad525x_dpot.c @@ -139,6 +139,9 @@ static s32 dpot_read_spi(struct dpot_data *dpot, u8 reg) value = dpot_read_r8d8(dpot, DPOT_AD5291_READ_RDAC << 2); + if (value < 0) + return value; + if (dpot->uid == DPOT_UID(AD5291_ID)) value = value >> 2; -- 2.17.1
Re: Re: [PATCH] tpm: Add missing check in tpm_inf_recv
Jarkko Sakkinen jar...@kernel.org写道： > On Sun, Feb 28, 2021 at 05:32:30PM +0800, Dinghao Liu wrote: > > The use of wait() in tpm_inf_recv() is almost the same. It's odd that > > we only check the return value and terminate execution flow of one call. > > > > Signed-off-by: Dinghao Liu > > Is the unchecked return value of wait() the problem? I don't see the > function even mentioned in the description. > Yes. This issue is reported by my static analysis tool. I think we should treat wait() equally in this function (check the return value and return an error code on failure). Regards, Dinghao
Re: Re: [PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr
> On Mon, Mar 1, 2021 at 9:44 AM Dinghao Liu wrote: > > > > ata_qc_from_tag() may return a null pointer and further lead to > > null-pointer-dereference. Add a return value check to avoid such case. > > Can you elaborate more on this? Is it a real case? > I have a hardware, how can I reproduce this? > In the branch 'if (intpr & SATA_DWC_INTPR_NEWFP)', we call ata_qc_from_tag() and access qc->ap->link.active_tag immediately. If ata_qc_from_tag() returns a null pointer, accessing qc->ap->link.active_tag may crash the system. This issue is reported by my static analysis tool, so I don't have the vulnerable input currently. Regards, Dinghao
[PATCH] scsi: aic7xxx: aic79xx: Add missing check in ahd_handle_seqint
ahd_lookup_scb() may return a null pointer and further lead to
null pointer dereference in case DATA_OVERRUN. Fix this by adding
a null check.
Signed-off-by: Dinghao Liu
---
drivers/scsi/aic7xxx/aic79xx_core.c | 44 +++--
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c
b/drivers/scsi/aic7xxx/aic79xx_core.c
index 3e3100dbfda3..f990f7f48f49 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -2199,30 +2199,32 @@ ahd_handle_seqint(struct ahd_softc *ahd, u_int intstat)
scbindex = ahd_get_scbptr(ahd);
scb = ahd_lookup_scb(ahd, scbindex);
+ if (scb != NULL) {
#ifdef AHD_DEBUG
- lastphase = ahd_inb(ahd, LASTPHASE);
- if ((ahd_debug & AHD_SHOW_RECOVERY) != 0) {
- ahd_print_path(ahd, scb);
- printk("data overrun detected %s. Tag == 0x%x.\n",
- ahd_lookup_phase_entry(lastphase)->phasemsg,
- SCB_GET_TAG(scb));
- ahd_print_path(ahd, scb);
- printk("%s seen Data Phase. Length = %ld. "
- "NumSGs = %d.\n",
- ahd_inb(ahd, SEQ_FLAGS) & DPHASE
- ? "Have" : "Haven't",
- ahd_get_transfer_length(scb), scb->sg_count);
- ahd_dump_sglist(scb);
- }
+ lastphase = ahd_inb(ahd, LASTPHASE);
+ if ((ahd_debug & AHD_SHOW_RECOVERY) != 0) {
+ ahd_print_path(ahd, scb);
+ printk("data overrun detected %s. Tag ==
0x%x.\n",
+
ahd_lookup_phase_entry(lastphase)->phasemsg,
+ SCB_GET_TAG(scb));
+ ahd_print_path(ahd, scb);
+ printk("%s seen Data Phase. Length = %ld. "
+ "NumSGs = %d.\n",
+ ahd_inb(ahd, SEQ_FLAGS) & DPHASE
+ ? "Have" : "Haven't",
+ ahd_get_transfer_length(scb),
scb->sg_count);
+ ahd_dump_sglist(scb);
+ }
#endif
- /*
-* Set this and it will take effect when the
-* target does a command complete.
-*/
- ahd_freeze_devq(ahd, scb);
- ahd_set_transaction_status(scb, CAM_DATA_RUN_ERR);
- ahd_freeze_scb(scb);
+ /*
+ * Set this and it will take effect when the
+ * target does a command complete.
+ */
+ ahd_freeze_devq(ahd, scb);
+ ahd_set_transaction_status(scb, CAM_DATA_RUN_ERR);
+ ahd_freeze_scb(scb);
+ }
break;
}
case MKMSG_FAILED:
--
2.17.1
[PATCH] iio: adc: Fix error handling in vadc_do_conversion
There is one vadc_poll_wait_eoc() call in vadc_do_conversion
that we have caught its return value but lack further handling.
Check and jump to err_disable label just like the other
vadc_poll_wait_eoc() in this function.
Signed-off-by: Dinghao Liu
---
drivers/iio/adc/qcom-spmi-vadc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c
index 05ff948372b3..fe36b0ba8273 100644
--- a/drivers/iio/adc/qcom-spmi-vadc.c
+++ b/drivers/iio/adc/qcom-spmi-vadc.c
@@ -324,6 +324,8 @@ static int vadc_do_conversion(struct vadc_priv *vadc,
if (vadc->poll_eoc) {
ret = vadc_poll_wait_eoc(vadc, timeout);
+ if (ret)
+ goto err_disable;
} else {
ret = wait_for_completion_timeout(>complete, timeout);
if (!ret) {
--
2.17.1
[PATCH] iio: gyro: mpu3050: Fix error handling in mpu3050_trigger_handler
There is one regmap_bulk_read() call in mpu3050_trigger_handler that we have caught its return value bug lack further handling. Check and terminate the execution flow just like the other three regmap_bulk_read() calls in this function. Signed-off-by: Dinghao Liu --- drivers/iio/gyro/mpu3050-core.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c index dfa31a23500f..ac90be03332a 100644 --- a/drivers/iio/gyro/mpu3050-core.c +++ b/drivers/iio/gyro/mpu3050-core.c @@ -551,6 +551,8 @@ static irqreturn_t mpu3050_trigger_handler(int irq, void *p) MPU3050_FIFO_R, _values[offset], toread); + if (ret) + goto out_trigger_unlock; dev_dbg(mpu3050->dev, "%04x %04x %04x %04x %04x\n", -- 2.17.1
Re: Re: [PATCH] iwlegacy: Add missing check in il4965_commit_rxon
> On Sun, Feb 28, 2021 at 08:25:22PM +0800, Dinghao Liu wrote:
> > There is one il_set_tx_power() call in this function without
> > return value check. Print error message and return error code
> > on failure just like the other il_set_tx_power() call.
>
> We have few calls for il_set_tx_power(), on some cases we
> check return on some not. That correct as setting tx power
> can be deferred internally if not possible at the moment.
>
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/net/wireless/intel/iwlegacy/4965.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/intel/iwlegacy/4965.c
> > b/drivers/net/wireless/intel/iwlegacy/4965.c
> > index 9fa556486511..3235b8be1894 100644
> > --- a/drivers/net/wireless/intel/iwlegacy/4965.c
> > +++ b/drivers/net/wireless/intel/iwlegacy/4965.c
> > @@ -1361,7 +1361,11 @@ il4965_commit_rxon(struct il_priv *il)
> > * We do not commit tx power settings while channel changing,
> > * do it now if tx power changed.
> > */
> > - il_set_tx_power(il, il->tx_power_next, false);
> > + ret = il_set_tx_power(il, il->tx_power_next, false);
> > + if (ret) {
> > + IL_ERR("Error sending TX power (%d)\n", ret);
> > + return ret;
> > +
>
> This is not good change. We do not check return value of
> il_commit_rxon(), except when creating interface. So this change might
> broke creating interface, what worked otherwise when il_set_tx_power()
> returned error.
>
It's clear to me, Thanks for your explanation!
Regards,
Dinghao
[PATCH] sata_dwc_460ex: Fix missing check in sata_dwc_isr
ata_qc_from_tag() may return a null pointer and further lead to
null-pointer-dereference. Add a return value check to avoid such case.
Signed-off-by: Dinghao Liu
---
drivers/ata/sata_dwc_460ex.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 9dcef6ac643b..0068247ffc06 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -548,8 +548,10 @@ static irqreturn_t sata_dwc_isr(int irq, void
*dev_instance)
* active tag. It is the tag that matches the command about to
* be completed.
*/
- qc->ap->link.active_tag = tag;
- sata_dwc_bmdma_start_by_tag(qc, tag);
+ if (qc) {
+ qc->ap->link.active_tag = tag;
+ sata_dwc_bmdma_start_by_tag(qc, tag);
+ }
handled = 1;
goto DONE;
--
2.17.1
[PATCH] iwlegacy: Add missing check in il4965_commit_rxon
There is one il_set_tx_power() call in this function without
return value check. Print error message and return error code
on failure just like the other il_set_tx_power() call.
Signed-off-by: Dinghao Liu
---
drivers/net/wireless/intel/iwlegacy/4965.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/4965.c
b/drivers/net/wireless/intel/iwlegacy/4965.c
index 9fa556486511..3235b8be1894 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965.c
@@ -1361,7 +1361,11 @@ il4965_commit_rxon(struct il_priv *il)
* We do not commit tx power settings while channel changing,
* do it now if tx power changed.
*/
- il_set_tx_power(il, il->tx_power_next, false);
+ ret = il_set_tx_power(il, il->tx_power_next, false);
+ if (ret) {
+ IL_ERR("Error sending TX power (%d)\n", ret);
+ return ret;
+ }
return 0;
}
--
2.17.1
[PATCH] i40e: Fix error handling in i40e_vsi_open
When vsi->type == I40E_VSI_FDIR, we have caught the return value of
i40e_vsi_request_irq() but without further handling. Check and execute
memory clean on failure just like the other i40e_vsi_request_irq().
Fixes: 8a9eb7d3cbcab ("i40e: rework fdir setup and teardown")
Signed-off-by: Dinghao Liu
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 353deae139f9..c3bbc1310f8e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -8724,6 +8724,8 @@ int i40e_vsi_open(struct i40e_vsi *vsi)
dev_driver_string(>pdev->dev),
dev_name(>pdev->dev));
err = i40e_vsi_request_irq(vsi, int_name);
+ if (err)
+ goto err_setup_rx;
} else {
err = -EINVAL;
--
2.17.1
[PATCH] e1000e: Fix error handling in e1000_set_d0_lplu_state_82571
There is one e1e_wphy() call in e1000_set_d0_lplu_state_82571
that we have caught its return value but lack further handling.
Check and terminate the execution flow just like other e1e_wphy()
in this function.
Signed-off-by: Dinghao Liu
---
drivers/net/ethernet/intel/e1000e/82571.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/e1000e/82571.c
b/drivers/net/ethernet/intel/e1000e/82571.c
index 88faf05e23ba..0b1e890dd583 100644
--- a/drivers/net/ethernet/intel/e1000e/82571.c
+++ b/drivers/net/ethernet/intel/e1000e/82571.c
@@ -899,6 +899,8 @@ static s32 e1000_set_d0_lplu_state_82571(struct e1000_hw
*hw, bool active)
} else {
data &= ~IGP02E1000_PM_D0_LPLU;
ret_val = e1e_wphy(hw, IGP02E1000_PHY_POWER_MGMT, data);
+ if (ret_val)
+ return ret_val;
/* LPLU and SmartSpeed are mutually exclusive. LPLU is used
* during Dx states where the power conservation is most
* important. During driver activity we should enable
--
2.17.1
[PATCH] tpm: Add missing check in tpm_inf_recv
The use of wait() in tpm_inf_recv() is almost the same. It's odd that
we only check the return value and terminate execution flow of one call.
Signed-off-by: Dinghao Liu
---
drivers/char/tpm/tpm_infineon.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index 9c924a1440a9..abe00f45aa45 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -263,7 +263,9 @@ static int tpm_inf_recv(struct tpm_chip *chip, u8 * buf,
size_t count)
size = ((buf[2] << 8) | buf[3]);
for (i = 0; i < size; i++) {
- wait(chip, STAT_RDA);
+ ret = wait(chip, STAT_RDA);
+ if (ret)
+ return -EIO;
buf[i] = tpm_data_in(RDFIFO);
}
--
2.17.1
Re: Re: [PATCH] RDMA/siw: Fix missing check in siw_get_hdr
Bernard Metzler b...@zurich.ibm.com写道： > -----"Dinghao Liu" wrote: - > > >To: dinghao@zju.edu.cn, k...@umn.edu > >From: "Dinghao Liu" > >Date: 02/26/2021 08:56AM > >Cc: "Bernard Metzler" , "Doug Ledford" > >, "Jason Gunthorpe" , > >linux-r...@vger.kernel.org, linux-kernel@vger.kernel.org > >Subject: [EXTERNAL] [PATCH] RDMA/siw: Fix missing check in > >siw_get_hdr > > > >We should also check the range of opcode after calling > >__rdmap_get_opcode() in the else branch to prevent potential > >overflow. > > Hi Dinghao, > No this is not needed. We always first read the minimum > header information (MPA len, DDP flags, RDMAP opcode, > STag, target offset). Only if we have received that > into local buffer, we check for the opcode this one time. > Now the opcode determines the remaining length of the > variably sized part of the header to be received. > > We do not have to check the opcode again, since we > already received and checked it. > It's clear to me, thanks! Regards, Dinghao
[PATCH] mmc: sdhci-pci-o2micro: Add missing checks in sdhci_pci_o2_probe
It's odd to adopt different error handling on failure of
pci_read_config_dword(). Check the return value and terminate
execution flow on failure of all pci_read_config_dword() calls
in this function.
Signed-off-by: Dinghao Liu
---
drivers/mmc/host/sdhci-pci-o2micro.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/mmc/host/sdhci-pci-o2micro.c
b/drivers/mmc/host/sdhci-pci-o2micro.c
index 94e3f72f6405..51d55a87aebe 100644
--- a/drivers/mmc/host/sdhci-pci-o2micro.c
+++ b/drivers/mmc/host/sdhci-pci-o2micro.c
@@ -706,6 +706,8 @@ static int sdhci_pci_o2_probe(struct sdhci_pci_chip *chip)
ret = pci_read_config_dword(chip->pdev,
O2_SD_FUNC_REG0,
_32);
+ if (ret)
+ return ret;
scratch_32 = ((scratch_32 & 0xFF00) >> 24);
/* Check Whether subId is 0x11 or 0x12 */
@@ -716,6 +718,8 @@ static int sdhci_pci_o2_probe(struct sdhci_pci_chip *chip)
ret = pci_read_config_dword(chip->pdev,
O2_SD_FUNC_REG4,
_32);
+ if (ret)
+ return ret;
/* Enable Base Clk setting change */
scratch_32 |= O2_SD_FREG4_ENABLE_CLK_SET;
@@ -795,6 +799,8 @@ static int sdhci_pci_o2_probe(struct sdhci_pci_chip *chip)
ret = pci_read_config_dword(chip->pdev,
O2_SD_PLL_SETTING, _32);
+ if (ret)
+ return ret;
if ((scratch_32 & 0xff00) == 0x0100) {
scratch_32 &= 0x;
@@ -812,6 +818,8 @@ static int sdhci_pci_o2_probe(struct sdhci_pci_chip *chip)
ret = pci_read_config_dword(chip->pdev,
O2_SD_FUNC_REG4,
_32);
+ if (ret)
+ return ret;
scratch_32 |= (1 << 22);
pci_write_config_dword(chip->pdev,
O2_SD_FUNC_REG4, scratch_32);
--
2.17.1
[PATCH] RDMA/siw: Fix missing check in siw_get_hdr
We should also check the range of opcode after calling
__rdmap_get_opcode() in the else branch to prevent potential
overflow.
Fixes: 8b6a361b8c482 ("rdma/siw: receive path")
Signed-off-by: Dinghao Liu
---
drivers/infiniband/sw/siw/siw_qp_rx.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c
b/drivers/infiniband/sw/siw/siw_qp_rx.c
index 60116f20653c..301e7fe2c61a 100644
--- a/drivers/infiniband/sw/siw/siw_qp_rx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
@@ -1072,6 +1072,16 @@ static int siw_get_hdr(struct siw_rx_stream *srx)
siw_dbg_qp(rx_qp(srx), "new header, opcode %u\n", opcode);
} else {
opcode = __rdmap_get_opcode(c_hdr);
+
+ if (opcode > RDMAP_TERMINATE) {
+ pr_warn("siw: received unknown packet type %u\n",
+ opcode);
+
+ siw_init_terminate(rx_qp(srx), TERM_ERROR_LAYER_RDMAP,
+ RDMAP_ETYPE_REMOTE_OPERATION,
+ RDMAP_ECODE_OPCODE, 0);
+ return -EINVAL;
+ }
}
set_rx_fpdu_context(qp, opcode);
frx = qp->rx_fpdu;
--
2.17.1
Re: Re: [PATCH] ASoC: Intel: Skylake: Fix missing check in skl_pcm_trigger
> > On 2/15/21 7:13 AM, Dinghao Liu wrote: > > When cmd == SNDRV_PCM_TRIGGER_STOP, we should also check > > the return value of skl_decoupled_trigger() just like what > > we have done in case SNDRV_PCM_TRIGGER_PAUSE_RELEASE. > > > > Signed-off-by: Dinghao Liu > > --- > > sound/soc/intel/skylake/skl-pcm.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/sound/soc/intel/skylake/skl-pcm.c > > b/sound/soc/intel/skylake/skl-pcm.c > > index b1ca64d2f7ea..a5b1f333a500 100644 > > --- a/sound/soc/intel/skylake/skl-pcm.c > > +++ b/sound/soc/intel/skylake/skl-pcm.c > > @@ -516,6 +516,9 @@ static int skl_pcm_trigger(struct snd_pcm_substream > > *substream, int cmd, > > return ret; > > Is there any additional error handling to be done for the > > skl_stop_pipe that just happened ? > I think the check is enough here. Regards, Dinghao
[PATCH] ASoC: Intel: Skylake: Fix missing check in skl_pcm_trigger
When cmd == SNDRV_PCM_TRIGGER_STOP, we should also check
the return value of skl_decoupled_trigger() just like what
we have done in case SNDRV_PCM_TRIGGER_PAUSE_RELEASE.
Signed-off-by: Dinghao Liu
---
sound/soc/intel/skylake/skl-pcm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-pcm.c
b/sound/soc/intel/skylake/skl-pcm.c
index b1ca64d2f7ea..a5b1f333a500 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -516,6 +516,9 @@ static int skl_pcm_trigger(struct snd_pcm_substream
*substream, int cmd,
return ret;
ret = skl_decoupled_trigger(substream, cmd);
+ if (ret < 0)
+ return ret;
+
if ((cmd == SNDRV_PCM_TRIGGER_SUSPEND) && !w->ignore_suspend) {
/* save the dpib and lpib positions */
stream->dpib = readl(bus->remap_addr +
--
2.17.1
[PATCH] ALSA: intel8x0: Fix missing check in snd_intel8x0m_create
When device_type == DEVICE_ALI, we should also check the return
value of pci_iomap() to avoid potential null pointer dereference.
Signed-off-by: Dinghao Liu
---
sound/pci/intel8x0m.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/pci/intel8x0m.c b/sound/pci/intel8x0m.c
index 1b7df0c4e57c..19872cecc9d2 100644
--- a/sound/pci/intel8x0m.c
+++ b/sound/pci/intel8x0m.c
@@ -1129,13 +1129,14 @@ static int snd_intel8x0m_create(struct snd_card *card,
chip->bmaddr = pci_iomap(pci, 3, 0);
else
chip->bmaddr = pci_iomap(pci, 1, 0);
+
+port_inited:
if (!chip->bmaddr) {
dev_err(card->dev, "Controller space ioremap problem\n");
snd_intel8x0m_free(chip);
return -EIO;
}
- port_inited:
/* initialize offsets */
chip->bdbars_count = 2;
tbl = intel_regs;
--
2.17.1
[PATCH] extcon: Fix error handling in extcon_dev_register
When devm_kcalloc() fails, we should execute device_unregister()
to unregister edev->dev from system.
Fixes: 046050f6e623e ("extcon: Update the prototype of
extcon_register_notifier() with enum extcon")
Signed-off-by: Dinghao Liu
---
drivers/extcon/extcon.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 0a6438cbb3f3..e7a9561a826d 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1241,6 +1241,7 @@ int extcon_dev_register(struct extcon_dev *edev)
sizeof(*edev->nh), GFP_KERNEL);
if (!edev->nh) {
ret = -ENOMEM;
+ device_unregister(>dev);
goto err_dev;
}
--
2.17.1
[PATCH] [v3] block: Fix an error handling in add_partition
Once we have called device_initialize(), we should use put_device() to give up the reference on error, just like what we have done on failure of device_add(). Signed-off-by: Dinghao Liu --- Changelog: v2: - Refine commit message. v3: - Add '[v3]' to the title. --- block/partitions/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/partitions/core.c b/block/partitions/core.c index e7d776db803b..23460cee9de5 100644 --- a/block/partitions/core.c +++ b/block/partitions/core.c @@ -384,7 +384,7 @@ static struct block_device *add_partition(struct gendisk *disk, int partno, err = blk_alloc_devt(bdev, ); if (err) - goto out_bdput; + goto out_put; pdev->devt = devt; /* delay uevent until 'holders' subdir is created */ -- 2.17.1
[PATCH] block: Fix an error handling in add_partition
Once we have called device_initialize(), we should use put_device() to give up the reference on error, just like what we have done on failure of device_add(). Signed-off-by: Dinghao Liu --- Changelog: v2: - Refine commit message. --- block/partitions/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/partitions/core.c b/block/partitions/core.c index e7d776db803b..23460cee9de5 100644 --- a/block/partitions/core.c +++ b/block/partitions/core.c @@ -384,7 +384,7 @@ static struct block_device *add_partition(struct gendisk *disk, int partno, err = blk_alloc_devt(bdev, ); if (err) - goto out_bdput; + goto out_put; pdev->devt = devt; /* delay uevent until 'holders' subdir is created */ -- 2.17.1
Re: Re: [PATCH] block: Fix an error handling in add_partition
> On 1/15/21 11:34 PM, Dinghao Liu wrote: > > Once we have called device_initialize(), we should > > use put_device() to give up the reference on error, > > just like what we have done on failure of device_add(). > > > > Signed-off-by: Dinghao Liu > Please consider having following commit message, since above > commit message is looking odd from what we have in the tree :- > > > Once we have called device_initialize(), we should use put_device() to > give up the reference on error, just like what we have done on failure > of device_add(). > Thanks for this suggestion! > > Also have you tested this patch with the with generating appropriate error ? > No, this problem is found through comparing existing source code. Regards, Dinghao
[PATCH] block: Fix an error handling in add_partition
Once we have called device_initialize(), we should use put_device() to give up the reference on error, just like what we have done on failure of device_add(). Signed-off-by: Dinghao Liu --- block/partitions/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/partitions/core.c b/block/partitions/core.c index e7d776db803b..23460cee9de5 100644 --- a/block/partitions/core.c +++ b/block/partitions/core.c @@ -384,7 +384,7 @@ static struct block_device *add_partition(struct gendisk *disk, int partno, err = blk_alloc_devt(bdev, ); if (err) - goto out_bdput; + goto out_put; pdev->devt = devt; /* delay uevent until 'holders' subdir is created */ -- 2.17.1
[PATCH] [v2] evm: Fix memleak in init_desc
When kmalloc() fails, tmp_tfm allocated by
crypto_alloc_shash() has not been freed, which
leads to memleak.
Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Remove checks against tmp_tfm before freeing.
---
security/integrity/evm/evm_crypto.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/security/integrity/evm/evm_crypto.c
b/security/integrity/evm/evm_crypto.c
index 168c3b78ac47..a6dd47eb086d 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type, uint8_t
hash_algo)
{
long rc;
const char *algo;
- struct crypto_shash **tfm, *tmp_tfm;
+ struct crypto_shash **tfm, *tmp_tfm = NULL;
struct shash_desc *desc;
if (type == EVM_XATTR_HMAC) {
@@ -118,13 +118,16 @@ static struct shash_desc *init_desc(char type, uint8_t
hash_algo)
alloc:
desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
GFP_KERNEL);
- if (!desc)
+ if (!desc) {
+ crypto_free_shash(tmp_tfm);
return ERR_PTR(-ENOMEM);
+ }
desc->tfm = *tfm;
rc = crypto_shash_init(desc);
if (rc) {
+ crypto_free_shash(tmp_tfm);
kfree(desc);
return ERR_PTR(rc);
}
--
2.17.1
Re: Re: Re: [PATCH] evm: Fix memleak in init_desc
> On Sun, Jan 10, 2021 at 01:27:09PM +0800, dinghao@zju.edu.cn wrote:
> > > On Sat, Jan 09, 2021 at 07:33:05PM +0800, Dinghao Liu wrote:
> > > > When kmalloc() fails, tmp_tfm allocated by
> > > > crypto_alloc_shash() has not been freed, which
> > > > leads to memleak.
> > > >
> > > > Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
> > > > Signed-off-by: Dinghao Liu
> > > > ---
> > > > security/integrity/evm/evm_crypto.c | 9 +++--
> > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/security/integrity/evm/evm_crypto.c
> > > > b/security/integrity/evm/evm_crypto.c
> > > > index 168c3b78ac47..39fb31a638ac 100644
> > > > --- a/security/integrity/evm/evm_crypto.c
> > > > +++ b/security/integrity/evm/evm_crypto.c
> > > > @@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type,
> > > > uint8_t hash_algo)
> > > > {
> > > > long rc;
> > > > const char *algo;
> > > > - struct crypto_shash **tfm, *tmp_tfm;
> > > > + struct crypto_shash **tfm, *tmp_tfm = NULL;
> > > > struct shash_desc *desc;
> > > >
> > > > if (type == EVM_XATTR_HMAC) {
> > > > @@ -118,13 +118,18 @@ static struct shash_desc *init_desc(char type,
> > > > uint8_t hash_algo)
> > > > alloc:
> > > > desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> > > > GFP_KERNEL);
> > > > - if (!desc)
> > > > + if (!desc) {
> > > > + if (tmp_tfm)
> > > > + crypto_free_shash(tmp_tfm);
> > > > return ERR_PTR(-ENOMEM);
> > > > + }
> > > >
> > > > desc->tfm = *tfm;
> > > >
> > > > rc = crypto_shash_init(desc);
> > > > if (rc) {
> > > > + if (tmp_tfm)
> > > > + crypto_free_shash(tmp_tfm);
> > > > kfree(desc);
> > > > return ERR_PTR(rc);
> > > > }
> > >
> > > There's no need to check for NULL before calling crypto_free_shash().
> > >
> >
> > I find there is a crypto_shash_tfm() in the definition of
> > crypto_free_shash(). Will this lead to null pointer dereference
> > when we use it to free a NULL pointer?
> >
>
> No. It does >base, not tfm->base.
>
Thank you for your advice! I will resend a new patch soon.
Regards,
Dinghao
Re: Re: [PATCH] evm: Fix memleak in init_desc
> On Sat, Jan 09, 2021 at 07:33:05PM +0800, Dinghao Liu wrote:
> > When kmalloc() fails, tmp_tfm allocated by
> > crypto_alloc_shash() has not been freed, which
> > leads to memleak.
> >
> > Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
> > Signed-off-by: Dinghao Liu
> > ---
> > security/integrity/evm/evm_crypto.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_crypto.c
> > b/security/integrity/evm/evm_crypto.c
> > index 168c3b78ac47..39fb31a638ac 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type, uint8_t
> > hash_algo)
> > {
> > long rc;
> > const char *algo;
> > - struct crypto_shash **tfm, *tmp_tfm;
> > + struct crypto_shash **tfm, *tmp_tfm = NULL;
> > struct shash_desc *desc;
> >
> > if (type == EVM_XATTR_HMAC) {
> > @@ -118,13 +118,18 @@ static struct shash_desc *init_desc(char type,
> > uint8_t hash_algo)
> > alloc:
> > desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> > GFP_KERNEL);
> > - if (!desc)
> > + if (!desc) {
> > + if (tmp_tfm)
> > + crypto_free_shash(tmp_tfm);
> > return ERR_PTR(-ENOMEM);
> > + }
> >
> > desc->tfm = *tfm;
> >
> > rc = crypto_shash_init(desc);
> > if (rc) {
> > + if (tmp_tfm)
> > + crypto_free_shash(tmp_tfm);
> > kfree(desc);
> > return ERR_PTR(rc);
> > }
>
> There's no need to check for NULL before calling crypto_free_shash().
>
I find there is a crypto_shash_tfm() in the definition of
crypto_free_shash(). Will this lead to null pointer dereference
when we use it to free a NULL pointer?
Regards,
Dinghao
[PATCH] netfilter: Fix memleak in nf_nat_init
When register_pernet_subsys() fails, nf_nat_bysource
should be freed just like when nf_ct_extend_register()
fails.
Fixes: 1cd472bf036ca ("netfilter: nf_nat: add nat hook register functions to
nf_nat")
Signed-off-by: Dinghao Liu
---
net/netfilter/nf_nat_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index ea923f8cf9c4..b7c3c902290f 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -1174,6 +1174,7 @@ static int __init nf_nat_init(void)
ret = register_pernet_subsys(_net_ops);
if (ret < 0) {
nf_ct_extend_unregister(_extend);
+ kvfree(nf_nat_bysource);
return ret;
}
--
2.17.1
[PATCH] evm: Fix memleak in init_desc
When kmalloc() fails, tmp_tfm allocated by
crypto_alloc_shash() has not been freed, which
leads to memleak.
Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
Signed-off-by: Dinghao Liu
---
security/integrity/evm/evm_crypto.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/security/integrity/evm/evm_crypto.c
b/security/integrity/evm/evm_crypto.c
index 168c3b78ac47..39fb31a638ac 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type, uint8_t
hash_algo)
{
long rc;
const char *algo;
- struct crypto_shash **tfm, *tmp_tfm;
+ struct crypto_shash **tfm, *tmp_tfm = NULL;
struct shash_desc *desc;
if (type == EVM_XATTR_HMAC) {
@@ -118,13 +118,18 @@ static struct shash_desc *init_desc(char type, uint8_t
hash_algo)
alloc:
desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
GFP_KERNEL);
- if (!desc)
+ if (!desc) {
+ if (tmp_tfm)
+ crypto_free_shash(tmp_tfm);
return ERR_PTR(-ENOMEM);
+ }
desc->tfm = *tfm;
rc = crypto_shash_init(desc);
if (rc) {
+ if (tmp_tfm)
+ crypto_free_shash(tmp_tfm);
kfree(desc);
return ERR_PTR(rc);
}
--
2.17.1
Re: Re: [PATCH] media: v4l2: Fix memleak in videobuf_read_one
> On 05/01/2021 08:59, Dinghao Liu wrote:
> > When videobuf_waiton() fails, we should execute clean
> > functions to prevent memleak. It's the same when
> > __videobuf_copy_to_user() fails.
> >
> > Fixes: 7a7d9a89d0307 ("V4L/DVB (6251): Replace video-buf to a more generic
> > approach")
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/media/v4l2-core/videobuf-core.c | 12 ++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/videobuf-core.c
> > b/drivers/media/v4l2-core/videobuf-core.c
> > index 606a271bdd2d..0709b75d11cd 100644
> > --- a/drivers/media/v4l2-core/videobuf-core.c
> > +++ b/drivers/media/v4l2-core/videobuf-core.c
> > @@ -924,8 +924,12 @@ ssize_t videobuf_read_one(struct videobuf_queue *q,
> >
> > /* wait until capture is done */
> > retval = videobuf_waiton(q, q->read_buf, nonblocking, 1);
> > - if (0 != retval)
> > + if (retval != 0) {
> > + q->ops->buf_release(q, q->read_buf);
> > + kfree(q->read_buf);
> > + q->read_buf = NULL;
> > goto done;
> > + }
>
> I'm fairly certain that this is wrong: if waiton returns an error, then
> that means that the wait is either interrupted or that we are in non-blocking
> mode and no buffer has arrived yet. In that case you just go to done since
> there is nothing to clean up.
>
I found there was a similar error handling in videobuf_read_zerocopy(), where
q->read_buf was freed on failure of videobuf_waiton(), thus I reported this as
a memleak. Do you think the error handling in videobuf_read_zerocopy() is right?
> >
> > CALL(q, sync, q, q->read_buf);
> >
> > @@ -940,8 +944,12 @@ ssize_t videobuf_read_one(struct videobuf_queue *q,
> >
> > /* Copy to userspace */
> > retval = __videobuf_copy_to_user(q, q->read_buf, data, count,
> > nonblocking);
> > - if (retval < 0)
> > + if (retval < 0) {
> > + q->ops->buf_release(q, q->read_buf);
> > + kfree(q->read_buf);
> > + q->read_buf = NULL;
> > goto done;
>
> I'm not sure about this either: if userspace gave a crappy pointer and this
> copy_to_user fails, then that doesn't mean you should release the buffer.
> The next read() might have a valid pointer or, more likely, the application
> exits or crashes and everything is cleaned up when the filehandle is closed.
>
You are right. Let's keep this part as it was for security.
Regards,
Dinghao
Re: Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe
> On Wed, 6 Jan 2021 18:56:23 +0800 (GMT+08:00) dinghao@zju.edu.cn > wrote: > > > I used this one for a test: > > > > > > https://patchwork.kernel.org/project/netdevbpf/patch/1609312994-121032-1-git-send-email-abaci-bug...@linux.alibaba.com/ > > > > > > I'm not getting the Fixes tag when I download the mbox. > > > > It seems that automatically generating Fixes tags is a hard work. > > Both patches and bugs could be complex. Sometimes even human cannot > > determine which commit introduced a target bug. > > > > Is this an already implemented functionality? > > I'm not sure I understand. Indeed finding the right commit to use in > a Fixes tag is not always easy, and definitely not easy to automate. > Human validation is always required. > > If we could easily automate finding the commit which introduced a > problem we wouldn't need to add the explicit tag, backporters could > just run such script locally.. That's why it's best if the author > does the digging and provides the right tag. > > The conversation with Konstantin and Florian was about automatically > picking up Fixes tags from the mailing list by the patchwork software, > when such tags are posted in reply to the original posting, just like > review tags. But the tags are still generated by humans. It's clear to me, thanks. Regards, Dinghao
Re: Re: [PATCH] net: ethernet: Fix memleak in ethoc_probe
> On Mon, 28 Dec 2020 16:14:17 -0500 Konstantin Ryabitsev wrote: > > On Mon, Dec 28, 2020 at 01:05:26PM -0800, Florian Fainelli wrote: > > > On 12/28/2020 12:23 PM, Konstantin Ryabitsev wrote: > > > > On Thu, Dec 24, 2020 at 01:57:40PM -0800, Florian Fainelli wrote: > > > Konstantin, would you be willing to mod the kernel.org instance of > > > patchwork to populate Fixes tags in the generated mboxes? > > > >>> > > > >>> I'd really rather not -- we try not to diverge from project upstream > > > >>> if at all > > > >>> possible, as this dramatically complicates upgrades. > > > >> > > > >> Well that is really unfortunate then because the Linux developer > > > >> community settled on using the Fixes: tag for years now and having > > > >> patchwork automatically append those tags would greatly help > > > >> maintainers. > > > > > > > > I agree -- but this is something that needs to be implemented upstream. > > > > Picking up a one-off patch just for patchwork.kernel.org is not the > > > > right way > > > > to go about this. > > > > > > You should be able to tune this from the patchwork administrative > > > interface and add new tags there, would not that be acceptable? > > > > Oh, oops, I got confused by the mention of a rejected upstream patch -- I > > didn't realize that this is already possible with a configuration setting. > > > > Sure, I added a match for ^Fixes: -- let me know if it's not doing the right > > thing. > > I used this one for a test: > > https://patchwork.kernel.org/project/netdevbpf/patch/1609312994-121032-1-git-send-email-abaci-bug...@linux.alibaba.com/ > > I'm not getting the Fixes tag when I download the mbox. It seems that automatically generating Fixes tags is a hard work. Both patches and bugs could be complex. Sometimes even human cannot determine which commit introduced a target bug. Is this an already implemented functionality? Regards, Dinghao
[PATCH] media: v4l2: Fix memleak in videobuf_read_one
When videobuf_waiton() fails, we should execute clean
functions to prevent memleak. It's the same when
__videobuf_copy_to_user() fails.
Fixes: 7a7d9a89d0307 ("V4L/DVB (6251): Replace video-buf to a more generic
approach")
Signed-off-by: Dinghao Liu
---
drivers/media/v4l2-core/videobuf-core.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf-core.c
b/drivers/media/v4l2-core/videobuf-core.c
index 606a271bdd2d..0709b75d11cd 100644
--- a/drivers/media/v4l2-core/videobuf-core.c
+++ b/drivers/media/v4l2-core/videobuf-core.c
@@ -924,8 +924,12 @@ ssize_t videobuf_read_one(struct videobuf_queue *q,
/* wait until capture is done */
retval = videobuf_waiton(q, q->read_buf, nonblocking, 1);
- if (0 != retval)
+ if (retval != 0) {
+ q->ops->buf_release(q, q->read_buf);
+ kfree(q->read_buf);
+ q->read_buf = NULL;
goto done;
+ }
CALL(q, sync, q, q->read_buf);
@@ -940,8 +944,12 @@ ssize_t videobuf_read_one(struct videobuf_queue *q,
/* Copy to userspace */
retval = __videobuf_copy_to_user(q, q->read_buf, data, count,
nonblocking);
- if (retval < 0)
+ if (retval < 0) {
+ q->ops->buf_release(q, q->read_buf);
+ kfree(q->read_buf);
+ q->read_buf = NULL;
goto done;
+ }
q->read_off += retval;
if (q->read_off == q->read_buf->size) {
--
2.17.1
[PATCH] ubifs: Fix memleak in ubifs_init_authentication
When crypto_shash_digestsize() fails, c->hmac_tfm
has not been freed before returning, which leads
to memleak.
Fixes: 49525e5eecca5 ("ubifs: Add helper functions for authentication support")
Signed-off-by: Dinghao Liu
---
fs/ubifs/auth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ubifs/auth.c b/fs/ubifs/auth.c
index 51a7c8c2c3f0..e564d5ff8781 100644
--- a/fs/ubifs/auth.c
+++ b/fs/ubifs/auth.c
@@ -327,7 +327,7 @@ int ubifs_init_authentication(struct ubifs_info *c)
ubifs_err(c, "hmac %s is bigger than maximum allowed hmac size
(%d > %d)",
hmac_name, c->hmac_desc_len, UBIFS_HMAC_ARR_SZ);
err = -EINVAL;
- goto out_free_hash;
+ goto out_free_hmac;
}
err = crypto_shash_setkey(c->hmac_tfm, ukp->data, ukp->datalen);
--
2.17.1
[PATCH] [v2] iommu/intel: Fix memleak in intel_irq_remapping_alloc
When irq_domain_get_irq_data() or irqd_cfg() fails
at i == 0, data allocated by kzalloc() has not been
freed before returning, which leads to memleak.
Fixes: b106ee63abccb ("irq_remapping/vt-d: Enhance Intel IR driver to support
hierarchical irqdomains")
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Add a check against i instead of setting data to NULL.
---
drivers/iommu/intel/irq_remapping.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/iommu/intel/irq_remapping.c
b/drivers/iommu/intel/irq_remapping.c
index aeffda92b10b..685200a5cff0 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -1353,6 +1353,8 @@ static int intel_irq_remapping_alloc(struct irq_domain
*domain,
irq_data = irq_domain_get_irq_data(domain, virq + i);
irq_cfg = irqd_cfg(irq_data);
if (!irq_data || !irq_cfg) {
+ if (!i)
+ kfree(data);
ret = -EINVAL;
goto out_free_data;
}
--
2.17.1
Re: Re: [PATCH] iommu/intel: Fix memleak in intel_irq_remapping_alloc
> On 1/3/21 2:22 PM, dinghao@zju.edu.cn wrote:
> >> On 2021/1/3 12:08, dinghao@zju.edu.cn wrote:
> >>>> Hi,
> >>>>
> >>>> On 2021/1/2 17:50, Dinghao Liu wrote:
> >>>>> When irq_domain_get_irq_data() or irqd_cfg() fails
> >>>>> meanwhile i == 0, data allocated by kzalloc() has not
> >>>>> been freed before returning, which leads to memleak.
> >>>>>
> >>>>> Fixes: b106ee63abccb ("irq_remapping/vt-d: Enhance Intel IR driver to
> >>>>> support hierarchical irqdomains")
> >>>>> Signed-off-by: Dinghao Liu
> >>>>> ---
> >>>>> drivers/iommu/intel/irq_remapping.c | 2 ++
> >>>>> 1 file changed, 2 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/iommu/intel/irq_remapping.c
> >>>>> b/drivers/iommu/intel/irq_remapping.c
> >>>>> index aeffda92b10b..cdaeed36750f 100644
> >>>>> --- a/drivers/iommu/intel/irq_remapping.c
> >>>>> +++ b/drivers/iommu/intel/irq_remapping.c
> >>>>> @@ -1354,6 +1354,8 @@ static int intel_irq_remapping_alloc(struct
> >>>>> irq_domain *domain,
> >>>>> irq_cfg = irqd_cfg(irq_data);
> >>>>> if (!irq_data || !irq_cfg) {
> >>>>> ret = -EINVAL;
> >>>>> + kfree(data);
> >>>>> + data = NULL;
> >>>>
> >>>> Do you need to check (i == 0) here? @data will not be used anymore as it
> >>>> goes to out branch, why setting it to NULL here?
> >>>>
> >>>
> >>> data will be passed to ire_data->chip_data when i == 0 and
> >>> intel_free_irq_resources() will free it on failure. Thus I
> >>
> >> Isn't it going to "goto out_free_data"? If "i == 0", the allocated @data
> >> won't be freed by intel_free_irq_resources(), hence memory leaking. Does
> >> this patch aim to fix this?
> >>
> >> Best regards,
> >> baolu
> >>
> >
> > Correct, this is what I mean. When i > 0, data has been passed to
> > irq_data->chip_data, which will be freed in intel_free_irq_resources()
> > on failure. So there is no memleak in this case. The memleak only occurs
> > on failure when i == 0 (data has not been passed to irq_data->chip_data).
>
> So how about
>
> diff --git a/drivers/iommu/intel/irq_remapping.c
> b/drivers/iommu/intel/irq_remapping.c
> index aeffda92b10b..685200a5cff0 100644
> --- a/drivers/iommu/intel/irq_remapping.c
> +++ b/drivers/iommu/intel/irq_remapping.c
> @@ -1353,6 +1353,8 @@ static int intel_irq_remapping_alloc(struct
> irq_domain *domain,
> irq_data = irq_domain_get_irq_data(domain, virq + i);
> irq_cfg = irqd_cfg(irq_data);
> if (!irq_data || !irq_cfg) {
> + if (!i)
> + kfree(data);
> ret = -EINVAL;
> goto out_free_data;
> }
>
> > I set data to NULL after kfree() in this patch to prevent double-free
> > when the failure occurs at i > 0.
>
> if i>0, @data has been passed and will be freed by
> intel_free_irq_resources() on the failure path. No need to free or
> clear, right?
Right, this is clearer. Thank you for your advice and I will resend a
new patch soon.
Regards,
Dinghao
>
> Best regards,
> baolu
>
> >
> > Regards,
> > Dinghao
> >
> >>> set it to NULL to prevent double-free. However, if we add
> >>> a check (i == 0) here, we will not need to set it to NULL.
> >>> If this is better, I will resend a new patch soon.
> >>>
> >>> Regards,
> >>> Dinghao
> >>>
Re: Re: [Intel-wired-lan] [PATCH] net: ixgbe: Fix memleak in ixgbe_configure_clsu32
> Dear Dinghao, > > > Am 03.01.21 um 09:08 schrieb Dinghao Liu: > > When ixgbe_fdir_write_perfect_filter_82599() fails, > > input allocated by kzalloc() has not been freed, > > which leads to memleak. > > Nice find. Thank you for your patches. Out of curiosity, do you use an > analysis tool to find these issues? > Yes, these bugs are suggested by my static analysis tool. > > Signed-off-by: Dinghao Liu > > --- > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 -- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > index 393d1c2cd853..e9c2d28efc81 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > @@ -9582,8 +9582,10 @@ static int ixgbe_configure_clsu32(struct > > ixgbe_adapter *adapter, > > ixgbe_atr_compute_perfect_hash_82599(>filter, mask); > > err = ixgbe_fdir_write_perfect_filter_82599(hw, >filter, > > input->sw_idx, queue); > > - if (!err) > > - ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); > > + if (err) > > + goto err_out_w_lock; > > + > > + ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); > > spin_unlock(>fdir_perfect_lock); > > > > if ((uhtid != 0x800) && (adapter->jump_tables[uhtid])) > > Reviewed-by: Paul Menzel > > I wonder, in the non-error case, how `input` and `jump` are freed. > I'm not sure if kfree(jump) will introduce bug. jump is allocated in a for loop and has been passed to adapter->jump_tables. input and mask have new definitions (kzalloc) after this loop, it's reasonable to free them on failure. But jump is different. Maybe we should not free jump after the loop? > Old code: > > > if (!err) > > ixgbe_update_ethtool_fdir_entry(adapter, input, > > input->sw_idx); > > spin_unlock(>fdir_perfect_lock); > > > > if ((uhtid != 0x800) && (adapter->jump_tables[uhtid])) > > set_bit(loc - 1, > > (adapter->jump_tables[uhtid])->child_loc_map); > > > > kfree(mask); > > return err; > > Should these two lines be replaced with `goto err_out`? (Though the name > is confusing then, as it’s the non-error case.) > This also makes me confused. It seems that the check against untied is not error handling code, but there is a kfree(mask) after it. Freeing allocated data on success is not reasonable. Regards, Dinghao > > err_out_w_lock: > > spin_unlock(>fdir_perfect_lock); > > err_out: > > kfree(mask); > > free_input: > > kfree(input); > > free_jump: > > kfree(jump); > > return err; > > } > > Kind regards, > > Paul
[PATCH] net: ixgbe: Fix memleak in ixgbe_configure_clsu32
When ixgbe_fdir_write_perfect_filter_82599() fails, input allocated by kzalloc() has not been freed, which leads to memleak. Signed-off-by: Dinghao Liu --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 393d1c2cd853..e9c2d28efc81 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -9582,8 +9582,10 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter, ixgbe_atr_compute_perfect_hash_82599(>filter, mask); err = ixgbe_fdir_write_perfect_filter_82599(hw, >filter, input->sw_idx, queue); - if (!err) - ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); + if (err) + goto err_out_w_lock; + + ixgbe_update_ethtool_fdir_entry(adapter, input, input->sw_idx); spin_unlock(>fdir_perfect_lock); if ((uhtid != 0x800) && (adapter->jump_tables[uhtid])) -- 2.17.1
Re: Re: [PATCH] iommu/intel: Fix memleak in intel_irq_remapping_alloc
> On 2021/1/3 12:08, dinghao@zju.edu.cn wrote:
> >> Hi,
> >>
> >> On 2021/1/2 17:50, Dinghao Liu wrote:
> >>> When irq_domain_get_irq_data() or irqd_cfg() fails
> >>> meanwhile i == 0, data allocated by kzalloc() has not
> >>> been freed before returning, which leads to memleak.
> >>>
> >>> Fixes: b106ee63abccb ("irq_remapping/vt-d: Enhance Intel IR driver to
> >>> support hierarchical irqdomains")
> >>> Signed-off-by: Dinghao Liu
> >>> ---
> >>>drivers/iommu/intel/irq_remapping.c | 2 ++
> >>>1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/iommu/intel/irq_remapping.c
> >>> b/drivers/iommu/intel/irq_remapping.c
> >>> index aeffda92b10b..cdaeed36750f 100644
> >>> --- a/drivers/iommu/intel/irq_remapping.c
> >>> +++ b/drivers/iommu/intel/irq_remapping.c
> >>> @@ -1354,6 +1354,8 @@ static int intel_irq_remapping_alloc(struct
> >>> irq_domain *domain,
> >>> irq_cfg = irqd_cfg(irq_data);
> >>> if (!irq_data || !irq_cfg) {
> >>> ret = -EINVAL;
> >>> + kfree(data);
> >>> + data = NULL;
> >>
> >> Do you need to check (i == 0) here? @data will not be used anymore as it
> >> goes to out branch, why setting it to NULL here?
> >>
> >
> > data will be passed to ire_data->chip_data when i == 0 and
> > intel_free_irq_resources() will free it on failure. Thus I
>
> Isn't it going to "goto out_free_data"? If "i == 0", the allocated @data
> won't be freed by intel_free_irq_resources(), hence memory leaking. Does
> this patch aim to fix this?
>
> Best regards,
> baolu
>
Correct, this is what I mean. When i > 0, data has been passed to
irq_data->chip_data, which will be freed in intel_free_irq_resources()
on failure. So there is no memleak in this case. The memleak only occurs
on failure when i == 0 (data has not been passed to irq_data->chip_data).
I set data to NULL after kfree() in this patch to prevent double-free
when the failure occurs at i > 0.
Regards,
Dinghao
> > set it to NULL to prevent double-free. However, if we add
> > a check (i == 0) here, we will not need to set it to NULL.
> > If this is better, I will resend a new patch soon.
> >
> > Regards,
> > Dinghao
> >
Re: Re: [PATCH] iommu/intel: Fix memleak in intel_irq_remapping_alloc
> Hi,
>
> On 2021/1/2 17:50, Dinghao Liu wrote:
> > When irq_domain_get_irq_data() or irqd_cfg() fails
> > meanwhile i == 0, data allocated by kzalloc() has not
> > been freed before returning, which leads to memleak.
> >
> > Fixes: b106ee63abccb ("irq_remapping/vt-d: Enhance Intel IR driver to
> > support hierarchical irqdomains")
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/iommu/intel/irq_remapping.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/iommu/intel/irq_remapping.c
> > b/drivers/iommu/intel/irq_remapping.c
> > index aeffda92b10b..cdaeed36750f 100644
> > --- a/drivers/iommu/intel/irq_remapping.c
> > +++ b/drivers/iommu/intel/irq_remapping.c
> > @@ -1354,6 +1354,8 @@ static int intel_irq_remapping_alloc(struct
> > irq_domain *domain,
> > irq_cfg = irqd_cfg(irq_data);
> > if (!irq_data || !irq_cfg) {
> > ret = -EINVAL;
> > + kfree(data);
> > + data = NULL;
>
> Do you need to check (i == 0) here? @data will not be used anymore as it
> goes to out branch, why setting it to NULL here?
>
data will be passed to ire_data->chip_data when i == 0 and
intel_free_irq_resources() will free it on failure. Thus I
set it to NULL to prevent double-free. However, if we add
a check (i == 0) here, we will not need to set it to NULL.
If this is better, I will resend a new patch soon.
Regards,
Dinghao
[PATCH] iommu/intel: Fix memleak in intel_irq_remapping_alloc
When irq_domain_get_irq_data() or irqd_cfg() fails
meanwhile i == 0, data allocated by kzalloc() has not
been freed before returning, which leads to memleak.
Fixes: b106ee63abccb ("irq_remapping/vt-d: Enhance Intel IR driver to support
hierarchical irqdomains")
Signed-off-by: Dinghao Liu
---
drivers/iommu/intel/irq_remapping.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/iommu/intel/irq_remapping.c
b/drivers/iommu/intel/irq_remapping.c
index aeffda92b10b..cdaeed36750f 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -1354,6 +1354,8 @@ static int intel_irq_remapping_alloc(struct irq_domain
*domain,
irq_cfg = irqd_cfg(irq_data);
if (!irq_data || !irq_cfg) {
ret = -EINVAL;
+ kfree(data);
+ data = NULL;
goto out_free_data;
}
--
2.17.1
