[PATCH 0/5] Fix issues reported by covscan and newer GCC

[PATCH 0/5] Fix issues reported by covscan and newer GCC

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

I had some errors thrown at me by covscan and GCC 4.9.2 which prompted
some furthe inspection. In particular patch 3 could result in a local
stack variable passed back to the calling function, and patch 5 with
code not being executed as expected.

Please have a look.

Cheers,
Jes

Jes Sorensen (5):
  Grow.c: Fix classic readlink() buffer overflow
  Check return of stat() to avoid covscan complaining
  add_orom(): Compare content of struct imsm_orom rather than pointers
    to it
  IncrementalScan(): Make sure 'st' is valid before dereferencing it
  write_super_imsm_spares(): C statements are terminated by ;

 Assemble.c       |  6 +++++-
 Grow.c           |  2 +-
 Incremental.c    |  2 +-
 platform-intel.c |  4 ++--
 super-intel.c    | 12 ++++++------
 5 files changed, 15 insertions(+), 11 deletions(-)

-- 
2.1.0

[PATCH 1/5] Grow.c: Fix classic readlink() buffer overflow

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

The buffer passed on to readlink() needs to contain space for the
terminating \0. See 'man 3 readlink' for details.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Grow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Grow.c b/Grow.c
index b78d063..a8bbf2b 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3319,7 +3319,7 @@ started:
 		bul = make_backup(sra->sys_name);
 		if (bul) {
 			char buf[1024];
-			int l = readlink(bul, buf, sizeof(buf));
+			int l = readlink(bul, buf, sizeof(buf) - 1);
 			if (l > 0) {
 				buf[l]=0;
 				unlink(buf);
-- 
2.1.0

[PATCH 2/5] Check return of stat() to avoid covscan complaining

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Assemble.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Assemble.c b/Assemble.c
index 131f871..b392214 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -688,7 +688,11 @@ static int load_devices(struct devs *devices, char *devmap,
 			close(dfd);
 		}
 
-		stat(devname, &stb);
+		if (stat(devname, &stb)) {
+			pr_err("Unsable to stat(%s) - skipping device.\n",
+			       devname);
+			continue;
+		}
 
 		if (c->verbose > 0)
 			pr_err("%s is identified as a member of %s, slot %d%s.\n",
-- 
2.1.0

Re: [PATCH 2/5] Check return of stat() to avoid covscan complaining

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2608 bytes --]

On Tue, 24 Feb 2015 16:00:37 -0500 Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  Assemble.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Assemble.c b/Assemble.c
> index 131f871..b392214 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -688,7 +688,11 @@ static int load_devices(struct devs *devices, char *devmap,
>  			close(dfd);
>  		}
>  
> -		stat(devname, &stb);
> +		if (stat(devname, &stb)) {
> +			pr_err("Unsable to stat(%s) - skipping device.\n",
> +			       devname);
> +			continue;
> +		}
>  
>  		if (c->verbose > 0)
>  			pr_err("%s is identified as a member of %s, slot %d%s.\n",

I've applied the other 4.  I think I'd rather this one was fixed by changing
  stat(devname,
to
  fstat(dfd,
and keep dfd open a bit longer.

Does this look OK to you?

Thanks,
NeilBrown

diff --git a/Assemble.c b/Assemble.c
index 131f871a6d1e..1e529c1b3126 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -576,13 +576,13 @@ static int load_devices(struct devs *devices, char *devmap,
 		struct stat stb;
 		struct supertype *tst;
 		int i;
+		int dfd;
 
 		if (tmpdev->used != 1)
 			continue;
 		/* looks like a good enough match to update the super block if needed */
 #ifndef MDASSEMBLE
 		if (c->update) {
-			int dfd;
 			/* prepare useful information in info structures */
 			struct stat stb2;
 			int err;
@@ -652,7 +652,6 @@ static int load_devices(struct devs *devices, char *devmap,
 			if (tst->ss->store_super(tst, dfd))
 				pr_err("Could not re-write superblock on %s.\n",
 				       devname);
-			close(dfd);
 
 			if (strcmp(c->update, "uuid")==0 &&
 			    ident->bitmap_fd >= 0 && !bitmap_done) {
@@ -666,9 +665,9 @@ static int load_devices(struct devs *devices, char *devmap,
 		} else
 #endif
 		{
-			int dfd = dev_open(devname,
-					   tmpdev->disposition == 'I'
-					   ? O_RDWR : (O_RDWR|O_EXCL));
+			dfd = dev_open(devname,
+				       tmpdev->disposition == 'I'
+				       ? O_RDWR : (O_RDWR|O_EXCL));
 			tst = dup_super(st);
 
 			if (dfd < 0 || tst->ss->load_super(tst, dfd, NULL) != 0) {
@@ -685,10 +684,10 @@ static int load_devices(struct devs *devices, char *devmap,
 				return -1;
 			}
 			tst->ss->getinfo_super(tst, content, devmap + devcnt * content->array.raid_disks);
-			close(dfd);
 		}
 
-		stat(devname, &stb);
+		fstat(dfd, &stb);
+		close(dfd);
 
 		if (c->verbose > 0)
 			pr_err("%s is identified as a member of %s, slot %d%s.\n",

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 2/5] Check return of stat() to avoid covscan complaining

[Jes Sorensen]

NeilBrown <neilb@suse.de> writes:
> On Tue, 24 Feb 2015 16:00:37 -0500 Jes.Sorensen@redhat.com wrote:
>
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>> 
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  Assemble.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Assemble.c b/Assemble.c
>> index 131f871..b392214 100644
>> --- a/Assemble.c
>> +++ b/Assemble.c
>> @@ -688,7 +688,11 @@ static int load_devices(struct devs *devices, char *devmap,
>>  			close(dfd);
>>  		}
>>  
>> -		stat(devname, &stb);
>> +		if (stat(devname, &stb)) {
>> +			pr_err("Unsable to stat(%s) - skipping device.\n",
>> +			       devname);
>> +			continue;
>> +		}
>>  
>>  		if (c->verbose > 0)
>>  			pr_err("%s is identified as a member of %s, slot %d%s.\n",
>
> I've applied the other 4.  I think I'd rather this one was fixed by changing
>   stat(devname,
> to
>   fstat(dfd,
> and keep dfd open a bit longer.
>
> Does this look OK to you?

I got the warning from covscan because we ignored the return value from
stat, so I think you still need to check the return value from fstat()
as well.

Cheers,
Jes

>
> Thanks,
> NeilBrown
>
> diff --git a/Assemble.c b/Assemble.c
> index 131f871a6d1e..1e529c1b3126 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -576,13 +576,13 @@ static int load_devices(struct devs *devices, char *devmap,
>  		struct stat stb;
>  		struct supertype *tst;
>  		int i;
> +		int dfd;
>  
>  		if (tmpdev->used != 1)
>  			continue;
>  		/* looks like a good enough match to update the super block if needed */
>  #ifndef MDASSEMBLE
>  		if (c->update) {
> -			int dfd;
>  			/* prepare useful information in info structures */
>  			struct stat stb2;
>  			int err;
> @@ -652,7 +652,6 @@ static int load_devices(struct devs *devices, char *devmap,
>  			if (tst->ss->store_super(tst, dfd))
>  				pr_err("Could not re-write superblock on %s.\n",
>  				       devname);
> -			close(dfd);
>  
>  			if (strcmp(c->update, "uuid")==0 &&
>  			    ident->bitmap_fd >= 0 && !bitmap_done) {
> @@ -666,9 +665,9 @@ static int load_devices(struct devs *devices, char *devmap,
>  		} else
>  #endif
>  		{
> -			int dfd = dev_open(devname,
> -					   tmpdev->disposition == 'I'
> -					   ? O_RDWR : (O_RDWR|O_EXCL));
> +			dfd = dev_open(devname,
> +				       tmpdev->disposition == 'I'
> +				       ? O_RDWR : (O_RDWR|O_EXCL));
>  			tst = dup_super(st);
>  
>  			if (dfd < 0 || tst->ss->load_super(tst, dfd, NULL) != 0) {
> @@ -685,10 +684,10 @@ static int load_devices(struct devs *devices, char *devmap,
>  				return -1;
>  			}
>  			tst->ss->getinfo_super(tst, content, devmap + devcnt * content->array.raid_disks);
> -			close(dfd);
>  		}
>  
> -		stat(devname, &stb);
> +		fstat(dfd, &stb);
> +		close(dfd);
>  
>  		if (c->verbose > 0)
>  			pr_err("%s is identified as a member of %s, slot %d%s.\n",

Re: [PATCH 2/5] Check return of stat() to avoid covscan complaining

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1674 bytes --]

On Tue, 24 Feb 2015 16:56:29 -0500 Jes Sorensen <Jes.Sorensen@redhat.com>
wrote:

> NeilBrown <neilb@suse.de> writes:
> > On Tue, 24 Feb 2015 16:00:37 -0500 Jes.Sorensen@redhat.com wrote:
> >
> >> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >> 
> >> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> >> ---
> >>  Assemble.c | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/Assemble.c b/Assemble.c
> >> index 131f871..b392214 100644
> >> --- a/Assemble.c
> >> +++ b/Assemble.c
> >> @@ -688,7 +688,11 @@ static int load_devices(struct devs *devices, char *devmap,
> >>  			close(dfd);
> >>  		}
> >>  
> >> -		stat(devname, &stb);
> >> +		if (stat(devname, &stb)) {
> >> +			pr_err("Unsable to stat(%s) - skipping device.\n",
> >> +			       devname);
> >> +			continue;
> >> +		}
> >>  
> >>  		if (c->verbose > 0)
> >>  			pr_err("%s is identified as a member of %s, slot %d%s.\n",
> >
> > I've applied the other 4.  I think I'd rather this one was fixed by changing
> >   stat(devname,
> > to
> >   fstat(dfd,
> > and keep dfd open a bit longer.
> >
> > Does this look OK to you?
> 
> I got the warning from covscan because we ignored the return value from
> stat, so I think you still need to check the return value from fstat()
> as well.

I hope not.
You can only get errors from fstat if you do something stupid like passing
NULL as the stat pointer, or passing a non-open file descriptior.
So if covscan complains, then covscan is broken.

In contrast, stat can certainly given an error, such a ENOENT, which cannot
possibly be avoided by not being stupid.

NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 2/5] Check return of stat() to avoid covscan complaining

[Jes Sorensen]

NeilBrown <neilb@suse.de> writes:
> On Tue, 24 Feb 2015 16:56:29 -0500 Jes Sorensen <Jes.Sorensen@redhat.com>
> wrote:
>
>> NeilBrown <neilb@suse.de> writes:
>> > On Tue, 24 Feb 2015 16:00:37 -0500 Jes.Sorensen@redhat.com wrote:
>> >
>> >> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>> >> 
>> >> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> >> ---
>> >>  Assemble.c | 6 +++++-
>> >>  1 file changed, 5 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/Assemble.c b/Assemble.c
>> >> index 131f871..b392214 100644
>> >> --- a/Assemble.c
>> >> +++ b/Assemble.c
>> >> @@ -688,7 +688,11 @@ static int load_devices(struct devs *devices, char *devmap,
>> >>  			close(dfd);
>> >>  		}
>> >>  
>> >> -		stat(devname, &stb);
>> >> +		if (stat(devname, &stb)) {
>> >> +			pr_err("Unsable to stat(%s) - skipping device.\n",
>> >> +			       devname);
>> >> +			continue;
>> >> +		}
>> >>  
>> >>  		if (c->verbose > 0)
>> >>  			pr_err("%s is identified as a member of %s, slot %d%s.\n",
>> >
>> > I've applied the other 4.  I think I'd rather this one was fixed by changing
>> >   stat(devname,
>> > to
>> >   fstat(dfd,
>> > and keep dfd open a bit longer.
>> >
>> > Does this look OK to you?
>> 
>> I got the warning from covscan because we ignored the return value from
>> stat, so I think you still need to check the return value from fstat()
>> as well.
>
> I hope not.
> You can only get errors from fstat if you do something stupid like passing
> NULL as the stat pointer, or passing a non-open file descriptior.
> So if covscan complains, then covscan is broken.
>
> In contrast, stat can certainly given an error, such a ENOENT, which cannot
> possibly be avoided by not being stupid.

Hmmm OK you got a point, even if the file is removed, it shouldn't
disappear until all users close it.

Cheers,
Jes

[PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

This avoids adding the same orom entry to the oroms list multiple
times, as the comparison of pointers is never going to succeed, in
particular when '*orom' points to a local stack variable in the
calling function.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 platform-intel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/platform-intel.c b/platform-intel.c
index 37274da..a4ffa9f 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
 	int i;
 
 	for (i = 0; i < SYS_DEV_MAX; i++) {
-		if (&oroms[i].orom == orom)
-			return orom;
+		if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
+			return &oroms[i].orom;
 		if (oroms[i].orom.signature[0] == 0) {
 			oroms[i].orom = *orom;
 			return &oroms[i].orom;
-- 
2.1.0

Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it

[Artur Paszkiewicz]

On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> This avoids adding the same orom entry to the oroms list multiple
> times, as the comparison of pointers is never going to succeed, in
> particular when '*orom' points to a local stack variable in the
> calling function.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  platform-intel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/platform-intel.c b/platform-intel.c
> index 37274da..a4ffa9f 100644
> --- a/platform-intel.c
> +++ b/platform-intel.c
> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>  	int i;
>  
>  	for (i = 0; i < SYS_DEV_MAX; i++) {
> -		if (&oroms[i].orom == orom)
> -			return orom;
> +		if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
> +			return &oroms[i].orom;
>  		if (oroms[i].orom.signature[0] == 0) {
>  			oroms[i].orom = *orom;
>  			return &oroms[i].orom;
> 

Hi Jes,

You are right that this can add the same entry multiple times, but this
is how it is supposed to work. The oroms list should contain all the
platform's oroms and they can be the same, this is why memcmp() should
not be used here. We don't want to compare the contents of the
structure, just its address. Sorry if it's not clear.

Artur

Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it

[Jes Sorensen]

Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>> 
>> This avoids adding the same orom entry to the oroms list multiple
>> times, as the comparison of pointers is never going to succeed, in
>> particular when '*orom' points to a local stack variable in the
>> calling function.
>> 
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  platform-intel.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/platform-intel.c b/platform-intel.c
>> index 37274da..a4ffa9f 100644
>> --- a/platform-intel.c
>> +++ b/platform-intel.c
>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>  	int i;
>>  
>>  	for (i = 0; i < SYS_DEV_MAX; i++) {
>> -		if (&oroms[i].orom == orom)
>> -			return orom;
>> +		if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>> +			return &oroms[i].orom;
>>  		if (oroms[i].orom.signature[0] == 0) {
>>  			oroms[i].orom = *orom;
>>  			return &oroms[i].orom;
>> 
>
> Hi Jes,
>
> You are right that this can add the same entry multiple times, but this
> is how it is supposed to work. The oroms list should contain all the
> platform's oroms and they can be the same, this is why memcmp() should
> not be used here. We don't want to compare the contents of the
> structure, just its address. Sorry if it's not clear.

Artur,

Then the code is fundamentally broken, since you end up comparing a
stack variable against the oroms array when you call it from
find_imsm_efi(). Worse you can end up returning the local stack variable
declared in find_imsm_efi() to the calling function - there is no way
that can be correct.

Look at this:

static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
{
        int i;

        for (i = 0; i < SYS_DEV_MAX; i++) {
                if (&oroms[i].orom == orom)
                        return orom;
                if (oroms[i].orom.signature[0] == 0) {
                        oroms[i].orom = *orom;
                        return &oroms[i].orom;
                }
        }
        return NULL;
}

const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
{
        struct imsm_orom orom;
        const struct imsm_orom *ret;
        int err;

....

        ret = add_orom(&orom);
        add_orom_device_id(ret, hba->dev_id);

        return ret;
}

Cheers,
Jes

Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it

[Artur Paszkiewicz]

On 02/25/2015 01:29 PM, Jes Sorensen wrote:
> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> This avoids adding the same orom entry to the oroms list multiple
>>> times, as the comparison of pointers is never going to succeed, in
>>> particular when '*orom' points to a local stack variable in the
>>> calling function.
>>>
>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>> ---
>>>  platform-intel.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/platform-intel.c b/platform-intel.c
>>> index 37274da..a4ffa9f 100644
>>> --- a/platform-intel.c
>>> +++ b/platform-intel.c
>>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>>  	int i;
>>>  
>>>  	for (i = 0; i < SYS_DEV_MAX; i++) {
>>> -		if (&oroms[i].orom == orom)
>>> -			return orom;
>>> +		if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>>> +			return &oroms[i].orom;
>>>  		if (oroms[i].orom.signature[0] == 0) {
>>>  			oroms[i].orom = *orom;
>>>  			return &oroms[i].orom;
>>>
>>
>> Hi Jes,
>>
>> You are right that this can add the same entry multiple times, but this
>> is how it is supposed to work. The oroms list should contain all the
>> platform's oroms and they can be the same, this is why memcmp() should
>> not be used here. We don't want to compare the contents of the
>> structure, just its address. Sorry if it's not clear.
> 
> Artur,
> 
> Then the code is fundamentally broken, since you end up comparing a
> stack variable against the oroms array when you call it from
> find_imsm_efi(). Worse you can end up returning the local stack variable
> declared in find_imsm_efi() to the calling function - there is no way
> that can be correct.
> 
> Look at this:
> 
> static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
> {
>         int i;
> 
>         for (i = 0; i < SYS_DEV_MAX; i++) {
>                 if (&oroms[i].orom == orom)
>                         return orom;
>                 if (oroms[i].orom.signature[0] == 0) {
>                         oroms[i].orom = *orom;
>                         return &oroms[i].orom;
>                 }
>         }
>         return NULL;
> }
> 
> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
> {
>         struct imsm_orom orom;
>         const struct imsm_orom *ret;
>         int err;
> 
> ....
> 
>         ret = add_orom(&orom);
>         add_orom_device_id(ret, hba->dev_id);
> 
>         return ret;
> }

I can't see how this can lead to returning a stack variable. The oroms
array is global and add_orom() will always return a pointer to a struct
in this array. This comparison will always fail when we pass a pointer
to a stack variable to add_orom():

if (&oroms[i].orom == orom)
	return orom;

This was meant to prevent adding an orom again like this:

ret = add_orom(&orom);
add_orom(ret);

Maybe it would be more appropriate to return NULL to indicate that
nothing was added instead of returning back the same pointer. I can do a
patch for this. What do you think?

Regards,
Artur

Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it

[Jes Sorensen]

Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
> On 02/25/2015 01:29 PM, Jes Sorensen wrote:
>> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>>> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
>>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>
>>>> This avoids adding the same orom entry to the oroms list multiple
>>>> times, as the comparison of pointers is never going to succeed, in
>>>> particular when '*orom' points to a local stack variable in the
>>>> calling function.
>>>>
>>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>> ---
>>>>  platform-intel.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/platform-intel.c b/platform-intel.c
>>>> index 37274da..a4ffa9f 100644
>>>> --- a/platform-intel.c
>>>> +++ b/platform-intel.c
>>>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>>>  	int i;
>>>>  
>>>>  	for (i = 0; i < SYS_DEV_MAX; i++) {
>>>> -		if (&oroms[i].orom == orom)
>>>> -			return orom;
>>>> +		if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>>>> +			return &oroms[i].orom;
>>>>  		if (oroms[i].orom.signature[0] == 0) {
>>>>  			oroms[i].orom = *orom;
>>>>  			return &oroms[i].orom;
>>>>
>>>
>>> Hi Jes,
>>>
>>> You are right that this can add the same entry multiple times, but this
>>> is how it is supposed to work. The oroms list should contain all the
>>> platform's oroms and they can be the same, this is why memcmp() should
>>> not be used here. We don't want to compare the contents of the
>>> structure, just its address. Sorry if it's not clear.
>> 
>> Artur,
>> 
>> Then the code is fundamentally broken, since you end up comparing a
>> stack variable against the oroms array when you call it from
>> find_imsm_efi(). Worse you can end up returning the local stack variable
>> declared in find_imsm_efi() to the calling function - there is no way
>> that can be correct.
>> 
>> Look at this:
>> 
>> static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>> {
>>         int i;
>> 
>>         for (i = 0; i < SYS_DEV_MAX; i++) {
>>                 if (&oroms[i].orom == orom)
>>                         return orom;
>>                 if (oroms[i].orom.signature[0] == 0) {
>>                         oroms[i].orom = *orom;
>>                         return &oroms[i].orom;
>>                 }
>>         }
>>         return NULL;
>> }
>> 
>> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>> {
>>         struct imsm_orom orom;
>>         const struct imsm_orom *ret;
>>         int err;
>> 
>> ....
>> 
>>         ret = add_orom(&orom);
>>         add_orom_device_id(ret, hba->dev_id);
>> 
>>         return ret;
>> }
>
> I can't see how this can lead to returning a stack variable. The oroms
> array is global and add_orom() will always return a pointer to a struct
> in this array. This comparison will always fail when we pass a pointer
> to a stack variable to add_orom():
>
> if (&oroms[i].orom == orom)
> 	return orom;
>
> This was meant to prevent adding an orom again like this:
>
> ret = add_orom(&orom);
> add_orom(ret);
>
> Maybe it would be more appropriate to return NULL to indicate that
> nothing was added instead of returning back the same pointer. I can do a
> patch for this. What do you think?

It will fail because we know we're comparing a stack pointer, but it
raises red flags with tools like coverity and it is really bad coding
practice to rely on hacks like this.

I also don't understand why you want to keep a table of identical
entries in the orom structure if multiple identical entries are found.
Each entry ought to match onto a specific physical controller, unless I
get something wrong?

Cheers,
Jes

Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it

[Artur Paszkiewicz]

On 02/25/2015 06:15 PM, Jes Sorensen wrote:
> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>> On 02/25/2015 01:29 PM, Jes Sorensen wrote:
>>> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>>>> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
>>>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>>
>>>>> This avoids adding the same orom entry to the oroms list multiple
>>>>> times, as the comparison of pointers is never going to succeed, in
>>>>> particular when '*orom' points to a local stack variable in the
>>>>> calling function.
>>>>>
>>>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>> ---
>>>>>  platform-intel.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/platform-intel.c b/platform-intel.c
>>>>> index 37274da..a4ffa9f 100644
>>>>> --- a/platform-intel.c
>>>>> +++ b/platform-intel.c
>>>>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>>>>  	int i;
>>>>>  
>>>>>  	for (i = 0; i < SYS_DEV_MAX; i++) {
>>>>> -		if (&oroms[i].orom == orom)
>>>>> -			return orom;
>>>>> +		if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>>>>> +			return &oroms[i].orom;
>>>>>  		if (oroms[i].orom.signature[0] == 0) {
>>>>>  			oroms[i].orom = *orom;
>>>>>  			return &oroms[i].orom;
>>>>>
>>>>
>>>> Hi Jes,
>>>>
>>>> You are right that this can add the same entry multiple times, but this
>>>> is how it is supposed to work. The oroms list should contain all the
>>>> platform's oroms and they can be the same, this is why memcmp() should
>>>> not be used here. We don't want to compare the contents of the
>>>> structure, just its address. Sorry if it's not clear.
>>>
>>> Artur,
>>>
>>> Then the code is fundamentally broken, since you end up comparing a
>>> stack variable against the oroms array when you call it from
>>> find_imsm_efi(). Worse you can end up returning the local stack variable
>>> declared in find_imsm_efi() to the calling function - there is no way
>>> that can be correct.
>>>
>>> Look at this:
>>>
>>> static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>> {
>>>         int i;
>>>
>>>         for (i = 0; i < SYS_DEV_MAX; i++) {
>>>                 if (&oroms[i].orom == orom)
>>>                         return orom;
>>>                 if (oroms[i].orom.signature[0] == 0) {
>>>                         oroms[i].orom = *orom;
>>>                         return &oroms[i].orom;
>>>                 }
>>>         }
>>>         return NULL;
>>> }
>>>
>>> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>>> {
>>>         struct imsm_orom orom;
>>>         const struct imsm_orom *ret;
>>>         int err;
>>>
>>> ....
>>>
>>>         ret = add_orom(&orom);
>>>         add_orom_device_id(ret, hba->dev_id);
>>>
>>>         return ret;
>>> }
>>
>> I can't see how this can lead to returning a stack variable. The oroms
>> array is global and add_orom() will always return a pointer to a struct
>> in this array. This comparison will always fail when we pass a pointer
>> to a stack variable to add_orom():
>>
>> if (&oroms[i].orom == orom)
>> 	return orom;
>>
>> This was meant to prevent adding an orom again like this:
>>
>> ret = add_orom(&orom);
>> add_orom(ret);
>>
>> Maybe it would be more appropriate to return NULL to indicate that
>> nothing was added instead of returning back the same pointer. I can do a
>> patch for this. What do you think?
> 
> It will fail because we know we're comparing a stack pointer, but it
> raises red flags with tools like coverity and it is really bad coding
> practice to rely on hacks like this.
> 
> I also don't understand why you want to keep a table of identical
> entries in the orom structure if multiple identical entries are found.
> Each entry ought to match onto a specific physical controller, unless I
> get something wrong?
> 

OK, you're right, it is a hack. I thought it over and redesigned those
orom functions. This should make it simpler and more consistent.

Thanks,
Artur

From 673ecf1c0539f0050cc5934203af6d79cd68234d Mon Sep 17 00:00:00 2001
From: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Date: Fri, 27 Feb 2015 10:34:20 +0100
Subject: [PATCH] imsm: simplified multiple OROMs support

Replaced oroms array with list, add_orom() now only appends to this list
and add_orom_device_id() only appends devid_list node to an orom_entry.

Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
 platform-intel.c | 96 +++++++++++++++++++++++++++-----------------------------
 platform-intel.h |  4 ++-
 super-intel.c    | 18 +++++------
 3 files changed, 57 insertions(+), 61 deletions(-)

diff --git a/platform-intel.c b/platform-intel.c
index 37274da..9c89c20 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -229,65 +229,61 @@ struct pciExpDataStructFormat {
 	__u16 devListOffset;
 } __attribute__ ((packed));
 
-static struct orom_entry oroms[SYS_DEV_MAX];
-
-const struct orom_entry *get_oroms(void)
-{
-	return (const struct orom_entry *)&oroms;
-}
+struct orom_entry *orom_entries;
 
 const struct imsm_orom *get_orom_by_device_id(__u16 dev_id)
 {
-	int i;
-	struct devid_list *list;
+	struct orom_entry *entry;
+	struct devid_list *devid;
 
-	for (i = 0; i < SYS_DEV_MAX; i++) {
-		for (list = oroms[i].devid_list; list; list = list->next) {
-			if (list->devid == dev_id)
-				return &oroms[i].orom;
+	for (entry = orom_entries; entry; entry = entry->next) {
+		for (devid = entry->devid_list; devid; devid = devid->next) {
+			if (devid->devid == dev_id)
+				return &entry->orom;
 		}
 	}
+
 	return NULL;
 }
 
-static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
+static struct orom_entry *add_orom(const struct imsm_orom *orom)
 {
-	int i;
-
-	for (i = 0; i < SYS_DEV_MAX; i++) {
-		if (&oroms[i].orom == orom)
-			return orom;
-		if (oroms[i].orom.signature[0] == 0) {
-			oroms[i].orom = *orom;
-			return &oroms[i].orom;
-		}
-	}
-	return NULL;
+	struct orom_entry *list;
+	struct orom_entry *prev = NULL;
+
+	for (list = orom_entries; list; prev = list, list = list->next)
+		;
+
+	list = xmalloc(sizeof(struct orom_entry));
+	list->orom = *orom;
+	list->devid_list = NULL;
+	list->next = NULL;
+
+	if (prev == NULL)
+		orom_entries = list;
+	else
+		prev->next = list;
+
+	return list;
 }
 
-static void add_orom_device_id(const struct imsm_orom *orom, __u16 dev_id)
+static void add_orom_device_id(struct orom_entry *entry, __u16 dev_id)
 {
-	int i;
 	struct devid_list *list;
 	struct devid_list *prev = NULL;
 
-	for (i = 0; i < SYS_DEV_MAX; i++) {
-		if (&oroms[i].orom == orom) {
-			for (list = oroms[i].devid_list; list; prev = list, list = list->next) {
-				if (list->devid == dev_id)
-					return;
-			}
-			list = xmalloc(sizeof(struct devid_list));
-			list->devid = dev_id;
-			list->next = NULL;
-
-			if (prev == NULL)
-				oroms[i].devid_list = list;
-			else
-				prev->next = list;
+	for (list = entry->devid_list; list; prev = list, list = list->next) {
+		if (list->devid == dev_id)
 			return;
-		}
 	}
+	list = xmalloc(sizeof(struct devid_list));
+	list->devid = dev_id;
+	list->next = NULL;
+
+	if (prev == NULL)
+		entry->devid_list = list;
+	else
+		prev->next = list;
 }
 
 static int scan(const void *start, const void *end, const void *data)
@@ -321,7 +317,7 @@ static int scan(const void *start, const void *end, const void *data)
 	if (!imsm_mem)
 		return 0;
 
-	const struct imsm_orom *orom = add_orom(imsm_mem);
+	struct orom_entry *orom = add_orom(imsm_mem);
 
 	if (ptr->devListOffset) {
 		const __u16 *dev_list = (void *)ptr + ptr->devListOffset;
@@ -367,11 +363,11 @@ const struct imsm_orom *imsm_platform_test(struct sys_dev *hba)
 				IMSM_OROM_RLC_RAID10;
 	}
 
-	const struct imsm_orom *ret = add_orom(&orom);
+	struct orom_entry *ret = add_orom(&orom);
 
 	add_orom_device_id(ret, hba->dev_id);
 
-	return ret;
+	return &ret->orom;
 }
 
 static const struct imsm_orom *find_imsm_hba_orom(struct sys_dev *hba)
@@ -508,7 +504,7 @@ static int read_efi_variable(void *buffer, ssize_t buf_size, char *variable_name
 const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
 {
 	struct imsm_orom orom;
-	const struct imsm_orom *ret;
+	struct orom_entry *ret;
 	int err;
 
 	if (check_env("IMSM_TEST_AHCI_EFI") || check_env("IMSM_TEST_SCU_EFI"))
@@ -529,14 +525,14 @@ const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
 
 	/* try to read variable for combined AHCI controllers */
 	if (err && hba->type == SYS_DEV_SATA) {
-		static const struct imsm_orom *csata;
+		static struct orom_entry *csata;
 
 		err = read_efi_variable(&orom, sizeof(orom), AHCI_CSATA_PROP, VENDOR_GUID);
 		if (!err) {
 			if (!csata)
 				csata = add_orom(&orom);
 			add_orom_device_id(csata, hba->dev_id);
-			return csata;
+			return &csata->orom;
 		}
 	}
 
@@ -546,12 +542,12 @@ const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
 	ret = add_orom(&orom);
 	add_orom_device_id(ret, hba->dev_id);
 
-	return ret;
+	return &ret->orom;
 }
 
 const struct imsm_orom *find_imsm_nvme(struct sys_dev *hba)
 {
-	static const struct imsm_orom *nvme_orom;
+	static struct orom_entry *nvme_orom;
 
 	if (hba->type != SYS_DEV_NVME)
 		return NULL;
@@ -574,7 +570,7 @@ const struct imsm_orom *find_imsm_nvme(struct sys_dev *hba)
 		nvme_orom = add_orom(&nvme_orom_compat);
 	}
 	add_orom_device_id(nvme_orom, hba->dev_id);
-	return nvme_orom;
+	return &nvme_orom->orom;
 }
 
 const struct imsm_orom *find_imsm_capability(struct sys_dev *hba)
diff --git a/platform-intel.h b/platform-intel.h
index 2ead431..631fa76 100644
--- a/platform-intel.h
+++ b/platform-intel.h
@@ -213,8 +213,11 @@ struct devid_list {
 struct orom_entry {
 	struct imsm_orom orom;
 	struct devid_list *devid_list;
+	struct orom_entry *next;
 };
 
+extern struct orom_entry *orom_entries;
+
 static inline char *guid_str(char *buf, struct efi_guid guid)
 {
 	sprintf(buf, "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
@@ -235,6 +238,5 @@ int devt_attached_to_hba(dev_t dev, const char *hba_path);
 char *devt_to_devpath(dev_t dev);
 int path_attached_to_hba(const char *disk_path, const char *hba_path);
 const char *get_sys_dev_type(enum sys_dev_type);
-const struct orom_entry * get_oroms(void);
 const struct imsm_orom *get_orom_by_device_id(__u16 device_id);
 struct sys_dev *device_by_id(__u16 device_id);
diff --git a/super-intel.c b/super-intel.c
index 819e0da..53269fd 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1948,13 +1948,12 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
 		return result;
 	}
 
-	const struct orom_entry *oroms = get_oroms();
-	int i;
+	const struct orom_entry *entry;
 
-	for (i = 0; i < SYS_DEV_MAX && oroms[i].devid_list; i++) {
-		print_imsm_capability(&oroms[i].orom);
+	for (entry = orom_entries; entry; entry = entry->next) {
+		print_imsm_capability(&entry->orom);
 
-		if (imsm_orom_is_nvme(&oroms[i].orom)) {
+		if (imsm_orom_is_nvme(&entry->orom)) {
 			for (hba = list; hba; hba = hba->next) {
 				if (hba->type == SYS_DEV_NVME)
 					printf("    NVMe Device : %s\n", hba->path);
@@ -1963,7 +1962,7 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
 		}
 
 		struct devid_list *devid;
-		for (devid = oroms[i].devid_list; devid; devid = devid->next) {
+		for (devid = entry->devid_list; devid; devid = devid->next) {
 			hba = device_by_id(devid->devid);
 			if (!hba)
 				continue;
@@ -2007,11 +2006,10 @@ static int export_detail_platform_imsm(int verbose, char *controller_path)
 			result = 0;
 	}
 
-	const struct orom_entry *oroms = get_oroms();
-	int i;
+	const struct orom_entry *entry;
 
-	for (i = 0; i < SYS_DEV_MAX && oroms[i].devid_list; i++)
-		print_imsm_capability_export(&oroms[i].orom);
+	for (entry = orom_entries; entry; entry = entry->next)
+		print_imsm_capability_export(&entry->orom);
 
 	return result;
 }
-- 
2.1.4

Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it

[Jes Sorensen]

Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
> On 02/25/2015 06:15 PM, Jes Sorensen wrote:
>> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>>> On 02/25/2015 01:29 PM, Jes Sorensen wrote:
>>>> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
>>>>> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
>>>>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>>>
>>>>>> This avoids adding the same orom entry to the oroms list multiple
>>>>>> times, as the comparison of pointers is never going to succeed, in
>>>>>> particular when '*orom' points to a local stack variable in the
>>>>>> calling function.
>>>>>>
>>>>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>>> ---
>>>>>>  platform-intel.c | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/platform-intel.c b/platform-intel.c
>>>>>> index 37274da..a4ffa9f 100644
>>>>>> --- a/platform-intel.c
>>>>>> +++ b/platform-intel.c
>>>>>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>>>>>  	int i;
>>>>>>  
>>>>>>  	for (i = 0; i < SYS_DEV_MAX; i++) {
>>>>>> -		if (&oroms[i].orom == orom)
>>>>>> -			return orom;
>>>>>> +		if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>>>>>> +			return &oroms[i].orom;
>>>>>>  		if (oroms[i].orom.signature[0] == 0) {
>>>>>>  			oroms[i].orom = *orom;
>>>>>>  			return &oroms[i].orom;
>>>>>>
>>>>>
>>>>> Hi Jes,
>>>>>
>>>>> You are right that this can add the same entry multiple times, but this
>>>>> is how it is supposed to work. The oroms list should contain all the
>>>>> platform's oroms and they can be the same, this is why memcmp() should
>>>>> not be used here. We don't want to compare the contents of the
>>>>> structure, just its address. Sorry if it's not clear.
>>>>
>>>> Artur,
>>>>
>>>> Then the code is fundamentally broken, since you end up comparing a
>>>> stack variable against the oroms array when you call it from
>>>> find_imsm_efi(). Worse you can end up returning the local stack variable
>>>> declared in find_imsm_efi() to the calling function - there is no way
>>>> that can be correct.
>>>>
>>>> Look at this:
>>>>
>>>> static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>>> {
>>>>         int i;
>>>>
>>>>         for (i = 0; i < SYS_DEV_MAX; i++) {
>>>>                 if (&oroms[i].orom == orom)
>>>>                         return orom;
>>>>                 if (oroms[i].orom.signature[0] == 0) {
>>>>                         oroms[i].orom = *orom;
>>>>                         return &oroms[i].orom;
>>>>                 }
>>>>         }
>>>>         return NULL;
>>>> }
>>>>
>>>> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>>>> {
>>>>         struct imsm_orom orom;
>>>>         const struct imsm_orom *ret;
>>>>         int err;
>>>>
>>>> ....
>>>>
>>>>         ret = add_orom(&orom);
>>>>         add_orom_device_id(ret, hba->dev_id);
>>>>
>>>>         return ret;
>>>> }
>>>
>>> I can't see how this can lead to returning a stack variable. The oroms
>>> array is global and add_orom() will always return a pointer to a struct
>>> in this array. This comparison will always fail when we pass a pointer
>>> to a stack variable to add_orom():
>>>
>>> if (&oroms[i].orom == orom)
>>> 	return orom;
>>>
>>> This was meant to prevent adding an orom again like this:
>>>
>>> ret = add_orom(&orom);
>>> add_orom(ret);
>>>
>>> Maybe it would be more appropriate to return NULL to indicate that
>>> nothing was added instead of returning back the same pointer. I can do a
>>> patch for this. What do you think?
>> 
>> It will fail because we know we're comparing a stack pointer, but it
>> raises red flags with tools like coverity and it is really bad coding
>> practice to rely on hacks like this.
>> 
>> I also don't understand why you want to keep a table of identical
>> entries in the orom structure if multiple identical entries are found.
>> Each entry ought to match onto a specific physical controller, unless I
>> get something wrong?
>> 
>
> OK, you're right, it is a hack. I thought it over and redesigned those
> orom functions. This should make it simpler and more consistent.

Looks a lot nicer to me :)

Jes

> Thanks,
> Artur
>
> From 673ecf1c0539f0050cc5934203af6d79cd68234d Mon Sep 17 00:00:00 2001
> From: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> Date: Fri, 27 Feb 2015 10:34:20 +0100
> Subject: [PATCH] imsm: simplified multiple OROMs support
>
> Replaced oroms array with list, add_orom() now only appends to this list
> and add_orom_device_id() only appends devid_list node to an orom_entry.
>
> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
>  platform-intel.c | 96 +++++++++++++++++++++++++++-----------------------------
>  platform-intel.h |  4 ++-
>  super-intel.c    | 18 +++++------
>  3 files changed, 57 insertions(+), 61 deletions(-)
>
> diff --git a/platform-intel.c b/platform-intel.c
> index 37274da..9c89c20 100644
> --- a/platform-intel.c
> +++ b/platform-intel.c
> @@ -229,65 +229,61 @@ struct pciExpDataStructFormat {
>  	__u16 devListOffset;
>  } __attribute__ ((packed));
>  
> -static struct orom_entry oroms[SYS_DEV_MAX];
> -
> -const struct orom_entry *get_oroms(void)
> -{
> -	return (const struct orom_entry *)&oroms;
> -}
> +struct orom_entry *orom_entries;
>  
>  const struct imsm_orom *get_orom_by_device_id(__u16 dev_id)
>  {
> -	int i;
> -	struct devid_list *list;
> +	struct orom_entry *entry;
> +	struct devid_list *devid;
>  
> -	for (i = 0; i < SYS_DEV_MAX; i++) {
> -		for (list = oroms[i].devid_list; list; list = list->next) {
> -			if (list->devid == dev_id)
> -				return &oroms[i].orom;
> +	for (entry = orom_entries; entry; entry = entry->next) {
> +		for (devid = entry->devid_list; devid; devid = devid->next) {
> +			if (devid->devid == dev_id)
> +				return &entry->orom;
>  		}
>  	}
> +
>  	return NULL;
>  }
>  
> -static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
> +static struct orom_entry *add_orom(const struct imsm_orom *orom)
>  {
> -	int i;
> -
> -	for (i = 0; i < SYS_DEV_MAX; i++) {
> -		if (&oroms[i].orom == orom)
> -			return orom;
> -		if (oroms[i].orom.signature[0] == 0) {
> -			oroms[i].orom = *orom;
> -			return &oroms[i].orom;
> -		}
> -	}
> -	return NULL;
> +	struct orom_entry *list;
> +	struct orom_entry *prev = NULL;
> +
> +	for (list = orom_entries; list; prev = list, list = list->next)
> +		;
> +
> +	list = xmalloc(sizeof(struct orom_entry));
> +	list->orom = *orom;
> +	list->devid_list = NULL;
> +	list->next = NULL;
> +
> +	if (prev == NULL)
> +		orom_entries = list;
> +	else
> +		prev->next = list;
> +
> +	return list;
>  }
>  
> -static void add_orom_device_id(const struct imsm_orom *orom, __u16 dev_id)
> +static void add_orom_device_id(struct orom_entry *entry, __u16 dev_id)
>  {
> -	int i;
>  	struct devid_list *list;
>  	struct devid_list *prev = NULL;
>  
> -	for (i = 0; i < SYS_DEV_MAX; i++) {
> -		if (&oroms[i].orom == orom) {
> -			for (list = oroms[i].devid_list; list; prev = list, list = list->next) {
> -				if (list->devid == dev_id)
> -					return;
> -			}
> -			list = xmalloc(sizeof(struct devid_list));
> -			list->devid = dev_id;
> -			list->next = NULL;
> -
> -			if (prev == NULL)
> -				oroms[i].devid_list = list;
> -			else
> -				prev->next = list;
> +	for (list = entry->devid_list; list; prev = list, list = list->next) {
> +		if (list->devid == dev_id)
>  			return;
> -		}
>  	}
> +	list = xmalloc(sizeof(struct devid_list));
> +	list->devid = dev_id;
> +	list->next = NULL;
> +
> +	if (prev == NULL)
> +		entry->devid_list = list;
> +	else
> +		prev->next = list;
>  }
>  
>  static int scan(const void *start, const void *end, const void *data)
> @@ -321,7 +317,7 @@ static int scan(const void *start, const void *end, const void *data)
>  	if (!imsm_mem)
>  		return 0;
>  
> -	const struct imsm_orom *orom = add_orom(imsm_mem);
> +	struct orom_entry *orom = add_orom(imsm_mem);
>  
>  	if (ptr->devListOffset) {
>  		const __u16 *dev_list = (void *)ptr + ptr->devListOffset;
> @@ -367,11 +363,11 @@ const struct imsm_orom *imsm_platform_test(struct sys_dev *hba)
>  				IMSM_OROM_RLC_RAID10;
>  	}
>  
> -	const struct imsm_orom *ret = add_orom(&orom);
> +	struct orom_entry *ret = add_orom(&orom);
>  
>  	add_orom_device_id(ret, hba->dev_id);
>  
> -	return ret;
> +	return &ret->orom;
>  }
>  
>  static const struct imsm_orom *find_imsm_hba_orom(struct sys_dev *hba)
> @@ -508,7 +504,7 @@ static int read_efi_variable(void *buffer, ssize_t buf_size, char *variable_name
>  const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>  {
>  	struct imsm_orom orom;
> -	const struct imsm_orom *ret;
> +	struct orom_entry *ret;
>  	int err;
>  
>  	if (check_env("IMSM_TEST_AHCI_EFI") || check_env("IMSM_TEST_SCU_EFI"))
> @@ -529,14 +525,14 @@ const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>  
>  	/* try to read variable for combined AHCI controllers */
>  	if (err && hba->type == SYS_DEV_SATA) {
> -		static const struct imsm_orom *csata;
> +		static struct orom_entry *csata;
>  
>  		err = read_efi_variable(&orom, sizeof(orom), AHCI_CSATA_PROP, VENDOR_GUID);
>  		if (!err) {
>  			if (!csata)
>  				csata = add_orom(&orom);
>  			add_orom_device_id(csata, hba->dev_id);
> -			return csata;
> +			return &csata->orom;
>  		}
>  	}
>  
> @@ -546,12 +542,12 @@ const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>  	ret = add_orom(&orom);
>  	add_orom_device_id(ret, hba->dev_id);
>  
> -	return ret;
> +	return &ret->orom;
>  }
>  
>  const struct imsm_orom *find_imsm_nvme(struct sys_dev *hba)
>  {
> -	static const struct imsm_orom *nvme_orom;
> +	static struct orom_entry *nvme_orom;
>  
>  	if (hba->type != SYS_DEV_NVME)
>  		return NULL;
> @@ -574,7 +570,7 @@ const struct imsm_orom *find_imsm_nvme(struct sys_dev *hba)
>  		nvme_orom = add_orom(&nvme_orom_compat);
>  	}
>  	add_orom_device_id(nvme_orom, hba->dev_id);
> -	return nvme_orom;
> +	return &nvme_orom->orom;
>  }
>  
>  const struct imsm_orom *find_imsm_capability(struct sys_dev *hba)
> diff --git a/platform-intel.h b/platform-intel.h
> index 2ead431..631fa76 100644
> --- a/platform-intel.h
> +++ b/platform-intel.h
> @@ -213,8 +213,11 @@ struct devid_list {
>  struct orom_entry {
>  	struct imsm_orom orom;
>  	struct devid_list *devid_list;
> +	struct orom_entry *next;
>  };
>  
> +extern struct orom_entry *orom_entries;
> +
>  static inline char *guid_str(char *buf, struct efi_guid guid)
>  {
>  	sprintf(buf, "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> @@ -235,6 +238,5 @@ int devt_attached_to_hba(dev_t dev, const char *hba_path);
>  char *devt_to_devpath(dev_t dev);
>  int path_attached_to_hba(const char *disk_path, const char *hba_path);
>  const char *get_sys_dev_type(enum sys_dev_type);
> -const struct orom_entry * get_oroms(void);
>  const struct imsm_orom *get_orom_by_device_id(__u16 device_id);
>  struct sys_dev *device_by_id(__u16 device_id);
> diff --git a/super-intel.c b/super-intel.c
> index 819e0da..53269fd 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -1948,13 +1948,12 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
>  		return result;
>  	}
>  
> -	const struct orom_entry *oroms = get_oroms();
> -	int i;
> +	const struct orom_entry *entry;
>  
> -	for (i = 0; i < SYS_DEV_MAX && oroms[i].devid_list; i++) {
> -		print_imsm_capability(&oroms[i].orom);
> +	for (entry = orom_entries; entry; entry = entry->next) {
> +		print_imsm_capability(&entry->orom);
>  
> -		if (imsm_orom_is_nvme(&oroms[i].orom)) {
> +		if (imsm_orom_is_nvme(&entry->orom)) {
>  			for (hba = list; hba; hba = hba->next) {
>  				if (hba->type == SYS_DEV_NVME)
>  					printf("    NVMe Device : %s\n", hba->path);
> @@ -1963,7 +1962,7 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
>  		}
>  
>  		struct devid_list *devid;
> -		for (devid = oroms[i].devid_list; devid; devid = devid->next) {
> +		for (devid = entry->devid_list; devid; devid = devid->next) {
>  			hba = device_by_id(devid->devid);
>  			if (!hba)
>  				continue;
> @@ -2007,11 +2006,10 @@ static int export_detail_platform_imsm(int verbose, char *controller_path)
>  			result = 0;
>  	}
>  
> -	const struct orom_entry *oroms = get_oroms();
> -	int i;
> +	const struct orom_entry *entry;
>  
> -	for (i = 0; i < SYS_DEV_MAX && oroms[i].devid_list; i++)
> -		print_imsm_capability_export(&oroms[i].orom);
> +	for (entry = orom_entries; entry; entry = entry->next)
> +		print_imsm_capability_export(&entry->orom);
>  
>  	return result;
>  }

Re: [PATCH 3/5] add_orom(): Compare content of struct imsm_orom rather than pointers to it

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 13155 bytes --]

On Fri, 27 Feb 2015 14:39:42 +0100 Artur Paszkiewicz
<artur.paszkiewicz@intel.com> wrote:

> On 02/25/2015 06:15 PM, Jes Sorensen wrote:
> > Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
> >> On 02/25/2015 01:29 PM, Jes Sorensen wrote:
> >>> Artur Paszkiewicz <artur.paszkiewicz@intel.com> writes:
> >>>> On 02/24/2015 10:00 PM, Jes.Sorensen@redhat.com wrote:
> >>>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >>>>>
> >>>>> This avoids adding the same orom entry to the oroms list multiple
> >>>>> times, as the comparison of pointers is never going to succeed, in
> >>>>> particular when '*orom' points to a local stack variable in the
> >>>>> calling function.
> >>>>>
> >>>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> >>>>> ---
> >>>>>  platform-intel.c | 4 ++--
> >>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/platform-intel.c b/platform-intel.c
> >>>>> index 37274da..a4ffa9f 100644
> >>>>> --- a/platform-intel.c
> >>>>> +++ b/platform-intel.c
> >>>>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
> >>>>>  	int i;
> >>>>>  
> >>>>>  	for (i = 0; i < SYS_DEV_MAX; i++) {
> >>>>> -		if (&oroms[i].orom == orom)
> >>>>> -			return orom;
> >>>>> +		if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
> >>>>> +			return &oroms[i].orom;
> >>>>>  		if (oroms[i].orom.signature[0] == 0) {
> >>>>>  			oroms[i].orom = *orom;
> >>>>>  			return &oroms[i].orom;
> >>>>>
> >>>>
> >>>> Hi Jes,
> >>>>
> >>>> You are right that this can add the same entry multiple times, but this
> >>>> is how it is supposed to work. The oroms list should contain all the
> >>>> platform's oroms and they can be the same, this is why memcmp() should
> >>>> not be used here. We don't want to compare the contents of the
> >>>> structure, just its address. Sorry if it's not clear.
> >>>
> >>> Artur,
> >>>
> >>> Then the code is fundamentally broken, since you end up comparing a
> >>> stack variable against the oroms array when you call it from
> >>> find_imsm_efi(). Worse you can end up returning the local stack variable
> >>> declared in find_imsm_efi() to the calling function - there is no way
> >>> that can be correct.
> >>>
> >>> Look at this:
> >>>
> >>> static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
> >>> {
> >>>         int i;
> >>>
> >>>         for (i = 0; i < SYS_DEV_MAX; i++) {
> >>>                 if (&oroms[i].orom == orom)
> >>>                         return orom;
> >>>                 if (oroms[i].orom.signature[0] == 0) {
> >>>                         oroms[i].orom = *orom;
> >>>                         return &oroms[i].orom;
> >>>                 }
> >>>         }
> >>>         return NULL;
> >>> }
> >>>
> >>> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
> >>> {
> >>>         struct imsm_orom orom;
> >>>         const struct imsm_orom *ret;
> >>>         int err;
> >>>
> >>> ....
> >>>
> >>>         ret = add_orom(&orom);
> >>>         add_orom_device_id(ret, hba->dev_id);
> >>>
> >>>         return ret;
> >>> }
> >>
> >> I can't see how this can lead to returning a stack variable. The oroms
> >> array is global and add_orom() will always return a pointer to a struct
> >> in this array. This comparison will always fail when we pass a pointer
> >> to a stack variable to add_orom():
> >>
> >> if (&oroms[i].orom == orom)
> >> 	return orom;
> >>
> >> This was meant to prevent adding an orom again like this:
> >>
> >> ret = add_orom(&orom);
> >> add_orom(ret);
> >>
> >> Maybe it would be more appropriate to return NULL to indicate that
> >> nothing was added instead of returning back the same pointer. I can do a
> >> patch for this. What do you think?
> > 
> > It will fail because we know we're comparing a stack pointer, but it
> > raises red flags with tools like coverity and it is really bad coding
> > practice to rely on hacks like this.
> > 
> > I also don't understand why you want to keep a table of identical
> > entries in the orom structure if multiple identical entries are found.
> > Each entry ought to match onto a specific physical controller, unless I
> > get something wrong?
> > 
> 
> OK, you're right, it is a hack. I thought it over and redesigned those
> orom functions. This should make it simpler and more consistent.
> 
> Thanks,
> Artur
> 
> >From 673ecf1c0539f0050cc5934203af6d79cd68234d Mon Sep 17 00:00:00 2001
> From: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> Date: Fri, 27 Feb 2015 10:34:20 +0100
> Subject: [PATCH] imsm: simplified multiple OROMs support
> 
> Replaced oroms array with list, add_orom() now only appends to this list
> and add_orom_device_id() only appends devid_list node to an orom_entry.
> 
> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
>  platform-intel.c | 96 +++++++++++++++++++++++++++-----------------------------
>  platform-intel.h |  4 ++-
>  super-intel.c    | 18 +++++------
>  3 files changed, 57 insertions(+), 61 deletions(-)
> 
> diff --git a/platform-intel.c b/platform-intel.c
> index 37274da..9c89c20 100644
> --- a/platform-intel.c
> +++ b/platform-intel.c
> @@ -229,65 +229,61 @@ struct pciExpDataStructFormat {
>  	__u16 devListOffset;
>  } __attribute__ ((packed));
>  
> -static struct orom_entry oroms[SYS_DEV_MAX];
> -
> -const struct orom_entry *get_oroms(void)
> -{
> -	return (const struct orom_entry *)&oroms;
> -}
> +struct orom_entry *orom_entries;
>  
>  const struct imsm_orom *get_orom_by_device_id(__u16 dev_id)
>  {
> -	int i;
> -	struct devid_list *list;
> +	struct orom_entry *entry;
> +	struct devid_list *devid;
>  
> -	for (i = 0; i < SYS_DEV_MAX; i++) {
> -		for (list = oroms[i].devid_list; list; list = list->next) {
> -			if (list->devid == dev_id)
> -				return &oroms[i].orom;
> +	for (entry = orom_entries; entry; entry = entry->next) {
> +		for (devid = entry->devid_list; devid; devid = devid->next) {
> +			if (devid->devid == dev_id)
> +				return &entry->orom;
>  		}
>  	}
> +
>  	return NULL;
>  }
>  
> -static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
> +static struct orom_entry *add_orom(const struct imsm_orom *orom)
>  {
> -	int i;
> -
> -	for (i = 0; i < SYS_DEV_MAX; i++) {
> -		if (&oroms[i].orom == orom)
> -			return orom;
> -		if (oroms[i].orom.signature[0] == 0) {
> -			oroms[i].orom = *orom;
> -			return &oroms[i].orom;
> -		}
> -	}
> -	return NULL;
> +	struct orom_entry *list;
> +	struct orom_entry *prev = NULL;
> +
> +	for (list = orom_entries; list; prev = list, list = list->next)
> +		;
> +
> +	list = xmalloc(sizeof(struct orom_entry));
> +	list->orom = *orom;
> +	list->devid_list = NULL;
> +	list->next = NULL;
> +
> +	if (prev == NULL)
> +		orom_entries = list;
> +	else
> +		prev->next = list;
> +
> +	return list;
>  }
>  
> -static void add_orom_device_id(const struct imsm_orom *orom, __u16 dev_id)
> +static void add_orom_device_id(struct orom_entry *entry, __u16 dev_id)
>  {
> -	int i;
>  	struct devid_list *list;
>  	struct devid_list *prev = NULL;
>  
> -	for (i = 0; i < SYS_DEV_MAX; i++) {
> -		if (&oroms[i].orom == orom) {
> -			for (list = oroms[i].devid_list; list; prev = list, list = list->next) {
> -				if (list->devid == dev_id)
> -					return;
> -			}
> -			list = xmalloc(sizeof(struct devid_list));
> -			list->devid = dev_id;
> -			list->next = NULL;
> -
> -			if (prev == NULL)
> -				oroms[i].devid_list = list;
> -			else
> -				prev->next = list;
> +	for (list = entry->devid_list; list; prev = list, list = list->next) {
> +		if (list->devid == dev_id)
>  			return;
> -		}
>  	}
> +	list = xmalloc(sizeof(struct devid_list));
> +	list->devid = dev_id;
> +	list->next = NULL;
> +
> +	if (prev == NULL)
> +		entry->devid_list = list;
> +	else
> +		prev->next = list;
>  }
>  
>  static int scan(const void *start, const void *end, const void *data)
> @@ -321,7 +317,7 @@ static int scan(const void *start, const void *end, const void *data)
>  	if (!imsm_mem)
>  		return 0;
>  
> -	const struct imsm_orom *orom = add_orom(imsm_mem);
> +	struct orom_entry *orom = add_orom(imsm_mem);
>  
>  	if (ptr->devListOffset) {
>  		const __u16 *dev_list = (void *)ptr + ptr->devListOffset;
> @@ -367,11 +363,11 @@ const struct imsm_orom *imsm_platform_test(struct sys_dev *hba)
>  				IMSM_OROM_RLC_RAID10;
>  	}
>  
> -	const struct imsm_orom *ret = add_orom(&orom);
> +	struct orom_entry *ret = add_orom(&orom);
>  
>  	add_orom_device_id(ret, hba->dev_id);
>  
> -	return ret;
> +	return &ret->orom;
>  }
>  
>  static const struct imsm_orom *find_imsm_hba_orom(struct sys_dev *hba)
> @@ -508,7 +504,7 @@ static int read_efi_variable(void *buffer, ssize_t buf_size, char *variable_name
>  const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>  {
>  	struct imsm_orom orom;
> -	const struct imsm_orom *ret;
> +	struct orom_entry *ret;
>  	int err;
>  
>  	if (check_env("IMSM_TEST_AHCI_EFI") || check_env("IMSM_TEST_SCU_EFI"))
> @@ -529,14 +525,14 @@ const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>  
>  	/* try to read variable for combined AHCI controllers */
>  	if (err && hba->type == SYS_DEV_SATA) {
> -		static const struct imsm_orom *csata;
> +		static struct orom_entry *csata;
>  
>  		err = read_efi_variable(&orom, sizeof(orom), AHCI_CSATA_PROP, VENDOR_GUID);
>  		if (!err) {
>  			if (!csata)
>  				csata = add_orom(&orom);
>  			add_orom_device_id(csata, hba->dev_id);
> -			return csata;
> +			return &csata->orom;
>  		}
>  	}
>  
> @@ -546,12 +542,12 @@ const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>  	ret = add_orom(&orom);
>  	add_orom_device_id(ret, hba->dev_id);
>  
> -	return ret;
> +	return &ret->orom;
>  }
>  
>  const struct imsm_orom *find_imsm_nvme(struct sys_dev *hba)
>  {
> -	static const struct imsm_orom *nvme_orom;
> +	static struct orom_entry *nvme_orom;
>  
>  	if (hba->type != SYS_DEV_NVME)
>  		return NULL;
> @@ -574,7 +570,7 @@ const struct imsm_orom *find_imsm_nvme(struct sys_dev *hba)
>  		nvme_orom = add_orom(&nvme_orom_compat);
>  	}
>  	add_orom_device_id(nvme_orom, hba->dev_id);
> -	return nvme_orom;
> +	return &nvme_orom->orom;
>  }
>  
>  const struct imsm_orom *find_imsm_capability(struct sys_dev *hba)
> diff --git a/platform-intel.h b/platform-intel.h
> index 2ead431..631fa76 100644
> --- a/platform-intel.h
> +++ b/platform-intel.h
> @@ -213,8 +213,11 @@ struct devid_list {
>  struct orom_entry {
>  	struct imsm_orom orom;
>  	struct devid_list *devid_list;
> +	struct orom_entry *next;
>  };
>  
> +extern struct orom_entry *orom_entries;
> +
>  static inline char *guid_str(char *buf, struct efi_guid guid)
>  {
>  	sprintf(buf, "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> @@ -235,6 +238,5 @@ int devt_attached_to_hba(dev_t dev, const char *hba_path);
>  char *devt_to_devpath(dev_t dev);
>  int path_attached_to_hba(const char *disk_path, const char *hba_path);
>  const char *get_sys_dev_type(enum sys_dev_type);
> -const struct orom_entry * get_oroms(void);
>  const struct imsm_orom *get_orom_by_device_id(__u16 device_id);
>  struct sys_dev *device_by_id(__u16 device_id);
> diff --git a/super-intel.c b/super-intel.c
> index 819e0da..53269fd 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -1948,13 +1948,12 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
>  		return result;
>  	}
>  
> -	const struct orom_entry *oroms = get_oroms();
> -	int i;
> +	const struct orom_entry *entry;
>  
> -	for (i = 0; i < SYS_DEV_MAX && oroms[i].devid_list; i++) {
> -		print_imsm_capability(&oroms[i].orom);
> +	for (entry = orom_entries; entry; entry = entry->next) {
> +		print_imsm_capability(&entry->orom);
>  
> -		if (imsm_orom_is_nvme(&oroms[i].orom)) {
> +		if (imsm_orom_is_nvme(&entry->orom)) {
>  			for (hba = list; hba; hba = hba->next) {
>  				if (hba->type == SYS_DEV_NVME)
>  					printf("    NVMe Device : %s\n", hba->path);
> @@ -1963,7 +1962,7 @@ static int detail_platform_imsm(int verbose, int enumerate_only, char *controlle
>  		}
>  
>  		struct devid_list *devid;
> -		for (devid = oroms[i].devid_list; devid; devid = devid->next) {
> +		for (devid = entry->devid_list; devid; devid = devid->next) {
>  			hba = device_by_id(devid->devid);
>  			if (!hba)
>  				continue;
> @@ -2007,11 +2006,10 @@ static int export_detail_platform_imsm(int verbose, char *controller_path)
>  			result = 0;
>  	}
>  
> -	const struct orom_entry *oroms = get_oroms();
> -	int i;
> +	const struct orom_entry *entry;
>  
> -	for (i = 0; i < SYS_DEV_MAX && oroms[i].devid_list; i++)
> -		print_imsm_capability_export(&oroms[i].orom);
> +	for (entry = orom_entries; entry; entry = entry->next)
> +		print_imsm_capability_export(&entry->orom);
>  
>  	return result;
>  }


Applied, thanks.

NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH 4/5] IncrementalScan(): Make sure 'st' is valid before dereferencing it

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Incremental.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Incremental.c b/Incremental.c
index 87d9114..33c0d7f 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -1354,7 +1354,7 @@ restart:
 			if (st && st->ss->load_container)
 				ret = st->ss->load_container(st, mdfd, NULL);
 			close(mdfd);
-			if (!ret && st->ss->container_content) {
+			if (!ret && st && st->ss->container_content) {
 				if (map_lock(&map))
 					pr_err("failed to get exclusive lock on mapfile\n");
 				ret = Incremental_container(st, me->path, c, only);
-- 
2.1.0

Re: [PATCH 4/5] IncrementalScan(): Make sure 'st' is valid before dereferencing it

[John Stoffel]

>>>>> "Jes" == Jes Sorensen <Jes.Sorensen@redhat.com> writes:

Jes> From: Jes Sorensen <Jes.Sorensen@redhat.com>
Jes> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Jes> ---
Jes>  Incremental.c | 2 +-
Jes>  1 file changed, 1 insertion(+), 1 deletion(-)

Jes> diff --git a/Incremental.c b/Incremental.c
Jes> index 87d9114..33c0d7f 100644
Jes> --- a/Incremental.c
Jes> +++ b/Incremental.c
Jes> @@ -1354,7 +1354,7 @@ restart:
Jes>  			if (st && st->ss->load_container)
Jes>  				ret = st->ss->load_container(st, mdfd, NULL);
Jes>  			close(mdfd);
Jes> -			if (!ret && st->ss->container_content) {
Jes> +			if (!ret && st && st->ss->container_content) {
Jes>  				if (map_lock(&map))
Jes>  					pr_err("failed to get exclusive lock on mapfile\n");
Jes>  				ret = Incremental_container(st, me->path, c, only);
Jes> -- 
Jes> 2.1.0

Forgive my stupidity, but how does this really help anything?  You
already did the check above for a valid 'st', and now you're just
repeating it.  Maybe if needs to be more of:

  if (st) {
    if (st->ss->load_container)
       ret = st->ss->load_container(st,mdfd, NULL);
    close(mdfd);
    if (!ret && st->ss->container_content) {
      .....
    }
  }

but maybe I'm just missing something here. 

John

  

Re: [PATCH 4/5] IncrementalScan(): Make sure 'st' is valid before dereferencing it

[Jes Sorensen]

"John Stoffel" <john@stoffel.org> writes:
>>>>>> "Jes" == Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>
> Jes> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> Jes> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> Jes> ---
> Jes>  Incremental.c | 2 +-
> Jes>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Jes> diff --git a/Incremental.c b/Incremental.c
> Jes> index 87d9114..33c0d7f 100644
> Jes> --- a/Incremental.c
> Jes> +++ b/Incremental.c
> Jes> @@ -1354,7 +1354,7 @@ restart:
> Jes>  			if (st && st->ss->load_container)
> Jes>  				ret = st->ss->load_container(st, mdfd, NULL);
> Jes>  			close(mdfd);
> Jes> -			if (!ret && st->ss->container_content) {
> Jes> +			if (!ret && st && st->ss->container_content) {
> Jes>  				if (map_lock(&map))
> Jes>  					pr_err("failed to get exclusive lock on mapfile\n");
> Jes>  				ret = Incremental_container(st, me->path, c, only);
> Jes> -- 
> Jes> 2.1.0
>
> Forgive my stupidity, but how does this really help anything?  You
> already did the check above for a valid 'st', and now you're just
> repeating it.  Maybe if needs to be more of:
>
>   if (st) {
>     if (st->ss->load_container)
>        ret = st->ss->load_container(st,mdfd, NULL);
>     close(mdfd);
>     if (!ret && st->ss->container_content) {
>       .....
>     }
>   }
>
> but maybe I'm just missing something here. 

Please look more carefully - the checks above are in place so 'st' is
only dereferenced if 'st is valid. The code does not bail out if
st = NULL.

Your example results in mdfd not getting closed if st == NULL.

Jes

Re: [PATCH 4/5] IncrementalScan(): Make sure 'st' is valid before dereferencing it

[John Stoffel]

>>>>> "Jes" == Jes Sorensen <Jes.Sorensen@redhat.com> writes:

Jes> "John Stoffel" <john@stoffel.org> writes:
>>>>>>> "Jes" == Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>> 
Jes> From: Jes Sorensen <Jes.Sorensen@redhat.com>
Jes> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Jes> ---
Jes> Incremental.c | 2 +-
Jes> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
Jes> diff --git a/Incremental.c b/Incremental.c
Jes> index 87d9114..33c0d7f 100644
Jes> --- a/Incremental.c
Jes> +++ b/Incremental.c
Jes> @@ -1354,7 +1354,7 @@ restart:
Jes> if (st && st->ss->load_container)
Jes> ret = st->ss->load_container(st, mdfd, NULL);
Jes> close(mdfd);
Jes> -			if (!ret && st->ss->container_content) {
Jes> +			if (!ret && st && st->ss->container_content) {
Jes> if (map_lock(&map))
Jes> pr_err("failed to get exclusive lock on mapfile\n");
Jes> ret = Incremental_container(st, me->path, c, only);
Jes> -- 
Jes> 2.1.0
>> 
>> Forgive my stupidity, but how does this really help anything?  You
>> already did the check above for a valid 'st', and now you're just
>> repeating it.  Maybe if needs to be more of:
>> 
>> if (st) {
>> if (st->ss->load_container)
>> ret = st->ss->load_container(st,mdfd, NULL);
>> close(mdfd);
>> if (!ret && st->ss->container_content) {
>> .....
>> }
>> }
>> 
>> but maybe I'm just missing something here. 

Jes> Please look more carefully - the checks above are in place so 'st' is
Jes> only dereferenced if 'st is valid. The code does not bail out if
Jes> st = NULL.

Jes> Your example results in mdfd not getting closed if st == NULL.

Right, I agree my example isn't perfect either, but I was just
commenting more that I think the check for st not being NULL should be
performed once, instead of multiple times.  The closing the mdfd is
just a detail of the structure of the code.  It's a nitpick I agree...

[PATCH 5/5] write_super_imsm_spares(): C statements are terminated by ;

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 super-intel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 819e0da..7f75b53 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5115,13 +5115,13 @@ static int write_super_imsm_spares(struct intel_super *super, int doclose)
 	__u32 sum;
 	struct dl *d;
 
-	spare->mpb_size = __cpu_to_le32(sizeof(struct imsm_super)),
-	spare->generation_num = __cpu_to_le32(1UL),
+	spare->mpb_size = __cpu_to_le32(sizeof(struct imsm_super));
+	spare->generation_num = __cpu_to_le32(1UL);
 	spare->attributes = MPB_ATTRIB_CHECKSUM_VERIFY;
-	spare->num_disks = 1,
-	spare->num_raid_devs = 0,
-	spare->cache_size = mpb->cache_size,
-	spare->pwr_cycle_count = __cpu_to_le32(1),
+	spare->num_disks = 1;
+	spare->num_raid_devs = 0;
+	spare->cache_size = mpb->cache_size;
+	spare->pwr_cycle_count = __cpu_to_le32(1);
 
 	snprintf((char *) spare->sig, MAX_SIGNATURE_LENGTH,
 		 MPB_SIGNATURE MPB_VERSION_RAID0);
-- 
2.1.0