Hi Ursula,
On Mon, 10 Sep 2018 16:03:23 +0200, Ursula Braun wrote:
> On 09/10/2018 11:09 AM, Jean Delvare wrote:
> > Functions qeth_get_ipa_msg and qeth_get_ipa_cmd_name are modifying
> > the last member of global arrays without any locking that I can see.
> > If two instances of either function are running at the same time,
> > it could cause a race ultimately leading to an array overrun (the
> > contents of the last entry of the array is the only guarantee that
> > the loop will ever stop).
> >
> > Performing the lookups without modifying the arrays is admittedly
> > slower (two comparisons per iteration instead of one) but these
> > are operations which are rare (should only be needed in error
> > cases or when debugging, not during successful operation) and it
> > seems still less costly than introducing a mutex to protect the
> > arrays in question.
> >
> > As a side bonus, it allows us to declare both arrays as const data.
> >
> > Signed-off-by: Jean Delvare <jdelvare@xxxxxxx>
> > Cc: Julian Wiedmann <jwi@xxxxxxxxxxxxx>
> > Cc: Ursula Braun <ubraun@xxxxxxxxxxxxx>
> > Cc: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
> > Cc: Heiko Carstens <heiko.carstens@xxxxxxxxxx>
> > ---
> > Note: build-tested only.
> > (...)
> > -static struct ipa_cmd_names qeth_ipa_cmd_names[] = {
> > +static const struct ipa_cmd_names qeth_ipa_cmd_names[] = {
> > {IPA_CMD_STARTLAN, "startlan"},
> > {IPA_CMD_STOPLAN, "stoplan"},
> > {IPA_CMD_SETVMAC, "setvmac"},
> > @@ -263,14 +264,14 @@ static struct ipa_cmd_names qeth_ipa_cmd
> > {IPA_CMD_REGISTER_LOCAL_ADDR, "register_local_addr"},
> > {IPA_CMD_UNREGISTER_LOCAL_ADDR, "unregister_local_addr"},
> > {IPA_CMD_ADDRESS_CHANGE_NOTIF, "address_change_notification"},
> > - {IPA_CMD_UNKNOWN, "unknown"},
>
> Why is this line removed? Doesn't the last line of function qeth_get_ipa_cmd_name()
> still refer to this line?
> And if IPA_CMD_UNKNOWN is really removed, then make sure its definition is removed
> from qeth_core_mpc.h as well.
That's a bug in the patch, sorry about that, I will resend :(
In an earlier version of the patch, IPA_CMD_UNKNOWN was removed, but
then I found it was more elegant to keep it so that the code in
qeth_get_ipa_cmd_name() and qeth_get_ipa_msg() would be the same. I
restored the definition in qeth_core_mpc.h but forgot to also restore
the last entry of qeth_ipa_cmd_names[].
Thanks for catching it and sorry again,
--
Jean Delvare
SUSE L3 Support
