[PATCH][next] erofs: fix uninitialized variable i used in a while-loop

[PATCH][next] erofs: fix uninitialized variable i used in a while-loop

[Colin King]

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

Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop

[Gao Xiang]

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
> 

Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop

[Gao Xiang]

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
> 

Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop

[Joe Perches]

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);

Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop

[Joe Perches]

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);

Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop

[Gao Xiang]

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

Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop

[Gao Xiang]

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