| 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
| ||
|
Date: Fri, 25 Feb 2011 21:30:17 -0800
From: Alexander Duyck <alexander.duyck@...il.com>
To: Ben Hutchings <bhutchings@...arflare.com>
Cc: Alexander Duyck <alexander.h.duyck@...el.com>, davem@...emloft.net,
jeffrey.t.kirsher@...el.com, netdev@...r.kernel.org
Subject: Re: [net-next-2.6 PATCH 02/10] ethtool: add ntuple flow specifier to
network flow classifier
On Fri, Feb 25, 2011 at 5:00 PM, Ben Hutchings
<bhutchings@...arflare.com> wrote:
> On Fri, 2011-02-25 at 15:32 -0800, Alexander Duyck wrote:
>> This change is meant to add an ntuple define type to the rx network flow
>> classification specifiers. The idea is to allow ntuple to be displayed and
>> possibly configured via the network flow classification interface. To do
>> this I added a ntuple_flow_spec_ext to the lsit of supported filters, and
>> added a flow_type_ext value to the structure in an unused hole within the
>> ethtool_rx_flow_spec structure.
>
> There's a hole there on 64-bit architectures. Unfortunately, on i386
> and other architectures where u64 is not 64-bit-aligned, there isn't.
> We actually need to add compat handling for the commands that use it.
>
> Also, we don't want these flags to be ignored by older kernel versions
> and drivers - they should reject specs that they don't understand. So
> any extension flags need to be added to flow_type.
>
>> Due to the fact that the flow specifier structures are only 4 byte aligned
>> instead of 8 I had to break the user data field into 2 sections. In
>> addition I added the vlan ethertype field since this is what ixgbe was
>> using the user-data for currently and it allows for the fields to stay 4
>> byte aligned while occupying space at the end of the flow_spec.
>>
>> In order to guarantee byte ordering I also thought it best to keep all
>> fields in the flow_spec area a big endian value, as such I added vlan, vlan
>> ethertype, and data as big endian values.
>
> It's not important that byte order is consistent across architectures.
>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@...el.com>
>> ---
>>
>> include/linux/ethtool.h | 20 ++++++++++++++++++++
>> 1 files changed, 20 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index aac3e2e..3d1f8e0 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -378,10 +378,25 @@ struct ethtool_usrip4_spec {
>> };
>>
>> /**
>> + * struct ethtool_ntuple_spec_ext - flow spec extension for ntuple in nfc
>> + * @unused: space unused by extension
>> + * @vlan_etype: EtherType for vlan tagged packet to match
>> + * @vlan_tci: VLAN tag to match
>> + * @data: Driver-dependent data to match
>> + */
>> +struct ethtool_ntuple_spec_ext {
>> + __be32 unused[15];
>> + __be16 vlan_etype;
>> + __be16 vlan_tci;
>> + __be32 data[2];
>> +};
> [...]
>
> This is a really nasty way to reclaim space in the union.
>
> Let's name the union, shrink it and insert the extra fields that way:
>
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -377,27 +377,43 @@ struct ethtool_usrip4_spec {
> __u8 proto;
> };
>
> +union ethtool_flow_union {
> + struct ethtool_tcpip4_spec tcp_ip4_spec;
> + struct ethtool_tcpip4_spec udp_ip4_spec;
> + struct ethtool_tcpip4_spec sctp_ip4_spec;
> + struct ethtool_ah_espip4_spec ah_ip4_spec;
> + struct ethtool_ah_espip4_spec esp_ip4_spec;
> + struct ethtool_usrip4_spec usr_ip4_spec;
> + struct ethhdr ether_spec;
> + __u8 hdata[52];
> +};
> +
> +struct ethtool_flow_ext {
> + __be16 vlan_etype;
> + __be16 vlan_tci;
> + __be32 data[2];
> + __u32 reserved[2];
> +};
> +
Any chance of getting the reserved fields moved to the top of the
structure? My only concern is that we might end up with a flow spec
larger than 52 bytes at some point and moving the reserved fields to
the front might give us a little more wiggle room future
compatibility.
> /**
> * struct ethtool_rx_flow_spec - specification for RX flow filter
> * @flow_type: Type of match to perform, e.g. %TCP_V4_FLOW
> * @h_u: Flow fields to match (dependent on @flow_type)
> + * @h_ext: Additional fields to match
> * @m_u: Masks for flow field bits to be ignored
> + * @m_ext: Masks for additional field bits to be ignored.
> + * Note, all additional fields must be ignored unless @flow_type
> + * includes the %FLOW_EXT flag.
> * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC
> * if packets should be discarded
> * @location: Index of filter in hardware table
> */
> struct ethtool_rx_flow_spec {
> __u32 flow_type;
> - union {
> - struct ethtool_tcpip4_spec tcp_ip4_spec;
> - struct ethtool_tcpip4_spec udp_ip4_spec;
> - struct ethtool_tcpip4_spec sctp_ip4_spec;
> - struct ethtool_ah_espip4_spec ah_ip4_spec;
> - struct ethtool_ah_espip4_spec esp_ip4_spec;
> - struct ethtool_usrip4_spec usr_ip4_spec;
> - struct ethhdr ether_spec;
> - __u8 hdata[72];
> - } h_u, m_u;
> + union ethtool_flow_union h_u;
> + struct ethtool_flow_ext h_ext;
> + union ethtool_flow_union m_u;
> + struct ethtool_flow_ext m_ext;
> __u64 ring_cookie;
> __u32 location;
> };
> @@ -954,6 +970,8 @@ struct ethtool_ops {
> #define IPV4_FLOW 0x10 /* hash only */
> #define IPV6_FLOW 0x11 /* hash only */
> #define ETHER_FLOW 0x12 /* spec only (ether_spec) */
> +/* Flag to enable additional fields in struct ethtool_rx_flow_spec */
> +#define FLOW_EXT 0x80000000
>
> /* L3-L4 network traffic flow hash options */
> #define RXH_L2DA (1 << 1)
> ---
>
> Ben.
This works for my purposes other than the one comment above. However
if you are fine with it I am good with it since I can't think of any
filters that we might need in the near future that would require more
than 52 bytes.
Acked-by: Alexander Duyck <alexander.h.duyck@...el.com>
--
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
Powered by blists - more mailing lists