netdev - Re: [PATCH net-next 6/8] bnxt_en: Implement ethtool -X to set indirection table.

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: <20200630120517.2237bb87@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 30 Jun 2020 12:05:17 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Michael Chan <michael.chan@...adcom.com>
Cc:     David Miller <davem@...emloft.net>, Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 6/8] bnxt_en: Implement ethtool -X to set
 indirection table.

On Mon, 29 Jun 2020 17:38:33 -0700 Michael Chan wrote:
> On Mon, Jun 29, 2020 at 5:06 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > On Mon, 29 Jun 2020 02:34:22 -0400 Michael Chan wrote:
> > > With the new infrastructure in place, we can now support the setting of
> > > the indirection table from ethtool.
> > >
> > > The user-configured indirection table will need to be reset to default
> > > if we are unable to reserve the requested number of RX rings or if the
> > > RSS table size changes.
> > >
> > > Signed-off-by: Michael Chan <michael.chan@...adcom.com>  
> >
> > Hm. Clearing IFF_RXFH_CONFIGURED seems wrong. The user has clearly
> > requested a RSS mapping, if it can't be maintained driver should
> > return an error from the operation which attempts to change the ring
> > count.  
> 
> Right.  In this case the user has requested non default RSS map and is
> now attempting to change rings.  We have a first level check by
> calling bnxt_check_rings().  Firmware will tell us if the requested
> rings are available or not.  If not, we will return error and the
> existing rings and RSS map will be kept.  This should be the expected
> outcome in most cases.
> 
> In rare cases, firmware can return success during bnxt_check_rings()
> but during the actual ring reservation, it fails to reserve all the
> rings it promised were available earlier.  In this case, we fall back
> and accept the fewer rings and set the RSS map to default.  I have
> never seen this scenario but we need to put the code in just in case
> it happens.  It should be rare.

What's the expected application flow? Every time the application 
makes a change to NIC settings it has to re-validate that some of 
the previous configuration didn't get lost? I don't see the driver
returning the error if FW gave it less rings than requested. There 
isn't even a warning printed..

I'd prefer if the driver wrapped the rss indexes, and printed a
warning, but left the config intact. And IMO set_channels should 
return an error.