| 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
| ||
|
Date: Tue, 22 Aug 2023 20:45:39 +0000
From: "Keller, Jacob E" <jacob.e.keller@...el.com>
To: "Stillwell Jr, Paul M" <paul.m.stillwell.jr@...el.com>, "Kitszel,
Przemyslaw" <przemyslaw.kitszel@...el.com>, Leon Romanovsky <leon@...nel.org>
CC: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "davem@...emloft.net"
<davem@...emloft.net>, "kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>, "edumazet@...gle.com"
<edumazet@...gle.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"horms@...nel.org" <horms@...nel.org>, "Pucha, HimasekharX Reddy"
<himasekharx.reddy.pucha@...el.com>
Subject: RE: [PATCH net-next v3 2/5] ice: configure FW logging
> -----Original Message-----
> From: Stillwell Jr, Paul M <paul.m.stillwell.jr@...el.com>
> Sent: Monday, August 21, 2023 4:21 PM
> To: Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>; Leon Romanovsky
> <leon@...nel.org>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; davem@...emloft.net;
> kuba@...nel.org; pabeni@...hat.com; edumazet@...gle.com;
> netdev@...r.kernel.org; Keller, Jacob E <jacob.e.keller@...el.com>;
> horms@...nel.org; Pucha, HimasekharX Reddy
> <himasekharx.reddy.pucha@...el.com>
> Subject: Re: [PATCH net-next v3 2/5] ice: configure FW logging
>
> On 8/18/2023 5:31 AM, Przemek Kitszel wrote:
> > On 8/18/23 13:10, Leon Romanovsky wrote:
> >> On Thu, Aug 17, 2023 at 02:25:34PM -0700, Paul M Stillwell Jr wrote:
> >>> On 8/15/2023 11:38 AM, Leon Romanovsky wrote:
> >>>> On Tue, Aug 15, 2023 at 09:57:47AM -0700, Tony Nguyen wrote:
> >>>>> From: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com>
> >>>>>
> >>>>> Users want the ability to debug FW issues by retrieving the
> >>>>> FW logs from the E8xx devices. Use debugfs to allow the user to
> >>>>> read/write the FW log configuration by adding a 'fwlog/modules' file.
> >>>>> Reading the file will show either the currently running
> >>>>> configuration or
> >>>>> the next configuration (if the user has changed the configuration).
> >>>>> Writing to the file will update the configuration, but NOT enable the
> >>>>> configuration (that is a separate command).
> >>>>>
> >>>>> To see the status of FW logging then read the 'fwlog/modules' file
> >>>>> like
> >>>>> this:
> >>>>>
> >>>>> cat /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> >>>>>
> >>>>> To change the configuration of FW logging then write to the
> >>>>> 'fwlog/modules'
> >>>>> file like this:
> >>>>>
> >>>>> echo DCB NORMAL >
> /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/modules
> >>>>>
> >>>>> The format to change the configuration is
> >>>>>
> >>>>> echo <fwlog_module> <fwlog_level> > /sys/kernel/debug/ice/<pci device
> >>>>
> >>>> This line is truncated, it is not clear where you are writing.
> >>>
> >>> Good catch, I don't know how I missed this... Will fix
> >>>
> >>>> And more general question, a long time ago, netdev had a policy of
> >>>> not-allowing writing to debugfs, was it changed?
> >>>>
> >>>
> >>> I had this same thought and it seems like there were 2 concerns in
> >>> the past
> >>
> >> Maybe, I'm not enough time in netdev world to know the history.
> >>
> >>>
> >>> 1. Having a single file that was read/write with lots of commands going
> >>> through it
> >>> 2. Having code in the driver to parse the text from the commands that
> >>> was
> >>> error/security prone
> >>>
> >>> We have addressed this in 2 ways:
> >>> 1. Split the commands into multiple files that have a single purpose
> >>> 2. Use kernel parsing functions for anything where we *have* to pass
> >>> text to
> >>> decode
> >>>
> >>>>>
> >>>>> where
> >>>>>
> >>>>> * fwlog_level is a name as described below. Each level includes the
> >>>>> messages from the previous/lower level
> >>>>>
> >>>>> * NONE
> >>>>> * ERROR
> >>>>> * WARNING
> >>>>> * NORMAL
> >>>>> * VERBOSE
> >>>>>
> >>>>> * fwlog_event is a name that represents the module to receive
> >>>>> events for.
> >>>>> The module names are
> >>>>>
> >>>>> * GENERAL
> >>>>> * CTRL
> >>>>> * LINK
> >>>>> * LINK_TOPO
> >>>>> * DNL
> >>>>> * I2C
> >>>>> * SDP
> >>>>> * MDIO
> >>>>> * ADMINQ
> >>>>> * HDMA
> >>>>> * LLDP
> >>>>> * DCBX
> >>>>> * DCB
> >>>>> * XLR
> >>>>> * NVM
> >>>>> * AUTH
> >>>>> * VPD
> >>>>> * IOSF
> >>>>> * PARSER
> >>>>> * SW
> >>>>> * SCHEDULER
> >>>>> * TXQ
> >>>>> * RSVD
> >>>>> * POST
> >>>>> * WATCHDOG
> >>>>> * TASK_DISPATCH
> >>>>> * MNG
> >>>>> * SYNCE
> >>>>> * HEALTH
> >>>>> * TSDRV
> >>>>> * PFREG
> >>>>> * MDLVER
> >>>>> * ALL
> >>>>>
> >>>>> The name ALL is special and specifies setting all of the modules to
> >>>>> the
> >>>>> specified fwlog_level.
> >>>>>
> >>>>> If the NVM supports FW logging then the file 'fwlog' will be created
> >>>>> under the PCI device ID for the ice driver. If the file does not exist
> >>>>> then either the NVM doesn't support FW logging or debugfs is not
> >>>>> enabled
> >>>>> on the system.
> >>>>>
> >>>>> In addition to configuring the modules, the user can also configure
> >>>>> the
> >>>>> number of log messages (resolution) to include in a single Admin
> >>>>> Receive
> >>>>> Queue (ARQ) event.The range is 1-128 (1 means push every log
> >>>>> message, 128
> >>>>> means push only when the max AQ command buffer is full). The suggested
> >>>>> value is 10.
> >>>>>
> >>>>> To see/change the resolution the user can read/write the
> >>>>> 'fwlog/resolution' file. An example changing the value to 50 is
> >>>>>
> >>>>> echo 50 > /sys/kernel/debug/ice/0000\:18\:00.0/fwlog/resolution
> >>>>>
> >>>>> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com>
> >>>>> Tested-by: Pucha Himasekhar Reddy
> >>>>> <himasekharx.reddy.pucha@...el.com> (A Contingent worker at Intel)
> >>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> >>>>> ---
> >>>>> drivers/net/ethernet/intel/ice/Makefile | 4 +-
> >>>>> drivers/net/ethernet/intel/ice/ice.h | 18 +
> >>>>> .../net/ethernet/intel/ice/ice_adminq_cmd.h | 80 ++++
> >>>>> drivers/net/ethernet/intel/ice/ice_common.c | 5 +
> >>>>> drivers/net/ethernet/intel/ice/ice_debugfs.c | 450
> >>>>> ++++++++++++++++++
> >>>>> drivers/net/ethernet/intel/ice/ice_fwlog.c | 231 +++++++++
> >>>>> drivers/net/ethernet/intel/ice/ice_fwlog.h | 55 +++
> >>>>> drivers/net/ethernet/intel/ice/ice_main.c | 21 +
> >>>>> drivers/net/ethernet/intel/ice/ice_type.h | 4 +
> >>>>> 9 files changed, 867 insertions(+), 1 deletion(-)
> >>>>> create mode 100644 drivers/net/ethernet/intel/ice/ice_debugfs.c
> >>>>> create mode 100644 drivers/net/ethernet/intel/ice/ice_fwlog.c
> >>>>> create mode 100644 drivers/net/ethernet/intel/ice/ice_fwlog.h
> >>>>>
> >>>>> diff --git a/drivers/net/ethernet/intel/ice/Makefile
> >>>>> b/drivers/net/ethernet/intel/ice/Makefile
> >>>>> index 960277d78e09..d43a59e5f8ee 100644
> >>>>> --- a/drivers/net/ethernet/intel/ice/Makefile
> >>>>> +++ b/drivers/net/ethernet/intel/ice/Makefile
> >>>>> @@ -34,7 +34,8 @@ ice-y := ice_main.o \
> >>>>> ice_lag.o \
> >>>>> ice_ethtool.o \
> >>>>> ice_repr.o \
> >>>>> - ice_tc_lib.o
> >>>>> + ice_tc_lib.o \
> >>>>> + ice_fwlog.o
> >>>>> ice-$(CONFIG_PCI_IOV) += \
> >>>>> ice_sriov.o \
> >>>>> ice_virtchnl.o \
> >>>>> @@ -49,3 +50,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
> >>>>> ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
> >>>>> ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
> >>>>> ice-$(CONFIG_GNSS) += ice_gnss.o
> >>>>> +ice-$(CONFIG_DEBUG_FS) += ice_debugfs.o
> >>>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h
> >>>>> b/drivers/net/ethernet/intel/ice/ice.h
> >>>>> index 5ac0ad12f9f1..e6dd9f6f9eee 100644
> >>>>> --- a/drivers/net/ethernet/intel/ice/ice.h
> >>>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
> >>>>> @@ -556,6 +556,8 @@ struct ice_pf {
> >>>>> struct ice_vsi_stats **vsi_stats;
> >>>>> struct ice_sw *first_sw; /* first switch created by
> >>>>> firmware */
> >>>>> u16 eswitch_mode; /* current mode of eswitch */
> >>>>> + struct dentry *ice_debugfs_pf;
> >>>>> + struct dentry *ice_debugfs_pf_fwlog;
> >>>>> struct ice_vfs vfs;
> >>>>> DECLARE_BITMAP(features, ICE_F_MAX);
> >>>>> DECLARE_BITMAP(state, ICE_STATE_NBITS);
> >>>>> @@ -861,6 +863,22 @@ static inline bool ice_is_adq_active(struct
> >>>>> ice_pf *pf)
> >>>>> return false;
> >>>>> }
> >>>>> +#ifdef CONFIG_DEBUG_FS
> >>>>
> >>>> There is no need in this CONFIG_DEBUG_FS and code should be written
> >>>> without debugfs stubs.
> >>>>
> >>>
> >>> I don't understand this comment... If the kernel is configured *without*
> >>> debugfs, won't the kernel fail to compile due to missing functions if we
> >>> don't do this?
> >>
> >> It will work fine, see include/linux/debugfs.h.
> >
> > Nice, as-is impl of ice_debugfs_fwlog_init() would just fail on first
> > debugfs API call.
> >
>
> I've thought about this some more and I am confused what to do. In the
> Makefile there is a bit that removes ice_debugfs.o if CONFIG_DEBUG_FS is
> not set. This would result in the stubs being needed (since the
> functions are called from ice_fwlog.c). In this case the code would not
> compile (since the functions would be missing). Should I remove the code
> from the Makefile that deals with ice_debugfs.o (which doesn't make
> sense since then there will be code in the driver that doesn't get used)
> or do I leave the stubs in?
>
Or, since ice_fwlog depends on debugfs support, also strip the fwlog.o object when CONFIG_DEBUGFS=n.
Thanks.
Jake
Powered by blists - more mailing lists