| 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
| ||
|
Message-ID: <1d151cf9153b4ce78d6accd063d0776c@AMSPEX02CL03.citrite.net>
Date: Mon, 15 Feb 2016 08:50:46 +0000
From: Paul Durrant <Paul.Durrant@...rix.com>
To: Tom Herbert <tom@...bertland.com>
CC: Linux Kernel Network Developers <netdev@...r.kernel.org>,
"xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
"David S. Miller" <davem@...emloft.net>,
Jay Vosburgh <j.vosburgh@...il.com>,
Veaceslav Falico <vfalico@...il.com>,
Andy Gospodarek <gospo@...ulusnetworks.com>
Subject: RE: [PATCH net-next v1 1/8] skbuff: store hash type in socket
buffer...
> -----Original Message-----
> From: Tom Herbert [mailto:tom@...bertland.com]
> Sent: 14 February 2016 22:25
> To: Paul Durrant
> Cc: Linux Kernel Network Developers; xen-devel@...ts.xenproject.org; David
> S. Miller; Jay Vosburgh; Veaceslav Falico; Andy Gospodarek
> Subject: Re: [PATCH net-next v1 1/8] skbuff: store hash type in socket
> buffer...
>
> On Fri, Feb 12, 2016 at 11:13 AM, Paul Durrant <paul.durrant@...rix.com>
> wrote:
> > ...rather than a boolean merely indicating a canonical L4 hash.
> >
> > skb_set_hash() takes a hash type (from enum pkt_hash_types) as an
> > argument but information is lost since only a single bit in the skb
> > stores whether that hash type is PKT_HASH_TYPE_L4 or not.
> >
> > By using two bits it's possible to store the complete hash type
> > information for use by drivers. A subsequent patch to xen-netback
> > needs this information to forward packet hashes to VM frontend drivers.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@...rix.com>
> > Cc: "David S. Miller" <davem@...emloft.net>
> > Cc: Jay Vosburgh <j.vosburgh@...il.com>
> > Cc: Veaceslav Falico <vfalico@...il.com>
> > Cc: Andy Gospodarek <gospo@...ulusnetworks.com>
> > ---
> > drivers/net/bonding/bond_main.c | 2 +-
> > include/linux/skbuff.h | 53 ++++++++++++++++++++++++++++-------
> ------
> > include/net/flow_dissector.h | 5 ++++
> > include/net/sock.h | 2 +-
> > include/trace/events/net.h | 2 +-
> > net/core/flow_dissector.c | 27 ++++++++++++++++-----
> > 6 files changed, 65 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> > index 45bdd87..ad0ee00 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -3173,7 +3173,7 @@ u32 bond_xmit_hash(struct bonding *bond,
> struct sk_buff *skb)
> > u32 hash;
> >
> > if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> > - skb->l4_hash)
> > + skb_has_l4_hash(skb))
> > return skb->hash;
> >
> > if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 6ec86f1..9021b52 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -595,8 +595,7 @@ static inline bool skb_mstamp_after(const struct
> skb_mstamp *t1,
> > * @xmit_more: More SKBs are pending for this queue
> > * @ndisc_nodetype: router type (from link layer)
> > * @ooo_okay: allow the mapping of a socket to a queue to be changed
> > - * @l4_hash: indicate hash is a canonical 4-tuple hash over transport
> > - * ports.
> > + * @hash_type: indicates type of hash (see enum pkt_hash_types
> below)
> > * @sw_hash: indicates hash was computed in software stack
> > * @wifi_acked_valid: wifi_acked was set
> > * @wifi_acked: whether frame was acked on wifi or not
> > @@ -701,10 +700,10 @@ struct sk_buff {
> > __u8 nf_trace:1;
> > __u8 ip_summed:2;
> > __u8 ooo_okay:1;
> > - __u8 l4_hash:1;
> > __u8 sw_hash:1;
> > __u8 wifi_acked_valid:1;
> > __u8 wifi_acked:1;
> > + /* 1 bit hole */
> >
> > __u8 no_fcs:1;
> > /* Indicates the inner headers are valid in the skbuff. */
> > @@ -721,7 +720,8 @@ struct sk_buff {
> > __u8 ipvs_property:1;
> > __u8 inner_protocol_type:1;
> > __u8 remcsum_offload:1;
> > - /* 3 or 5 bit hole */
> > + __u8 hash_type:2;
>
> This isn't needed by the stack-- we don't differentiate between L2 and
> L3 hash anywhere. Also it doesn't help in the xen case for passing a
> hash to Windows without having another bit to indicate that the hash
> is indeed Toeplitz.
>
I hadn't read this before I split the patch and re-posted...
Given that the header defines an enum for hash types that *does* distinguish between L2 and L3 I find this confusing. Shouldn't that enum be removed if no-one cares?
Paul
> > + /* 1 or 3 bit hole */
> >
> > #ifdef CONFIG_NET_SCHED
> > __u16 tc_index; /* traffic control index */
> > @@ -1030,19 +1030,35 @@ static inline void skb_clear_hash(struct sk_buff
> *skb)
> > {
> > skb->hash = 0;
> > skb->sw_hash = 0;
> > - skb->l4_hash = 0;
> > + skb->hash_type = 0;
> > +}
> > +
> > +static inline enum pkt_hash_types skb_hash_type(struct sk_buff *skb)
> > +{
> > + return skb->hash_type;
> > +}
> > +
> > +static inline bool skb_has_l4_hash(struct sk_buff *skb)
> > +{
> > + return skb_hash_type(skb) == PKT_HASH_TYPE_L4;
> > +}
> > +
> > +static inline bool skb_has_sw_hash(struct sk_buff *skb)
> > +{
> > + return !!skb->sw_hash;
> > }
> >
> > static inline void skb_clear_hash_if_not_l4(struct sk_buff *skb)
> > {
> > - if (!skb->l4_hash)
> > + if (!skb_has_l4_hash(skb))
> > skb_clear_hash(skb);
> > }
> >
> > static inline void
> > -__skb_set_hash(struct sk_buff *skb, __u32 hash, bool is_sw, bool is_l4)
> > +__skb_set_hash(struct sk_buff *skb, __u32 hash, bool is_sw,
> > + enum pkt_hash_types type)
> > {
> > - skb->l4_hash = is_l4;
> > + skb->hash_type = type;
> > skb->sw_hash = is_sw;
> > skb->hash = hash;
> > }
> > @@ -1051,13 +1067,13 @@ static inline void
> > skb_set_hash(struct sk_buff *skb, __u32 hash, enum pkt_hash_types
> type)
> > {
> > /* Used by drivers to set hash from HW */
> > - __skb_set_hash(skb, hash, false, type == PKT_HASH_TYPE_L4);
> > + __skb_set_hash(skb, hash, false, type);
> > }
> >
> > static inline void
> > -__skb_set_sw_hash(struct sk_buff *skb, __u32 hash, bool is_l4)
> > +__skb_set_sw_hash(struct sk_buff *skb, __u32 hash, enum
> pkt_hash_types type)
> > {
> > - __skb_set_hash(skb, hash, true, is_l4);
> > + __skb_set_hash(skb, hash, true, type);
> > }
> >
> > void __skb_get_hash(struct sk_buff *skb);
> > @@ -1110,9 +1126,10 @@ static inline bool
> skb_flow_dissect_flow_keys_buf(struct flow_keys *flow,
> > data, proto, nhoff, hlen, flags);
> > }
> >
> > +
>
> Extra line
>
> > static inline __u32 skb_get_hash(struct sk_buff *skb)
> > {
> > - if (!skb->l4_hash && !skb->sw_hash)
> > + if (!skb_has_l4_hash(skb) && !skb_has_sw_hash(skb))
> > __skb_get_hash(skb);
> >
> > return skb->hash;
> > @@ -1122,11 +1139,12 @@ __u32 __skb_get_hash_flowi6(struct sk_buff
> *skb, const struct flowi6 *fl6);
> >
> > static inline __u32 skb_get_hash_flowi6(struct sk_buff *skb, const struct
> flowi6 *fl6)
> > {
> > - if (!skb->l4_hash && !skb->sw_hash) {
> > + if (!skb_has_l4_hash(skb) && !skb_has_sw_hash(skb)) {
> > struct flow_keys keys;
> > __u32 hash = __get_hash_from_flowi6(fl6, &keys);
> >
> > - __skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys));
> > + __skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys) ?
> > + PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
> > }
> >
> > return skb->hash;
> > @@ -1136,11 +1154,12 @@ __u32 __skb_get_hash_flowi4(struct sk_buff
> *skb, const struct flowi4 *fl);
> >
> > static inline __u32 skb_get_hash_flowi4(struct sk_buff *skb, const struct
> flowi4 *fl4)
> > {
> > - if (!skb->l4_hash && !skb->sw_hash) {
> > + if (!skb_has_l4_hash(skb) && !skb_has_sw_hash(skb)) {
> > struct flow_keys keys;
> > __u32 hash = __get_hash_from_flowi4(fl4, &keys);
> >
> > - __skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys));
> > + __skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys) ?
> > + PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
> > }
> >
> > return skb->hash;
> > @@ -1157,7 +1176,7 @@ static inline void skb_copy_hash(struct sk_buff
> *to, const struct sk_buff *from)
> > {
> > to->hash = from->hash;
> > to->sw_hash = from->sw_hash;
> > - to->l4_hash = from->l4_hash;
> > + to->hash_type = from->hash_type;
> > };
> >
> > static inline void skb_sender_cpu_clear(struct sk_buff *skb)
> > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> > index 8c8548c..418b8c5 100644
> > --- a/include/net/flow_dissector.h
> > +++ b/include/net/flow_dissector.h
> > @@ -182,6 +182,11 @@ static inline bool flow_keys_have_l4(struct
> flow_keys *keys)
> > return (keys->ports.ports || keys->tags.flow_label);
> > }
> >
> > +static inline bool flow_keys_have_l3(struct flow_keys *keys)
> > +{
> > + return !!keys->control.addr_type;
> > +}
> > +
> > u32 flow_hash_from_keys(struct flow_keys *keys);
> >
> > #endif
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 255d3e0..21b8cdc 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -1828,7 +1828,7 @@ static inline void sock_poll_wait(struct file *filp,
> > static inline void skb_set_hash_from_sk(struct sk_buff *skb, struct sock
> *sk)
> > {
> > if (sk->sk_txhash) {
> > - skb->l4_hash = 1;
> > + skb->hash_type = PKT_HASH_TYPE_L4;
> > skb->hash = sk->sk_txhash;
> > }
> > }
> > diff --git a/include/trace/events/net.h b/include/trace/events/net.h
> > index 49cc7c3..25e7979 100644
> > --- a/include/trace/events/net.h
> > +++ b/include/trace/events/net.h
> > @@ -180,7 +180,7 @@
> DECLARE_EVENT_CLASS(net_dev_rx_verbose_template,
> > __entry->protocol = ntohs(skb->protocol);
> > __entry->ip_summed = skb->ip_summed;
> > __entry->hash = skb->hash;
> > - __entry->l4_hash = skb->l4_hash;
> > + __entry->l4_hash = skb->hash_type == PKT_HASH_TYPE_L4;
> > __entry->len = skb->len;
> > __entry->data_len = skb->data_len;
> > __entry->truesize = skb->truesize;
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index d79699c..956208b 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -658,17 +658,30 @@ EXPORT_SYMBOL(make_flow_keys_digest);
> > *
> > * This function calculates a flow hash based on src/dst addresses
> > * and src/dst port numbers. Sets hash in skb to non-zero hash value
> > - * on success, zero indicates no valid hash. Also, sets l4_hash in skb
> > - * if hash is a canonical 4-tuple hash over transport ports.
> > + * on success, zero indicates no valid hash in which case the hash type
> > + * is set to NONE. If the hash is a canonical 4-tuple hash over transport
> > + * ports then type is set to L4. If the hash did not include transport
> > + * then type is set to L3, otherwise it is assumed to be L2 only.
> > */
> > void __skb_get_hash(struct sk_buff *skb)
> > {
> > struct flow_keys keys;
> > + u32 hash;
> > + enum pkt_hash_types type;
> >
> > __flow_hash_secret_init();
> >
> > - __skb_set_sw_hash(skb, ___skb_get_hash(skb, &keys, hashrnd),
> > - flow_keys_have_l4(&keys));
> > + hash = ___skb_get_hash(skb, &keys, hashrnd);
> > + if (hash == 0)
> > + type = PKT_HASH_TYPE_NONE;
> > + else if (flow_keys_have_l4(&keys))
> > + type = PKT_HASH_TYPE_L4;
> > + else if (flow_keys_have_l3(&keys))
> > + type = PKT_HASH_TYPE_L3;
> > + else
> > + type = PKT_HASH_TYPE_L2;
> > +
> > + __skb_set_sw_hash(skb, hash, type);
> > }
> > EXPORT_SYMBOL(__skb_get_hash);
> >
> > @@ -698,7 +711,8 @@ __u32 __skb_get_hash_flowi6(struct sk_buff *skb,
> const struct flowi6 *fl6)
> > keys.basic.ip_proto = fl6->flowi6_proto;
> >
> > __skb_set_sw_hash(skb, flow_hash_from_keys(&keys),
> > - flow_keys_have_l4(&keys));
> > + flow_keys_have_l4(&keys) ?
> > + PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
> >
> > return skb->hash;
> > }
> > @@ -719,7 +733,8 @@ __u32 __skb_get_hash_flowi4(struct sk_buff *skb,
> const struct flowi4 *fl4)
> > keys.basic.ip_proto = fl4->flowi4_proto;
> >
> > __skb_set_sw_hash(skb, flow_hash_from_keys(&keys),
> > - flow_keys_have_l4(&keys));
> > + flow_keys_have_l4(&keys) ?
> > + PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
> >
> > return skb->hash;
> > }
> > --
> > 2.1.4
> >
Powered by blists - more mailing lists