linux-kernel - Re: [Intel-wired-lan] [PATCH iwl-net v2 1/3] igc: Fix qbv_config_change

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: <00161b3e-44cf-4335-af66-85ffecd673ae@intel.com>
Date: Thu, 1 Aug 2024 10:32:42 +0300
From: Mor Bar-Gabay <morx.bar.gabay@...el.com>
To: Faizal Rahim <faizal.abdul.rahim@...ux.intel.com>, "David S . Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Jesse Brandeburg
	<jesse.brandeburg@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Vinicius Costa Gomes <vinicius.gomes@...el.com>, Simon Horman
	<horms@...nel.org>, Richard Cochran <richardcochran@...il.com>, Paul Menzel
	<pmenzel@...gen.mpg.de>, Sasha Neftin <sasha.neftin@...el.com>
CC: <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
	<linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-net v2 1/3] igc: Fix
 qbv_config_change_errors logics

On 07/07/2024 15:53, Faizal Rahim wrote:
> When user issues these cmds:
> 1. Either a) or b)
>     a) mqprio with hardware offload disabled
>     b) taprio with txtime-assist feature enabled
> 2. etf
> 3. tc qdisc delete
> 4. taprio with base time in the past
> 
> At step 4, qbv_config_change_errors wrongly increased by 1.
> 
> Excerpt from IEEE 802.1Q-2018 8.6.9.3.1:
> "If AdminBaseTime specifies a time in the past, and the current schedule
> is running, then: Increment ConfigChangeError counter"
> 
> qbv_config_change_errors should only increase if base time is in the past
> and no taprio is active. In user perspective, taprio was not active when
> first triggered at step 4. However, i225/6 reuses qbv for etf, so qbv is
> enabled with a dummy schedule at step 2 where it enters
> igc_tsn_enable_offload() and qbv_count got incremented to 1. At step 4, it
> enters igc_tsn_enable_offload() again, qbv_count is incremented to 2.
> Because taprio is running, tc_setup_type is TC_SETUP_QDISC_ETF and
> qbv_count > 1, qbv_config_change_errors value got incremented.
> 
> This issue happens due to reliance on qbv_count field where a non-zero
> value indicates that taprio is running. But qbv_count increases
> regardless if taprio is triggered by user or by other tsn feature. It does
> not align with qbv_config_change_errors expectation where it is only
> concerned with taprio triggered by user.
> 
> Fixing this by relocating the qbv_config_change_errors logic to
> igc_save_qbv_schedule(), eliminating reliance on qbv_count and its
> inaccuracies from i225/6's multiple uses of qbv feature for other TSN
> features.
> 
> The new function created: igc_tsn_is_taprio_activated_by_user() uses
> taprio_offload_enable field to indicate that the current running taprio
> was triggered by user, instead of triggered by non-qbv feature like etf.
> 
> Fixes: ae4fe4698300 ("igc: Add qbv_config_change_errors counter")
> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@...ux.intel.com>
> Reviewed-by: Simon Horman <horms@...nel.org>
> Acked-by: Vinicius Costa Gomes <vinicius.gomes@...el.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c |  8 ++++++--
>   drivers/net/ethernet/intel/igc/igc_tsn.c  | 16 ++++++++--------
>   drivers/net/ethernet/intel/igc/igc_tsn.h  |  1 +
>   3 files changed, 15 insertions(+), 10 deletions(-)
> 
Tested-by: Mor Bar-Gabay <morx.bar.gabay@...el.com>