linux-kernel - Re: 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <26467a3c-00e7-7358-5087-bf35469feb06@redhat.com>
Date:   Wed, 25 Oct 2017 19:28:57 +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: iio/accel/bmc150: Improve unlocking of a mutex in two functions

Hi,

On 25-10-17 18:58, SF Markus Elfring wrote:
>> 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.
> 
> Thanks for your quick feedback.
> 
> 
>>> 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,
> 
> I would prefer the approach above so that an extra goto statement
> could also be avoided before.

Usually code in a function follows a pattern of:

	err = step1()
	if (err)
		handle-err


	err = step2()
	if (err)
		handle-err


	err = step3()
	if (err)
		handle-err

What you are suggesting breaks this pattern (not
using a goto in the last if (err) case) which makes
the code harder to read and makes things harder
(and potentially leads to introducing bugs) when
a step4() gets added.

>> 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.
> 
> Do any more contributors insist on such a code layout?

There definitely is some personal preference involved here,
but I do believe that consistency is more important then
saving a goto here.

>>> How long should I wait for corresponding feedback before another small
>>> source code adjustment will be appropriate?
>>
>> That is hard to say.
> 
> I am just curious on how we can achieve progress here.
> 
> 
>> I usually just do a new version when I've time,
> 
> This is generally fine.
> 
> 
>> 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.
> 
> I asked also because other well-known contributors showed strong
> reactions for this change pattern (with the help of a script
> for the semantic patch language).
> Would you care for similar updates in source files like the following?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id=1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760

So I just checked this one, this one is tricky because the
lock is taking inside a switch-case and doing gotos inside
the case is not pretty. If I were to refactor this, I would
add an "if (mask == IIO_CHAN_INFO_SCALE) {}" block to
handle the unlocked scale case and then take the lock around
the entire switch-case, using breaks on error to jump to
the unlock after the switch-case without needing gotos.

To me this seems the right thing here, since the scale is
special here in that it does not need locking. Or optionally
one can just take the lock for scale regardless, it won't
hurt (much).

Basically I believe there is no one-size fits all solution
here and refactoring like this may introduce bugs, so one
needs to weight the amount of work + regression risk vs
the benefits of the code being cleaner going forward.

Regards,

Hans



> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/stk8312.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n432
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/adc/palmas_gpadc.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n304
> 
> Regards,
> Markus
>