| Message ID | 739e6aaf-a634-4e33-98a8-16546379ec9f@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/damon/sysfs: eliminate potential uninitialized variable warning | expand |
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 >
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
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
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. regards, deepak. > >
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. > > > > > >
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
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);
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(-)