linux-kernel - Re: [PATCH] iio/accel/bmc150: Improve unlocking of a mutex in two functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Oct 2017 18:22:02 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     SF Markus Elfring <elfring@...rs.sourceforge.net>,
        linux-iio@...r.kernel.org
Cc:     Hartmut Knaack <knaack.h@....de>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] iio/accel/bmc150: Improve unlocking of a mutex in two
 functions

Hi,

On 25-10-17 18:15, SF Markus Elfring wrote:
>> IMHO, if you do this, you should rework the function so that there is a single unlock call
>> at the end, not a separate one in in error label.
> 
> Thanks for your update suggestion.
> 
> Does it indicate that I may propose similar source code adjustments
> in this software area?
> 
> 
>> Could e.g. change this:
>>
>>          ret = bmc150_accel_set_power_state(data, false);
>>          mutex_unlock(&data->mutex);
>>          if (ret < 0)
>>                  return ret;
>>
>>          return IIO_VAL_INT;
>> }
>>
>> To:
>>
>>          ret = bmc150_accel_set_power_state(data, false);
>>          if (ret < 0)
>>                  goto unlock;
>>
>>      ret = IIO_VAL_INT;

If that is the only unlock in the function, then it is probably
best to keep things as is. In general gotos are considered
better then multiple unlocks, but not having either is even
better.

> How do you think about to use the following code variant then?
> 
> 	if (!ret)
> 		ret = IIO_VAL_INT;

I believe the goto unlock variant and setting ret = IIO_VAL_INT;
directly above the unlock label variant is better, because that
way the error handling is consistent between all steps and if
another step is later added at the end, the last step will
not require modification.

>> unlock:
>>          mutex_unlock(&data->mutex);
>>
>>          return ret;
>> }
>>
>> And also use the unlock label in the other cases, this is actually
>> quite a normal pattern. I see little use in a patch like this if there
>> are still 2 unlock paths after the patch.
> 
> How long should I wait for corresponding feedback before another small
> source code adjustment will be appropriate?

That is hard to say. I usually just do a new version when I've time,
seldomly someone complains I should have waited longer for feedback
(when I'm quite quick) but usually sending out a new version as soon
as you've time to work on a new version is best, since if you wait
you may then not have time for the entire next week or so, at least
that is my experience :)  There is really no clear rule here.

Regards,

Hans