On 02/25/2015 01:29 PM, Jes Sorensen wrote:
> 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;
> }
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
--
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
