netdev - Re: [net-next 09/15] net/mlx5: Create FW devlink health reporter

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: <1bd839d1adea5fe999ecdd2d31b31936789ce58e.camel@mellanox.com>
Date:   Mon, 6 May 2019 19:52:18 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     Moshe Shemesh <moshe@...lanox.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>
CC:     Eran Ben Elisha <eranbe@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [net-next 09/15] net/mlx5: Create FW devlink health reporter

On Mon, 2019-05-06 at 13:38 +0200, Jiri Pirko wrote:
> Mon, May 06, 2019 at 12:45:44PM CEST, moshe@...lanox.com wrote:
> > 
> > On 5/5/2019 6:42 PM, Jiri Pirko wrote:
> > > Sun, May 05, 2019 at 02:33:23AM CEST, saeedm@...lanox.com wrote:
> > > > From: Moshe Shemesh <moshe@...lanox.com>
> > > > 
> > > > Create mlx5_devlink_health_reporter for FW reporter. The FW
> > > > reporter
> > > > implements devlink_health_reporter diagnose callback.
> > > > 
> > > > The fw reporter diagnose command can be triggered any time by
> > > > the user
> > > > to check current fw status.
> > > > In healthy status, it will return clear syndrome. Otherwise it
> > > > will dump
> > > > the health info buffer.
> > > > 
> > > > Command example and output on healthy status:
> > > > $ devlink health diagnose pci/0000:82:00.0 reporter fw
> > > > Syndrome: 0
> > > > 
> > > > Command example and output on non healthy status:
> > > > $ devlink health diagnose pci/0000:82:00.0 reporter fw
> > > > diagnose data:
> > > > assert_var[0] 0xfc3fc043
> > > > assert_var[1] 0x0001b41c
> > > > assert_var[2] 0x00000000
> > > > assert_var[3] 0x00000000
> > > > assert_var[4] 0x00000000
> > > > assert_exit_ptr 0x008033b4
> > > > assert_callra 0x0080365c
> > > > fw_ver 16.24.1000
> > > > hw_id 0x0000020d
> > > > irisc_index 0
> > > > synd 0x8: unrecoverable hardware error
> > > > ext_synd 0x003d
> > > > raw fw_ver 0x101803e8
> > > > 
> > > > Signed-off-by: Moshe Shemesh <moshe@...lanox.com>
> > > > Signed-off-by: Eran Ben Elisha <eranbe@...lanox.com>
> > > > Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
> > > 
> > > 	
> > > [...]	
> > > 	
> > > 	
> > > > +static int
> > > > +mlx5_fw_reporter_diagnose(struct devlink_health_reporter
> > > > *reporter,
> > > > +			  struct devlink_fmsg *fmsg)
> > > > +{
> > > > +	struct mlx5_core_dev *dev =
> > > > devlink_health_reporter_priv(reporter);
> > > > +	struct mlx5_core_health *health = &dev->priv.health;
> > > > +	u8 synd;
> > > > +	int err;
> > > > +
> > > > +	mutex_lock(&health->info_buf_lock);
> > > > +	mlx5_get_health_info(dev, &synd);
> > > > +
> > > > +	if (!synd) {
> > > > +		mutex_unlock(&health->info_buf_lock);
> > > > +		return devlink_fmsg_u8_pair_put(fmsg,
> > > > "Syndrome", synd);
> > > > +	}
> > > > +
> > > > +	err = devlink_fmsg_string_pair_put(fmsg, "diagnose
> > > > data",
> > > > +					   health->info_buf);
> > > 
> > > No! This is wrong! You are sneaking in text blob. Please put the
> > > info in
> > > structured form using proper fmsg helpers.
> > > 
> > This is the fw output format, it is already in use, I don't want
> > to 
> > change it, just have it here in the diagnose output too.
> 
> Already in use where? in dmesg? Sorry, but that is not an argument.
> Please format the message properly.
> 

What is wrong here ? 

Unlike binary dump data, I thought diagnose data is allowed to be
developer friendly free text format, if not then let's enforce it in
devlink API. Jiri,  you can't audit each and every use of
devlink_fmsg_string_pair_put, it must be done by design.

Thanks,
Saeed