netdev - RE: [PATCH net-next] ethtool: Fail number of channels change when it conflicts with rxnfc

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: <CO1PR11MB50896EA9D92C148FD2A40443D6399@CO1PR11MB5089.namprd11.prod.outlook.com>
Date:   Wed, 2 Nov 2022 19:38:57 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Gal Pressman <gal@...dia.com>, Jakub Kicinski <kuba@...nel.org>
CC:     "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...dia.com>
Subject: RE: [PATCH net-next] ethtool: Fail number of channels change when it
 conflicts with rxnfc



> -----Original Message-----
> From: Gal Pressman <gal@...dia.com>
> Sent: Wednesday, November 2, 2022 5:45 AM
> To: Keller, Jacob E <jacob.e.keller@...el.com>; Jakub Kicinski <kuba@...nel.org>
> Cc: David S. Miller <davem@...emloft.net>; netdev@...r.kernel.org; Tariq
> Toukan <tariqt@...dia.com>
> Subject: Re: [PATCH net-next] ethtool: Fail number of channels change when it
> conflicts with rxnfc
> 
> On 01/11/2022 18:50, Jacob Keller wrote:
> >
> >
> > On 10/31/2022 6:23 PM, Jakub Kicinski wrote:
> >> On Mon, 31 Oct 2022 12:00:16 +0200 Gal Pressman wrote:
> >>> Similar to what we do with the hash indirection table [1], when network
> >>> flow classification rules are forwarding traffic to channels greater
> >>> than the requested number of channels, fail the operation.
> >>> Without this, traffic could be directed to channels which no longer
> >>> exist (dropped) after changing number of channels.
> >>>
> >>> [1] commit d4ab4286276f ("ethtool: correctly ensure {GS}CHANNELS
> >>> doesn't conflict with GS{RXFH}")
> >>
> >> Have you made sure there are no magic encodings of queue numbers this
> >> would break? I seem to recall some vendors used magic queue values to
> >> redirect to VFs before TC and switchdev. If that's the case we'd need
> >> to locate the drivers that do that and flag them so we can enforce this
> >> only going forward?
> >
> > I believe these all use the same encoding defined by
> > ethtool_get_flow_spec_ring and ethtool_get_flow_spec_vf, at least
> > that's what ixgbe uses.
> >
> > This sets the lower 32 bits as the queue index and the next 8 bits as
> > the VF identifier as defined by ETHTOOL_RX_FLOW_SPEC_RING and
> > ETHTOOL_RX_FLOW_SPEC_RING_VF.
> >
> > It looks like this change should just exempt ring_cookie with
> > ethtool_get_flow_spec_vf as non-zero?
> >
> > We maybe ought to mark this whole thing as deprecated now given the
> > advances in TC.
> 
> Oh, I was not aware of this encoding scheme, shouldn't VF rules be added
> on the VF interface?
> What is this used for?
> 

It's rather old, and the idea was to allow forwarding traffic by rules in the host. It predates switchdev, which I think would be the modern method now.

> How does the PF verify the rules are in range for the VF queues?

I believe the PF driver has to check this, and I think its sort of just a hack/independent of the PF queues. I think it would depend on the driver. ixgbe knows how many VF queues there are, and which ones belong to which VF.

I suspect we don't want to use this on new drivers or add it to any existing drivers that don't already have it.

> Anyway, I'll go ahead and verify that VF == 0 in the if statement.
> 
> Thanks for the review!