| Message ID | 20230710163503.2821068-6-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c1bca9ac0bcb355be11354c2e68bc7bf31f5ac5a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | igc: Fix corner cases for TSN offload | expand |
On Mon, Jul 10, 2023 at 09:35:02AM -0700, Tony Nguyen wrote: > From: Florian Kauer <florian.kauer@linutronix.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@linutronix.de> > Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> > Tested-by: Naama Meir <naamax.meir@linux.intel.com> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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); > s64 n; > > n = div64_s64(ktime_sub_ns(now, base_time), cycle_time); > -- > 2.38.1 > >
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@linutronix.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@linutronix.de> >> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> >> Tested-by: Naama Meir <naamax.meir@linux.intel.com> >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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 >> >>
On Tue, Jul 11, 2023 at 10:37:48AM +0200, Florian Kauer wrote: > 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@linutronix.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@linutronix.de> > >> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> > >> Tested-by: Naama Meir <naamax.meir@linux.intel.com> > >> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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. I suggest you to use proper variable types, what about the following snippet? ktime_t launchtime; launchtime = ktime_sub_ns(txtime, baset_est); WARN_ON(upper_32_bits(launchtime)); div_s64_rem(launchtime, cycle_time, &launchtime); return cpu_to_le32(lower_32_bits(launchtime)); > > 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 > >> > >> >
On Tue, 11 Jul 2023 13:12:33 +0300 Leon Romanovsky wrote: > > 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. > > I suggest you to use proper variable types, what about the following > snippet? > > ktime_t launchtime; > > launchtime = ktime_sub_ns(txtime, baset_est); > WARN_ON(upper_32_bits(launchtime)); > div_s64_rem(launchtime, cycle_time, &launchtime); > > return cpu_to_le32(lower_32_bits(launchtime)); That needs to be coupled with some control path checks on cycle_time? Seems like a separate fix TBH.
On Tue, Jul 11, 2023 at 05:54:54PM -0700, Jakub Kicinski wrote: > On Tue, 11 Jul 2023 13:12:33 +0300 Leon Romanovsky wrote: > > > 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. > > > > I suggest you to use proper variable types, what about the following > > snippet? > > > > ktime_t launchtime; > > > > launchtime = ktime_sub_ns(txtime, baset_est); > > WARN_ON(upper_32_bits(launchtime)); > > div_s64_rem(launchtime, cycle_time, &launchtime); > > > > return cpu_to_le32(lower_32_bits(launchtime)); > > That needs to be coupled with some control path checks on cycle_time? I think so, as I didn't find any checks which protect from overflow. Thanks
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; s64 n; n = div64_s64(ktime_sub_ns(now, base_time), cycle_time);