netdev - Re: [PATCH net-next] cxgb4/chcr: nic-tls stats in 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] [day] [month] [year] [list]

Message-ID: <cad731af-19c0-f16c-0c43-332de5d3dee0@chelsio.com>
Date:   Tue, 31 Mar 2020 19:56:31 +0530
From:   rohit maheshwari <rohitm@...lsio.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, borisp@...lanox.com,
        secdev@...lsio.com
Subject: Re: [PATCH net-next] cxgb4/chcr: nic-tls stats in ethtool


On 23/03/20 2:27 AM, Jakub Kicinski wrote:
> On Sat, 21 Mar 2020 16:53:36 +0530 Rohit Maheshwari wrote:
>> Included nic tls statistics in ethtool stats.
>>
>> Signed-off-by: Rohit Maheshwari <rohitm@...lsio.com>
>> ---
>>   .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    | 57 +++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
>> index 398ade42476c..4998f1d1e218 100644
>> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
>> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
>> @@ -134,6 +134,28 @@ static char loopback_stats_strings[][ETH_GSTRING_LEN] = {
>>   	"bg3_frames_trunc       ",
>>   };
>>   
>> +#ifdef CONFIG_CHELSIO_TLS_DEVICE
>> +struct chcr_tls_stats {
>> +	u64 tx_tls_encrypted_packets;
>> +	u64 tx_tls_encrypted_bytes;
>> +	u64 tx_tls_ctx;
>> +	u64 tx_tls_ooo;
>> +	u64 tx_tls_skip_no_sync_data;
>> +	u64 tx_tls_drop_no_sync_data;
>> +	u64 tx_tls_drop_bypass_req;
> I don't understand why you need to have a structure for this, and then
> you memset it to 0, unnecessarily, but I guess that's just a matter of
> taste.
Yeah, this memset is pure stupidity, I'll remove it and this new
structure as well. Thanks for the suggestion.
>> +};
>> +
>> +static char chcr_tls_stats_strings[][ETH_GSTRING_LEN] = {
>> +	"tx_tls_encrypted_pkts  ",
>> +	"tx_tls_encrypted_bytes ",
>> +	"tx_tls_ctx             ",
>> +	"tx_tls_ooo             ",
>> +	"tx_tls_skip_nosync_data",
>> +	"tx_tls_drop_nosync_data",
>> +	"tx_tls_drop_bypass_req ",
> These, however, are not correct - please remove the spaces at the end,
> otherwise your names are different than for other vendors. And there is
> an underscore in the middle of "no_sync".
>
> When you're told to adhere to API recommendation, please adhere to it
> exactly.
These spaces are used for alignment, so the statistics will be readable
for end user. As far as I understood, these are minimum set of TLS
related statistics, with or without space it will remain same for end
user. Please let me know if I am interpreting it wrong.
  However I agree about the no_sync, I'll change it in my
next patch.