| 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
| ||
|
Message-ID: <20230818111049.GY22185@unreal>
Date: Fri, 18 Aug 2023 14:10:49 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Paul M Stillwell Jr <paul.m.stillwell.jr@...el.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
netdev@...r.kernel.org, jacob.e.keller@...el.com, horms@...nel.org,
Pucha Himasekhar Reddy <himasekharx.reddy.pucha@...el.com>
Subject: Re: [PATCH net-next v3 2/5] ice: configure FW logging
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.
>
> > > +void ice_debugfs_fwlog_init(struct ice_pf *pf);
> > > +void ice_debugfs_init(void);
> > > +void ice_debugfs_exit(void);
> > > +void ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module);
> > > +#else
> > > +static inline void ice_debugfs_fwlog_init(struct ice_pf *pf) { }
> > > +static inline void ice_debugfs_init(void) { }
> > > +static inline void ice_debugfs_exit(void) { }
> > > +static void
> > > +ice_pf_fwlog_update_module(struct ice_pf *pf, int log_level, int module)
> > > +{
> > > + return -EOPNOTSUPP;
> > > +}
> > > +#endif /* CONFIG_DEBUG_FS */
> >
> > Thanks
>
Powered by blists - more mailing lists