linux-kernel - Re: [PATCH v1 1/4] iio: afe: rescale: Don't use ^ for booleans

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] [day] [month] [year] [list]

Message-ID: <631f114e-c59f-ca69-6873-09dad1694913@axentia.se>
Date: Fri, 6 Dec 2024 23:27:52 +0100
From: Peter Rosin <peda@...ntia.se>
To: David Laight <David.Laight@...LAB.COM>,
 'Andy Shevchenko' <andriy.shevchenko@...ux.intel.com>
Cc: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>
Subject: Re: [PATCH v1 1/4] iio: afe: rescale: Don't use ^ for booleans

Hi!

2024-12-06 at 21:13, David Laight wrote:
> From: 'Andy Shevchenko'
>> Sent: 06 December 2024 15:20
> ...
>> ...
>>
>>>>  		 * If only one of the rescaler elements or the schan scale is
>>>>  		 * negative, the combined scale is negative.
>>>>  		 */
>>>> -		if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
>>>> +		if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) {
>>>
>>> That is wrong, the || would also need to be !=.
>>
>> Why do you think so? Maybe it's comment(s) that is(are) wrong?
> 
> The old code certainly negates for each of them - so you can get the double
> and triple negative cases.

Indeed. And for me, xor is the natural operator for getting to such
"oddness" when three or more values needs to be considered. So, I
prefer to keep the code as is since a ^ b ^ c is nice and succinct,
while anything you try to write using != is going to be convoluted
when you have three or more values.

> So believe the code not the comment.

I claim that the comment is correct. Or at least not incorrect. It might
not be complete, but what it does state holds. It does not spell out that
the combined scale is positive if both of the rescaler elements and the
schan scale are positive. It does not spell out that the combined scale
is negative if all three are negative. What it does give you though, is
a hint that the whole if () is all about the sign of the combined scale.

But yes, the comment could be improved.

I expect a fail from this test in iio-test-rescale.c with the new code

	{
		.name = "negative IIO_VAL_INT_PLUS_NANO, 3 negative",
		.numerator = -1000000,
		.denominator = -8060,
		.schan_scale_type = IIO_VAL_INT_PLUS_NANO,
		.schan_val = -10,
		.schan_val2 = 123456,
		.expected = "-1240.710106203",
	},

but the 0-day has been silent. I wonder why? Does it not actually
run the tests?

There could also be some more tests to try make sure the logic doesn't
regress. The first of these should also fail with this patch, while
the second should be ok.

	{
		.name = "positive IIO_VAL_INT_PLUS_MICRO, 2 negative",
		.numerator = -1,
		.denominator = -2,
		.schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
		.schan_val = 5,
		.schan_val2 = 1234,
		.expected = "2.500617",
	},
	{
		.name = "positive IIO_VAL_INT_PLUS_MICRO, 2 negative",
		.numerator = 1,
		.denominator = -2,
		.schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
		.schan_val = -5,
		.schan_val2 = 1234,
		.expected = "2.500617",
	},

Cheers,
Peter