[PATCH 00/15] mdadm: fix coverity issues

[PATCH 00/15] mdadm: fix coverity issues

[Xiao Ni]

Hi all

This patch set fixes coverity issues.

Xiao Ni (15):
  mdadm/Manage: 01r1fail cases fails
  mdadm/Grow: fix coverity issue CHECKED_RETURN
  mdadm/Grow: fix coverity issue RESOURCE_LEAK
  mdadm/Grow: fix coverity issue STRING_OVERFLOW
  mdadm/Incremental: fix coverity issues.
  mdadm/mdmon: fix coverity issue CHECKED_RETURN
  mdadm/mdmon: fix coverity issue RESOURCE_LEAK
  mdadm/mdopen: fix coverity issue CHECKED_RETURN
  mdadm/mdopen: fix coverity issue STRING_OVERFLOW
  mdadm/mdstat: fix coverity issue CHECKED_RETURN
  mdadm/super0: fix coverity issue CHECKED_RETURN and EVALUATION_ORDER
  mdadm/super1: fix coverity issue CHECKED_RETURN
  mdadm/super1: fix coverity issue DEADCODE
  mdadm/super1: fix coverity issue EVALUATION_ORDER
  mdadm/super1: fix coverity issue RESOURCE_LEAK

 Grow.c        | 98 ++++++++++++++++++++++++++++++++++++++++-----------
 Incremental.c | 20 +++++------
 Manage.c      |  2 +-
 mdmon.c       | 11 ++++--
 mdopen.c      |  8 +++--
 mdstat.c      | 12 +++++--
 super0.c      | 10 ++++--
 super1.c      | 32 ++++++++++++-----
 8 files changed, 144 insertions(+), 49 deletions(-)

-- 
2.32.0 (Apple Git-132)

[PATCH 01/15] mdadm/Manage: 01r1fail cases fails

[Xiao Ni]

This patch fixes a regression issue which is checked by 01r1fail case.

Fixes: 1b4b73fd535a ('mdadm: Manage.c fix coverity issues')
Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Manage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Manage.c b/Manage.c
index aa5e80b2805d..f0304e1e4d3d 100644
--- a/Manage.c
+++ b/Manage.c
@@ -1333,7 +1333,7 @@ bool is_remove_safe(mdu_array_info_t *array, const int fd, char *devname, const
 
 	char *avail = xcalloc(array->raid_disks, sizeof(char));
 
-	for (disk = mdi->devs; disk; disk = mdi->next) {
+	for (disk = mdi->devs; disk; disk = disk->next) {
 		if (disk->disk.raid_disk < 0)
 			continue;
 		if (!(disk->disk.state & (1 << MD_DISK_SYNC)))
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 01/15] mdadm/Manage: 01r1fail cases fails

[Mariusz Tkaczyk]

On Mon, 15 Jul 2024 15:35:50 +0800
Xiao Ni <xni@redhat.com> wrote:

> This patch fixes a regression issue which is checked by 01r1fail case.
> 
> Fixes: 1b4b73fd535a ('mdadm: Manage.c fix coverity issues')
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Manage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Manage.c b/Manage.c
> index aa5e80b2805d..f0304e1e4d3d 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -1333,7 +1333,7 @@ bool is_remove_safe(mdu_array_info_t *array, const int
> fd, char *devname, const 
>  	char *avail = xcalloc(array->raid_disks, sizeof(char));
>  
> -	for (disk = mdi->devs; disk; disk = mdi->next) {
> +	for (disk = mdi->devs; disk; disk = disk->next) {
>  		if (disk->disk.raid_disk < 0)
>  			continue;
>  		if (!(disk->disk.state & (1 << MD_DISK_SYNC)))

Hi Xiao,
I didn't looked into your serie and I fixed it myself today.

I will skip this patch then.

Thanks,
Mariusz

[PATCH 02/15] mdadm/Grow: fix coverity issue CHECKED_RETURN

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/Grow.c b/Grow.c
index b135930d05b8..7ae967bda067 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3261,7 +3261,12 @@ static int reshape_array(char *container, int fd, char *devname,
 					/* This is a spare that wants to
 					 * be part of the array.
 					 */
-					add_disk(fd, st, info2, d);
+					if (add_disk(fd, st, info2, d) < 0) {
+						pr_err("Can not add disk %s\n",
+								d->sys_name);
+						free(info2);
+						goto release;
+					}
 				}
 			}
 			sysfs_free(info2);
@@ -4413,7 +4418,10 @@ static void validate(int afd, int bfd, unsigned long long offset)
 	 */
 	if (afd < 0)
 		return;
-	lseek64(bfd, offset - 4096, 0);
+	if (lseek64(bfd, offset - 4096, 0) < 0) {
+		pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));
+		return;
+	}
 	if (read(bfd, &bsb2, 512) != 512)
 		fail("cannot read bsb");
 	if (bsb2.sb_csum != bsb_csum((char*)&bsb2,
@@ -4444,12 +4452,19 @@ static void validate(int afd, int bfd, unsigned long long offset)
 			}
 		}
 
-		lseek64(bfd, offset, 0);
+		if (lseek64(bfd, offset, 0) < 0) {
+			pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));
+			goto out;
+		}
 		if ((unsigned long long)read(bfd, bbuf, len) != len) {
 			//printf("len %llu\n", len);
 			fail("read first backup failed");
 		}
-		lseek64(afd, __le64_to_cpu(bsb2.arraystart)*512, 0);
+
+		if (lseek64(afd, __le64_to_cpu(bsb2.arraystart)*512, 0) < 0) {
+			pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));
+			goto out;
+		}
 		if ((unsigned long long)read(afd, abuf, len) != len)
 			fail("read first from array failed");
 		if (memcmp(bbuf, abuf, len) != 0)
@@ -4466,15 +4481,25 @@ static void validate(int afd, int bfd, unsigned long long offset)
 			bbuf = xmalloc(abuflen);
 		}
 
-		lseek64(bfd, offset+__le64_to_cpu(bsb2.devstart2)*512, 0);
+		if (lseek64(bfd, offset+__le64_to_cpu(bsb2.devstart2)*512, 0) < 0) {
+			pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));
+			goto out;
+		}
 		if ((unsigned long long)read(bfd, bbuf, len) != len)
 			fail("read second backup failed");
-		lseek64(afd, __le64_to_cpu(bsb2.arraystart2)*512, 0);
+		if (lseek64(afd, __le64_to_cpu(bsb2.arraystart2)*512, 0) < 0) {
+			pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));
+			goto out;
+		}
 		if ((unsigned long long)read(afd, abuf, len) != len)
 			fail("read second from array failed");
 		if (memcmp(bbuf, abuf, len) != 0)
 			fail("data2 compare failed");
 	}
+out:
+	free(abuf);
+	free(bbuf);
+	return;
 }
 
 int child_monitor(int afd, struct mdinfo *sra, struct reshape *reshape,
@@ -5033,7 +5058,11 @@ int Grow_continue_command(char *devname, int fd, struct context *c)
 			goto Grow_continue_command_exit;
 		}
 		content = &array;
-		sysfs_init(content, fd, NULL);
+		if (sysfs_init(content, fd, NULL) < 0) {
+			pr_err("sysfs_init fails\n");
+			ret_val = 1;
+			goto Grow_continue_command_exit;
+		}
 		/* Need to load a superblock.
 		 * FIXME we should really get what we need from
 		 * sysfs
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 02/15] mdadm/Grow: fix coverity issue CHECKED_RETURN

[Mariusz Tkaczyk]

On Mon, 15 Jul 2024 15:35:51 +0800
Xiao Ni <xni@redhat.com> wrote:

> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Grow.c | 43 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index b135930d05b8..7ae967bda067 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -3261,7 +3261,12 @@ static int reshape_array(char *container, int fd, char
> *devname, /* This is a spare that wants to
>  					 * be part of the array.
>  					 */
> -					add_disk(fd, st, info2, d);
> +					if (add_disk(fd, st, info2, d) < 0) {
> +						pr_err("Can not add disk
> %s\n",
> +								d->sys_name);
> +						free(info2);
> +						goto release;
> +					}
>  				}
>  			}
>  			sysfs_free(info2);
> @@ -4413,7 +4418,10 @@ static void validate(int afd, int bfd, unsigned long
> long offset) */
>  	if (afd < 0)
>  		return;
> -	lseek64(bfd, offset - 4096, 0);
> +	if (lseek64(bfd, offset - 4096, 0) < 0) {
> +		pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));

