* [PATCH][next] erofs: fix uninitialized variable i used in a while-loop @ 2021-04-06 16:27 Colin King 2021-04-06 23:54 ` Gao Xiang 0 siblings, 1 reply; 4+ messages in thread From: Colin King @ 2021-04-06 16:27 UTC (permalink / raw) To: Gao Xiang, Chao Yu, linux-erofs; +Cc: kernel-janitors, linux-kernel From: Colin Ian King <colin.king@canonical.com> The while-loop iterates until src is non-null or i is 3, however, the loop counter i is not intinitialied to zero, causing incorrect iteration counts. Fix this by initializing it to zero. Addresses-Coverity: ("Uninitialized scalar variable") Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4 backend") Signed-off-by: Colin Ian King <colin.king@canonical.com> --- fs/erofs/decompressor.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c index fe46a9c34923..8687ff81406b 100644 --- a/fs/erofs/decompressor.c +++ b/fs/erofs/decompressor.c @@ -154,6 +154,7 @@ static void *z_erofs_handle_inplace_io(struct z_erofs_decompress_req *rq, } kunmap_atomic(inpage); might_sleep(); + i = 0; while (1) { src = vm_map_ram(rq->in, nrpages_in, -1); /* retry two more times (totally 3 times) */ -- 2.30.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop 2021-04-06 16:27 [PATCH][next] erofs: fix uninitialized variable i used in a while-loop Colin King @ 2021-04-06 23:54 ` Gao Xiang 2021-04-07 3:38 ` Joe Perches 0 siblings, 1 reply; 4+ messages in thread From: Gao Xiang @ 2021-04-06 23:54 UTC (permalink / raw) To: Colin King; +Cc: kernel-janitors, Gao Xiang, linux-erofs, linux-kernel Hi Colin, On Tue, Apr 06, 2021 at 05:27:18PM +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > The while-loop iterates until src is non-null or i is 3, however, the > loop counter i is not intinitialied to zero, causing incorrect iteration > counts. Fix this by initializing it to zero. > > Addresses-Coverity: ("Uninitialized scalar variable") > Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4 backend") > Signed-off-by: Colin Ian King <colin.king@canonical.com> Thank you very much for catching this! It looks good to me, Reviewed-by: Gao Xiang <hsiangkao@redhat.com> (btw, may I fold this into the original patchset? since such big pcluster patchset is just applied to for-next for further integration testing, and the commit id is not stable yet..) Thanks, Gao Xiang > --- > fs/erofs/decompressor.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c > index fe46a9c34923..8687ff81406b 100644 > --- a/fs/erofs/decompressor.c > +++ b/fs/erofs/decompressor.c > @@ -154,6 +154,7 @@ static void *z_erofs_handle_inplace_io(struct z_erofs_decompress_req *rq, > } > kunmap_atomic(inpage); > might_sleep(); > + i = 0; > while (1) { > src = vm_map_ram(rq->in, nrpages_in, -1); > /* retry two more times (totally 3 times) */ > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop 2021-04-06 23:54 ` Gao Xiang @ 2021-04-07 3:38 ` Joe Perches 2021-04-07 3:51 ` Gao Xiang 0 siblings, 1 reply; 4+ messages in thread From: Joe Perches @ 2021-04-07 3:38 UTC (permalink / raw) To: Gao Xiang, Colin King Cc: kernel-janitors, Gao Xiang, linux-erofs, linux-kernel On Wed, 2021-04-07 at 07:54 +0800, Gao Xiang wrote: > Hi Colin, > > On Tue, Apr 06, 2021 at 05:27:18PM +0100, Colin King wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > The while-loop iterates until src is non-null or i is 3, however, the > > loop counter i is not intinitialied to zero, causing incorrect iteration > > counts. Fix this by initializing it to zero. > > > > Addresses-Coverity: ("Uninitialized scalar variable") > > Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4 backend") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > Thank you very much for catching this! It looks good to me, > Reviewed-by: Gao Xiang <hsiangkao@redhat.com> > > (btw, may I fold this into the original patchset? since such big pcluster > patchset is just applied to for-next for further integration testing, and > the commit id is not stable yet..) > > Thanks, > Gao Xiang I think this code is odd and would be more intelligible using a for loop like: --- fs/erofs/decompressor.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c index 27aa6a99b371..5a64f4649414 100644 --- a/fs/erofs/decompressor.c +++ b/fs/erofs/decompressor.c @@ -286,28 +286,24 @@ static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq, } ret = alg->prepare_destpages(rq, pagepool); - if (ret < 0) { + if (ret < 0) return ret; - } else if (ret) { + if (ret) { dst = page_address(*rq->out); dst_maptype = 1; goto dstmap_out; } - i = 0; - while (1) { + for (i = 0; i < 3; i++) { dst = vm_map_ram(rq->out, nrpages_out, -1); - + if (dst) { + dst_maptype = 2; + goto dstmap_out; + } /* retry two more times (totally 3 times) */ - if (dst || ++i >= 3) - break; vm_unmap_aliases(); } - - if (!dst) - return -ENOMEM; - - dst_maptype = 2; + return -ENOMEM; dstmap_out: ret = alg->decompress(rq, dst + rq->pageofs_out); ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop 2021-04-07 3:38 ` Joe Perches @ 2021-04-07 3:51 ` Gao Xiang 0 siblings, 0 replies; 4+ messages in thread From: Gao Xiang @ 2021-04-07 3:51 UTC (permalink / raw) To: Joe Perches Cc: kernel-janitors, linux-kernel, Colin King, Gao Xiang, linux-erofs Hi Joe, On Tue, Apr 06, 2021 at 08:38:44PM -0700, Joe Perches wrote: > On Wed, 2021-04-07 at 07:54 +0800, Gao Xiang wrote: > > Hi Colin, > > > > On Tue, Apr 06, 2021 at 05:27:18PM +0100, Colin King wrote: > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > The while-loop iterates until src is non-null or i is 3, however, the > > > loop counter i is not intinitialied to zero, causing incorrect iteration > > > counts. Fix this by initializing it to zero. > > > > > > Addresses-Coverity: ("Uninitialized scalar variable") > > > Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4 backend") > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > > Thank you very much for catching this! It looks good to me, > > Reviewed-by: Gao Xiang <hsiangkao@redhat.com> > > > > (btw, may I fold this into the original patchset? since such big pcluster > > patchset is just applied to for-next for further integration testing, and > > the commit id is not stable yet..) > > > > Thanks, > > Gao Xiang > > I think this code is odd and would be more intelligible using > a for loop like: Thanks for your reply/suggestion. > --- > fs/erofs/decompressor.c | 20 ++++++++------------ > 1 file changed, 8 insertions(+), 12 deletions(-) > > diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c > index 27aa6a99b371..5a64f4649414 100644 > --- a/fs/erofs/decompressor.c > +++ b/fs/erofs/decompressor.c > @@ -286,28 +286,24 @@ static int z_erofs_decompress_generic(struct z_erofs_decompress_req *rq, > } > > ret = alg->prepare_destpages(rq, pagepool); > - if (ret < 0) { > + if (ret < 0) > return ret; > - } else if (ret) { > + if (ret) { > dst = page_address(*rq->out); > dst_maptype = 1; > goto dstmap_out; > } I agree with the modification here, thanks! > > - i = 0; > - while (1) { > + for (i = 0; i < 3; i++) { > dst = vm_map_ram(rq->out, nrpages_out, -1); > - > + if (dst) { > + dst_maptype = 2; > + goto dstmap_out; > + } > /* retry two more times (totally 3 times) */ > - if (dst || ++i >= 3) > - break; > vm_unmap_aliases(); That is not quite equivalent, since after trying more than 3 times, (I think) no need to do the final vm_unmap_aliases(), since it's only used for the next vm_map_ram(). Similar logic also see: fs/xfs/xfs_buf.c: _xfs_buf_map_pages(): /* * vm_map_ram() will allocate auxiliary structures (e.g. * pagetables) with GFP_KERNEL, yet we are likely to be under * GFP_NOFS context here. Hence we need to tell memory reclaim * that we are in such a context via PF_MEMALLOC_NOFS to prevent * memory reclaim re-entering the filesystem here and * potentially deadlocking. */ nofs_flag = memalloc_nofs_save(); do { bp->b_addr = vm_map_ram(bp->b_pages, bp->b_page_count, -1); if (bp->b_addr) break; vm_unmap_aliases(); } while (retried++ <= 1); memalloc_nofs_restore(nofs_flag); if (!bp->b_addr) return -ENOMEM; but yeah with some modification (and extra vm_unmap_aliases() here as well...) Thanks, Gao Xiang ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-07 3:51 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-06 16:27 [PATCH][next] erofs: fix uninitialized variable i used in a while-loop Colin King 2021-04-06 23:54 ` Gao Xiang 2021-04-07 3:38 ` Joe Perches 2021-04-07 3:51 ` Gao Xiang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).