* [PATCH net-next] openvswitch: add NSH support
From: Yi Yang @ 2017-08-08 4:59 UTC (permalink / raw)
To: netdev; +Cc: dev, jbenc, davem, Yi Yang
OVS master and 2.8 branch has merged NSH userspace
patch series, this patch is to enable NSH support
in kernel data path in order that OVS can support
NSH in 2.8 release in compat mode by porting this.
Signed-off-by: Yi Yang <yi.y.yang@intel.com>
---
drivers/net/vxlan.c | 7 ++
include/net/nsh.h | 126 ++++++++++++++++++++++++++++++
include/uapi/linux/openvswitch.h | 33 ++++++++
net/openvswitch/actions.c | 165 +++++++++++++++++++++++++++++++++++++++
net/openvswitch/flow.c | 41 ++++++++++
net/openvswitch/flow.h | 1 +
net/openvswitch/flow_netlink.c | 54 ++++++++++++-
7 files changed, 426 insertions(+), 1 deletion(-)
create mode 100644 include/net/nsh.h
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index dbca067..843714c 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -27,6 +27,7 @@
#include <net/net_namespace.h>
#include <net/netns/generic.h>
#include <net/vxlan.h>
+#include <net/nsh.h>
#if IS_ENABLED(CONFIG_IPV6)
#include <net/ip6_tunnel.h>
@@ -1267,6 +1268,9 @@ static bool vxlan_parse_gpe_hdr(struct vxlanhdr *unparsed,
case VXLAN_GPE_NP_IPV6:
*protocol = htons(ETH_P_IPV6);
break;
+ case VXLAN_GPE_NP_NSH:
+ *protocol = htons(ETH_P_NSH);
+ break;
case VXLAN_GPE_NP_ETHERNET:
*protocol = htons(ETH_P_TEB);
break;
@@ -1806,6 +1810,9 @@ static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, u32 vxflags,
case htons(ETH_P_IPV6):
gpe->next_protocol = VXLAN_GPE_NP_IPV6;
return 0;
+ case htons(ETH_P_NSH):
+ gpe->next_protocol = VXLAN_GPE_NP_NSH;
+ return 0;
case htons(ETH_P_TEB):
gpe->next_protocol = VXLAN_GPE_NP_ETHERNET;
return 0;
diff --git a/include/net/nsh.h b/include/net/nsh.h
new file mode 100644
index 0000000..96477a1
--- /dev/null
+++ b/include/net/nsh.h
@@ -0,0 +1,126 @@
+#ifndef __NET_NSH_H
+#define __NET_NSH_H 1
+
+
+/*
+ * Network Service Header:
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |Ver|O|C|R|R|R|R|R|R| Length | MD Type | Next Proto |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | Service Path ID | Service Index |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | |
+ * ~ Mandatory/Optional Context Header ~
+ * | |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * Ver = The version field is used to ensure backward compatibility
+ * going forward with future NSH updates. It MUST be set to 0x0
+ * by the sender, in this first revision of NSH.
+ *
+ * O = OAM. when set to 0x1 indicates that this packet is an operations
+ * and management (OAM) packet. The receiving SFF and SFs nodes
+ * MUST examine the payload and take appropriate action.
+ *
+ * C = context. Indicates that a critical metadata TLV is present.
+ *
+ * Length : total length, in 4-byte words, of NSH including the Base
+ * Header, the Service Path Header and the optional variable
+ * TLVs.
+ * MD Type: indicates the format of NSH beyond the mandatory Base Header
+ * and the Service Path Header.
+ *
+ * Next Protocol: indicates the protocol type of the original packet. A
+ * new IANA registry will be created for protocol type.
+ *
+ * Service Path Identifier (SPI): identifies a service path.
+ * Participating nodes MUST use this identifier for Service
+ * Function Path selection.
+ *
+ * Service Index (SI): provides location within the SFP.
+ *
+ * [0] https://tools.ietf.org/html/draft-ietf-sfc-nsh-13
+ */
+
+/**
+ * struct nsh_md1_ctx - Keeps track of NSH context data
+ * @nshc<1-4>: NSH Contexts.
+ */
+struct nsh_md1_ctx {
+ __be32 c[4];
+};
+
+struct nsh_md2_tlv {
+ __be16 md_class;
+ u8 type;
+ u8 length;
+ u8 md_value[];
+};
+
+struct nsh_hdr {
+ __be16 ver_flags_len;
+ u8 md_type;
+ u8 next_proto;
+ __be32 path_hdr;
+ union {
+ struct nsh_md1_ctx md1;
+ struct nsh_md2_tlv md2[0];
+ };
+};
+
+/* Masking NSH header fields. */
+#define NSH_VER_MASK 0xc000
+#define NSH_VER_SHIFT 14
+#define NSH_FLAGS_MASK 0x3fc0
+#define NSH_FLAGS_SHIFT 6
+#define NSH_LEN_MASK 0x003f
+#define NSH_LEN_SHIFT 0
+
+#define NSH_SPI_MASK 0xffffff00
+#define NSH_SPI_SHIFT 8
+#define NSH_SI_MASK 0x000000ff
+#define NSH_SI_SHIFT 0
+
+#define NSH_DST_PORT 4790 /* UDP Port for NSH on VXLAN. */
+#define ETH_P_NSH 0x894F /* Ethertype for NSH. */
+
+/* NSH Base Header Next Protocol. */
+#define NSH_P_IPV4 0x01
+#define NSH_P_IPV6 0x02
+#define NSH_P_ETHERNET 0x03
+#define NSH_P_NSH 0x04
+#define NSH_P_MPLS 0x05
+
+/* MD Type Registry. */
+#define NSH_M_TYPE1 0x01
+#define NSH_M_TYPE2 0x02
+#define NSH_M_EXP1 0xFE
+#define NSH_M_EXP2 0xFF
+
+/* NSH Metadata Length. */
+#define NSH_M_TYPE1_MDLEN 16
+
+/* NSH Base Header Length */
+#define NSH_BASE_HDR_LEN 8
+
+/* NSH MD Type 1 header Length. */
+#define NSH_M_TYPE1_LEN 24
+
+static inline u16
+nsh_hdr_len(const struct nsh_hdr *nsh)
+{
+ return 4 * (ntohs(nsh->ver_flags_len) & NSH_LEN_MASK) >> NSH_LEN_SHIFT;
+}
+
+static inline struct nsh_md1_ctx *
+nsh_md1_ctx(struct nsh_hdr *nsh)
+{
+ return &nsh->md1;
+}
+
+static inline struct nsh_md2_tlv *
+nsh_md2_ctx(struct nsh_hdr *nsh)
+{
+ return nsh->md2;
+}
+
+#endif /* __NET_NSH_H */
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 156ee4c..b9c072c 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -333,6 +333,7 @@ enum ovs_key_attr {
OVS_KEY_ATTR_CT_LABELS, /* 16-octet connection tracking label */
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4, /* struct ovs_key_ct_tuple_ipv4 */
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6, /* struct ovs_key_ct_tuple_ipv6 */
+ OVS_KEY_ATTR_NSH, /* struct ovs_key_nsh */
#ifdef __KERNEL__
OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info */
@@ -491,6 +492,15 @@ struct ovs_key_ct_tuple_ipv6 {
__u8 ipv6_proto;
};
+struct ovs_key_nsh {
+ __u8 flags;
+ __u8 mdtype;
+ __u8 np;
+ __u8 pad;
+ __be32 path_hdr;
+ __be32 c[4];
+};
+
/**
* enum ovs_flow_attr - attributes for %OVS_FLOW_* commands.
* @OVS_FLOW_ATTR_KEY: Nested %OVS_KEY_ATTR_* attributes specifying the flow
@@ -769,6 +779,25 @@ struct ovs_action_push_eth {
struct ovs_key_ethernet addresses;
};
+#define OVS_ENCAP_NSH_MAX_MD_LEN 16
+/*
+ * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH
+ * @flags: NSH header flags.
+ * @mdtype: NSH metadata type.
+ * @mdlen: Length of NSH metadata in bytes.
+ * @np: NSH next_protocol: Inner packet type.
+ * @path_hdr: NSH service path id and service index.
+ * @metadata: NSH metadata for MD type 1 or 2
+ */
+struct ovs_action_encap_nsh {
+ __u8 flags;
+ __u8 mdtype;
+ __u8 mdlen;
+ __u8 np;
+ __be32 path_hdr;
+ __u8 metadata[OVS_ENCAP_NSH_MAX_MD_LEN];
+};
+
/**
* enum ovs_action_attr - Action types.
*
@@ -806,6 +835,8 @@ struct ovs_action_push_eth {
* packet.
* @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the
* packet.
+ * @OVS_ACTION_ATTR_ENCAP_NSH: encap NSH action to push NSH header.
+ * @OVS_ACTION_ATTR_DECAP_NSH: decap NSH action to remove NSH header.
*
* Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all
* fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -835,6 +866,8 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_TRUNC, /* u32 struct ovs_action_trunc. */
OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
OVS_ACTION_ATTR_POP_ETH, /* No argument. */
+ OVS_ACTION_ATTR_ENCAP_NSH, /* struct ovs_action_encap_nsh. */
+ OVS_ACTION_ATTR_DECAP_NSH, /* No argument. */
__OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
* from userspace. */
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index e461067..6ba67e1 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -38,6 +38,7 @@
#include <net/dsfield.h>
#include <net/mpls.h>
#include <net/sctp/checksum.h>
+#include <net/nsh.h>
#include "datapath.h"
#include "flow.h"
@@ -380,6 +381,114 @@ static int push_eth(struct sk_buff *skb, struct sw_flow_key *key,
return 0;
}
+static int encap_nsh(struct sk_buff *skb, struct sw_flow_key *key,
+ const struct ovs_action_encap_nsh *encap)
+{
+ struct nsh_hdr *nsh;
+ size_t length = NSH_BASE_HDR_LEN + encap->mdlen;
+ u8 next_proto;
+
+ if (key->mac_proto == MAC_PROTO_ETHERNET) {
+ next_proto = NSH_P_ETHERNET;
+ } else {
+ switch (ntohs(skb->protocol)) {
+ case ETH_P_IP:
+ next_proto = NSH_P_IPV4;
+ break;
+ case ETH_P_IPV6:
+ next_proto = NSH_P_IPV6;
+ break;
+ case ETH_P_NSH:
+ next_proto = NSH_P_NSH;
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+ }
+
+ /* Add the NSH header */
+ if (skb_cow_head(skb, length) < 0)
+ return -ENOMEM;
+
+ skb_push(skb, length);
+ nsh = (struct nsh_hdr *)(skb->data);
+ nsh->ver_flags_len = htons((encap->flags << NSH_FLAGS_SHIFT) |
+ (length >> 2));
+ nsh->next_proto = next_proto;
+ nsh->path_hdr = encap->path_hdr;
+ nsh->md_type = encap->mdtype;
+ switch (nsh->md_type) {
+ case NSH_M_TYPE1:
+ nsh->md1 = *(struct nsh_md1_ctx *)encap->metadata;
+ break;
+ case NSH_M_TYPE2: {
+ /* The MD2 metadata in encap is already padded to 4 bytes. */
+ size_t len = DIV_ROUND_UP(encap->mdlen, 4) * 4;
+
+ memcpy(nsh->md2, encap->metadata, len);
+ break;
+ }
+ default:
+ return -ENOTSUPP;
+ }
+
+ if (!skb->inner_protocol)
+ skb_set_inner_protocol(skb, skb->protocol);
+
+ skb->protocol = htons(ETH_P_NSH);
+ key->eth.type = htons(ETH_P_NSH);
+ skb_reset_mac_header(skb);
+ skb_reset_mac_len(skb);
+
+ /* safe right before invalidate_flow_key */
+ key->mac_proto = MAC_PROTO_NONE;
+ invalidate_flow_key(key);
+ return 0;
+}
+
+static int decap_nsh(struct sk_buff *skb, struct sw_flow_key *key)
+{
+ struct nsh_hdr *nsh = (struct nsh_hdr *)(skb->data);
+ size_t length;
+ u16 inner_proto;
+
+ if (ovs_key_mac_proto(key) != MAC_PROTO_NONE ||
+ skb->protocol != htons(ETH_P_NSH)) {
+ return -EINVAL;
+ }
+
+ switch (nsh->next_proto) {
+ case NSH_P_ETHERNET:
+ inner_proto = htons(ETH_P_TEB);
+ break;
+ case NSH_P_IPV4:
+ inner_proto = htons(ETH_P_IP);
+ break;
+ case NSH_P_IPV6:
+ inner_proto = htons(ETH_P_IPV6);
+ break;
+ case NSH_P_NSH:
+ inner_proto = htons(ETH_P_NSH);
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ length = nsh_hdr_len(nsh);
+ skb_pull(skb, length);
+ skb_reset_mac_header(skb);
+ skb_reset_mac_len(skb);
+ skb->protocol = inner_proto;
+
+ /* safe right before invalidate_flow_key */
+ if (inner_proto == htons(ETH_P_TEB))
+ key->mac_proto = MAC_PROTO_ETHERNET;
+ else
+ key->mac_proto = MAC_PROTO_NONE;
+ invalidate_flow_key(key);
+ return 0;
+}
+
static void update_ip_l4_checksum(struct sk_buff *skb, struct iphdr *nh,
__be32 addr, __be32 new_addr)
{
@@ -602,6 +711,49 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *flow_key,
return 0;
}
+static int set_nsh(struct sk_buff *skb, struct sw_flow_key *flow_key,
+ const struct ovs_key_nsh *key,
+ const struct ovs_key_nsh *mask)
+{
+ struct nsh_hdr *nsh;
+ int err;
+ u8 flags;
+ int i;
+
+ err = skb_ensure_writable(skb, skb_network_offset(skb) +
+ sizeof(struct nsh_hdr));
+ if (unlikely(err))
+ return err;
+
+ nsh = (struct nsh_hdr *)skb_network_header(skb);
+
+ flags = (ntohs(nsh->ver_flags_len) & NSH_FLAGS_MASK) >>
+ NSH_FLAGS_SHIFT;
+ flags = OVS_MASKED(flags, key->flags, mask->flags);
+ flow_key->nsh.flags = flags;
+ nsh->ver_flags_len = htons(flags << NSH_FLAGS_SHIFT) |
+ (nsh->ver_flags_len & ~htons(NSH_FLAGS_MASK));
+ nsh->path_hdr = OVS_MASKED(nsh->path_hdr, key->path_hdr,
+ mask->path_hdr);
+ flow_key->nsh.path_hdr = nsh->path_hdr;
+ switch (nsh->md_type) {
+ case NSH_M_TYPE1:
+ for (i = 0; i < 4; i++) {
+ nsh->md1.c[i] =
+ OVS_MASKED(nsh->md1.c[i], key->c[i], mask->c[i]);
+ flow_key->nsh.c[i] = nsh->md1.c[i];
+ }
+ break;
+ case NSH_M_TYPE2:
+ for (i = 0; i < 4; i++)
+ flow_key->nsh.c[i] = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
/* Must follow skb_ensure_writable() since that can move the skb data. */
static void set_tp_port(struct sk_buff *skb, __be16 *port,
__be16 new_port, __sum16 *check)
@@ -1024,6 +1176,11 @@ static int execute_masked_set_action(struct sk_buff *skb,
get_mask(a, struct ovs_key_ethernet *));
break;
+ case OVS_KEY_ATTR_NSH:
+ err = set_nsh(skb, flow_key, nla_data(a),
+ get_mask(a, struct ovs_key_nsh *));
+ break;
+
case OVS_KEY_ATTR_IPV4:
err = set_ipv4(skb, flow_key, nla_data(a),
get_mask(a, struct ovs_key_ipv4 *));
@@ -1210,6 +1367,14 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
case OVS_ACTION_ATTR_POP_ETH:
err = pop_eth(skb, key);
break;
+
+ case OVS_ACTION_ATTR_ENCAP_NSH:
+ err = encap_nsh(skb, key, nla_data(a));
+ break;
+
+ case OVS_ACTION_ATTR_DECAP_NSH:
+ err = decap_nsh(skb, key);
+ break;
}
if (unlikely(err)) {
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 8c94cef..dc8631c 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -46,6 +46,7 @@
#include <net/ipv6.h>
#include <net/mpls.h>
#include <net/ndisc.h>
+#include <net/nsh.h>
#include "conntrack.h"
#include "datapath.h"
@@ -490,6 +491,42 @@ static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,
return 0;
}
+static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
+{
+ struct nsh_hdr *nsh = (struct nsh_hdr *)skb_network_header(skb);
+ u16 ver_flags_len;
+ u8 version, length;
+ u32 path_hdr;
+ int i;
+
+ memset(&key->nsh, 0, sizeof(struct ovs_key_nsh));
+ ver_flags_len = ntohs(nsh->ver_flags_len);
+ version = (ver_flags_len & NSH_VER_MASK) >> NSH_VER_SHIFT;
+ length = (ver_flags_len & NSH_LEN_MASK) >> NSH_LEN_SHIFT;
+
+ key->nsh.flags = (ver_flags_len & NSH_FLAGS_MASK) >> NSH_FLAGS_SHIFT;
+ key->nsh.mdtype = nsh->md_type;
+ key->nsh.np = nsh->next_proto;
+ path_hdr = ntohl(nsh->path_hdr);
+ key->nsh.path_hdr = nsh->path_hdr;
+ switch (key->nsh.mdtype) {
+ case NSH_M_TYPE1:
+ if ((length << 2) != NSH_M_TYPE1_LEN)
+ return -EINVAL;
+
+ for (i = 0; i < 4; i++)
+ key->nsh.c[i] = nsh->md1.c[i];
+
+ break;
+ case NSH_M_TYPE2:
+ /* Don't support MD type 2 yet */
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
/**
* key_extract - extracts a flow key from an Ethernet frame.
* @skb: sk_buff that contains the frame, with skb->data pointing to the
@@ -735,6 +772,10 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
memset(&key->tp, 0, sizeof(key->tp));
}
}
+ } else if (key->eth.type == htons(ETH_P_NSH)) {
+ error = parse_nsh(skb, key);
+ if (error)
+ return error;
}
return 0;
}
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index 1875bba..d2a0e56 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -144,6 +144,7 @@ struct sw_flow_key {
};
} ipv6;
};
+ struct ovs_key_nsh nsh; /* network service header */
struct {
/* Connection tracking fields not packed above. */
struct {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index f07d10a..147b3c0 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -48,6 +48,7 @@
#include <net/ndisc.h>
#include <net/mpls.h>
#include <net/vxlan.h>
+#include <net/nsh.h>
#include "flow_netlink.h"
@@ -76,9 +77,11 @@ static bool actions_may_change_flow(const struct nlattr *actions)
case OVS_ACTION_ATTR_CT:
case OVS_ACTION_ATTR_HASH:
+ case OVS_ACTION_ATTR_DECAP_NSH:
case OVS_ACTION_ATTR_POP_ETH:
case OVS_ACTION_ATTR_POP_MPLS:
case OVS_ACTION_ATTR_POP_VLAN:
+ case OVS_ACTION_ATTR_ENCAP_NSH:
case OVS_ACTION_ATTR_PUSH_ETH:
case OVS_ACTION_ATTR_PUSH_MPLS:
case OVS_ACTION_ATTR_PUSH_VLAN:
@@ -327,7 +330,7 @@ size_t ovs_key_attr_size(void)
/* Whenever adding new OVS_KEY_ FIELDS, we should consider
* updating this function.
*/
- BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 28);
+ BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 29);
return nla_total_size(4) /* OVS_KEY_ATTR_PRIORITY */
+ nla_total_size(0) /* OVS_KEY_ATTR_TUNNEL */
@@ -341,6 +344,7 @@ size_t ovs_key_attr_size(void)
+ nla_total_size(4) /* OVS_KEY_ATTR_CT_MARK */
+ nla_total_size(16) /* OVS_KEY_ATTR_CT_LABELS */
+ nla_total_size(40) /* OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6 */
+ + nla_total_size(24) /* OVS_KEY_ATTR_NSH */
+ nla_total_size(12) /* OVS_KEY_ATTR_ETHERNET */
+ nla_total_size(2) /* OVS_KEY_ATTR_ETHERTYPE */
+ nla_total_size(4) /* OVS_KEY_ATTR_VLAN */
@@ -405,6 +409,7 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
.len = sizeof(struct ovs_key_ct_tuple_ipv4) },
[OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6] = {
.len = sizeof(struct ovs_key_ct_tuple_ipv6) },
+ [OVS_KEY_ATTR_NSH] = { .len = sizeof(struct ovs_key_nsh) },
};
static bool check_attr_len(unsigned int attr_len, unsigned int expected_len)
@@ -1306,6 +1311,22 @@ static int ovs_key_from_nlattrs(struct net *net, struct sw_flow_match *match,
attrs &= ~(1 << OVS_KEY_ATTR_ARP);
}
+ if (attrs & (1 << OVS_KEY_ATTR_NSH)) {
+ int i;
+ const struct ovs_key_nsh *nsh_key;
+
+ nsh_key = nla_data(a[OVS_KEY_ATTR_NSH]);
+ SW_FLOW_KEY_PUT(match, nsh.flags, nsh_key->flags, is_mask);
+ SW_FLOW_KEY_PUT(match, nsh.mdtype, nsh_key->mdtype, is_mask);
+ SW_FLOW_KEY_PUT(match, nsh.np, nsh_key->np, is_mask);
+ SW_FLOW_KEY_PUT(match, nsh.path_hdr, nsh_key->path_hdr,
+ is_mask);
+ for (i = 0; i < 4; i++)
+ SW_FLOW_KEY_PUT(match, nsh.c[i], nsh_key->c[i],
+ is_mask);
+ attrs &= ~(1 << OVS_KEY_ATTR_NSH);
+ }
+
if (attrs & (1 << OVS_KEY_ATTR_MPLS)) {
const struct ovs_key_mpls *mpls_key;
@@ -1750,6 +1771,21 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
ipv6_key->ipv6_tclass = output->ip.tos;
ipv6_key->ipv6_hlimit = output->ip.ttl;
ipv6_key->ipv6_frag = output->ip.frag;
+ } else if (swkey->eth.type == htons(ETH_P_NSH)) {
+ int i;
+ struct ovs_key_nsh *nsh_key;
+
+ nla = nla_reserve(skb, OVS_KEY_ATTR_NSH, sizeof(*nsh_key));
+ if (!nla)
+ goto nla_put_failure;
+ nsh_key = nla_data(nla);
+ memset(nsh_key, 0, sizeof(struct ovs_key_nsh));
+ nsh_key->flags = output->nsh.flags;
+ nsh_key->mdtype = output->nsh.mdtype;
+ nsh_key->np = output->nsh.np;
+ nsh_key->path_hdr = output->nsh.path_hdr;
+ for (i = 0; i < 4; i++)
+ nsh_key->c[0] = output->nsh.c[i];
} else if (swkey->eth.type == htons(ETH_P_ARP) ||
swkey->eth.type == htons(ETH_P_RARP)) {
struct ovs_key_arp *arp_key;
@@ -2384,6 +2420,9 @@ static int validate_set(const struct nlattr *a,
break;
+ case OVS_KEY_ATTR_NSH:
+ break;
+
default:
return -EINVAL;
}
@@ -2482,6 +2521,8 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
[OVS_ACTION_ATTR_TRUNC] = sizeof(struct ovs_action_trunc),
[OVS_ACTION_ATTR_PUSH_ETH] = sizeof(struct ovs_action_push_eth),
[OVS_ACTION_ATTR_POP_ETH] = 0,
+ [OVS_ACTION_ATTR_ENCAP_NSH] = sizeof(struct ovs_action_encap_nsh),
+ [OVS_ACTION_ATTR_DECAP_NSH] = 0,
};
const struct ovs_action_push_vlan *vlan;
int type = nla_type(a);
@@ -2636,6 +2677,17 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
mac_proto = MAC_PROTO_ETHERNET;
break;
+ case OVS_ACTION_ATTR_ENCAP_NSH:
+ mac_proto = MAC_PROTO_NONE;
+ break;
+
+ case OVS_ACTION_ATTR_DECAP_NSH:
+ if (key->nsh.np == NSH_P_ETHERNET)
+ mac_proto = MAC_PROTO_ETHERNET;
+ else
+ mac_proto = MAC_PROTO_NONE;
+ break;
+
default:
OVS_NLERR(log, "Unknown Action type %d", type);
return -EINVAL;
--
2.5.5
^ permalink raw reply related
* Re: [PATCH 0/6] In-kernel QMI handling
From: Bjorn Andersson @ 2017-08-08 4:45 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Dan Williams, David S. Miller, Andy Gross, David Brown,
linux-arm-msm, linux-soc, Netdev list, linux-kernel
In-Reply-To: <08647880-96CF-4A0A-A75D-0A324022CF32@holtmann.org>
On Mon 07 Aug 12:19 PDT 2017, Marcel Holtmann wrote:
> Hi Bjorn,
>
> >>> This series starts by moving the common definitions of the QMUX
> >>> protocol to the
> >>> uapi header, as they are shared with clients - both in kernel and
> >>> userspace.
> >>>
> >>> This series then introduces in-kernel helper functions for aiding the
> >>> handling
> >>> of QMI encoded messages in the kernel. QMI encoding is a wire-format
> >>> used in
> >>> exchanging messages between the majority of QRTR clients and
> >>> services.
> >>
> >> This raises a few red-flags for me.
> >
> > I'm glad it does. In discussions with the responsible team within
> > Qualcomm I've highlighted a number of concerns about enabling this
> > support in the kernel. Together we're continuously looking into what
> > should be pushed out to user space, and trying to not introduce
> > unnecessary new users.
> >
> >> So far, we've kept almost everything QMI related in userspace and
> >> handled all QMI control-channel messages from libraries like libqmi or
> >> uqmi via the cdc-wdm driver and the "rmnet" interface via the qmi_wwan
> >> driver. The kernel drivers just serve as the transport.
> >>
> >
> > The path that was taken to support the MSM-style devices was to
> > implement net/qrtr, which exposes a socket interface to abstract the
> > physical transports (QMUX or IPCROUTER in Qualcomm terminology).
> >
> > As I share you view on letting the kernel handle the transportation only
> > the task of keeping track of registered services (service id -> node and
> > port mapping) was done in a user space process and so far we've only
> > ever have to deal with QMI encoded messages in various user space tools.
>
> I think that the transport and multiplexing can be in the kernel as
> long as it is done as proper subsystem. Similar to Phonet or CAIF.
> Meaning it should have a well defined socket interface that can be
> easily used from userspace, but also a clean in-kernel interface
> handling.
>
In a mobile Qualcomm device there's a few different components involved
here: message routing, QMUX protocol and QMI-encoding.
The downstream Qualcomm kernel implements the two first in the
IPCROUTER, upstream this is split between the kernel net/qrtr and a user
space service-register implementing the QMUX protocol for knowing where
services are located.
The common encoding of messages passed between endpoints of the message
routing is QMI, which is made an affair totally that of each client.
> If Qualcomm is supportive of this effort and is willing to actually
> assist and/or open some of the specs or interface descriptions, then
> this is a good thing. Service registration and cleanup is really done
> best in the kernel. Same applies to multiplexing. Trying to do
> multiplexing in userspace is always cumbersome and leads to overhead
> that is of no gain. For example within oFono, we had to force
> everything to go via oFono since it was the only sane way of handling
> it. Other approaches were error prone and full of race conditions. You
> need a central entity that can clean up.
>
The current upstream solution depends on a collaboration between
net/qrtr and the user space service register for figuring out whom to
send messages to. After that muxing et al is handled by the socket
interface and service registry does not need to be involved.
Qualcomm is very supporting of this solution and we're collaborating on
transitioning "downstream" to use this implementation.
> For the definition of an UAPI to share some code, I am actually not
> sure that is such a good idea. For example the QMI code in oFono
> follows a way simpler approach. And I am not convinced that all the
> macros are actually beneficial. For example, the whole netlink macros
> are pretty cumbersome. Adding some Documentation/qmi.txt on how the
> wire format looks like and what is expected seems to be a way better
> approach.
>
The socket interface provided by the kernel expects some knowledge of
the QMUX protocol, for service management. The majority of this
knowledge is already public, but I agree that it would be good to gather
this in a document. The common data structure for the control message is
what I've put in the uapi, as this is used by anyone dealing with
control messages.
When it comes to the QMI-encoded messages these are application
specific, just like e.g. protobuf definitions are application specific.
As the core infrastructure is becoming available upstream and boards
like the DB410c and DB820c aim to be supported by open solutions we will
have a natural place to discuss publication of at least some of the
application level protocols.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH net-next 03/14] sctp: remove the typedef sctp_scope_policy_t
From: Xin Long @ 2017-08-08 2:45 UTC (permalink / raw)
To: David Laight
Cc: network dev, linux-sctp@vger.kernel.org, Marcelo Ricardo Leitner,
Neil Horman, davem@davemloft.net
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD004C7B8@AcuExch.aculab.com>
On Mon, Aug 7, 2017 at 9:28 PM, David Laight <David.Laight@aculab.com> wrote:
> From: Xin Long
>> Sent: 05 August 2017 13:00
>> This patch is to remove the typedef sctp_scope_policy_t and keep
>> it's members as an anonymous enum.
>>
>> It is also to define SCTP_SCOPE_POLICY_MAX to replace the num 3
>> in sysctl.c to make codes clear.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>> include/net/sctp/constants.h | 6 ++++--
>> net/sctp/sysctl.c | 2 +-
>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>> index 922fba5..acb03eb 100644
>> --- a/include/net/sctp/constants.h
>> +++ b/include/net/sctp/constants.h
>> @@ -341,12 +341,14 @@ typedef enum {
>> SCTP_SCOPE_UNUSABLE, /* IPv4 unusable addresses */
>> } sctp_scope_t;
>>
>> -typedef enum {
>> +enum {
>> SCTP_SCOPE_POLICY_DISABLE, /* Disable IPv4 address scoping */
>> SCTP_SCOPE_POLICY_ENABLE, /* Enable IPv4 address scoping */
>> SCTP_SCOPE_POLICY_PRIVATE, /* Follow draft but allow IPv4 private addresses */
>> SCTP_SCOPE_POLICY_LINK, /* Follow draft but allow IPv4 link local addresses */
>> -} sctp_scope_policy_t;
>> +};
>> +
>> +#define SCTP_SCOPE_POLICY_MAX SCTP_SCOPE_POLICY_LINK
>
> Perhaps slightly better to end the enum with:
> SCTP_SCOPE_POLICY_COUNT, /* Number of policies */
> SCTP_SCOPE_POLICY_MAX = SCTP_SCOPE_POLICY_COUNT - 1 /* Last policy */
> };
It might be, so that new member coming will not change too much.
I just copied the idea of SCTP_EVENT_xxxx_MAX, SCTP_STATE_MAX :-)
^ permalink raw reply
* Re: [PATCH net] sctp: use __GFP_NOWARN for sctpw.fifo allocation
From: Xin Long @ 2017-08-08 2:35 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: network dev, linux-sctp, davem, Neil Horman
In-Reply-To: <20170806233927.GB7613@localhost.localdomain>
On Mon, Aug 7, 2017 at 11:39 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Sun, Aug 06, 2017 at 06:14:39PM +1200, Xin Long wrote:
>> On Sun, Aug 6, 2017 at 5:08 AM, Marcelo Ricardo Leitner
>> <marcelo.leitner@gmail.com> wrote:
>> > On Sat, Aug 05, 2017 at 08:31:09PM +0800, Xin Long wrote:
>> >> Chen Wei found a kernel call trace when modprobe sctp_probe with
>> >> bufsize set with a huge value.
>> >>
>> >> It's because in sctpprobe_init when alloc memory for sctpw.fifo,
>> >> the size is got from userspace. If it is too large, kernel will
>> >> fail and give a warning.
>> >
>> > Yes but sctp_probe can only be loaded by an admin and it would happen
>> > only during modprobe. It's different from the commit mentioned below, on
>> > which any user could trigger it.
>> yeah, in this way it's different, I think generally it's acceptable to have
>> this kinda warning call trace by admin.
>>
>> But it could get the feedback from the return value and the warning
>> call trace seems not useful. sometimes users may be confused
>
> users or admins?
admins.
>
>> with this call trace. So it may be better not to dump the warning ?
>>
>> Or you think it can be helpful if we leave it here ?
>
> I'm afraid we may be exagerating here. There are several other ways that
> an admin can trigger scary warnings, this one is no special. I'd rather
> leave this one to the mm defaults instead.
OK, I'm all for that.
>
>>
>> >
>> >>
>> >> As there will be a fallback allocation later, this patch is just
>> >> to fail silently and return ret, just as commit 0ccc22f425e5
>> >> ("sit: use __GFP_NOWARN for user controlled allocation") did.
>> >>
>> >> Reported-by: Chen Wei <weichen@redhat.com>
>> >> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> >> ---
>> >> net/sctp/probe.c | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/sctp/probe.c b/net/sctp/probe.c
>> >> index 6cc2152..5bf3164 100644
>> >> --- a/net/sctp/probe.c
>> >> +++ b/net/sctp/probe.c
>> >> @@ -210,7 +210,7 @@ static __init int sctpprobe_init(void)
>> >>
>> >> init_waitqueue_head(&sctpw.wait);
>> >> spin_lock_init(&sctpw.lock);
>> >> - if (kfifo_alloc(&sctpw.fifo, bufsize, GFP_KERNEL))
>> >> + if (kfifo_alloc(&sctpw.fifo, bufsize, GFP_KERNEL | __GFP_NOWARN))
>> >> return ret;
>> >>
>> >> if (!proc_create(procname, S_IRUSR, init_net.proc_net,
>> >> --
>> >> 2.1.0
>> >>
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> >> the body of a message to majordomo@vger.kernel.org
>> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
^ permalink raw reply
* Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
From: Xin Long @ 2017-08-08 2:33 UTC (permalink / raw)
To: Cong Wang; +Cc: network dev, David Miller, netfilter-devel, Jamal Hadi Salim
In-Reply-To: <CAM_iQpWcg0a46gJ6Qp3F1pqvvsaJYajmDC2uG_mi9Av_vkFo2w@mail.gmail.com>
On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> (Cc'ing netfilter and Jamal)
>
> On Sat, Aug 5, 2017 at 4:35 AM, Xin Long <lucien.xin@gmail.com> wrote:
>> As we know in some target's checkentry it may dereference par.entryinfo
>> to check entry stuff inside. But when sched action calls xt_check_target,
>> par.entryinfo is set with NULL. It would cause kernel panic when calling
>> some targets.
>>
>> It can be reproduce with:
>> # tc qd add dev eth1 ingress handle ffff:
>> # tc filter add dev eth1 parent ffff: u32 match u32 0 0 action xt \
>> -j ECN --ecn-tcp-remove
>>
>> It could also crash kernel when using target CLUSTERIP or TPROXY.
>>
[1]
>> By now there's no proper value for par.entryinfo in ipt_init_target,
>> but it can not be set with NULL. This patch is to void all these
>> panics by setting it with an ipt_entry obj with all members 0.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>> net/sched/act_ipt.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
>> index 7c4816b..0f09f70 100644
>> --- a/net/sched/act_ipt.c
>> +++ b/net/sched/act_ipt.c
>> @@ -41,6 +41,7 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t,
>> {
>> struct xt_tgchk_param par;
>> struct xt_target *target;
>> + struct ipt_entry e;
>> int ret = 0;
>>
>> target = xt_request_find_target(AF_INET, t->u.user.name,
>> @@ -48,10 +49,11 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t,
>> if (IS_ERR(target))
>> return PTR_ERR(target);
>>
>> + memset(&e, 0, sizeof(e));
>> t->u.kernel.target = target;
>> par.net = net;
>> par.table = table;
>> - par.entryinfo = NULL;
>> + par.entryinfo = &e;
>
> This looks like a completely API burden?
netfilter xt targets are not really compatible with netsched action.
I've got to say, the patch is just a way to make checkentry return
false and avoid panic. like [1] said
^ permalink raw reply
* Re: [PATCH net] net: sched: set xt_tgchk_param par.nft_compat with false in ipt_init_target
From: Xin Long @ 2017-08-08 2:17 UTC (permalink / raw)
To: Cong Wang; +Cc: network dev, David Miller, Pablo Neira Ayuso
In-Reply-To: <CAM_iQpUT8CMdz1sgBaVS+7drzPPFp10APOHiKPib8oNq7kPsFg@mail.gmail.com>
On Tue, Aug 8, 2017 at 9:03 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Sat, Aug 5, 2017 at 4:32 AM, Xin Long <lucien.xin@gmail.com> wrote:
>> Commit 55917a21d0cc ("netfilter: x_tables: add context to know if
>> extension runs from nft_compat") introduced a member nft_compat to
>> xt_tgchk_param structure.
>>
>> But it didn't set it's value for ipt_init_target. With unexpected
>> value in par.nft_compat, it may return unexpected result in some
>> target's checkentry.
>>
>> This patch is to set par.nft_compat with false in ipt_init_target.
>
> It's time to set all these fields to 0 and only initialize those non-zero
> fields, in case we will add more fields in the future.
ok, the new fix is depend on the net_id one.
I will post v2 after that one gets accepted. thanks.
^ permalink raw reply
* [PATCHv2 net] net: sched: set xt_tgchk_param par.net properly in ipt_init_target
From: Xin Long @ 2017-08-08 2:13 UTC (permalink / raw)
To: network dev; +Cc: davem, Cong Wang
Now xt_tgchk_param par in ipt_init_target is a local varibale,
par.net is not initialized there. Later when xt_check_target
calls target's checkentry in which it may access par.net, it
would cause kernel panic.
Jaroslav found this panic when running:
# ip link add TestIface type dummy
# tc qd add dev TestIface ingress handle ffff:
# tc filter add dev TestIface parent ffff: u32 match u32 0 0 \
action xt -j CONNMARK --set-mark 4
This patch is to pass net param into ipt_init_target and set
par.net with it properly in there.
v1->v2:
As Wang Cong pointed, I missed ipt_net_id != xt_net_id, so fix
it by also passing net_id to __tcf_ipt_init.
Reported-by: Jaroslav Aster <jaster@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sched/act_ipt.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 36f0ced..94ba5cf 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -36,8 +36,8 @@ static struct tc_action_ops act_ipt_ops;
static unsigned int xt_net_id;
static struct tc_action_ops act_xt_ops;
-static int ipt_init_target(struct xt_entry_target *t, char *table,
- unsigned int hook)
+static int ipt_init_target(struct net *net, struct xt_entry_target *t,
+ char *table, unsigned int hook)
{
struct xt_tgchk_param par;
struct xt_target *target;
@@ -49,6 +49,7 @@ static int ipt_init_target(struct xt_entry_target *t, char *table,
return PTR_ERR(target);
t->u.kernel.target = target;
+ par.net = net;
par.table = table;
par.entryinfo = NULL;
par.target = target;
@@ -91,10 +92,11 @@ static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
[TCA_IPT_TARG] = { .len = sizeof(struct xt_entry_target) },
};
-static int __tcf_ipt_init(struct tc_action_net *tn, struct nlattr *nla,
+static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
const struct tc_action_ops *ops, int ovr, int bind)
{
+ struct tc_action_net *tn = net_generic(net, id);
struct nlattr *tb[TCA_IPT_MAX + 1];
struct tcf_ipt *ipt;
struct xt_entry_target *td, *t;
@@ -159,7 +161,7 @@ static int __tcf_ipt_init(struct tc_action_net *tn, struct nlattr *nla,
if (unlikely(!t))
goto err2;
- err = ipt_init_target(t, tname, hook);
+ err = ipt_init_target(net, t, tname, hook);
if (err < 0)
goto err3;
@@ -193,18 +195,16 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
int bind)
{
- struct tc_action_net *tn = net_generic(net, ipt_net_id);
-
- return __tcf_ipt_init(tn, nla, est, a, &act_ipt_ops, ovr, bind);
+ return __tcf_ipt_init(net, ipt_net_id, nla, est, a, &act_ipt_ops, ovr,
+ bind);
}
static int tcf_xt_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
int bind)
{
- struct tc_action_net *tn = net_generic(net, xt_net_id);
-
- return __tcf_ipt_init(tn, nla, est, a, &act_xt_ops, ovr, bind);
+ return __tcf_ipt_init(net, xt_net_id, nla, est, a, &act_xt_ops, ovr,
+ bind);
}
static int tcf_ipt(struct sk_buff *skb, const struct tc_action *a,
--
2.1.0
^ permalink raw reply related
* Re: [PATCH v9 0/4] Add new PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag
From: Bjorn Helgaas @ 2017-08-08 1:56 UTC (permalink / raw)
To: David Miller
Cc: dingtianhong, bhelgaas, leedom, ashok.raj, werner, ganeshgr,
asit.k.mallick, patrick.j.cramer, Suravee.Suthikulpanit, Bob.Shaw,
l.stach, amira, gabriele.paoloni, David.Laight, jeffrey.t.kirsher,
catalin.marinas, will.deacon, mark.rutland, robin.murphy,
alexander.duyck, linux-arm-kernel, netdev, linux-pci,
linux-kernel, linuxarm
In-Reply-To: <20170807.141448.1388035794329323595.davem@davemloft.net>
On Mon, Aug 07, 2017 at 02:14:48PM -0700, David Miller wrote:
> From: Ding Tianhong <dingtianhong@huawei.com>
> Date: Mon, 7 Aug 2017 12:13:17 +0800
>
> > Hi David：
> >
> > I think networking tree merge it is a better choice, as it mainly used to tell the NIC
> > drivers how to use the Relaxed Ordering Attribute, and later we need send patch to enable
> > RO for ixgbe driver base on this patch. But I am not sure whether Bjorn has some of his own
> > view. :)
> >
> > Hi Bjorn:
> >
> > Could you help review this patch or give some feedback ?
>
> I'm still waiting on this...
>
> Bjorn?
I was on vacation Friday-today, but I'll look at this series this week.
^ permalink raw reply
* Re: [PATCH RFC v2 3/5] samples/bpf: Fix inline asm issues building samples on arm64
From: Joel Fernandes @ 2017-08-08 1:20 UTC (permalink / raw)
To: David Miller
Cc: LKML, Chenbo Feng, Alison Chaiken, Juri Lelli, Alexei Starovoitov,
Daniel Borkmann, open list:BPF (Safe dynamic programs and tools)
In-Reply-To: <20170807.112840.395506747161262549.davem@davemloft.net>
Hi Dave,
On Mon, Aug 7, 2017 at 11:28 AM, David Miller <davem@davemloft.net> wrote:
>
> Please, no.
Sorry you dislike it, I had intentionally marked it as RFC as its an
idea I was just toying with the idea and posted it early to get
feedback.
>
> The amount of hellish hacks we are adding to deal with this is getting
> way out of control.
I agree with you that hellish hacks are being added which is why it
keeps breaking. I think one of the things my series does is to add
back inclusion of asm headers that were previously removed (that is
the worst hellish hack in my opinion that existing in mainline). So in
that respect my patch is an improvement and makes it possible to build
for arm64 platforms (which is currently broken in mainline).
>
> BPF programs MUST have their own set of asm headers, this is the
> only way to get around this issue in the long term.
Wouldn't that break scripts or bpf code that instruments/trace arch
specific code?
>
> I am also strongly against adding -static to the build.
I can drop -static if you prefer, that's not an issue.
As I understand it, there are no other cleaner alternatives and this
patchset makes the samples work. I would even argue that's its more
functional than previous attempts and fixes something broken in
mainline in a more generic way. If you can provide an example of where
my patchset may not work, I would love to hear it. My whole idea was
to do it in a way that makes future breakage not happen. I don't think
that leaving things broken in this state for extended periods of time
makes sense and IMHO will slow usage of bpf samples on other
platforms.
thanks,
-Joel
^ permalink raw reply
* Re:Re:Re: Re: [PATCH net] ppp: Fix a scheduling-while-atomic bug in del_chan
From: Gao Feng @ 2017-08-08 1:10 UTC (permalink / raw)
To: Gao Feng; +Cc: Cong Wang, xeb, David Miller, Linux Kernel Network Developers
In-Reply-To: <697dbbd.7911.15dbf5ca3a6.Coremail.gfree.wind@vip.163.com>
At 2017-08-08 01:17:02, "Cong Wang" <xiyou.wangcong@gmail.com> wrote:
>On Sun, Aug 6, 2017 at 6:32 PM, Gao Feng <gfree.wind@vip.163.com> wrote:
>> I think the RCU should be supposed to avoid the race between del_chan and lookup_chan.
>
>More precisely, it is callid_sock which is protected by RCU.
>
>Unless I miss any other code path, pptp_exit_module() is
>problematic too, I don't think it can just vfree() the whole thing.
>
>
>> The synchronize_rcu could make sure if there was one which calls lookup_chan in this period, it would be finished and the sock refcnt is increased if necessary.
>>
>> So I think it is ok to invoke sock_put directly without SOCK_RCU_FREE, because the lookup_chan caller has already hold the sock refcnt,
>>
>
Hi Cong,
I just thought about this issue last night, then I get your this email this morning.
>If you mean the sock_hold() inside lookup_chan(), no,
>it doesn't help because we already dereference the sock
>before it.
>
Sorry, I don't get you clearly. Why the sock_hold() isn't helpful?
The pptp_release invokes synchronize_rcu after del_chan, it could make sure the others has increased the sock refcnt if necessary
and the lookup is over.
There is no one could get the sock after synchronize_rcu in pptp_release.
But I think about another problem.
It seems the pptp_sock_destruct should not invoke del_chan and pppox_unbind_sock.
Because when the sock refcnt is 0, the pptp_release must have be invoked already.
There are two cases totally.
1. when pptp_release invokes sock_put, the refcnt is 0. The del_chan and pppox_unbind_sock are invoked.
2. when pptp_release invokes sock_put, the refcnt is not 0. It means someone holds the sock during the period pptp_release invokes del_chan.
Then someone invokes sock_put and the sock refcnt reach 0, it would invoke sk_free and invokes pptp_sock_destruct.
So it is unnecessary to invoke del_chan and pppox_unbind_sock again.
And it would bring a race issue even if the pptp_sock_destruct invoked del_chan.
If so, I would send another patch for it.
>Also, lookup_chan_dst() does not have a refcnt, I don't
>find any code preventing it deref'ing other sock in callid_sock
>than the calling one.
Sorry, the last email is html format, not text.
So I send it with text format again.
Best Regards
Feng
^ permalink raw reply
* Re: [PATCH net] net: sched: set xt_tgchk_param par.net properly in ipt_init_target
From: Xin Long @ 2017-08-08 0:53 UTC (permalink / raw)
To: Cong Wang; +Cc: network dev, David Miller
In-Reply-To: <CAM_iQpWT3=kgc1Vh2BNjXSa6_n-p8+-kLhVbsaE0DzqCXMLEmQ@mail.gmail.com>
On Tue, Aug 8, 2017 at 9:00 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Sat, Aug 5, 2017 at 1:48 AM, Xin Long <lucien.xin@gmail.com> wrote:
>> -static int __tcf_ipt_init(struct tc_action_net *tn, struct nlattr *nla,
>> +static int __tcf_ipt_init(struct net *net, struct nlattr *nla,
>> struct nlattr *est, struct tc_action **a,
>> const struct tc_action_ops *ops, int ovr, int bind)
>> {
>> + struct tc_action_net *tn = net_generic(net, xt_net_id);
>
> ...
>
>> @@ -193,18 +195,14 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla,
>> struct nlattr *est, struct tc_action **a, int ovr,
>> int bind)
>> {
>> - struct tc_action_net *tn = net_generic(net, ipt_net_id);
>> -
>> - return __tcf_ipt_init(tn, nla, est, a, &act_ipt_ops, ovr, bind);
>> + return __tcf_ipt_init(net, nla, est, a, &act_ipt_ops, ovr, bind);
>> }
>>
>> static int tcf_xt_init(struct net *net, struct nlattr *nla,
>> struct nlattr *est, struct tc_action **a, int ovr,
>> int bind)
>> {
>> - struct tc_action_net *tn = net_generic(net, xt_net_id);
>> -
>> - return __tcf_ipt_init(tn, nla, est, a, &act_xt_ops, ovr, bind);
>> + return __tcf_ipt_init(net, nla, est, a, &act_xt_ops, ovr, bind);
>
> This is not correct.
>
> You miss ipt_net_id != xt_net_id.
right, that's a silly mistake. seems no better way but to pass both
net and net_id to __tcf_ipt_init. will send v2. thanks.
^ permalink raw reply
* Re: [PATCH v5 net-next 00/12] bpf: rewrite value tracking in verifier
From: Daniel Borkmann @ 2017-08-08 0:46 UTC (permalink / raw)
To: Edward Cree, davem
Cc: Alexei Starovoitov, Alexei Starovoitov, netdev, linux-kernel,
iovisor-dev
In-Reply-To: <ad840039-8d4a-b2a9-b2eb-a8f079926b53@solarflare.com>
On 08/07/2017 04:21 PM, Edward Cree wrote:
> This series simplifies alignment tracking, generalises bounds tracking and
> fixes some bounds-tracking bugs in the BPF verifier. Pointer arithmetic on
> packet pointers, stack pointers, map value pointers and context pointers has
> been unified, and bounds on these pointers are only checked when the pointer
> is dereferenced.
> Operations on pointers which destroy all relation to the original pointer
> (such as multiplies and shifts) are disallowed if !env->allow_ptr_leaks,
> otherwise they convert the pointer to an unknown scalar and feed it to the
> normal scalar arithmetic handling.
> Pointer types have been unified with the corresponding adjusted-pointer types
> where those existed (e.g. PTR_TO_MAP_VALUE[_ADJ] or FRAME_PTR vs
> PTR_TO_STACK); similarly, CONST_IMM and UNKNOWN_VALUE have been unified into
> SCALAR_VALUE.
> Pointer types (except CONST_PTR_TO_MAP, PTR_TO_MAP_VALUE_OR_NULL and
> PTR_TO_PACKET_END, which do not allow arithmetic) have a 'fixed offset' and
> a 'variable offset'; the former is used when e.g. adding an immediate or a
> known-constant register, as long as it does not overflow. Otherwise the
> latter is used, and any operation creating a new variable offset creates a
> new 'id' (and, for PTR_TO_PACKET, clears the 'range').
> SCALAR_VALUEs use the 'variable offset' fields to track the range of possible
> values; the 'fixed offset' should never be set on a scalar.
Been testing and reviewing the series over the last several days, looks
reasonable to me as far as I can tell. Thanks for all the hard work on
unifying this, Edward!
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* [PATCH] Allow passing tid or pid in SCM_CREDENTIALS without CAP_SYS_ADMIN
From: Prakash Sangappa @ 2017-08-08 0:07 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: davem, ebiederm, prakash.sangappa
Currently passing tid(gettid(2)) of a thread in struct ucred in
SCM_CREDENTIALS message requires CAP_SYS_ADMIN capability otherwise
it fails with EPERM error. Some applications deal with thread id
of a thread(tid) and so it would help to allow tid in SCM_CREDENTIALS
message. Basically, either tgid(pid of the process) or the tid of
the thread should be allowed without the need for CAP_SYS_ADMIN capability.
SCM_CREDENTIALS will be used to determine the global id of a process or
a thread running inside a pid namespace.
This patch adds necessary check to accept tid in SCM_CREDENTIALS
struct ucred.
Signed-off-by: Prakash Sangappa <prakash.sangappa@oracle.com>
---
net/core/scm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/core/scm.c b/net/core/scm.c
index b1ff8a4..9274197 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -55,6 +55,7 @@ static __inline__ int scm_check_creds(struct ucred *creds)
return -EINVAL;
if ((creds->pid == task_tgid_vnr(current) ||
+ creds->pid == task_pid_vnr(current) ||
ns_capable(task_active_pid_ns(current)->user_ns, CAP_SYS_ADMIN)) &&
((uid_eq(uid, cred->uid) || uid_eq(uid, cred->euid) ||
uid_eq(uid, cred->suid)) || ns_capable(cred->user_ns, CAP_SETUID)) &&
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] net: Reduce skb_warn_bad_offload() noise.
From: Tonghao Zhang @ 2017-08-07 23:44 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Eric Dumazet, Linux Kernel Network Developers, Eric Dumazet,
Willem de Bruijn, Pravin B Shelar
In-Reply-To: <CAF=yD-K_nbns=KA_4Acq_PRA=tCP0r+d3fD=jiVx9DkAdkyUYw@mail.gmail.com>
That is fine to me. I have tested it. Thanks.
On Mon, Aug 7, 2017 at 12:42 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>> The openvswitch kernel module calls the __skb_gso_segment()(and sets
>> tx_path = false) when passing packets to userspace. The UFO will set
>> the ip_summed to CHECKSUM_NONE. There are a lot of warn logs. The warn
>> log is shown as below. I guess we should revert the patch.
>
> Indeed, the software UFO code computes the checksum and
> sets ip_summed to CHECKSUM_NONE, as is correct on the
> egress path.
>
> Commit 6e7bc478c9a0 ("net: skb_needs_check() accepts
> CHECKSUM_NONE for tx") revised the tx_path case in
> skb_needs_check to avoid the warning exactly for the UFO case.
>
> We cannot just make an exception for CHECKSUM_NONE in the
> !tx_path case, as the entire statement then becomes false:
>
> return skb->ip_summed == CHECKSUM_NONE;
>
> Since on egress CHECKSUM_UNNECESSARY is equivalent to
> CHECKSUM_NONE, it should be fine to update the UFO code
> to set that, instead:
>
> @@ -235,7 +235,7 @@ static struct sk_buff *udp4_ufo_fragment(struct
> sk_buff *skb,
> if (uh->check == 0)
> uh->check = CSUM_MANGLED_0;
>
> - skb->ip_summed = CHECKSUM_NONE;
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
^ permalink raw reply
* Re: Qdisc->u32_node - licence to kill
From: Cong Wang @ 2017-08-07 23:21 UTC (permalink / raw)
To: John Fastabend
Cc: Jiri Pirko, Jamal Hadi Salim, David Miller,
Linux Kernel Network Developers, mlxsw
In-Reply-To: <5988C55B.60204@gmail.com>
On Mon, Aug 7, 2017 at 12:54 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> On 08/07/2017 12:06 PM, Jiri Pirko wrote:
>> Mon, Aug 07, 2017 at 07:47:14PM CEST, john.fastabend@gmail.com wrote:
>>> On 08/07/2017 09:41 AM, Jiri Pirko wrote:
>>>> Hi Jamal/Cong/David/all.
>>>>
>>>> Digging in the u32 code deeper now. I need to get rid of tp->q for shared
>>>> blocks, but I found out about this:
>>>>
>>>> struct Qdisc {
>>>> ......
>>>> void *u32_node;
>>>> ......
>>>> };
>>>>
>>>> Yeah, ugly. u32 uses it to store some shared data, tp_c. It actually
>>>> stores a linked list of all hashtables added to one qdiscs.
>>>>
>>>> So basically what you have is, you have 1 root ht per prio/pref. Then
>>>> you can have multiple hts, linked from any other ht, does not matter in
>>>> which prio/pref they are.
>>>>
>>>
>>> We can create arbitrary hash tables here independent of prio/pref via
>>> TCA_U32_DIVISOR. Then these can be linked to other hash tables via
>>> TCA_U32_LINK commands.
>>
>> Yeah, that's what I thought.
>>
>>
>>>
>>> prio/pref does not really play any part here from my reading, except as
>>> a further specifier in the walk callbacks. Making it a useful filter on
>>> dump operations.
>>
>> Not correct. prio/pref is one level up priority, independent on specific
>> cls implementation. You can have cls_u32 instance on prio 10 and
>> cls_flower instance on prio 20. Both work.
>
> ah right, lets make sure I got this right then (its been awhile since I've
> read this code). So the tcf_ctl_tfilter hook walks classifiers, inserting the
> classifier by prio. Then tcf_classify walks the list of classifiers looking
> for any matches, specifically any return codes it recognizes or a return code
> greater than zero. u32 though has this link notion that allows users to jump
> to other u32 classifiers that are in this list, because it has a global hash
> table list. So the per prio classifier isolation is not true in u32 case.
u32 filter supports multiple hash tables within a qdisc, struct
tc_u_common is supposed to link them together. This has to be
per qdisc because all of these hash tables belong to one qdisc
and their ID's are unique within the qdisc.
I dislike it too, and I actually tried to improve it in the past,
unfortunately didn't make any real progress. I think we can
definitely make it less ugly, but I don't think we can totally
get rid of it because of the design of u32.
Similar for tp->data.
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: lan9303: Only allocate 3 ports
From: Vivien Didelot @ 2017-08-07 23:10 UTC (permalink / raw)
To: Egil Hjelmeland, andrew, f.fainelli, netdev, linux-kernel; +Cc: Egil Hjelmeland
In-Reply-To: <20170807222221.28963-1-privat@egil-hjelmeland.no>
Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> Save 2628 bytes on arm eabi by allocate only the required 3 ports.
>
> Now that ds->num_ports is correct: In net/dsa/tag_lan9303.c
> eliminate duplicate LAN9303_MAX_PORTS, use ds->num_ports.
> (Matching the pattern of other net/dsa/tag_xxx.c files.)
>
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
^ permalink raw reply
* Re: [PATCH net-next] selftests: bpf: add a test for XDP redirect
From: John Fastabend @ 2017-08-07 23:03 UTC (permalink / raw)
To: William Tu, netdev; +Cc: Daniel Borkmann
In-Reply-To: <1502136882-40204-1-git-send-email-u9012063@gmail.com>
On 08/07/2017 01:14 PM, William Tu wrote:
> Add test for xdp_redirect by creating two namespaces with two
> veth peers, then forward packets in-between.
>
> Signed-off-by: William Tu <u9012063@gmail.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: John Fastabend <john.fastabend@gmail.com>
> ---
Thanks for doing this.
Acked-by: John Fastabend <john.fastabend@gmail.com>
^ permalink raw reply
* Re: [PATCH] ip/link_vti*.c: Fix output for ikey/okey
From: Stephen Hemminger @ 2017-08-07 22:38 UTC (permalink / raw)
To: Christian Langrock; +Cc: netdev
In-Reply-To: <bf2d09ad-28b8-c64c-dd60-4fb288259e3e@secunet.com>
[-- Attachment #1: Type: text/plain, Size: 565 bytes --]
On Mon, 7 Aug 2017 11:59:28 +0200
Christian Langrock <christian.langrock@secunet.com> wrote:
> ikey and okey are normal u32 values. There's no reason to print them as
> IPv4/IPv6 addresses.
>
> Signed-off-by: Christian Langrock <christian.langrock@secunet.com>
Changing output format breaks scripts that parse output.
But on the other hand, the VTI code breaks the assumption that ip command
output should be the same as input.
More likely the original output format was done to match Cisco output.
Why not print in hex like fwmark?
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: lan9303: Only allocate 3 ports
From: Florian Fainelli @ 2017-08-07 22:27 UTC (permalink / raw)
To: Egil Hjelmeland, andrew, vivien.didelot, netdev, linux-kernel
In-Reply-To: <20170807222221.28963-1-privat@egil-hjelmeland.no>
On 08/07/2017 03:22 PM, Egil Hjelmeland wrote:
> Save 2628 bytes on arm eabi by allocate only the required 3 ports.
>
> Now that ds->num_ports is correct: In net/dsa/tag_lan9303.c
> eliminate duplicate LAN9303_MAX_PORTS, use ds->num_ports.
> (Matching the pattern of other net/dsa/tag_xxx.c files.)
>
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* [Patch net-next] net_sched: get rid of some forward declarations
From: Cong Wang @ 2017-08-07 22:26 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Jamal Hadi Salim
If we move up tcf_fill_node() we can get rid of these
forward declarations.
Also, move down tfilter_notify_chain() to group them together.
Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/sched/cls_api.c | 214 +++++++++++++++++++++++++---------------------------
1 file changed, 103 insertions(+), 111 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 668afb6e9885..8d1157aebaf7 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -100,25 +100,6 @@ int unregister_tcf_proto_ops(struct tcf_proto_ops *ops)
}
EXPORT_SYMBOL(unregister_tcf_proto_ops);
-static int tfilter_notify(struct net *net, struct sk_buff *oskb,
- struct nlmsghdr *n, struct tcf_proto *tp,
- void *fh, int event, bool unicast);
-
-static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
- struct nlmsghdr *n, struct tcf_proto *tp,
- void *fh, bool unicast, bool *last);
-
-static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
- struct nlmsghdr *n,
- struct tcf_chain *chain, int event)
-{
- struct tcf_proto *tp;
-
- for (tp = rtnl_dereference(chain->filter_chain);
- tp; tp = rtnl_dereference(tp->next))
- tfilter_notify(net, oskb, n, tp, 0, event, false);
-}
-
/* Select new prio value from the range, managed by kernel. */
static inline u32 tcf_auto_prio(struct tcf_proto *tp)
@@ -411,6 +392,109 @@ static struct tcf_proto *tcf_chain_tp_find(struct tcf_chain *chain,
return tp;
}
+static int tcf_fill_node(struct net *net, struct sk_buff *skb,
+ struct tcf_proto *tp, void *fh, u32 portid,
+ u32 seq, u16 flags, int event)
+{
+ struct tcmsg *tcm;
+ struct nlmsghdr *nlh;
+ unsigned char *b = skb_tail_pointer(skb);
+
+ nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm), flags);
+ if (!nlh)
+ goto out_nlmsg_trim;
+ tcm = nlmsg_data(nlh);
+ tcm->tcm_family = AF_UNSPEC;
+ tcm->tcm__pad1 = 0;
+ tcm->tcm__pad2 = 0;
+ tcm->tcm_ifindex = qdisc_dev(tp->q)->ifindex;
+ tcm->tcm_parent = tp->classid;
+ tcm->tcm_info = TC_H_MAKE(tp->prio, tp->protocol);
+ if (nla_put_string(skb, TCA_KIND, tp->ops->kind))
+ goto nla_put_failure;
+ if (nla_put_u32(skb, TCA_CHAIN, tp->chain->index))
+ goto nla_put_failure;
+ if (!fh) {
+ tcm->tcm_handle = 0;
+ } else {
+ if (tp->ops->dump && tp->ops->dump(net, tp, fh, skb, tcm) < 0)
+ goto nla_put_failure;
+ }
+ nlh->nlmsg_len = skb_tail_pointer(skb) - b;
+ return skb->len;
+
+out_nlmsg_trim:
+nla_put_failure:
+ nlmsg_trim(skb, b);
+ return -1;
+}
+
+static int tfilter_notify(struct net *net, struct sk_buff *oskb,
+ struct nlmsghdr *n, struct tcf_proto *tp,
+ void *fh, int event, bool unicast)
+{
+ struct sk_buff *skb;
+ u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
+
+ skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
+ if (!skb)
+ return -ENOBUFS;
+
+ if (tcf_fill_node(net, skb, tp, fh, portid, n->nlmsg_seq,
+ n->nlmsg_flags, event) <= 0) {
+ kfree_skb(skb);
+ return -EINVAL;
+ }
+
+ if (unicast)
+ return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
+
+ return rtnetlink_send(skb, net, portid, RTNLGRP_TC,
+ n->nlmsg_flags & NLM_F_ECHO);
+}
+
+static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
+ struct nlmsghdr *n, struct tcf_proto *tp,
+ void *fh, bool unicast, bool *last)
+{
+ struct sk_buff *skb;
+ u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
+ int err;
+
+ skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
+ if (!skb)
+ return -ENOBUFS;
+
+ if (tcf_fill_node(net, skb, tp, fh, portid, n->nlmsg_seq,
+ n->nlmsg_flags, RTM_DELTFILTER) <= 0) {
+ kfree_skb(skb);
+ return -EINVAL;
+ }
+
+ err = tp->ops->delete(tp, fh, last);
+ if (err) {
+ kfree_skb(skb);
+ return err;
+ }
+
+ if (unicast)
+ return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
+
+ return rtnetlink_send(skb, net, portid, RTNLGRP_TC,
+ n->nlmsg_flags & NLM_F_ECHO);
+}
+
+static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
+ struct nlmsghdr *n,
+ struct tcf_chain *chain, int event)
+{
+ struct tcf_proto *tp;
+
+ for (tp = rtnl_dereference(chain->filter_chain);
+ tp; tp = rtnl_dereference(tp->next))
+ tfilter_notify(net, oskb, n, tp, 0, event, false);
+}
+
/* Add/change/delete/get a filter node */
static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
@@ -640,98 +724,6 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
return err;
}
-static int tcf_fill_node(struct net *net, struct sk_buff *skb,
- struct tcf_proto *tp, void *fh, u32 portid,
- u32 seq, u16 flags, int event)
-{
- struct tcmsg *tcm;
- struct nlmsghdr *nlh;
- unsigned char *b = skb_tail_pointer(skb);
-
- nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm), flags);
- if (!nlh)
- goto out_nlmsg_trim;
- tcm = nlmsg_data(nlh);
- tcm->tcm_family = AF_UNSPEC;
- tcm->tcm__pad1 = 0;
- tcm->tcm__pad2 = 0;
- tcm->tcm_ifindex = qdisc_dev(tp->q)->ifindex;
- tcm->tcm_parent = tp->classid;
- tcm->tcm_info = TC_H_MAKE(tp->prio, tp->protocol);
- if (nla_put_string(skb, TCA_KIND, tp->ops->kind))
- goto nla_put_failure;
- if (nla_put_u32(skb, TCA_CHAIN, tp->chain->index))
- goto nla_put_failure;
- if (!fh) {
- tcm->tcm_handle = 0;
- } else {
- if (tp->ops->dump && tp->ops->dump(net, tp, fh, skb, tcm) < 0)
- goto nla_put_failure;
- }
- nlh->nlmsg_len = skb_tail_pointer(skb) - b;
- return skb->len;
-
-out_nlmsg_trim:
-nla_put_failure:
- nlmsg_trim(skb, b);
- return -1;
-}
-
-static int tfilter_notify(struct net *net, struct sk_buff *oskb,
- struct nlmsghdr *n, struct tcf_proto *tp,
- void *fh, int event, bool unicast)
-{
- struct sk_buff *skb;
- u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
-
- skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
- if (!skb)
- return -ENOBUFS;
-
- if (tcf_fill_node(net, skb, tp, fh, portid, n->nlmsg_seq,
- n->nlmsg_flags, event) <= 0) {
- kfree_skb(skb);
- return -EINVAL;
- }
-
- if (unicast)
- return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
-
- return rtnetlink_send(skb, net, portid, RTNLGRP_TC,
- n->nlmsg_flags & NLM_F_ECHO);
-}
-
-static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
- struct nlmsghdr *n, struct tcf_proto *tp,
- void *fh, bool unicast, bool *last)
-{
- struct sk_buff *skb;
- u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
- int err;
-
- skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
- if (!skb)
- return -ENOBUFS;
-
- if (tcf_fill_node(net, skb, tp, fh, portid, n->nlmsg_seq,
- n->nlmsg_flags, RTM_DELTFILTER) <= 0) {
- kfree_skb(skb);
- return -EINVAL;
- }
-
- err = tp->ops->delete(tp, fh, last);
- if (err) {
- kfree_skb(skb);
- return err;
- }
-
- if (unicast)
- return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
-
- return rtnetlink_send(skb, net, portid, RTNLGRP_TC,
- n->nlmsg_flags & NLM_F_ECHO);
-}
-
struct tcf_dump_args {
struct tcf_walker w;
struct sk_buff *skb;
--
2.13.0
^ permalink raw reply related
* multi-queue over IFF_NO_QUEUE "virtual" devices
From: Florian Fainelli @ 2017-08-07 22:26 UTC (permalink / raw)
To: netdev, jiri, jhs, xiyou.wangcong; +Cc: davem, andrew, vivien.didelot
Hi,
Most DSA supported Broadcom switches have multiple queues per ports
(usually 8) and each of these queues can be configured with different
pause, drop, hysteresis thresholds and so on in order to make use of the
switch's internal buffering scheme and have some queues achieve some
kind of lossless behavior (e.g: LAN to LAN traffic for Q7 has a higher
priority than LAN to WAN for Q0).
This is obviously very workload specific, so I'd want maximum
programmability as much as possible.
This brings me to a few questions:
1) If we have the DSA slave network devices currently flagged with
IFF_NO_QUEUE becoming multi-queue (on TX) aware such that an application
can control exactly which switch egress queue is used on a per-flow
basis, would that be a problem (this is the dynamic selection of the TX
queue)?
2) The conduit interface (CPU) port network interface has a congestion
control scheme which requires each of its TX queues (32 or 16) to be
statically mapped to each of the underlying switch port queues because
the congestion/ HW needs to inspect the queue depths of the switch to
accept/reject a packet at the CPU's TX ring level. Do we have a good way
with tc to map a virtual/stacked device's queue(s) on-top of its
physical/underlying device's queues (this is the static queue mapping
necessary for congestion to work)?
Let me know if you think this is the right approach or not.
Thanks!
--
Florian
^ permalink raw reply
* [RFC PATCH 2/2] bpf: Initialise mod[] in bpf_trace_printk
From: James Hogan @ 2017-08-07 22:25 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: linux-kernel, James Hogan, Steven Rostedt, Ingo Molnar, netdev
In-Reply-To: <20170807222514.24292-1-james.hogan@imgtec.com>
In bpf_trace_printk(), the elements in mod[] are left uninitialised, but
they are then incremented to track the width of the formats. Zero
initialise the array just in case the memory contains non-zero values on
entry.
Fixes: 9c959c863f82 ("tracing: Allow BPF programs to call bpf_trace_printk()")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: netdev@vger.kernel.org
---
When I checked (on MIPS32), the elements tended to have the value zero
anyway (does BPF zero the stack or something clever?), so this is a
purely theoretical fix.
---
kernel/trace/bpf_trace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 32dcbe1b48f2..86a52857d941 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -129,7 +129,7 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
u64, arg2, u64, arg3)
{
bool str_seen = false;
- int mod[3] = {};
+ int mod[3] = { 0, 0, 0 };
int fmt_cnt = 0;
u64 unsafe_addr;
char buf[64];
--
2.13.2
^ permalink raw reply related
* [RFC PATCH 1/2] bpf: Fix bpf_trace_printk on 32-bit architectures
From: James Hogan @ 2017-08-07 22:25 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: linux-kernel, James Hogan, Steven Rostedt, Ingo Molnar, netdev
In-Reply-To: <20170807222514.24292-1-james.hogan@imgtec.com>
bpf_trace_printk() uses conditional operators to attempt to pass
different types to __trace_printk() depending on the format operators.
This doesn't work as intended on 32-bit architectures where u32 & long
are passed differently to u64, since the result of C conditional
operators follows the "usual arithmetic conversions" rules, such that
the values passed to __trace_printk() will always be u64.
For example the samples/bpf/tracex5 test printed lines like below on
MIPS, where the fd and buf have come from the u64 fd argument, and the
size from the buf argument:
dd-1176 [000] .... 1180.941542: 0x00000001: write(fd=1, buf= (null), size=6258688)
Instead of this:
dd-1217 [000] .... 1625.616026: 0x00000001: write(fd=1, buf=009e4000, size=512)
Work around this with an ugly hack which expands each combination of
argument types for the 3 arguments. On 64-bit kernels it is assumed that
u32, long & u64 are all passed the same way so no casting takes place
(it has apparently worked implicitly until now). On 32-bit kernels it is
assumed that long and u32 pass the same way so there are 8 combinations.
On 32-bit kernels bpf_trace_printk() increases in size but should now
work correctly. On 64-bit kernels it actually reduces in size slightly,
I presume due to removal of some of the casts (which as far as I can
tell are unnecessary for printk anyway due to the controlled nature of
the interpretation):
arch function old new delta
x86_64 bpf_trace_printk 532 412 -120
x86 bpf_trace_printk 676 1120 +444
MIPS64 bpf_trace_printk 760 612 -148
MIPS32 bpf_trace_printk 768 996 +228
Fixes: 9c959c863f82 ("tracing: Allow BPF programs to call bpf_trace_printk()")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: netdev@vger.kernel.org
---
I'm open to nicer ways of fixing this.
This is tested with samples/bpf/tracex5 on MIPS32 and MIPS64. Only build
tested on x86.
---
kernel/trace/bpf_trace.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 37385193a608..32dcbe1b48f2 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -204,10 +204,28 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
fmt_cnt++;
}
- return __trace_printk(1/* fake ip will not be printed */, fmt,
- mod[0] == 2 ? arg1 : mod[0] == 1 ? (long) arg1 : (u32) arg1,
- mod[1] == 2 ? arg2 : mod[1] == 1 ? (long) arg2 : (u32) arg2,
- mod[2] == 2 ? arg3 : mod[2] == 1 ? (long) arg3 : (u32) arg3);
+ /*
+ * This is a horribly ugly hack to allow different combinations of
+ * argument types to be used, particularly on 32-bit architectures where
+ * u32 & long pass the same as one another, but differently to u64.
+ *
+ * On 64-bit architectures it is assumed u32, long & u64 pass in the
+ * same way.
+ */
+
+#define __BPFTP_P(...) __trace_printk(1/* fake ip will not be printed */, \
+ fmt, ##__VA_ARGS__)
+#define __BPFTP_1(...) ((mod[0] == 2 || __BITS_PER_LONG == 64) \
+ ? __BPFTP_P(arg1, ##__VA_ARGS__) \
+ : __BPFTP_P((long)arg1, ##__VA_ARGS__))
+#define __BPFTP_2(...) ((mod[1] == 2 || __BITS_PER_LONG == 64) \
+ ? __BPFTP_1(arg2, ##__VA_ARGS__) \
+ : __BPFTP_1((long)arg2, ##__VA_ARGS__))
+#define __BPFTP_3(...) ((mod[2] == 2 || __BITS_PER_LONG == 64) \
+ ? __BPFTP_2(arg3, ##__VA_ARGS__) \
+ : __BPFTP_2((long)arg3, ##__VA_ARGS__))
+
+ return __BPFTP_3();
}
static const struct bpf_func_proto bpf_trace_printk_proto = {
--
2.13.2
^ permalink raw reply related
* [RFC PATCH 0/2] bpf_trace_printk() fixes
From: James Hogan @ 2017-08-07 22:25 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: linux-kernel, James Hogan, Steven Rostedt, Ingo Molnar, netdev
A couple of RFC fixes for bpf_trace_printk(). The first affects 32-bit
architectures in particular, the second is a theoretical uninitialised
variable fix.
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: netdev@vger.kernel.org
James Hogan (2):
bpf: Fix bpf_trace_printk on 32-bit architectures
bpf: Initialise mod[] in bpf_trace_printk
kernel/trace/bpf_trace.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
--
2.13.2
^ permalink raw reply
* [RFC PATCH] net: don't set __LINK_STATE_START until after dev->open() call
From: Jacob Keller @ 2017-08-07 22:24 UTC (permalink / raw)
To: netdev; +Cc: Jacob Keller
Fix an issue with relying on netif_running() which could be true during
when dev->open() handler is being called, even if it would exit with
a failure. This ensures the state does not get set and removed with
a narrow race for other callers to read it as open when infact it never
finished opening.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
I found this as a result of debugging a race condition in the i40evf
driver, in which we assumed that netif_running() would not be true until
after dev->open() had been called and succeeded. Unfortunately we can't
hold the rtnl_lock() while checking netif_running() because it would
cause a deadlock between our reset task and our ndo_open handler.
I am wondering whether the proposed change is acceptable here, or
whether some ndo_open handlers rely on __LINK_STATE_START being true
prior to their being called?
net/core/dev.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 1d75499add72..11953af90427 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1362,8 +1362,6 @@ static int __dev_open(struct net_device *dev)
if (ret)
return ret;
- set_bit(__LINK_STATE_START, &dev->state);
-
if (ops->ndo_validate_addr)
ret = ops->ndo_validate_addr(dev);
@@ -1372,9 +1370,8 @@ static int __dev_open(struct net_device *dev)
netpoll_poll_enable(dev);
- if (ret)
- clear_bit(__LINK_STATE_START, &dev->state);
- else {
+ if (!ret)
+ set_bit(__LINK_STATE_START, &dev->state);
dev->flags |= IFF_UP;
dev_set_rx_mode(dev);
dev_activate(dev);
--
2.14.0.rc1.251.g593d8d6362ce
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox