On Thu, Jun 1, 2023 at 11:18 AM Nathan Chancellor <nathan@xxxxxxxxxx> wrote:
>
> On Thu, Jun 01, 2023 at 11:09:48AM -0700, Yosry Ahmed wrote:
> > On Thu, Jun 1, 2023 at 8:26 AM Nathan Chancellor <nathan@xxxxxxxxxx> wrote:
> > >
> > > Clang warns:
> > >
> > > mm/zswap.c:1183:6: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
> > > 1183 | if (objcg && !obj_cgroup_may_zswap(objcg))
> > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > mm/zswap.c:1327:9: note: uninitialized use occurs here
> > > 1327 | return ret;
> > > | ^~~
> > > mm/zswap.c:1183:2: note: remove the 'if' if its condition is always false
> > > 1183 | if (objcg && !obj_cgroup_may_zswap(objcg))
> > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 1184 | goto reject;
> > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > mm/zswap.c:1158:9: note: initialize the variable 'ret' to silence this warning
> > > 1158 | int ret;
> > > | ^
> > > | = 0
> > > 1 error generated.
> > >
> > > Set ret to -EINVAL, which seems to best match the situation the comment
> > > mentions and matches an earlier bailout due to an unsupported type or
> > > situation.
> >
> > Shouldn't this be -ENOMEM? I think this was the error code returned in
> > this situation before 6804144bf1cf.
>
> Sure, I can update this to be -ENOMEM if that is appropriate. I am not
> sure it actually matters though, since as far as I can tell, this
> function is only called via the ->store() call in __frontswap_store(),
> which is only called by frontswap_store(), which in turn is only called by
> swap_writepage() and its return value is only checked against 0, no
> other error values. I could be missing a macro call though.
>
> If I should send a v2, please let me know.
You are right that no one actually checks the error code now, but it
would be nice to stay consistent, just in case we want to do proper
error handling at some point :)
>
> Cheers,
> Nathan
>
> > > Fixes: 6804144bf1cf ("zswap: do not shrink if cgroup may not zswap")
> > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > Closes: https://lore.kernel.org/202306011435.2BxsHFUE-lkp@xxxxxxxxx/
> > > Closes: https://lore.kernel.org/202306012152.o4FL7Y6J-lkp@xxxxxxxxx/
> > > Closes: https://github.com/ClangBuiltLinux/linux/issues/1861
> > > Signed-off-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> > > ---
> > > mm/zswap.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index cff93643a6ab..e8313e2f5807 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -1180,8 +1180,10 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> > > * local cgroup limits.
> > > */
> > > objcg = get_obj_cgroup_from_page(page);
> > > - if (objcg && !obj_cgroup_may_zswap(objcg))
> > > + if (objcg && !obj_cgroup_may_zswap(objcg)) {
> > > + ret = -EINVAL;
> > > goto reject;
> > > + }
> > >
> > > /* reclaim space if needed */
> > > if (zswap_is_full()) {
> > >
> > > ---
> > > base-commit: b2424568bd9b947b2698b693ff24fec63bb4b36a
> > > change-id: 20230601-zswap-cgroup-wsometimes-uninitialized-b549ff4247ed
> > >
> > > Best regards,
> > > --
> > > Nathan Chancellor <nathan@xxxxxxxxxx>
> > >
> > >
