netdev - Re: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

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: <20170508001955.GA5787@gwshan>
Date:   Mon, 8 May 2017 10:19:55 +1000
From:   Gavin Shan <gwshan@...ux.vnet.ibm.com>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     "'Gavin Shan'" <gwshan@...ux.vnet.ibm.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "joe@...ches.com" <joe@...ches.com>,
        "kubakici@...pl" <kubakici@...pl>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get
 NCSI topology

On Thu, May 04, 2017 at 09:31:20AM +0000, David Laight wrote:
>From: Gavin Shan
>> Sent: 04 May 2017 07:16
>> On Wed, May 03, 2017 at 10:19:44PM -0700, Stephen Hemminger wrote:
>> >On Wed,  3 May 2017 14:44:35 +1000
>> >Gavin Shan <gwshan@...ux.vnet.ibm.com> wrote:
>...
>> >> +{
>> >> +	struct ethtool_ncsi_channels *enc;
>> >> +	short nr_channels;
>> >Should be __u16 or unsigned not short.
>> >
>> 
>> Nope, It's for signed number. User expects to get number of available
>> channels when negative number is passed in. When it's positive, it's
>> going to get the channels' information.
>
>Why 16 bits?
>You are just making life hard for the compiler and possibly generating
>random padding.
>

It's because there are 256 NCSI channels to maximal degree. So 16-bits
is the minial data width to hold it in signed format. Yes, I think
__s32 would be better in this case. However, I would like to discard
the negotiation mechanism in next respin.

>I guess the user is expected to pass -1 first to get the number of
>channels, then allocate an appropriate sized array and call again
>specifying the number of channels?
>

It's correct.

>What happens if the number of channels changes between the two requests?
>

There are only one case the number changes from zero to x. In previous call,
zero is returned and userspace will get nothing. When x channels are probed,
it's stable and won't change. I don't see any problem because of it.

In next respin, I'll pass 256 entries directly. Each entry will have a flag
to indicate it's valid or not. No negotiation will be needed.

>I'd also suggest passing the size of each entry (in at least one direction).
>That way additional channel information can be added.
>

why? we have another command (ETHTOOL_GNCSICINFO) to retrieve information
about the specified channel.

Cheers,
Gavin