Artur Paszkiewicz <artur.paszkiewicz@xxxxxxxxx> writes:
> On 02/24/2015 10:00 PM, Jes.Sorensen@xxxxxxxxxx wrote:
>> From: Jes Sorensen <Jes.Sorensen@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