You are using same error message in many places, shouldn't we propose something
like:

__off64_t lseek64_log_err (int __fd, __off64_t __offset, int __whence)
{
    __off64_t ret = lseek64(fd, __offset, __whence);
    if (ret < 0)
         pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));

    return ret;
    
}

lseek64 errors are unusual, they are exceptional, and I'm fine with logging
same error message but I would prefer to avoid repeating same message in code.
In case of debug, developer can do some backtracking, starting from this
function rather than hunt for the particular error message you used multiple
times.

> +		return;


> +	}
>  	if (read(bfd, &bsb2, 512) != 512)
>  		fail("cannot read bsb");
>  	if (bsb2.sb_csum != bsb_csum((char*)&bsb2,
> @@ -4444,12 +4452,19 @@ static void validate(int afd, int bfd, unsigned long
> long offset) }
>  		}
>  
> -		lseek64(bfd, offset, 0);
> +		if (lseek64(bfd, offset, 0) < 0) {
> +			pr_err("lseek64 fails %d:%s\n", errno,
> strerror(errno));

> +			goto out;
> +		}
>  		if ((unsigned long long)read(bfd, bbuf, len) != len) {
>  			//printf("len %llu\n", len);
>  			fail("read first backup failed");
>  		}
> -		lseek64(afd, __le64_to_cpu(bsb2.arraystart)*512, 0);
> +
> +		if (lseek64(afd, __le64_to_cpu(bsb2.arraystart)*512, 0) < 0)
> {
> +			pr_err("lseek64 fails %d:%s\n", errno,
> strerror(errno));
> +			goto out;
> +		}
>  		if ((unsigned long long)read(afd, abuf, len) != len)
>  			fail("read first from array failed");
>  		if (memcmp(bbuf, abuf, len) != 0)
> @@ -4466,15 +4481,25 @@ static void validate(int afd, int bfd, unsigned long
> long offset) bbuf = xmalloc(abuflen);
>  		}
>  
> -		lseek64(bfd, offset+__le64_to_cpu(bsb2.devstart2)*512, 0);
> +		if (lseek64(bfd, offset+__le64_to_cpu(bsb2.devstart2)*512,
> 0) < 0) {
> +			pr_err("lseek64 fails %d:%s\n", errno,
> strerror(errno));
> +			goto out;
> +		}
>  		if ((unsigned long long)read(bfd, bbuf, len) != len)
>  			fail("read second backup failed");
> -		lseek64(afd, __le64_to_cpu(bsb2.arraystart2)*512, 0);
> +		if (lseek64(afd, __le64_to_cpu(bsb2.arraystart2)*512, 0) <
> 0) {
> +			pr_err("lseek64 fails %d:%s\n", errno,
> strerror(errno));
> +			goto out;
> +		}
>  		if ((unsigned long long)read(afd, abuf, len) != len)
>  			fail("read second from array failed");
>  		if (memcmp(bbuf, abuf, len) != 0)
>  			fail("data2 compare failed");
>  	}
> +out:
> +	free(abuf);
> +	free(bbuf);
> +	return;
>  }
>  
>  int child_monitor(int afd, struct mdinfo *sra, struct reshape *reshape,
> @@ -5033,7 +5058,11 @@ int Grow_continue_command(char *devname, int fd,
> struct context *c) goto Grow_continue_command_exit;
>  		}
>  		content = &array;
> -		sysfs_init(content, fd, NULL);
> +		if (sysfs_init(content, fd, NULL) < 0) {
> +			pr_err("sysfs_init fails\n");

Better error message is:
pr_err("failed to initialize sysfs.\n");
or
pr_err("unable to initialize sysfs for %s\n",st->devnm);

It is more user friendly.

It is already used multiple times so perhaps we can consider similar approach
to proposed in for lseek, we can move move printing error to sysfs_init().

What do you think?

Thanks,
Mariusz

Re: [PATCH 02/15] mdadm/Grow: fix coverity issue CHECKED_RETURN

[Xiao Ni]

On Wed, Jul 17, 2024 at 5:33 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Mon, 15 Jul 2024 15:35:51 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  Grow.c | 43 ++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 36 insertions(+), 7 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index b135930d05b8..7ae967bda067 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -3261,7 +3261,12 @@ static int reshape_array(char *container, int fd, char
> > *devname, /* This is a spare that wants to
> >                                        * be part of the array.
> >                                        */
> > -                                     add_disk(fd, st, info2, d);
> > +                                     if (add_disk(fd, st, info2, d) < 0) {
> > +                                             pr_err("Can not add disk
> > %s\n",
> > +                                                             d->sys_name);
> > +                                             free(info2);
> > +                                             goto release;
> > +                                     }
> >                               }
> >                       }
> >                       sysfs_free(info2);
> > @@ -4413,7 +4418,10 @@ static void validate(int afd, int bfd, unsigned long
> > long offset) */
> >       if (afd < 0)
> >               return;
> > -     lseek64(bfd, offset - 4096, 0);
> > +     if (lseek64(bfd, offset - 4096, 0) < 0) {
> > +             pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));
>
> You are using same error message in many places, shouldn't we propose something
> like:
>
> __off64_t lseek64_log_err (int __fd, __off64_t __offset, int __whence)
> {
>     __off64_t ret = lseek64(fd, __offset, __whence);
>     if (ret < 0)
>          pr_err("lseek64 fails %d:%s\n", errno, strerror(errno));
>
>     return ret;
>
> }
>
> lseek64 errors are unusual, they are exceptional, and I'm fine with logging
> same error message but I would prefer to avoid repeating same message in code.
> In case of debug, developer can do some backtracking, starting from this
> function rather than hunt for the particular error message you used multiple
> times.

Hi Mariusz

