[PATCH] mm/damon/sysfs: eliminate potential uninitialized variable warning

[PATCH] mm/damon/sysfs: eliminate potential uninitialized variable warning

[Dan Carpenter]

The "err" variable is not initialized if damon_target_has_pid(ctx) is
false and sys_target->regions->nr is zero.

Fixes: 0bcd216c4741 ("mm/damon/sysfs: update monitoring target regions for online input commit")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 mm/damon/sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index e27846708b5a..1dfa96d4de99 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -1172,7 +1172,7 @@ static int damon_sysfs_update_target(struct damon_target *target,
 		struct damon_ctx *ctx,
 		struct damon_sysfs_target *sys_target)
 {
-	int err;
+	int err = 0;
 
 	if (damon_target_has_pid(ctx)) {
 		err = damon_sysfs_update_target_pid(target, sys_target->pid);
-- 
2.42.0

Re: [PATCH] mm/damon/sysfs: eliminate potential uninitialized variable warning

[SeongJae Park]

Hi Dan,

On Mon, 6 Nov 2023 17:07:40 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote:

> The "err" variable is not initialized if damon_target_has_pid(ctx) is
> false and sys_target->regions->nr is zero.

Thank you for fixing this.  I actually noticed this issue from Coverity Weekly
Scan of -next tree (CID 1570878), and was about to send very same fix, but you
were faster :)

> 
> Fixes: 0bcd216c4741 ("mm/damon/sysfs: update monitoring target regions for online input commit")

Cc stable?

> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Reviewed-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

> ---
>  mm/damon/sysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index e27846708b5a..1dfa96d4de99 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -1172,7 +1172,7 @@ static int damon_sysfs_update_target(struct damon_target *target,
>  		struct damon_ctx *ctx,
>  		struct damon_sysfs_target *sys_target)
>  {
> -	int err;
> +	int err = 0;
>  
>  	if (damon_target_has_pid(ctx)) {
>  		err = damon_sysfs_update_target_pid(target, sys_target->pid);
> -- 
> 2.42.0
> 

Re: [PATCH] mm/damon/sysfs: eliminate potential uninitialized variable warning

[Dan Carpenter]

On Mon, Nov 06, 2023 at 06:41:37AM -0800, SeongJae Park wrote:
> Hi Dan,
> 
> On Mon, 6 Nov 2023 17:07:40 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> > The "err" variable is not initialized if damon_target_has_pid(ctx) is
> > false and sys_target->regions->nr is zero.
> 
> Thank you for fixing this.  I actually noticed this issue from Coverity Weekly
> Scan of -next tree (CID 1570878), and was about to send very same fix, but you
> were faster :)
> 
> > 
> > Fixes: 0bcd216c4741 ("mm/damon/sysfs: update monitoring target regions for online input commit")
> 
> Cc stable?
> 

The fixes tag is from last week so I assumed it wasn't in stable yet.

Also to be honest, I wasn't totally positive this bug could occur in
real life.

regards,
dan carpenter

Re: [PATCH] mm/damon/sysfs: eliminate potential uninitialized variable warning

[SeongJae Park]

On Mon, 6 Nov 2023 19:23:47 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Mon, Nov 06, 2023 at 06:41:37AM -0800, SeongJae Park wrote:
> > Hi Dan,
> > 
> > On Mon, 6 Nov 2023 17:07:40 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > 
> > > The "err" variable is not initialized if damon_target_has_pid(ctx) is
> > > false and sys_target->regions->nr is zero.
> > 
> > Thank you for fixing this.  I actually noticed this issue from Coverity Weekly
> > Scan of -next tree (CID 1570878), and was about to send very same fix, but you
> > were faster :)
> > 
> > > 
> > > Fixes: 0bcd216c4741 ("mm/damon/sysfs: update monitoring target regions for online input commit")
> > 
> > Cc stable?
> > 
> 
> The fixes tag is from last week so I assumed it wasn't in stable yet.

You're right.  But the broken commit is a fix for stable trees, so I was
thinking this fix should also be merged into the stable kernel together.  I
think stable maintainers would do that on their own, but I wanted to make sure
by explicitly Cc-ing them.

> 
> Also to be honest, I wasn't totally positive this bug could occur in
> real life.

Again, you're right.  The uninitialized value can be returned to the caller,
but the caller of the function is not checking the returned value, so no real
problem can occur here.

But I think the unhandling of the return value is a bug itself, that not making
system crash like things but might make user confused, so I was preparing a
followup fix for that, too.  So, though the problem the fixes will fix are not
significant, I'd prefer merging this and the followup fix to stable trees if
ok, since those are anyway bug fix, and should be simple to apply.  If
something conflicts, I will provide backports.


Thanks,
SJ

> 
> regards,
> dan carpenter

Re: [PATCH] mm/damon/sysfs: eliminate potential uninitialized variable warning

[Dan Carpenter]

On Mon, Nov 06, 2023 at 04:52:05PM +0000, SeongJae Park wrote:
> But I think the unhandling of the return value is a bug itself, that not making
> system crash like things but might make user confused, so I was preparing a
> followup fix for that, too.  So, though the problem the fixes will fix are not
> significant, I'd prefer merging this and the followup fix to stable trees if
> ok, since those are anyway bug fix, and should be simple to apply.  If
> something conflicts, I will provide backports.

Sounds like a plan.  Thank you for doing this.  :)

regards,
dan carpenter

Re: [PATCH] mm/damon/sysfs: eliminate potential uninitialized variable warning

[SeongJae Park]

Hello Deepak,

On Tue, 7 Nov 2023 00:35:55 +0530 Deepak R Varma <drv@mailo.com> wrote:

> On Mon, Nov 06, 2023 at 05:07:40PM +0300, Dan Carpenter wrote:
> > The "err" variable is not initialized if damon_target_has_pid(ctx) is
> > false and sys_target->regions->nr is zero.
> >
> > Fixes: 0bcd216c4741 ("mm/damon/sysfs: update monitoring target regions for online input commit")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> Should this be a void function instead as the caller is not checking the return
> value.

As I mentioned on a previous reply[1], I think the caller should check the
return value.  I will send the fix for that soon.

[1] https://lore.kernel.org/damon/20231106165205.48264-1-sj@kernel.org/


Thanks,
SJ

> 
> regards,
> deepak.
> 
> >
> >
>