netdev - RE: [PATCH iwl-next v3] i40e: add ability to reset vf for tx and rx mdd events

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: <SJ0PR11MB58667E28D6BB2B3CB663032FE5472@SJ0PR11MB5866.namprd11.prod.outlook.com>
Date: Thu, 17 Oct 2024 10:02:05 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>, "Fijalkowski,
 Maciej" <maciej.fijalkowski@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Sokolowski, Jan" <jan.sokolowski@...el.com>,
	"Connolly, Padraig J" <padraig.j.connolly@...el.com>
Subject: RE: [PATCH iwl-next v3] i40e: add ability to reset vf for tx and rx
 mdd events



> -----Original Message-----
> From: Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>
> Sent: Tuesday, September 10, 2024 1:11 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>;
> Fijalkowski, Maciej <maciej.fijalkowski@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; netdev@...r.kernel.org; Sokolowski,
> Jan <jan.sokolowski@...el.com>; Connolly, Padraig J
> <padraig.j.connolly@...el.com>
> Subject: Re: [PATCH iwl-next v3] i40e: add ability to reset vf for
> tx and rx mdd events
> 
> On 9/10/24 10:29, Loktionov, Aleksandr wrote:
> >> From: Fijalkowski, Maciej <maciej.fijalkowski@...el.com>
> >> Sent: Tuesday, September 3, 2024 9:59 PM
> >> To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>
> >> Cc: intel-wired-lan@...ts.osuosl.org; Nguyen, Anthony L
> >> <anthony.l.nguyen@...el.com>; netdev@...r.kernel.org;
> Sokolowski, Jan
> >> <jan.sokolowski@...el.com>; Connolly, Padraig J
> >> <padraig.j.connolly@...el.com>
> >> Subject: Re: [PATCH iwl-next v3] i40e: add ability to reset vf
> for tx
> >> and rx mdd events
> 
> please capitalize acronyms (Tx, Rx, VF, MDD, PF) (also in the
> subject line, but sent next version as v4).
> 
> >>
> >> On Fri, Aug 30, 2024 at 09:28:07PM +0200, Aleksandr Loktionov
> wrote:
> >>> In cases when vf sends malformed packets that are classified as
> >>> malicious, sometimes it causes tx queue to freeze. This frozen
> queue
> >>> can be stuck for several minutes being unusable. When mdd event
> >>> occurs, there is a posibility to perform a graceful vf reset to
> >>> quickly bring vf back to operational state.
> >>>
> >>> Currently vf iqueues are being disabled if mdd event occurs.
> >>> Add the ability to reset vf if tx or rx mdd occurs.
> >>> Add mdd events logging throttling /* avoid dmesg polution */.
> >>> Unify tx rx mdd messages formats.
> >>>
> >>> Co-developed-by: Jan Sokolowski <jan.sokolowski@...el.com>
> >>> Signed-off-by: Jan Sokolowski <jan.sokolowski@...el.com>
> >>> Co-developed-by: Padraig J Connolly
> <padraig.j.connolly@...el.com>
> >>> Signed-off-by:  Padraig J Connolly
> <padraig.j.connolly@...el.com>
> >>> Signed-off-by: Aleksandr Loktionov
> <aleksandr.loktionov@...el.com>
> 
> Next time please wait for review on our internal e1000-mailing-
> list.
> Feel free to ping me directly if there will be no one engaged in
> any future series of yours.
> 
> >>> ---
> >>> v2->v3 fix compilation issue
> >>> v1->v2 fix compilation issue
> >>> ---
> >>>   drivers/net/ethernet/intel/i40e/i40e.h        |   4 +-
> >>>   .../net/ethernet/intel/i40e/i40e_debugfs.c    |   2 +-
> >>>   .../net/ethernet/intel/i40e/i40e_ethtool.c    |   2 +
> >>>   drivers/net/ethernet/intel/i40e/i40e_main.c   | 116
> >> ++++++++++++++++--
> >>>   .../ethernet/intel/i40e/i40e_virtchnl_pf.c    |   2 +-
> >>>   .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  11 +-
> >>>   6 files changed, 122 insertions(+), 15 deletions(-)
> >>>
> 
> 
> >>> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> >>> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> >>> @@ -459,6 +459,8 @@ static const struct i40e_priv_flags
> >> i40e_gstrings_priv_flags[] = {
> >>>   	I40E_PRIV_FLAG("base-r-fec", I40E_FLAG_BASE_R_FEC, 0),
> >>>   	I40E_PRIV_FLAG("vf-vlan-pruning",
> >>>   		       I40E_FLAG_VF_VLAN_PRUNING_ENA, 0),
> >>> +	I40E_PRIV_FLAG("mdd-auto-reset-vf",
> >>> +		       I40E_FLAG_MDD_AUTO_RESET_VF, 0),
> >>
> >> you don't tell us that this is implemented via priv-flag in the
> >> commit message, would be good to include info about it.
> > This flag is implemented for other network adapters like ice, we
> thought it's kind of standard.
> > Can you suggest what exact part to change? Please be concrete.
> > Thank you
> 
> priv-flag is not a standard, by definition what we do in intel
> drivers is also not necessarily a standard
> 
> keeping the code quality as-is should be rather seen as an
> allowance for legacy drivers, instead of something that should be
> copy-pasted yet again. But commit messages are different, you need
> to obey the current standard, which is simply: describe non-obvious
> things, describe more if asked during review. Please do so :)
> 
> >>> +static void i40e_print_vf_rx_mdd_event(struct i40e_pf *pf,
> struct
> >>> +i40e_vf *vf) {
> >>> +	dev_err(&pf->pdev->dev, "%lld Rx Malicious Driver Detection
> >> events detected on PF %d VF %d MAC %pm. mdd-auto-reset-
> vfs=%s\n",
> >>> +		vf->mdd_rx_events.count,
> >>> +		pf->hw.pf_id,
> >>> +		vf->vf_id,
> >>> +		vf->default_lan_addr.addr,
> >>> +		test_bit(I40E_FLAG_MDD_AUTO_RESET_VF, pf->flags) ? "on"
> :
> >> "off"); }
> >>> +
> >>> +/**
> >>> + * i40e_print_vf_tx_mdd_event - print VF Tx malicious driver
> detect
> >>> +event
> >>> + * @pf: board private structure
> >>> + * @vf: pointer to the VF structure  */ static void
> >>> +i40e_print_vf_tx_mdd_event(struct i40e_pf *pf, struct i40e_vf
> *vf)
> >>> +{
> >>> +	dev_err(&pf->pdev->dev, "%lld Tx Malicious Driver Detection
> >> events detected on PF %d VF %d MAC %pm. mdd-auto-reset-
> vfs=%s\n",
> >>> +		vf->mdd_tx_events.count,
> >>> +		pf->hw.pf_id,
> >>> +		vf->vf_id,
> >>> +		vf->default_lan_addr.addr,
> >>> +		test_bit(I40E_FLAG_MDD_AUTO_RESET_VF, pf->flags) ? "on"
> :
> >> "off"); }
> >>
> >> Unnecesary code duplication, two functions with printing the
> very
> >> same statement with a single different letter.
> > But it's easy to grep and find as required by linux coding
> standards.
> 
> You could reword to have it still obvious what to grep for, like:
> 
> Malicious Driver Detected an Event, PF: %d, VF: %d, MAC: %pm, dir:
> %s...
> 
> with the last %s being "Tx" or "Rx"
> (note: I didn't copied all your stuff as this is just an example)
> 
> >>> +
> >>> +	/* VF MDD event logs are rate limited to one second intervals
> */
> >>> +	if (time_is_after_jiffies(pf->last_printed_mdd_jiffies + HZ *
> >> 1))
> >>> +		return;
> >>> +
> >>> +	pf->last_printed_mdd_jiffies = jiffies;
> >>
> >> why homegrown rate limiting?
> > Because it works! And other ideas probably didn't.
> > What is your suggestion exactly? Please be concrete.
> 
> dev_info_ratelimited()
It doesn't fit requirements. We need custom rate limiting because it needs to be re-set after each HW reset for example.