If we use the above way, pr_err only prints the line of code in the
function lseek64_log_err. We can't know where the error is. pr_err
prints the function name and code line number. We put pr_err in the
place where lseek fails, we can know which function and which lseek
fails. It should be easy for debug. Is it right?
>
> > +             return;
>
>
> > +     }
> >       if (read(bfd, &bsb2, 512) != 512)
> >               fail("cannot read bsb");
> >       if (bsb2.sb_csum != bsb_csum((char*)&bsb2,
> > @@ -4444,12 +4452,19 @@ static void validate(int afd, int bfd, unsigned long
> > long offset) }
> >               }
> >
> > -             lseek64(bfd, offset, 0);
> > +             if (lseek64(bfd, offset, 0) < 0) {
> > +                     pr_err("lseek64 fails %d:%s\n", errno,
> > strerror(errno));
>
> > +                     goto out;
> > +             }
> >               if ((unsigned long long)read(bfd, bbuf, len) != len) {
> >                       //printf("len %llu\n", len);
> >                       fail("read first backup failed");
> >               }
> > -             lseek64(afd, __le64_to_cpu(bsb2.arraystart)*512, 0);
> > +
> > +             if (lseek64(afd, __le64_to_cpu(bsb2.arraystart)*512, 0) < 0)
> > {
> > +                     pr_err("lseek64 fails %d:%s\n", errno,
> > strerror(errno));
> > +                     goto out;
> > +             }
> >               if ((unsigned long long)read(afd, abuf, len) != len)
> >                       fail("read first from array failed");
> >               if (memcmp(bbuf, abuf, len) != 0)
> > @@ -4466,15 +4481,25 @@ static void validate(int afd, int bfd, unsigned long
> > long offset) bbuf = xmalloc(abuflen);
> >               }
> >
> > -             lseek64(bfd, offset+__le64_to_cpu(bsb2.devstart2)*512, 0);
> > +             if (lseek64(bfd, offset+__le64_to_cpu(bsb2.devstart2)*512,
> > 0) < 0) {
> > +                     pr_err("lseek64 fails %d:%s\n", errno,
> > strerror(errno));
> > +                     goto out;
> > +             }
> >               if ((unsigned long long)read(bfd, bbuf, len) != len)
> >                       fail("read second backup failed");
> > -             lseek64(afd, __le64_to_cpu(bsb2.arraystart2)*512, 0);
> > +             if (lseek64(afd, __le64_to_cpu(bsb2.arraystart2)*512, 0) <
> > 0) {
> > +                     pr_err("lseek64 fails %d:%s\n", errno,
> > strerror(errno));
> > +                     goto out;
> > +             }
> >               if ((unsigned long long)read(afd, abuf, len) != len)
> >                       fail("read second from array failed");
> >               if (memcmp(bbuf, abuf, len) != 0)
> >                       fail("data2 compare failed");
> >       }
> > +out:
> > +     free(abuf);
> > +     free(bbuf);
> > +     return;
> >  }
> >
> >  int child_monitor(int afd, struct mdinfo *sra, struct reshape *reshape,
> > @@ -5033,7 +5058,11 @@ int Grow_continue_command(char *devname, int fd,
> > struct context *c) goto Grow_continue_command_exit;
> >               }
> >               content = &array;
> > -             sysfs_init(content, fd, NULL);
> > +             if (sysfs_init(content, fd, NULL) < 0) {
> > +                     pr_err("sysfs_init fails\n");
>
> Better error message is:
> pr_err("failed to initialize sysfs.\n");
> or
> pr_err("unable to initialize sysfs for %s\n",st->devnm);
>
> It is more user friendly.

Yes, it makes sense.
>
> It is already used multiple times so perhaps we can consider similar approach
> to proposed in for lseek, we can move move printing error to sysfs_init().
>
> What do you think?

It depends on the above answer you'll give.

Best Regards
Xiao
>
> Thanks,
> Mariusz
>

Re: [PATCH 02/15] mdadm/Grow: fix coverity issue CHECKED_RETURN

[Mariusz Tkaczyk]

On Thu, 18 Jul 2024 10:29:50 +0800
Xiao Ni <xni@redhat.com> wrote:

> > lseek64 errors are unusual, they are exceptional, and I'm fine with logging
> > same error message but I would prefer to avoid repeating same message in
> > code. In case of debug, developer can do some backtracking, starting from
> > this function rather than hunt for the particular error message you used
> > multiple times.  
> 
> Hi Mariusz
> 
> If we use the above way, pr_err only prints the line of code in the
> function lseek64_log_err. We can't know where the error is. pr_err
> prints the function name and code line number. We put pr_err in the
> place where lseek fails, we can know which function and which lseek
> fails. It should be easy for debug. Is it right?
> >  

Oh right, I forgot about this from simple reason - I'm never using this.
Line is printed with make CXFLAGS=DDEBUG. If you will enable DEBUG flag then
mdadm will also spam with debug messages (prr_dbg messages). Anyway, you are
right it is an option.

Enabling DDEBUG in the past many times broke my time sensitive reproductions. I
don't like it.

From the top of my head, there is a rule "do not repeat same code". If we can
avoid repeating same error message by wrapping that to function then you will
satisfy my perfectionist sense. That's all.

I'm not going to force you to follow it - you proved that there is a way to
hunt the line by DDEBUG so I'm satisfied.

Thanks,
Mariusz

[PATCH 03/15] mdadm/Grow: fix coverity issue RESOURCE_LEAK

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 13 deletions(-)

diff --git a/Grow.c b/Grow.c
index 7ae967bda067..632be7db8d38 100644
--- a/Grow.c
+++ b/Grow.c
@@ -485,6 +485,7 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
 		int bitmap_fd;
 		int d;
 		int max_devs = st->max_devs;
+		int err = 0;
 
 		/* try to load a superblock */
 		for (d = 0; d < max_devs; d++) {
@@ -525,13 +526,14 @@ int Grow_addbitmap(char *devname, int fd, struct context *c, struct shape *s)
 			return 1;
 		}
 		if (ioctl(fd, SET_BITMAP_FILE, bitmap_fd) < 0) {
-			int err = errno;
+			err = errno;
 			if (errno == EBUSY)
 				pr_err("Cannot add bitmap while array is resyncing or reshaping etc.\n");
 			pr_err("Cannot set bitmap file for %s: %s\n",
 				devname, strerror(err));
-			return 1;
 		}
+		close(bitmap_fd);
+		return err;
 	}
 
 	return 0;
@@ -3083,6 +3085,7 @@ static int reshape_array(char *container, int fd, char *devname,
 	int done;
 	struct mdinfo *sra = NULL;
 	char buf[SYSFS_MAX_BUF_SIZE];
+	bool located_backup = false;
 
 	/* when reshaping a RAID0, the component_size might be zero.
 	 * So try to fix that up.
@@ -3165,8 +3168,10 @@ static int reshape_array(char *container, int fd, char *devname,
 			goto release;
 		}
 
-		if (!backup_file)
+		if (!backup_file) {
 			backup_file = locate_backup(sra->sys_name);
+			located_backup = true;
+		}
 
 		goto started;
 	}
@@ -3612,15 +3617,15 @@ started:
 			mdstat_wait(30 - (delayed-1) * 25);
 	} while (delayed);
 	mdstat_close();
-	if (check_env("MDADM_GROW_VERIFY"))
-		fd = open(devname, O_RDONLY | O_DIRECT);
-	else
-		fd = -1;
 	mlockall(MCL_FUTURE);
 
 	if (signal_s(SIGTERM, catch_term) == SIG_ERR)
 		goto release;
 
+	if (check_env("MDADM_GROW_VERIFY"))
+		fd = open(devname, O_RDONLY | O_DIRECT);
+	else
+		fd = -1;
 	if (st->ss->external) {
 		/* metadata handler takes it from here */
 		done = st->ss->manage_reshape(
@@ -3632,6 +3637,8 @@ started:
 			fd, sra, &reshape, st, blocks, fdlist, offsets,
 			d - odisks, fdlist + odisks, offsets + odisks);
 
+	if (fd >= 0)
+		close(fd);
 	free(fdlist);
 	free(offsets);
 
@@ -3701,6 +3708,8 @@ out:
 	exit(0);
 
 release:
+	if (located_backup)
+		free(backup_file);
 	free(fdlist);
 	free(offsets);
 	if (orig_level != UnSet && sra) {
@@ -3839,6 +3848,7 @@ int reshape_container(char *container, char *devname,
 			pr_err("Unable to initialize sysfs for %s\n",
 			       mdstat->devnm);
 			rv = 1;
+			close(fd);
 			break;
 		}
 
@@ -4717,6 +4727,7 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist,
 	unsigned long long *offsets;
 	unsigned long long  nstripe, ostripe;
 	int ndata, odata;
+	int fd, backup_fd = -1;
 
 	odata = info->array.raid_disks - info->delta_disks - 1;
 	if (info->array.level == 6)
@@ -4732,9 +4743,18 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist,
 		 * been used
 		 */
 		old_disks = cnt;
+
+	if (backup_file) {
+		backup_fd = open(backup_file, O_RDONLY);
+		if (backup_fd < 0) {
+			pr_err("Can't open backup file %s : %s\n",
+				backup_file, strerror(errno));
+			return -EINVAL;
+		}
+	}
+
 	for (i=old_disks-(backup_file?1:0); i<cnt; i++) {
 		struct mdinfo dinfo;
-		int fd;
 		int bsbsize;
 		char *devname, namebuf[20];
 		unsigned long long lo, hi;
@@ -4747,12 +4767,9 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist,
 		 * else restore data and update all superblocks
 		 */
 		if (i == old_disks-1) {
-			fd = open(backup_file, O_RDONLY);
-			if (fd<0) {
-				pr_err("backup file %s inaccessible: %s\n",
-					backup_file, strerror(errno));
+			if (backup_fd < 0)
 				continue;
-			}
+			fd = backup_fd;
 			devname = backup_file;
 		} else {
 			fd = fdlist[i];
@@ -4907,6 +4924,8 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist,
 				pr_err("Error restoring backup from %s\n",
 					devname);
 			free(offsets);
+			if (backup_fd >= 0)
+				close(backup_fd);
 			return 1;
 		}
 
@@ -4923,6 +4942,8 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist,
 				pr_err("Error restoring second backup from %s\n",
 					devname);
 			free(offsets);
+			if (backup_fd >= 0)
+				close(backup_fd);
 			return 1;
 		}
 
@@ -4984,8 +5005,14 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist,
 			st->ss->store_super(st, fdlist[j]);
 			st->ss->free_super(st);
 		}
+		if (backup_fd >= 0)
+			close(backup_fd);
 		return 0;
 	}
