netdev - Re: [net-next-2.6 PATCH 02/10] ethtool: add ntuple flow specifier to network flow classifier

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: <1299091805.2664.16.camel@bwh-desktop>
Date:	Wed, 02 Mar 2011 18:50:05 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Alexander Duyck <alexander.duyck@...il.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, 2011-02-25 at 21:30 -0800, Alexander Duyck wrote:
> 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:
[...]
> >> 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.
[...]

OK, so how about this:

/**
 * union ethtool_flow_union - flow spec type-specific fields
 * @tcp_ip4_spec: TCP/IPv4 fields to match
 * @udp_ip4_spec: UDP/IPv4 fields to match
 * @sctp_ip4_spec: SCTP/IPv4 fields to match 
 * @ah_ip4_spec: AH/IPv4 fields to match
 * @esp_ip4_spec: ESP/IPv4 fields to match
 * @usr_ip4_spec: User-defined IPv4 fields to match
 * @ether_spec: Ethernet fields to match
 *
 * Note: The size of this union may shrink in future to allow for
 * expansion in &struct ethtool_flow_ext.
 */
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[60];
};

/**
 * struct ethtool_flow_ext - flow spec common extension fields
 * @vlan_etype: EtherType for vlan tagged packet to match
 * @vlan_tci: VLAN tag to match
 * @data: Driver-dependent data to match
 *
 * Note: Additional fields may be inserted before @vlan_etype in future,
 * but the offset of the existing fields within the containing structure
 * (&struct ethtool_rx_flow_spec) will be stable.
 */
struct ethtool_flow_ext {
	__be16	vlan_etype;
	__be16	vlan_tci;
	__be32	data[2];
};

Please can you check that these definitions won't affect the size of
struct ethtool_rx_flow_spec on i386 or x86-64?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
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