linux-kernel - Re: [PATCH v2 1/3] watchdog: wdat_wdg: Using the existed function to check parameter timeout

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: <57d4e920-2571-4496-59ac-1bfc27cc74b4@roeck-us.net>
Date:   Mon, 25 Apr 2022 20:35:21 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Tzung-Bi Shih <tzungbi@...nel.org>,
        Liu Xinpeng <liuxp11@...natelecom.cn>
Cc:     wim@...ux-watchdog.org, linux-watchdog@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] watchdog: wdat_wdg: Using the existed function to
 check parameter timeout

On 4/25/22 20:04, Tzung-Bi Shih wrote:
> On Mon, Apr 25, 2022 at 04:22:10PM +0800, Liu Xinpeng wrote:
>>   #define MAX_WDAT_ACTIONS ACPI_WDAT_ACTION_RESERVED
>> +#define WDAT_TIMEOUT_MIN     1
>>   
>>   /**
>>    * struct wdat_instruction - Single ACPI WDAT instruction
>> @@ -344,6 +345,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>>   	wdat->period = tbl->timer_period;
>>   	wdat->wdd.min_hw_heartbeat_ms = wdat->period * tbl->min_count;
>>   	wdat->wdd.max_hw_heartbeat_ms = wdat->period * tbl->max_count;
>> +	wdat->wdd.min_timeout = WDAT_TIMEOUT_MIN;
>>   	wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
>>   	wdat->wdd.info = &wdat_wdt_info;
>>   	wdat->wdd.ops = &wdat_wdt_ops;
>> @@ -450,8 +452,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
>>   	 * watchdog properly after it has opened the device. In some cases
>>   	 * the BIOS default is too short and causes immediate reboot.
>>   	 */
>> -	if (timeout * 1000 < wdat->wdd.min_hw_heartbeat_ms ||
>> -	    timeout * 1000 > wdat->wdd.max_hw_heartbeat_ms) {
>> +	if (watchdog_timeout_invalid(&wdat->wdd, timeout)) {
> 
> Probably lacking of the context, I failed to see why the checks are
> equivalent.  Could you provide more information?

Not exactly equivalent; this is a fix.

Context: If max_hw_heartbeat_ms is provided, the configured maximum timeout
is not limited by it. The limit check in this driver therefore doesn't make
much sense. Similar, the watchdog core ensures that minimum timeout limits
are met if min_hw_heartbeat_ms is set. Using watchdog_timeout_invalid()
makes more sense because it takes this into account.

Guenter