+
+	if (backup_fd >= 0)
+		close(backup_fd);
+
 	/* Didn't find any backup data, try to see if any
 	 * was needed.
 	 */
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 03/15] mdadm/Grow: fix coverity issue RESOURCE_LEAK

[Mariusz Tkaczyk]

On Mon, 15 Jul 2024 15:35:52 +0800
Xiao Ni <xni@redhat.com> wrote:

> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Grow.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 7ae967bda067..632be7db8d38 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -485,6 +485,7 @@ int Grow_addbitmap(char *devname, int fd, struct context
> *c, struct shape *s) int bitmap_fd;
>  		int d;
>  		int max_devs = st->max_devs;
> +		int err = 0;
>  
>  		/* try to load a superblock */
>  		for (d = 0; d < max_devs; d++) {
> @@ -525,13 +526,14 @@ int Grow_addbitmap(char *devname, int fd, struct
> context *c, struct shape *s) return 1;
>  		}
>  		if (ioctl(fd, SET_BITMAP_FILE, bitmap_fd) < 0) {
> -			int err = errno;
> +			err = errno;
>  			if (errno == EBUSY)
>  				pr_err("Cannot add bitmap while array is
> resyncing or reshaping etc.\n"); pr_err("Cannot set bitmap file for %s: %s\n",
>  				devname, strerror(err));
> -			return 1;
>  		}
> +		close(bitmap_fd);
> +		return err;

I don't think that we should return errno. I would say that mdadm should define
returned statues for functions, that is why I added mdadm_status_t.

Otherwise, same value may have two meanings. For example, in some cases we are
fine with particular error code from error might be misleading (it may match
allowed status).
This is not the case here, it is just an example.

I think that we should not return errno outside if not intended i.e. function is
projected to return errno in every case.

>  	}
>  
>  	return 0;
> @@ -3083,6 +3085,7 @@ static int reshape_array(char *container, int fd, char
> *devname, int done;
>  	struct mdinfo *sra = NULL;
>  	char buf[SYSFS_MAX_BUF_SIZE];
> +	bool located_backup = false;
>  
>  	/* when reshaping a RAID0, the component_size might be zero.
>  	 * So try to fix that up.
> @@ -3165,8 +3168,10 @@ static int reshape_array(char *container, int fd, char
> *devname, goto release;
>  		}
>  
> -		if (!backup_file)
> +		if (!backup_file) {
>  			backup_file = locate_backup(sra->sys_name);
> +			located_backup = true;
> +		}
>  
>  		goto started;
>  	}
> @@ -3612,15 +3617,15 @@ started:
>  			mdstat_wait(30 - (delayed-1) * 25);
>  	} while (delayed);
>  	mdstat_close();
> -	if (check_env("MDADM_GROW_VERIFY"))
> -		fd = open(devname, O_RDONLY | O_DIRECT);
> -	else
> -		fd = -1;
>  	mlockall(MCL_FUTURE);
>  
>  	if (signal_s(SIGTERM, catch_term) == SIG_ERR)
>  		goto release;
>  
> +	if (check_env("MDADM_GROW_VERIFY"))
> +		fd = open(devname, O_RDONLY | O_DIRECT);
> +	else
> +		fd = -1;

close_fd() is used unconditionally on fd few line earlier so it seems that else
path is not needed but this code is massive so please double check if I'm right.

We may call close_fd() on the closed resource and then it is not updated to -1,
assuming that close fails with EBADF.

Right way to fix it is to replace all close(fd) by close_fd(fd). It will give is
credibility that double close is not possible.


>  	if (st->ss->external) {
>  		/* metadata handler takes it from here */
>  		done = st->ss->manage_reshape(
> @@ -3632,6 +3637,8 @@ started:
>  			fd, sra, &reshape, st, blocks, fdlist, offsets,
>  			d - odisks, fdlist + odisks, offsets + odisks);
>  
> +	if (fd >= 0)
> +		close(fd);
>  	free(fdlist);
>  	free(offsets);
>  
> @@ -3701,6 +3708,8 @@ out:
>  	exit(0);
>  
>  release:
> +	if (located_backup)
> +		free(backup_file);

>  	free(fdlist);
>  	free(offsets);
>  	if (orig_level != UnSet && sra) {
> @@ -3839,6 +3848,7 @@ int reshape_container(char *container, char *devname,
>  			pr_err("Unable to initialize sysfs for %s\n",
>  			       mdstat->devnm);
>  			rv = 1;
> +			close(fd);
>  			break;
>  		}
>  
> @@ -4717,6 +4727,7 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist, unsigned long long *offsets;
>  	unsigned long long  nstripe, ostripe;
>  	int ndata, odata;
> +	int fd, backup_fd = -1;
>  
>  	odata = info->array.raid_disks - info->delta_disks - 1;
>  	if (info->array.level == 6)
> @@ -4732,9 +4743,18 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist,
>  		 * been used
>  		 */
>  		old_disks = cnt;
> +
> +	if (backup_file) {
> +		backup_fd = open(backup_file, O_RDONLY);
> +		if (backup_fd < 0) {
> +			pr_err("Can't open backup file %s : %s\n",
> +				backup_file, strerror(errno));
> +			return -EINVAL;
> +		}
> +	}
> +
>  	for (i=old_disks-(backup_file?1:0); i<cnt; i++) {
>  		struct mdinfo dinfo;
> -		int fd;
>  		int bsbsize;
>  		char *devname, namebuf[20];
>  		unsigned long long lo, hi;
> @@ -4747,12 +4767,9 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist,
>  		 * else restore data and update all superblocks
>  		 */
>  		if (i == old_disks-1) {
> -			fd = open(backup_file, O_RDONLY);
> -			if (fd<0) {
> -				pr_err("backup file %s inaccessible: %s\n",
> -					backup_file, strerror(errno));
> +			if (backup_fd < 0)
>  				continue;

You can use is_fd_valid(). Please also review other patches on that.

> -			}
> +			fd = backup_fd;
>  			devname = backup_file;
>  		} else {
>  			fd = fdlist[i];
> @@ -4907,6 +4924,8 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist, pr_err("Error restoring backup from %s\n",
>  					devname);
>  			free(offsets);
> +			if (backup_fd >= 0)
> +				close(backup_fd);
we have close_fd() for that.

>  			return 1;
>  		}
>  
> @@ -4923,6 +4942,8 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist, pr_err("Error restoring second backup from %s\n",
>  					devname);
>  			free(offsets);
> +			if (backup_fd >= 0)
> +				close(backup_fd);

You can use close_fd(). Also please analyze other patches on that.

>  			return 1;
>  		}
>  
> @@ -4984,8 +5005,14 @@ int Grow_restart(struct supertype *st, struct mdinfo
> *info, int *fdlist, st->ss->store_super(st, fdlist[j]);
>  			st->ss->free_super(st);
>  		}
> +		if (backup_fd >= 0)
> +			close(backup_fd);
>  		return 0;
>  	}
> +
> +	if (backup_fd >= 0)
> +		close(backup_fd);

As above (use close_fd())
> +
>  	/* Didn't find any backup data, try to see if any
>  	 * was needed.
>  	 */

Thanks,
Mariusz

Re: [PATCH 03/15] mdadm/Grow: fix coverity issue RESOURCE_LEAK

[Xiao Ni]

On Wed, Jul 17, 2024 at 7:29 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Mon, 15 Jul 2024 15:35:52 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  Grow.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 40 insertions(+), 13 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 7ae967bda067..632be7db8d38 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -485,6 +485,7 @@ int Grow_addbitmap(char *devname, int fd, struct context
> > *c, struct shape *s) int bitmap_fd;
> >               int d;
> >               int max_devs = st->max_devs;
> > +             int err = 0;
> >
> >               /* try to load a superblock */
> >               for (d = 0; d < max_devs; d++) {
> > @@ -525,13 +526,14 @@ int Grow_addbitmap(char *devname, int fd, struct
> > context *c, struct shape *s) return 1;
> >               }
> >               if (ioctl(fd, SET_BITMAP_FILE, bitmap_fd) < 0) {
> > -                     int err = errno;
> > +                     err = errno;
> >                       if (errno == EBUSY)
> >                               pr_err("Cannot add bitmap while array is
> > resyncing or reshaping etc.\n"); pr_err("Cannot set bitmap file for %s: %s\n",
> >                               devname, strerror(err));
> > -                     return 1;
> >               }
> > +             close(bitmap_fd);
> > +             return err;
>
> I don't think that we should return errno. I would say that mdadm should define
> returned statues for functions, that is why I added mdadm_status_t.
>
> Otherwise, same value may have two meanings. For example, in some cases we are
> fine with particular error code from error might be misleading (it may match
> allowed status).
> This is not the case here, it is just an example.
>
> I think that we should not return errno outside if not intended i.e. function is
> projected to return errno in every case.

Ok, I'll keep the original way.

@@ -530,8 +530,9 @@ int Grow_addbitmap(char *devname, int fd, struct
context *c, struct shape *s)
                                pr_err("Cannot add bitmap while array
is resyncing or reshaping etc.\n");
                        pr_err("Cannot set bitmap file for %s: %s\n",
                                devname, strerror(err));
-                       return 1;
                }
+               close(bitmap_fd);
+               return 1;
        }


