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: <7005af42-e546-6a7c-015f-037a5f0e08a9@linutronix.de>
Date: Tue, 11 Jul 2023 10:37:48 +0200
From: Florian Kauer <florian.kauer@...utronix.de>
To: Leon Romanovsky <leon@...nel.org>,
 Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, netdev@...r.kernel.org, kurt@...utronix.de,
 vinicius.gomes@...el.com, muhammad.husaini.zulkifli@...el.com,
 tee.min.tan@...ux.intel.com, aravindhan.gunasekaran@...el.com,
 sasha.neftin@...el.com, Naama Meir <naamax.meir@...ux.intel.com>
Subject: Re: [PATCH net 5/6] igc: Fix launchtime before start of cycle

On 11.07.23 09:09, Leon Romanovsky wrote:
> On Mon, Jul 10, 2023 at 09:35:02AM -0700, Tony Nguyen wrote:
>> From: Florian Kauer <florian.kauer@...utronix.de>
>>
>> It is possible (verified on a running system) that frames are processed
>> by igc_tx_launchtime with a txtime before the start of the cycle
>> (baset_est).
>>
>> However, the result of txtime - baset_est is written into a u32,
>> leading to a wrap around to a positive number. The following
>> launchtime > 0 check will only branch to executing launchtime = 0
>> if launchtime is already 0.
>>
>> Fix it by using a s32 before checking launchtime > 0.
>>
>> Fixes: db0b124f02ba ("igc: Enhance Qbv scheduling by using first flag bit")
>> Signed-off-by: Florian Kauer <florian.kauer@...utronix.de>
>> Reviewed-by: Kurt Kanzenbach <kurt@...utronix.de>
>> Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
>> ---
>>  drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 5d24930fed8f..4855caa3bae4 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -1016,7 +1016,7 @@ static __le32 igc_tx_launchtime(struct igc_ring *ring, ktime_t txtime,
>>  	ktime_t base_time = adapter->base_time;
>>  	ktime_t now = ktime_get_clocktai();
>>  	ktime_t baset_est, end_of_cycle;
>> -	u32 launchtime;
>> +	s32 launchtime;
> 
> The rest of igc_tx_launchtime() function is very questionable,
> as ktime_sub_ns() returns ktime_t which is s64.
> 
>   1049         launchtime = ktime_sub_ns(txtime, baset_est);
>   1050         if (launchtime > 0)
>   1051                 div_s64_rem(launchtime, cycle_time, &launchtime);
>   1052         else
>   1053                 launchtime = 0;
>   1054
>   1055         return cpu_to_le32(launchtime);
> 

If I understand correctly, ktime_sub_ns(txtime, baset_est) will only return
something larger than s32 max if cycle_time is larger than s32 max and if that
is the case everything will be broken anyway since the corresponding hardware
register only holds 30 bits.

However, I do not see on first inspection where that case is properly handled
(probably just by rejecting the TAPRIO schedule).

Can someone with more experience in that area please jump in?

Thanks,
Florian

> 
>>  	s64 n;
>>  
>>  	n = div64_s64(ktime_sub_ns(now, base_time), cycle_time);
>> -- 
>> 2.38.1
>>
>>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