netdev - Re: [PATCHv2 net-next-2.6 2/2] qlcnic: Take FW dump via ethtool

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: <3D85CC75-765C-4059-BB3F-82CCBF748FBC@qlogic.com>
Date:	Tue, 10 May 2011 14:00:55 -0700
From:	Anirban Chakraborty <anirban.chakraborty@...gic.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
CC:	netdev <netdev@...r.kernel.org>, David Miller <davem@...emloft.net>
Subject: Re: [PATCHv2 net-next-2.6 2/2] qlcnic: Take FW dump via ethtool


On May 10, 2011, at 11:40 AM, Ben Hutchings wrote:

> On Mon, 2011-05-09 at 18:02 -0700, Anirban Chakraborty wrote:
>> Driver checks if the previous dump has been cleared before taking the dump.
>> It doesn't take the dump if it is not cleared.
>> 
>> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@...gic.com>
>> ---
>> drivers/net/qlcnic/qlcnic_ethtool.c |   60 +++++++++++++++++++++++++++++++++++
>> 1 files changed, 60 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c
>> index c541461..1237449 100644
>> --- a/drivers/net/qlcnic/qlcnic_ethtool.c
>> +++ b/drivers/net/qlcnic/qlcnic_ethtool.c
>> @@ -965,6 +965,64 @@ static void qlcnic_set_msglevel(struct net_device *netdev, u32 msglvl)
>> 	adapter->msg_enable = msglvl;
>> }
>> 
>> +static int
>> +qlcnic_get_dump(struct net_device *netdev, struct ethtool_dump *dump,
>> +		void *buffer)
>> +{
>> +	int i, copy_sz;
>> +	u32 *hdr_ptr, *data;
>> +	struct qlcnic_adapter *adapter = netdev_priv(netdev);
>> +	struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump;
>> +
>> +	if (dump->type == ETHTOOL_DUMP_FLAG) {
>> +		dump->len = fw_dump->tmpl_hdr->size + fw_dump->size;
>> +		dump->flag = fw_dump->tmpl_hdr->drv_cap_mask;
>> +		return 0;
>> +	}
>> +	if (!fw_dump->clr) {
>> +		netdev_info(netdev, "Dump not available\n");
>> +		return -EINVAL;
>> +	}
>> +	copy_sz = fw_dump->tmpl_hdr->size;
>> +	/* Copy template header first */
>> +	hdr_ptr = (u32 *) fw_dump->tmpl_hdr;
>> +	data = (u32 *) buffer;
>> +	for (i = 0; i < copy_sz/sizeof(u32); i++)
>> +		*data++ = cpu_to_le32(*hdr_ptr++);
>> +	/* Copy captured dump data */
>> +	memcpy(buffer + copy_sz, fw_dump->data, fw_dump->size);
>> +	dump->len = copy_sz + fw_dump->size;
>> +	dump->flag = fw_dump->tmpl_hdr->drv_cap_mask;
>> +	/* free dump area once the whoel dump data has been captured */
>> +	vfree(fw_dump->data);
>> +	fw_dump->size = 0;
>> +	fw_dump->data = NULL;
>> +	fw_dump->clr = 0;
> 
> This doesn't seem to be serialised with the code that captures firmware
> dumps.  They need to use the same lock!
When we take the dump, we bring down the interface. So, ethtool will not get a chance to
fetch the dump data while the dump operation is in progress.

> 
>> +	return 0;
>> +}
>> +
>> +static int
>> +qlcnic_set_dump(struct net_device *netdev, struct ethtool_dump *val)
>> +{
>> +	struct qlcnic_adapter *adapter = netdev_priv(netdev);
>> +	struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump;
>> +	if (val->flag == QLCNIC_FORCE_FW_DUMP_KEY) {
>> +		netdev_info(netdev, "Forcing a FW dump\n");
>> +		qlcnic_dev_request_reset(adapter);
>> +	} else {
>> +		if (val->flag > QLCNIC_DUMP_MASK_MAX ||
>> +			val->flag < QLCNIC_DUMP_MASK_MIN) {
>> +				netdev_info(netdev,
>> +				"Invalid dump level: 0x%x\n", val->flag);
>> +				return -EINVAL;
>> +		}
>> +		fw_dump->tmpl_hdr->drv_cap_mask = val->flag & 0xff;
>> +		netdev_info(netdev, "Driver mask changed to: 0x%x\n",
>> +			fw_dump->tmpl_hdr->drv_cap_mask);
> 
> If the flags change, doesn't this invalidate any dump that has been
> collected by the driver but not saved?
If the flags change, its effect would be relevant from the next dump capture. The flag indicates to the
driver to gather FW dump for a specific level of detail.

> 
> Also, same locking problem here.
Addressed above.

Thanks for reviewing. 

-Anirban


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html