>
> >       }
> >
> >       return 0;
> > @@ -3083,6 +3085,7 @@ static int reshape_array(char *container, int fd, char
> > *devname, int done;
> >       struct mdinfo *sra = NULL;
> >       char buf[SYSFS_MAX_BUF_SIZE];
> > +     bool located_backup = false;
> >
> >       /* when reshaping a RAID0, the component_size might be zero.
> >        * So try to fix that up.
> > @@ -3165,8 +3168,10 @@ static int reshape_array(char *container, int fd, char
> > *devname, goto release;
> >               }
> >
> > -             if (!backup_file)
> > +             if (!backup_file) {
> >                       backup_file = locate_backup(sra->sys_name);
> > +                     located_backup = true;
> > +             }
> >
> >               goto started;
> >       }
> > @@ -3612,15 +3617,15 @@ started:
> >                       mdstat_wait(30 - (delayed-1) * 25);
> >       } while (delayed);
> >       mdstat_close();
> > -     if (check_env("MDADM_GROW_VERIFY"))
> > -             fd = open(devname, O_RDONLY | O_DIRECT);
> > -     else
> > -             fd = -1;
> >       mlockall(MCL_FUTURE);
> >
> >       if (signal_s(SIGTERM, catch_term) == SIG_ERR)
> >               goto release;
> >
> > +     if (check_env("MDADM_GROW_VERIFY"))
> > +             fd = open(devname, O_RDONLY | O_DIRECT);
> > +     else
> > +             fd = -1;
>
> close_fd() is used unconditionally on fd few line earlier so it seems that else
> path is not needed but this code is massive so please double check if I'm right.
>
> We may call close_fd() on the closed resource and then it is not updated to -1,
> assuming that close fails with EBADF.

How about this:
-       if (check_env("MDADM_GROW_VERIFY"))
-               fd = open(devname, O_RDONLY | O_DIRECT);
-       else
-               fd = -1;
        mlockall(MCL_FUTURE);

        if (signal_s(SIGTERM, catch_term) == SIG_ERR)
                goto release;

+       if (check_env("MDADM_GROW_VERIFY"))
+               fd = open(devname, O_RDONLY | O_DIRECT);
        if (st->ss->external) {
                /* metadata handler takes it from here */
                done = st->ss->manage_reshape(
@@ -3632,6 +3634,7 @@ started:
                        fd, sra, &reshape, st, blocks, fdlist, offsets,
                        d - odisks, fdlist + odisks, offsets + odisks);

+       close_fd(&fd);
        free(fdlist);
        free(offsets);
>
> Right way to fix it is to replace all close(fd) by close_fd(fd). It will give is
> credibility that double close is not possible.

I'll replace them in the next version. But I think it should depend on
the specific case. Sometimes, we don't need to reset fd to -1, because
we don't use it anymore. So the standard close function is better.

>
>
> >       if (st->ss->external) {
> >               /* metadata handler takes it from here */
> >               done = st->ss->manage_reshape(
> > @@ -3632,6 +3637,8 @@ started:
> >                       fd, sra, &reshape, st, blocks, fdlist, offsets,
> >                       d - odisks, fdlist + odisks, offsets + odisks);
> >
> > +     if (fd >= 0)
> > +             close(fd);
> >       free(fdlist);
> >       free(offsets);
> >
> > @@ -3701,6 +3708,8 @@ out:
> >       exit(0);
> >
> >  release:
> > +     if (located_backup)
> > +             free(backup_file);
>
> >       free(fdlist);
> >       free(offsets);
> >       if (orig_level != UnSet && sra) {
> > @@ -3839,6 +3848,7 @@ int reshape_container(char *container, char *devname,
> >                       pr_err("Unable to initialize sysfs for %s\n",
> >                              mdstat->devnm);
> >                       rv = 1;
> > +                     close(fd);
> >                       break;
> >               }
> >
> > @@ -4717,6 +4727,7 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist, unsigned long long *offsets;
> >       unsigned long long  nstripe, ostripe;
> >       int ndata, odata;
> > +     int fd, backup_fd = -1;
> >
> >       odata = info->array.raid_disks - info->delta_disks - 1;
> >       if (info->array.level == 6)
> > @@ -4732,9 +4743,18 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist,
> >                * been used
> >                */
> >               old_disks = cnt;
> > +
> > +     if (backup_file) {
> > +             backup_fd = open(backup_file, O_RDONLY);
> > +             if (backup_fd < 0) {
> > +                     pr_err("Can't open backup file %s : %s\n",
> > +                             backup_file, strerror(errno));
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> >       for (i=old_disks-(backup_file?1:0); i<cnt; i++) {
> >               struct mdinfo dinfo;
> > -             int fd;
> >               int bsbsize;
> >               char *devname, namebuf[20];
> >               unsigned long long lo, hi;
> > @@ -4747,12 +4767,9 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist,
> >                * else restore data and update all superblocks
> >                */
> >               if (i == old_disks-1) {
> > -                     fd = open(backup_file, O_RDONLY);
> > -                     if (fd<0) {
> > -                             pr_err("backup file %s inaccessible: %s\n",
> > -                                     backup_file, strerror(errno));
> > +                     if (backup_fd < 0)
> >                               continue;
>
> You can use is_fd_valid(). Please also review other patches on that.

I searched by command `cat * | grep "< 0"`, this is the only place.
I'll change it. But is it really convenient? if_fd_valid just does the
same thing. Because if_fd_valid is not a standard C api, developers
may not know this api.

>
> > -                     }
> > +                     fd = backup_fd;
> >                       devname = backup_file;
> >               } else {
> >                       fd = fdlist[i];
> > @@ -4907,6 +4924,8 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist, pr_err("Error restoring backup from %s\n",
> >                                       devname);
> >                       free(offsets);
> > +                     if (backup_fd >= 0)
> > +                             close(backup_fd);
> we have close_fd() for that.
>
> >                       return 1;
> >               }
> >
> > @@ -4923,6 +4942,8 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist, pr_err("Error restoring second backup from %s\n",
> >                                       devname);
> >                       free(offsets);
> > +                     if (backup_fd >= 0)
> > +                             close(backup_fd);
>
> You can use close_fd(). Also please analyze other patches on that.

Ok,  I will do it.

Best Regards
Xiao
>
> >                       return 1;
> >               }
> >
> > @@ -4984,8 +5005,14 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist, st->ss->store_super(st, fdlist[j]);
> >                       st->ss->free_super(st);
> >               }
> > +             if (backup_fd >= 0)
> > +                     close(backup_fd);
> >               return 0;
> >       }
> > +
> > +     if (backup_fd >= 0)
> > +             close(backup_fd);
>
> As above (use close_fd())
> > +
> >       /* Didn't find any backup data, try to see if any
> >        * was needed.
> >        */
>
> Thanks,
> Mariusz
>

Re: [PATCH 03/15] mdadm/Grow: fix coverity issue RESOURCE_LEAK

[Mariusz Tkaczyk]

On Thu, 18 Jul 2024 11:27:50 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Wed, Jul 17, 2024 at 7:29 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > On Mon, 15 Jul 2024 15:35:52 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >  
> > > Signed-off-by: Xiao Ni <xni@redhat.com>
> > > ---
> > >  Grow.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
> > >  1 file changed, 40 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/Grow.c b/Grow.c
> > > index 7ae967bda067..632be7db8d38 100644
> > > --- a/Grow.c
> > > +++ b/Grow.c
> > > @@ -485,6 +485,7 @@ int Grow_addbitmap(char *devname, int fd, struct
> > > context *c, struct shape *s) int bitmap_fd;
> > >               int d;
> > >               int max_devs = st->max_devs;
> > > +             int err = 0;
> > >
> > >               /* try to load a superblock */
> > >               for (d = 0; d < max_devs; d++) {
> > > @@ -525,13 +526,14 @@ int Grow_addbitmap(char *devname, int fd, struct
> > > context *c, struct shape *s) return 1;
> > >               }
> > >               if (ioctl(fd, SET_BITMAP_FILE, bitmap_fd) < 0) {
> > > -                     int err = errno;
> > > +                     err = errno;
> > >                       if (errno == EBUSY)
> > >                               pr_err("Cannot add bitmap while array is
> > > resyncing or reshaping etc.\n"); pr_err("Cannot set bitmap file for %s:
> > > %s\n", devname, strerror(err));
> > > -                     return 1;
> > >               }
> > > +             close(bitmap_fd);
> > > +             return err;  
> >
> > I don't think that we should return errno. I would say that mdadm should
> > define returned statues for functions, that is why I added mdadm_status_t.
> >
> > Otherwise, same value may have two meanings. For example, in some cases we
> > are fine with particular error code from error might be misleading (it may
> > match allowed status).
> > This is not the case here, it is just an example.
> >
> > I think that we should not return errno outside if not intended i.e.
> > function is projected to return errno in every case.  
> 
> Ok, I'll keep the original way.
> 
> @@ -530,8 +530,9 @@ int Grow_addbitmap(char *devname, int fd, struct
> context *c, struct shape *s)
>                                 pr_err("Cannot add bitmap while array
> is resyncing or reshaping etc.\n");
>                         pr_err("Cannot set bitmap file for %s: %s\n",
>                                 devname, strerror(err));
> -                       return 1;
>                 }
> +               close(bitmap_fd);
> +               return 1;
>         }
> 
> 
> >  
> > >       }
> > >
> > >       return 0;
> > > @@ -3083,6 +3085,7 @@ static int reshape_array(char *container, int fd,
> > > char *devname, int done;
> > >       struct mdinfo *sra = NULL;
> > >       char buf[SYSFS_MAX_BUF_SIZE];
> > > +     bool located_backup = false;
> > >
> > >       /* when reshaping a RAID0, the component_size might be zero.
> > >        * So try to fix that up.
> > > @@ -3165,8 +3168,10 @@ static int reshape_array(char *container, int fd,
> > > char *devname, goto release;
> > >               }
> > >
> > > -             if (!backup_file)
> > > +             if (!backup_file) {
> > >                       backup_file = locate_backup(sra->sys_name);
> > > +                     located_backup = true;
> > > +             }
> > >
> > >               goto started;
> > >       }
> > > @@ -3612,15 +3617,15 @@ started:
> > >                       mdstat_wait(30 - (delayed-1) * 25);
> > >       } while (delayed);
> > >       mdstat_close();
> > > -     if (check_env("MDADM_GROW_VERIFY"))
> > > -             fd = open(devname, O_RDONLY | O_DIRECT);
> > > -     else
> > > -             fd = -1;
> > >       mlockall(MCL_FUTURE);
> > >
> > >       if (signal_s(SIGTERM, catch_term) == SIG_ERR)
> > >               goto release;
> > >
> > > +     if (check_env("MDADM_GROW_VERIFY"))
> > > +             fd = open(devname, O_RDONLY | O_DIRECT);
> > > +     else
> > > +             fd = -1;  
> >
> > close_fd() is used unconditionally on fd few line earlier so it seems that
> > else path is not needed but this code is massive so please double check if
> > I'm right.
> >
> > We may call close_fd() on the closed resource and then it is not updated to
> > -1, assuming that close fails with EBADF.  
> 
> How about this:
> -       if (check_env("MDADM_GROW_VERIFY"))
> -               fd = open(devname, O_RDONLY | O_DIRECT);
> -       else
> -               fd = -1;
>         mlockall(MCL_FUTURE);
> 
>         if (signal_s(SIGTERM, catch_term) == SIG_ERR)
>                 goto release;
> 
> +       if (check_env("MDADM_GROW_VERIFY"))
> +               fd = open(devname, O_RDONLY | O_DIRECT);
>         if (st->ss->external) {
>                 /* metadata handler takes it from here */
>                 done = st->ss->manage_reshape(
> @@ -3632,6 +3634,7 @@ started:
>                         fd, sra, &reshape, st, blocks, fdlist, offsets,
>                         d - odisks, fdlist + odisks, offsets + odisks);
> 
> +       close_fd(&fd);
>         free(fdlist);
>         free(offsets);
> >
> > Right way to fix it is to replace all close(fd) by close_fd(fd). It will
> > give is credibility that double close is not possible.  
> 
> I'll replace them in the next version. But I think it should depend on
> the specific case. Sometimes, we don't need to reset fd to -1, because
> we don't use it anymore. So the standard close function is better.

If this is unused later then compiler will should skip the assignment (it should
be detected and optimized during compilation). Not a big deal I think
but you are right, if we know that, we don't have to use this extended close
logic.

In this case we have massive function and fd is closed and opened many times
and there is a fork in the middle of it. I cannot follow all usages.
I would prefer to make it safer. There must be a reason why there is else fd =
-1; branch. Having close_fd(&fd) instead of close(fd) everywhere has following
advantages:
- double close attempt is not possible (because fd is updated to -1 each time)
- If fd value is the number now reused (we may open different file descriptor
  after close(fd)), it won't be unexpectedly closed.
- if the mentioned close_fd(&fd) few lines earlier is not needed then is
  skipped.

I know that it might be not optimal but I accept that as a compromise between
massive, overextended code and application security.

> 
> >
> >  
> > >       if (st->ss->external) {
> > >               /* metadata handler takes it from here */
> > >               done = st->ss->manage_reshape(
> > > @@ -3632,6 +3637,8 @@ started:
> > >                       fd, sra, &reshape, st, blocks, fdlist, offsets,
> > >                       d - odisks, fdlist + odisks, offsets + odisks);
> > >
> > > +     if (fd >= 0)
> > > +             close(fd);
> > >       free(fdlist);
> > >       free(offsets);
> > >
> > > @@ -3701,6 +3708,8 @@ out:
> > >       exit(0);
> > >
> > >  release:
> > > +     if (located_backup)
> > > +             free(backup_file);  
> >  
> > >       free(fdlist);
> > >       free(offsets);
> > >       if (orig_level != UnSet && sra) {
> > > @@ -3839,6 +3848,7 @@ int reshape_container(char *container, char
> > > *devname, pr_err("Unable to initialize sysfs for %s\n",
> > >                              mdstat->devnm);
> > >                       rv = 1;
> > > +                     close(fd);
> > >                       break;
> > >               }
> > >
> > > @@ -4717,6 +4727,7 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > > *info, int *fdlist, unsigned long long *offsets;
> > >       unsigned long long  nstripe, ostripe;
> > >       int ndata, odata;
> > > +     int fd, backup_fd = -1;
> > >
> > >       odata = info->array.raid_disks - info->delta_disks - 1;
> > >       if (info->array.level == 6)
> > > @@ -4732,9 +4743,18 @@ int Grow_restart(struct supertype *st, struct
> > > mdinfo *info, int *fdlist,
> > >                * been used
> > >                */
> > >               old_disks = cnt;
> > > +
> > > +     if (backup_file) {
> > > +             backup_fd = open(backup_file, O_RDONLY);
> > > +             if (backup_fd < 0) {
> > > +                     pr_err("Can't open backup file %s : %s\n",
> > > +                             backup_file, strerror(errno));
> > > +                     return -EINVAL;
> > > +             }
> > > +     }
> > > +
> > >       for (i=old_disks-(backup_file?1:0); i<cnt; i++) {
> > >               struct mdinfo dinfo;
> > > -             int fd;
> > >               int bsbsize;
> > >               char *devname, namebuf[20];
> > >               unsigned long long lo, hi;
> > > @@ -4747,12 +4767,9 @@ int Grow_restart(struct supertype *st, struct
> > > mdinfo *info, int *fdlist,
> > >                * else restore data and update all superblocks
> > >                */
> > >               if (i == old_disks-1) {
> > > -                     fd = open(backup_file, O_RDONLY);
> > > -                     if (fd<0) {
> > > -                             pr_err("backup file %s inaccessible: %s\n",
> > > -                                     backup_file, strerror(errno));
> > > +                     if (backup_fd < 0)
> > >                               continue;  
> >
> > You can use is_fd_valid(). Please also review other patches on that.  
> 
> I searched by command `cat * | grep "< 0"`, this is the only place.
> I'll change it. But is it really convenient? if_fd_valid just does the
> same thing. Because if_fd_valid is not a standard C api, developers
> may not know this api.
> 

It is easy to find so I don't see it as a problem.

For example in systemd we have:

        _cleanup_close_ int fd = -1;

I don't know how it works and for that reason shouldn't I use it? I have to
believe that developer who added it implemented it correctly.

Same here, name is pretty straightforward so we can use this to avoid direct
comparisons which might be bug prone because it is easy to miss <=, > in
review. Especially, that our brains are likely to skip the common patterns.

We had following problem in the past:
https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=b71de056cec70784ef2727e2febd7a6c88e580db

Thanks,
Mariusz

[PATCH 04/15] mdadm/Grow: fix coverity issue STRING_OVERFLOW

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Grow.c b/Grow.c
index 632be7db8d38..24f8eb8d3dee 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1694,6 +1694,8 @@ char *analyse_change(char *devname, struct mdinfo *info, struct reshape *re)
 					/* Current RAID6 layout has a RAID5
 					 * equivalent - good
 					 */
+					if (strlen(ls) > (40-1))
+						return "layout %s length is bigger than destination";
 					strcat(strcpy(layout, ls), "-6");
 					l = map_name(r6layout, layout);
 					if (l == UnSet)
-- 
2.32.0 (Apple Git-132)

[PATCH 05/15] mdadm/Incremental: fix coverity issues.

[Xiao Ni]

There are two issues PW.PARAMETER_HIDDEN and INTEGER_OVERFLOW

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Incremental.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index 83db071214ee..508e2c7c9140 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -770,7 +770,7 @@ static int count_active(struct supertype *st, struct mdinfo *sra,
 			replcnt++;
 		st->ss->free_super(st);
 	}
-	if (max_journal_events >= max_events - 1)
+	if (max_events > 0 && max_journal_events >= max_events - 1)
 		bestinfo->journal_clean = 1;
 
 	if (!avail)
@@ -1113,7 +1113,7 @@ static int partition_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 		int fd = -1;
 		struct mdinfo info;
 		struct supertype *st2 = NULL;
-		char *devname = NULL;
+		char *dev_name = NULL;
 		unsigned long long devsectors;
 		char *pathlist[2];
 
@@ -1142,14 +1142,14 @@ static int partition_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 		domain_free(domlist);
 		domlist = NULL;
 
-		if (asprintf(&devname, "/dev/disk/by-path/%s", de->d_name) != 1) {
-			devname = NULL;
+		if (asprintf(&dev_name, "/dev/disk/by-path/%s", de->d_name) != 1) {
+			dev_name = NULL;
 			goto next;
 		}
-		fd = open(devname, O_RDONLY);
+		fd = open(dev_name, O_RDONLY);
 		if (fd < 0)
 			goto next;
-		if (get_dev_size(fd, devname, &devsectors) == 0)
+		if (get_dev_size(fd, dev_name, &devsectors) == 0)
 			goto next;
 		devsectors >>= 9;
 
@@ -1188,8 +1188,8 @@ static int partition_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 		if (chosen == NULL || chosen_size < info.component_size) {
 			chosen_size = info.component_size;
 			free(chosen);
-			chosen = devname;
-			devname = NULL;
+			chosen = dev_name;
+			dev_name = NULL;
 			if (chosen_st) {
 				chosen_st->ss->free_super(chosen_st);
 				free(chosen_st);
@@ -1199,7 +1199,7 @@ static int partition_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 		}
 
 	next:
-		free(devname);
+		free(dev_name);
 		domain_free(domlist);
 		dev_policy_free(pol2);
 		if (st2)
@@ -1246,7 +1246,7 @@ static int is_bare(int dfd)
 
 	/* OK, first 4K appear blank, try the end. */
 	get_dev_size(dfd, NULL, &size);
-	if (lseek(dfd, size-4096, SEEK_SET) < 0 ||
+	if ((size >= 4096 && lseek(dfd, size-4096, SEEK_SET) < 0) ||
 	    read(dfd, buf, 4096) != 4096)
 		return 0;
 
-- 
2.32.0 (Apple Git-132)

[PATCH 06/15] mdadm/mdmon: fix coverity issue CHECKED_RETURN

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 mdmon.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mdmon.c b/mdmon.c
index 5fdb5cdb5a49..b8f71e5db555 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -199,7 +199,8 @@ static void try_kill_monitor(pid_t pid, char *devname, int sock)
 	 * clearing the non-blocking flag */
 	fl = fcntl(sock, F_GETFL, 0);
 	fl &= ~O_NONBLOCK;
-	fcntl(sock, F_SETFL, fl);
+	if (fcntl(sock, F_SETFL, fl) < 0)
+		return;
 	n = read(sock, buf, 100);
 
 	/* If there is I/O going on it might took some time to get to
@@ -249,7 +250,10 @@ static int make_control_sock(char *devname)
 	listen(sfd, 10);
 	fl = fcntl(sfd, F_GETFL, 0);
 	fl |= O_NONBLOCK;
-	fcntl(sfd, F_SETFL, fl);
+	if (fcntl(sfd, F_SETFL, fl) < 0) {
+		close(sfd);
+		return -1;
+	}
 	return sfd;
 }
 
-- 
2.32.0 (Apple Git-132)

[PATCH 07/15] mdadm/mdmon: fix coverity issue RESOURCE_LEAK

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 mdmon.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mdmon.c b/mdmon.c
index b8f71e5db555..f0e89924aef7 100644
--- a/mdmon.c
+++ b/mdmon.c
@@ -455,11 +455,13 @@ static int mdmon(char *devnm, int must_fork, int takeover)
 	if (must_fork) {
 		if (pipe(pfd) != 0) {
 			pr_err("failed to create pipe\n");
+			close(mdfd);
 			return 1;
 		}
 		switch(fork()) {
 		case -1:
 			pr_err("failed to fork: %s\n", strerror(errno));
+			close(mdfd);
 			return 1;
 		case 0: /* child */
 			close(pfd[0]);
@@ -471,6 +473,7 @@ static int mdmon(char *devnm, int must_fork, int takeover)
 				status = WEXITSTATUS(status);
 			}
 			close(pfd[0]);
+			close(mdfd);
 			return status;
 		}
 	} else
-- 
2.32.0 (Apple Git-132)

[PATCH 08/15] mdadm/mdopen: fix coverity issue CHECKED_RETURN

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 mdopen.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mdopen.c b/mdopen.c
index eaa59b5925af..c9fda131558b 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -406,7 +406,11 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 				perror("chown");
 			if (chmod(devname, ci->mode))
 				perror("chmod");
-			stat(devname, &stb);
+			if (stat(devname, &stb) < 0) {
+				pr_err("failed to stat %s\n",
+						devname);
+				return -1;
+			}
 			add_dev(devname, &stb, 0, NULL);
 		}
 		if (use_mdp == 1)
-- 
2.32.0 (Apple Git-132)

[PATCH 09/15] mdadm/mdopen: fix coverity issue STRING_OVERFLOW

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 mdopen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mdopen.c b/mdopen.c
index c9fda131558b..e49defb6744d 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -376,7 +376,7 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
 
 	sprintf(devname, "/dev/%s", devnm);
 
-	if (dev && dev[0] == '/')
+	if (dev && dev[0] == '/' && strlen(dev) < 400)
 		strcpy(chosen, dev);
 	else if (cname[0] == 0)
 		strcpy(chosen, devname);
-- 
2.32.0 (Apple Git-132)

[PATCH 10/15] mdadm/mdstat: fix coverity issue CHECKED_RETURN

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 mdstat.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/mdstat.c b/mdstat.c
index e233f094c480..930d59ee2325 100644
--- a/mdstat.c
+++ b/mdstat.c
@@ -146,8 +146,11 @@ struct mdstat_ent *mdstat_read(int hold, int start)
 		f = fopen("/proc/mdstat", "r");
 	if (f == NULL)
 		return NULL;
-	else
-		fcntl(fileno(f), F_SETFD, FD_CLOEXEC);
+
+	if (fcntl(fileno(f), F_SETFD, FD_CLOEXEC) < 0) {
+		fclose(f);
+		return NULL;
+	}
 
 	all = NULL;
 	end = &all;
@@ -281,7 +284,10 @@ struct mdstat_ent *mdstat_read(int hold, int start)
 	}
 	if (hold && mdstat_fd == -1) {
 		mdstat_fd = dup(fileno(f));
-		fcntl(mdstat_fd, F_SETFD, FD_CLOEXEC);
+		if (fcntl(mdstat_fd, F_SETFD, FD_CLOEXEC) < 0) {
+			fclose(f);
+			return NULL;
+		}
 	}
 	fclose(f);
 
-- 
2.32.0 (Apple Git-132)

[PATCH 11/15] mdadm/super0: fix coverity issue CHECKED_RETURN and EVALUATION_ORDER

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 super0.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/super0.c b/super0.c
index 9b8a1bd63bb7..6f43b1671d44 100644
--- a/super0.c
+++ b/super0.c
@@ -83,6 +83,9 @@ static void examine_super0(struct supertype *st, char *homehost)
 	int d;
 	int delta_extra = 0;
 	char *c;
+	unsigned long expected_csum = 0;
+
+	expected_csum = calc_sb0_csum(sb);
 
 	printf("          Magic : %08x\n", sb->md_magic);
 	printf("        Version : %d.%02d.%02d\n",
@@ -187,11 +190,11 @@ static void examine_super0(struct supertype *st, char *homehost)
 	printf("Working Devices : %d\n", sb->working_disks);
 	printf(" Failed Devices : %d\n", sb->failed_disks);
 	printf("  Spare Devices : %d\n", sb->spare_disks);
-	if (calc_sb0_csum(sb) == sb->sb_csum)
+	if (expected_csum == sb->sb_csum)
 		printf("       Checksum : %x - correct\n", sb->sb_csum);
 	else
 		printf("       Checksum : %x - expected %lx\n",
-		       sb->sb_csum, calc_sb0_csum(sb));
+		       sb->sb_csum, expected_csum);
 	printf("         Events : %llu\n",
 	       ((unsigned long long)sb->events_hi << 32) + sb->events_lo);
 	printf("\n");
@@ -1212,7 +1215,8 @@ static int locate_bitmap0(struct supertype *st, int fd, int node_num)
 
 	offset += MD_SB_BYTES;
 
-	lseek64(fd, offset, 0);
+	if (lseek64(fd, offset, 0) < 0)
+		return -1;
 	return 0;
 }
 
-- 
2.32.0 (Apple Git-132)

[PATCH 12/15] mdadm/super1: fix coverity issue CHECKED_RETURN

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 super1.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/super1.c b/super1.c
index 81d29a652f36..4e4c7bfd15ae 100644
--- a/super1.c
+++ b/super1.c
@@ -260,7 +260,10 @@ static int aread(struct align_fd *afd, void *buf, int len)
 	n = read(afd->fd, b, iosize);
 	if (n <= 0)
 		return n;
-	lseek(afd->fd, len - n, 1);
+	if (lseek(afd->fd, len - n, 1) < 0) {
+		pr_err("lseek fails\n");
+		return -1;
+	}
 	if (n > len)
 		n = len;
 	memcpy(buf, b, n);
@@ -294,14 +297,20 @@ static int awrite(struct align_fd *afd, void *buf, int len)
 		n = read(afd->fd, b, iosize);
 		if (n <= 0)
 			return n;
-		lseek(afd->fd, -n, 1);
+		if (lseek(afd->fd, -n, 1) < 0) {
+			pr_err("lseek fails\n");
+			return -1;
+		}
 	}
 
 	memcpy(b, buf, len);
 	n = write(afd->fd, b, iosize);
 	if (n <= 0)
 		return n;
-	lseek(afd->fd, len - n, 1);
+	if (lseek(afd->fd, len - n, 1) < 0) {
+		pr_err("lseek fails\n");
+		return -1;
+	}
 	return len;
 }
 
@@ -2667,7 +2676,10 @@ static int locate_bitmap1(struct supertype *st, int fd, int node_num)
 	}
 	if (mustfree)
 		free(sb);
-	lseek64(fd, offset<<9, 0);
+	if (lseek64(fd, offset<<9, 0) < 0) {
+		pr_err("lseek fails\n");
+		ret = -1;
+	}
 	return ret;
 }
 
-- 
2.32.0 (Apple Git-132)

[PATCH 13/15] mdadm/super1: fix coverity issue DEADCODE

[Xiao Ni]

optimal_space is at most 2046. So space can't be larger than UINT16_MAX.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 super1.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/super1.c b/super1.c
index 4e4c7bfd15ae..24bc10269dbf 100644
--- a/super1.c
+++ b/super1.c
@@ -1466,8 +1466,6 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 						__le32_to_cpu(sb->chunksize));
 			if (space > optimal_space)
 				space = optimal_space;
-			if (space > UINT16_MAX)
-				space = UINT16_MAX;
 		}
 
 		sb->ppl.offset = __cpu_to_le16(offset);
-- 
2.32.0 (Apple Git-132)

[PATCH 14/15] mdadm/super1: fix coverity issue EVALUATION_ORDER

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 super1.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/super1.c b/super1.c
index 24bc10269dbf..243eeb1a0174 100644
--- a/super1.c
+++ b/super1.c
@@ -340,6 +340,9 @@ static void examine_super1(struct supertype *st, char *homehost)
 	unsigned long long sb_offset;
 	struct mdinfo info;
 	int inconsistent = 0;
+	unsigned int expected_csum = 0;
+
+	expected_csum = calc_sb_1_csum(sb);
 
 	printf("          Magic : %08x\n", __le32_to_cpu(sb->magic));
 	printf("        Version : 1");
@@ -507,13 +510,13 @@ static void examine_super1(struct supertype *st, char *homehost)
 		printf("\n");
 	}
 
-	if (calc_sb_1_csum(sb) == sb->sb_csum)
+	if (expected_csum == sb->sb_csum)
 		printf("       Checksum : %x - correct\n",
 		       __le32_to_cpu(sb->sb_csum));
 	else
 		printf("       Checksum : %x - expected %x\n",
 		       __le32_to_cpu(sb->sb_csum),
-		       __le32_to_cpu(calc_sb_1_csum(sb)));
+		       __le32_to_cpu(expected_csum));
 	printf("         Events : %llu\n",
 	       (unsigned long long)__le64_to_cpu(sb->events));
 	printf("\n");
-- 
2.32.0 (Apple Git-132)

[PATCH 15/15] mdadm/super1: fix coverity issue RESOURCE_LEAK

[Xiao Ni]

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 super1.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/super1.c b/super1.c
index 243eeb1a0174..9c9c7dd14c15 100644
--- a/super1.c
+++ b/super1.c
@@ -923,10 +923,12 @@ static int examine_badblocks_super1(struct supertype *st, int fd, char *devname)
 	offset <<= 9;
 	if (lseek64(fd, offset, 0) < 0) {
 		pr_err("Cannot seek to bad-blocks list\n");
+		free(bbl);
 		return 1;
 	}
 	if (read(fd, bbl, size) != size) {
 		pr_err("Cannot read bad-blocks list\n");
+		free(bbl);
 		return 1;
 	}
 	/* 64bits per entry. 10 bits is block-count, 54 bits is block
@@ -947,6 +949,7 @@ static int examine_badblocks_super1(struct supertype *st, int fd, char *devname)
 
 		printf("%20llu for %d sectors\n", sector, count);
 	}
+	free(bbl);
 	return 0;
 }
 
-- 
2.32.0 (Apple Git-132)