* Re: [PATCH v3 1/2] kretprobe: produce sane stack traces
From: Aleksa Sarai @ 2018-11-08 7:46 UTC (permalink / raw)
To: Steven Rostedt
Cc: Naveen N. Rao, Anil S Keshavamurthy, David S. Miller,
Masami Hiramatsu, Jonathan Corbet, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Shuah Khan, Alexei Starovoitov, Daniel Borkmann,
Brendan Gregg, Christian Brauner, Aleksa Sarai, netdev, linux-doc,
linux-kernel
In-Reply-To: <20181106171501.59ccabbc@gandalf.local.home>
[-- Attachment #1: Type: text/plain, Size: 1979 bytes --]
On 2018-11-06, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Sun, 4 Nov 2018 22:59:13 +1100
> Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> > The same issue is present in __save_stack_trace
> > (arch/x86/kernel/stacktrace.c). This is likely the only reason that --
> > as Steven said -- stacktraces wouldn't work with ftrace-graph (and thus
> > with the refactor both of you are discussing).
>
> By the way, I was playing with the the orc unwinder and stack traces
> from the function graph tracer return code, and got it working with the
> below patch. Caution, that patch also has a stack trace hardcoded in
> the return path of the function graph tracer, so you don't want to run
> function graph tracing without filtering.
Neat!
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 169b3c44ee97..aaeca73218cc 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -242,13 +242,16 @@ ftrace_pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret,
> trace->calltime = current->ret_stack[index].calltime;
> trace->overrun = atomic_read(¤t->trace_overrun);
> trace->depth = index;
> +
> + trace_dump_stack(0);
Right, this works because save_stack is not being passed a pt_regs. But if
you pass a pt_regs (as happens with bpf_getstackid -- which is what
spawned this discussion) then the top-most entry of the stack will still
be a trampoline because there is no ftrace_graph_ret_addr call.
(I'm struggling with how to fix this -- I can't figure out what retp
should be if you have a pt_regs. ->sp doesn't appear to work -- it's off
by a few bytes.)
I will attach what I have at the moment to hopefully explain what the
issue I've found is (re-using the kretprobe architecture but with the
shadow-stack idea).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH rdma] net/mlx5: Fix XRC SRQ umem valid bits
From: Jason Gunthorpe @ 2018-11-07 22:07 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Yishai Hadas, RDMA mailing list, Artemy Kovalyov,
Saeed Mahameed, linux-netdev
In-Reply-To: <20181107073434.GJ9230@mtr-leonro.mtl.com>
On Wed, Nov 07, 2018 at 09:34:34AM +0200, Leon Romanovsky wrote:
> On Tue, Nov 06, 2018 at 03:11:53PM -0700, Jason Gunthorpe wrote:
> > On Tue, Nov 06, 2018 at 05:10:53PM -0500, Doug Ledford wrote:
> > > On Tue, 2018-11-06 at 22:02 +0000, Jason Gunthorpe wrote:
> > > > On Tue, Nov 06, 2018 at 04:31:08PM -0500, Doug Ledford wrote:
> > > > > On Wed, 2018-10-31 at 12:20 +0200, Leon Romanovsky wrote:
> > > > > > From: Yishai Hadas <yishaih@mellanox.com>
> > > > > >
> > > > > > Adapt XRC SRQ to the latest HW specification with fixed definition
> > > > > > around umem valid bits. The previous definition relied on a bit which
> > > > > > was taken for other purposes in legacy FW.
> > > > > >
> > > > > > Fixes: bd37197554eb ("net/mlx5: Update mlx5_ifc with DEVX UID bits")
> > > > > > Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> > > > > > Reviewed-by: Artemy Kovalyov <artemyko@mellanox.com>
> > > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > > > > Hi Doug, Jason
> > > > > >
> > > > > > This commit fixes code sent in this merge window, so I'm not marking it
> > > > > > with any rdma-rc/rdma-next. It will be better to be sent during this merge
> > > > > > window if you have extra pull request to issue, or as a -rc material, if
> > > > > > not.
> > > > > >
> > > > > > BTW, we didn't combine reserved fields, because our convention is to align such
> > > > > > fields to 32 bits for better readability.
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > This looks fine. Let me know when it's in the mlx5-next tree to pull.
> > > >
> > > > It needs to go to -rc...
> > > >
> > > > This needs a mlx5-rc branch for this I guess?
> > >
> > > I don't think so. As long as it's the first commit in mlx5-next, and
> > > mlx5-next is 4.20-rc1 based, then pulling this commit into the -rc tree
> > > will only pull the single commit. Then when we pull into for-next for
> > > the first time, we will get this in for-next too. That seems best to
> > > me.
> >
> > That works too, if Leon is fast :)
>
> Thank you both for suggestion.
>
> I did it.
> 99b77fef3c6c net/mlx5: Fix XRC SRQ umem valid bits
>
> It is first commit and it is based on -rc1.
Okay, I put this branch in -rc
Jason
^ permalink raw reply
* [PATCH ipsec-next 11/11] xfrm: policy: add 2nd-level saddr trees for inexact policies
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
This adds the fourth and final search class, containing policies
where both saddr and daddr have prefix lengths (i.e., not wildcards).
Inexact policies now end up in one of the following four search classes:
1. "Any:Any" list, containing policies where both saddr and daddr are
wildcards or have very coarse prefixes, e.g. 10.0.0.0/8 and the like.
2. "saddr:any" list, containing policies with a fixed saddr/prefixlen,
but without destination restrictions.
These lists are stored in rbtree nodes; each node contains those
policies matching saddr/prefixlen.
3. "Any:daddr" list. Similar to 2), except for policies where only the
destinations are specified.
4. "saddr:daddr" lists, containing only those policies that
match the given source/destination network.
The root of the saddr/daddr nodes gets stored in the nodes of the
'daddr' tree.
This diagram illustrates the list classes, and their
placement in the lookup hierarchy:
xfrm_pol_inexact_bin = hash(dir,type,family,if_id);
|
+---- root_d: sorted by daddr:prefix
| |
| xfrm_pol_inexact_node
| |
| +- root: sorted by saddr/prefix
| | |
| | xfrm_pol_inexact_node
| | |
| | + root: unused
| | |
| | + hhead: saddr:daddr policies
| |
| +- coarse policies and all any:daddr policies
|
+---- root_s: sorted by saddr:prefix
| |
| xfrm_pol_inexact_node
| |
| + root: unused
| |
| + hhead: saddr:any policies
|
+---- coarse policies and all any:any policies
lookup for an inexact policy returns pointers to the four relevant list
classes, after which each of the lists needs to be searched for the policy
with the higher priority.
This will only speed up lookups in case we have many policies and a
sizeable portion of these have disjunct saddr/daddr addresses.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/xfrm/xfrm_policy.c | 118 +++++++++++++++++++++++++++++++++++++----
1 file changed, 108 insertions(+), 10 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 38e33326c856..bd80b8a4322f 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -58,6 +58,8 @@ struct xfrm_pol_inexact_node {
};
u8 prefixlen;
+ struct rb_root root;
+
/* the policies matching this node, can be empty list */
struct hlist_head hhead;
};
@@ -69,6 +71,14 @@ struct xfrm_pol_inexact_node {
* | |
* | xfrm_pol_inexact_node
* | |
+ * | +- root: sorted by saddr/prefix
+ * | | |
+ * | | xfrm_pol_inexact_node
+ * | | |
+ * | | + root: unused
+ * | | |
+ * | | + hhead: saddr:daddr policies
+ * | |
* | +- coarse policies and all any:daddr policies
* |
* +---- root_s: sorted by saddr:prefix
@@ -81,10 +91,11 @@ struct xfrm_pol_inexact_node {
* |
* +---- coarse policies and all any:any policies
*
- * Lookups return three candidate lists:
+ * Lookups return four candidate lists:
* 1. any:any list from top-level xfrm_pol_inexact_bin
* 2. any:daddr list from daddr tree
- * 2. saddr:any list from saddr tree
+ * 3. saddr:daddr list from 2nd level daddr tree
+ * 4. saddr:any list from saddr tree
*
* This result set then needs to be searched for the policy with
* the lowest priority. If two results have same prio, youngest one wins.
@@ -116,6 +127,7 @@ struct xfrm_pol_inexact_bin {
};
enum xfrm_pol_inexact_candidate_type {
+ XFRM_POL_CAND_BOTH,
XFRM_POL_CAND_SADDR,
XFRM_POL_CAND_DADDR,
XFRM_POL_CAND_ANY,
@@ -876,13 +888,82 @@ static void xfrm_policy_inexact_list_reinsert(struct net *net,
}
}
+static void xfrm_policy_inexact_node_reinsert(struct net *net,
+ struct xfrm_pol_inexact_node *n,
+ struct rb_root *new,
+ u16 family)
+{
+ struct rb_node **p, *parent = NULL;
+ struct xfrm_pol_inexact_node *node;
+
+ /* we should not have another subtree here */
+ WARN_ON_ONCE(!RB_EMPTY_ROOT(&n->root));
+
+ p = &new->rb_node;
+ while (*p) {
+ u8 prefixlen;
+ int delta;
+
+ parent = *p;
+ node = rb_entry(*p, struct xfrm_pol_inexact_node, node);
+
+ prefixlen = min(node->prefixlen, n->prefixlen);
+
+ delta = xfrm_policy_addr_delta(&n->addr, &node->addr,
+ prefixlen, family);
+ if (delta < 0) {
+ p = &parent->rb_left;
+ } else if (delta > 0) {
+ p = &parent->rb_right;
+ } else {
+ struct xfrm_policy *tmp;
+
+ hlist_for_each_entry(tmp, &node->hhead, bydst)
+ tmp->bydst_reinsert = true;
+ hlist_for_each_entry(tmp, &n->hhead, bydst)
+ tmp->bydst_reinsert = true;
+
+ INIT_HLIST_HEAD(&node->hhead);
+ xfrm_policy_inexact_list_reinsert(net, node, family);
+
+ if (node->prefixlen == n->prefixlen) {
+ kfree_rcu(n, rcu);
+ return;
+ }
+
+ rb_erase(*p, new);
+ kfree_rcu(n, rcu);
+ n = node;
+ n->prefixlen = prefixlen;
+ *p = new->rb_node;
+ parent = NULL;
+ }
+ }
+
+ rb_link_node_rcu(&n->node, parent, p);
+ rb_insert_color(&n->node, new);
+}
+
/* merge nodes v and n */
static void xfrm_policy_inexact_node_merge(struct net *net,
struct xfrm_pol_inexact_node *v,
struct xfrm_pol_inexact_node *n,
u16 family)
{
+ struct xfrm_pol_inexact_node *node;
struct xfrm_policy *tmp;
+ struct rb_node *rnode;
+
+ /* To-be-merged node v has a subtree.
+ *
+ * Dismantle it and insert its nodes to n->root.
+ */
+ while ((rnode = rb_first(&v->root)) != NULL) {
+ node = rb_entry(rnode, struct xfrm_pol_inexact_node, node);
+ rb_erase(&node->node, &v->root);
+ xfrm_policy_inexact_node_reinsert(net, node, &n->root,
+ family);
+ }
hlist_for_each_entry(tmp, &v->hhead, bydst)
tmp->bydst_reinsert = true;
@@ -978,9 +1059,10 @@ static void xfrm_policy_inexact_gc_tree(struct rb_root *r, bool rm)
while (rn) {
node = rb_entry(rn, struct xfrm_pol_inexact_node, node);
+ xfrm_policy_inexact_gc_tree(&node->root, rm);
rn = rb_next(rn);
- if (!hlist_empty(&node->hhead)) {
+ if (!hlist_empty(&node->hhead) || !RB_EMPTY_ROOT(&node->root)) {
WARN_ON_ONCE(rm);
continue;
}
@@ -1042,12 +1124,6 @@ xfrm_policy_inexact_alloc_chain(struct xfrm_pol_inexact_bin *bin,
if (xfrm_policy_inexact_insert_use_any_list(policy))
return &bin->hhead;
- /* saddr is wildcard */
- if (xfrm_pol_inexact_addr_use_any_list(&policy->selector.saddr,
- policy->family,
- policy->selector.prefixlen_s))
- return &bin->hhead;
-
if (xfrm_pol_inexact_addr_use_any_list(&policy->selector.daddr,
policy->family,
policy->selector.prefixlen_d)) {
@@ -1075,6 +1151,23 @@ xfrm_policy_inexact_alloc_chain(struct xfrm_pol_inexact_bin *bin,
write_seqcount_end(&bin->count);
if (!n)
return NULL;
+
+ /* saddr is wildcard */
+ if (xfrm_pol_inexact_addr_use_any_list(&policy->selector.saddr,
+ policy->family,
+ policy->selector.prefixlen_s))
+ return &n->hhead;
+
+ write_seqcount_begin(&bin->count);
+ n = xfrm_policy_inexact_insert_node(net,
+ &n->root,
+ &policy->selector.saddr,
+ policy->family,
+ policy->selector.prefixlen_s, dir);
+ write_seqcount_end(&bin->count);
+ if (!n)
+ return NULL;
+
return &n->hhead;
}
@@ -1856,8 +1949,13 @@ xfrm_policy_find_inexact_candidates(struct xfrm_pol_inexact_candidates *cand,
n = xfrm_policy_lookup_inexact_addr(&b->root_d, &b->count, daddr,
family);
- if (n)
+ if (n) {
cand->res[XFRM_POL_CAND_DADDR] = &n->hhead;
+ n = xfrm_policy_lookup_inexact_addr(&n->root, &b->count, saddr,
+ family);
+ if (n)
+ cand->res[XFRM_POL_CAND_BOTH] = &n->hhead;
+ }
n = xfrm_policy_lookup_inexact_addr(&b->root_s, &b->count, saddr,
family);
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 10/11] xfrm: policy: store inexact policies in a tree ordered by source address
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
This adds the 'saddr:any' search class. It contains all policies that have
a fixed saddr/prefixlen, but 'any' destination.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/xfrm/xfrm_policy.c | 46 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 57e28dcd7c53..38e33326c856 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -71,11 +71,20 @@ struct xfrm_pol_inexact_node {
* | |
* | +- coarse policies and all any:daddr policies
* |
+ * +---- root_s: sorted by saddr:prefix
+ * | |
+ * | xfrm_pol_inexact_node
+ * | |
+ * | + root: unused
+ * | |
+ * | + hhead: saddr:any policies
+ * |
* +---- coarse policies and all any:any policies
*
- * Lookups return two candidate lists:
+ * Lookups return three candidate lists:
* 1. any:any list from top-level xfrm_pol_inexact_bin
* 2. any:daddr list from daddr tree
+ * 2. saddr:any list from saddr tree
*
* This result set then needs to be searched for the policy with
* the lowest priority. If two results have same prio, youngest one wins.
@@ -98,12 +107,16 @@ struct xfrm_pol_inexact_bin {
/* tree sorted by daddr/prefix */
struct rb_root root_d;
+ /* tree sorted by saddr/prefix */
+ struct rb_root root_s;
+
/* slow path below */
struct list_head inexact_bins;
struct rcu_head rcu;
};
enum xfrm_pol_inexact_candidate_type {
+ XFRM_POL_CAND_SADDR,
XFRM_POL_CAND_DADDR,
XFRM_POL_CAND_ANY,
@@ -696,6 +709,7 @@ xfrm_policy_inexact_alloc_bin(const struct xfrm_policy *pol, u8 dir)
bin->k = k;
INIT_HLIST_HEAD(&bin->hhead);
bin->root_d = RB_ROOT;
+ bin->root_s = RB_ROOT;
seqcount_init(&bin->count);
prev = rhashtable_lookup_get_insert_key(&xfrm_policy_inexact_table,
@@ -980,9 +994,10 @@ static void __xfrm_policy_inexact_prune_bin(struct xfrm_pol_inexact_bin *b, bool
{
write_seqcount_begin(&b->count);
xfrm_policy_inexact_gc_tree(&b->root_d, net_exit);
+ xfrm_policy_inexact_gc_tree(&b->root_s, net_exit);
write_seqcount_end(&b->count);
- if (!RB_EMPTY_ROOT(&b->root_d) ||
+ if (!RB_EMPTY_ROOT(&b->root_d) || !RB_EMPTY_ROOT(&b->root_s) ||
!hlist_empty(&b->hhead)) {
WARN_ON_ONCE(net_exit);
return;
@@ -1027,11 +1042,29 @@ xfrm_policy_inexact_alloc_chain(struct xfrm_pol_inexact_bin *bin,
if (xfrm_policy_inexact_insert_use_any_list(policy))
return &bin->hhead;
- if (xfrm_pol_inexact_addr_use_any_list(&policy->selector.daddr,
+ /* saddr is wildcard */
+ if (xfrm_pol_inexact_addr_use_any_list(&policy->selector.saddr,
policy->family,
- policy->selector.prefixlen_d))
+ policy->selector.prefixlen_s))
return &bin->hhead;
+ if (xfrm_pol_inexact_addr_use_any_list(&policy->selector.daddr,
+ policy->family,
+ policy->selector.prefixlen_d)) {
+ write_seqcount_begin(&bin->count);
+ n = xfrm_policy_inexact_insert_node(net,
+ &bin->root_s,
+ &policy->selector.saddr,
+ policy->family,
+ policy->selector.prefixlen_s,
+ dir);
+ write_seqcount_end(&bin->count);
+ if (!n)
+ return NULL;
+
+ return &n->hhead;
+ }
+
/* daddr is fixed */
write_seqcount_begin(&bin->count);
n = xfrm_policy_inexact_insert_node(net,
@@ -1826,6 +1859,11 @@ xfrm_policy_find_inexact_candidates(struct xfrm_pol_inexact_candidates *cand,
if (n)
cand->res[XFRM_POL_CAND_DADDR] = &n->hhead;
+ n = xfrm_policy_lookup_inexact_addr(&b->root_s, &b->count, saddr,
+ family);
+ if (n)
+ cand->res[XFRM_POL_CAND_SADDR] = &n->hhead;
+
return true;
}
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 09/11] xfrm: policy: check reinserted policies match their node
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
validate the re-inserted policies match the lookup node.
Policies that fail this test won't be returned in the candidate set.
This is enabled by default for now, it should not cause noticeable
reinsert slow down.
Such reinserts are needed when we have to merge an existing node
(e.g. for 10.0.0.0/28 because a overlapping subnet was added (e.g.
10.0.0.0/24), so whenever this happens existing policies have to
be placed on the list of the new node.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/xfrm/xfrm_policy.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 81447d5d02e6..57e28dcd7c53 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -806,10 +806,16 @@ static void xfrm_policy_inexact_list_reinsert(struct net *net,
struct xfrm_pol_inexact_node *n,
u16 family)
{
+ unsigned int matched_s, matched_d;
struct hlist_node *newpos = NULL;
struct xfrm_policy *policy, *p;
+ matched_s = 0;
+ matched_d = 0;
+
list_for_each_entry_reverse(policy, &net->xfrm.policy_all, walk.all) {
+ bool matches_s, matches_d;
+
if (!policy->bydst_reinsert)
continue;
@@ -827,6 +833,32 @@ static void xfrm_policy_inexact_list_reinsert(struct net *net,
hlist_add_behind(&policy->bydst, newpos);
else
hlist_add_head(&policy->bydst, &n->hhead);
+
+ /* paranoia checks follow.
+ * Check that the reinserted policy matches at least
+ * saddr or daddr for current node prefix.
+ *
+ * Matching both is fine, matching saddr in one policy
+ * (but not daddr) and then matching only daddr in another
+ * is a bug.
+ */
+ matches_s = xfrm_policy_addr_delta(&policy->selector.saddr,
+ &n->addr,
+ n->prefixlen,
+ family) == 0;
+ matches_d = xfrm_policy_addr_delta(&policy->selector.daddr,
+ &n->addr,
+ n->prefixlen,
+ family) == 0;
+ if (matches_s && matches_d)
+ continue;
+
+ WARN_ON_ONCE(!matches_s && !matches_d);
+ if (matches_s)
+ matched_s++;
+ if (matches_d)
+ matched_d++;
+ WARN_ON_ONCE(matched_s && matched_d);
}
}
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 08/11] xfrm: policy: store inexact policies in a tree ordered by destination address
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
This adds inexact lists per destination network, stored in a search tree.
Inexact lookups now return two 'candidate lists', the 'any' policies
('any' destionations), and a list of policies that share same
daddr/prefix.
Next patch will add a second search tree for 'saddr:any' policies
so we can avoid placing those on the 'any:any' list too.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
include/net/xfrm.h | 1 +
net/xfrm/xfrm_policy.c | 333 ++++++++++++++++++++++++++++++++++++++++-
2 files changed, 328 insertions(+), 6 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 9df6dca17155..fa4b3c877fcf 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -590,6 +590,7 @@ struct xfrm_policy {
struct xfrm_lifetime_cur curlft;
struct xfrm_policy_walk_entry walk;
struct xfrm_policy_queue polq;
+ bool bydst_reinsert;
u8 type;
u8 action;
u8 flags;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 4eb12e9b40c2..81447d5d02e6 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -49,6 +49,38 @@ struct xfrm_flo {
/* prefixes smaller than this are stored in lists, not trees. */
#define INEXACT_PREFIXLEN_IPV4 16
#define INEXACT_PREFIXLEN_IPV6 48
+
+struct xfrm_pol_inexact_node {
+ struct rb_node node;
+ union {
+ xfrm_address_t addr;
+ struct rcu_head rcu;
+ };
+ u8 prefixlen;
+
+ /* the policies matching this node, can be empty list */
+ struct hlist_head hhead;
+};
+
+/* xfrm inexact policy search tree:
+ * xfrm_pol_inexact_bin = hash(dir,type,family,if_id);
+ * |
+ * +---- root_d: sorted by daddr:prefix
+ * | |
+ * | xfrm_pol_inexact_node
+ * | |
+ * | +- coarse policies and all any:daddr policies
+ * |
+ * +---- coarse policies and all any:any policies
+ *
+ * Lookups return two candidate lists:
+ * 1. any:any list from top-level xfrm_pol_inexact_bin
+ * 2. any:daddr list from daddr tree
+ *
+ * This result set then needs to be searched for the policy with
+ * the lowest priority. If two results have same prio, youngest one wins.
+ */
+
struct xfrm_pol_inexact_key {
possible_net_t net;
u32 if_id;
@@ -62,12 +94,17 @@ struct xfrm_pol_inexact_bin {
/* list containing '*:*' policies */
struct hlist_head hhead;
+ seqcount_t count;
+ /* tree sorted by daddr/prefix */
+ struct rb_root root_d;
+
/* slow path below */
struct list_head inexact_bins;
struct rcu_head rcu;
};
enum xfrm_pol_inexact_candidate_type {
+ XFRM_POL_CAND_DADDR,
XFRM_POL_CAND_ANY,
XFRM_POL_CAND_MAX,
@@ -658,6 +695,8 @@ xfrm_policy_inexact_alloc_bin(const struct xfrm_policy *pol, u8 dir)
bin->k = k;
INIT_HLIST_HEAD(&bin->hhead);
+ bin->root_d = RB_ROOT;
+ seqcount_init(&bin->count);
prev = rhashtable_lookup_get_insert_key(&xfrm_policy_inexact_table,
&bin->k, &bin->head,
@@ -708,9 +747,211 @@ xfrm_policy_inexact_insert_use_any_list(const struct xfrm_policy *policy)
return saddr_any && daddr_any;
}
+static void xfrm_pol_inexact_node_init(struct xfrm_pol_inexact_node *node,
+ const xfrm_address_t *addr, u8 prefixlen)
+{
+ node->addr = *addr;
+ node->prefixlen = prefixlen;
+}
+
+static struct xfrm_pol_inexact_node *
+xfrm_pol_inexact_node_alloc(const xfrm_address_t *addr, u8 prefixlen)
+{
+ struct xfrm_pol_inexact_node *node;
+
+ node = kzalloc(sizeof(*node), GFP_ATOMIC);
+ if (node)
+ xfrm_pol_inexact_node_init(node, addr, prefixlen);
+
+ return node;
+}
+
+static int xfrm_policy_addr_delta(const xfrm_address_t *a,
+ const xfrm_address_t *b,
+ u8 prefixlen, u16 family)
+{
+ unsigned int pdw, pbi;
+ int delta = 0;
+
+ switch (family) {
+ case AF_INET:
+ if (sizeof(long) == 4 && prefixlen == 0)
+ return ntohl(a->a4) - ntohl(b->a4);
+ return (ntohl(a->a4) & ((~0UL << (32 - prefixlen)))) -
+ (ntohl(b->a4) & ((~0UL << (32 - prefixlen))));
+ case AF_INET6:
+ pdw = prefixlen >> 5;
+ pbi = prefixlen & 0x1f;
+
+ if (pdw) {
+ delta = memcmp(a->a6, b->a6, pdw << 2);
+ if (delta)
+ return delta;
+ }
+ if (pbi) {
+ u32 mask = ~0u << (32 - pbi);
+
+ delta = (ntohl(a->a6[pdw]) & mask) -
+ (ntohl(b->a6[pdw]) & mask);
+ }
+ break;
+ default:
+ break;
+ }
+
+ return delta;
+}
+
+static void xfrm_policy_inexact_list_reinsert(struct net *net,
+ struct xfrm_pol_inexact_node *n,
+ u16 family)
+{
+ struct hlist_node *newpos = NULL;
+ struct xfrm_policy *policy, *p;
+
+ list_for_each_entry_reverse(policy, &net->xfrm.policy_all, walk.all) {
+ if (!policy->bydst_reinsert)
+ continue;
+
+ WARN_ON_ONCE(policy->family != family);
+
+ policy->bydst_reinsert = false;
+ hlist_for_each_entry(p, &n->hhead, bydst) {
+ if (policy->priority >= p->priority)
+ newpos = &p->bydst;
+ else
+ break;
+ }
+
+ if (newpos)
+ hlist_add_behind(&policy->bydst, newpos);
+ else
+ hlist_add_head(&policy->bydst, &n->hhead);
+ }
+}
+
+/* merge nodes v and n */
+static void xfrm_policy_inexact_node_merge(struct net *net,
+ struct xfrm_pol_inexact_node *v,
+ struct xfrm_pol_inexact_node *n,
+ u16 family)
+{
+ struct xfrm_policy *tmp;
+
+ hlist_for_each_entry(tmp, &v->hhead, bydst)
+ tmp->bydst_reinsert = true;
+ hlist_for_each_entry(tmp, &n->hhead, bydst)
+ tmp->bydst_reinsert = true;
+
+ INIT_HLIST_HEAD(&n->hhead);
+ xfrm_policy_inexact_list_reinsert(net, n, family);
+}
+
+static struct xfrm_pol_inexact_node *
+xfrm_policy_inexact_insert_node(struct net *net,
+ struct rb_root *root,
+ xfrm_address_t *addr,
+ u16 family, u8 prefixlen, u8 dir)
+{
+ struct xfrm_pol_inexact_node *cached = NULL;
+ struct rb_node **p, *parent = NULL;
+ struct xfrm_pol_inexact_node *node;
+
+ p = &root->rb_node;
+ while (*p) {
+ int delta;
+
+ parent = *p;
+ node = rb_entry(*p, struct xfrm_pol_inexact_node, node);
+
+ delta = xfrm_policy_addr_delta(addr, &node->addr,
+ node->prefixlen,
+ family);
+ if (delta == 0 && prefixlen >= node->prefixlen) {
+ WARN_ON_ONCE(cached); /* ipsec policies got lost */
+ return node;
+ }
+
+ if (delta < 0)
+ p = &parent->rb_left;
+ else
+ p = &parent->rb_right;
+
+ if (prefixlen < node->prefixlen) {
+ delta = xfrm_policy_addr_delta(addr, &node->addr,
+ prefixlen,
+ family);
+ if (delta)
+ continue;
+
+ /* This node is a subnet of the new prefix. It needs
+ * to be removed and re-inserted with the smaller
+ * prefix and all nodes that are now also covered
+ * by the reduced prefixlen.
+ */
+ rb_erase(&node->node, root);
+
+ if (!cached) {
+ xfrm_pol_inexact_node_init(node, addr,
+ prefixlen);
+ cached = node;
+ } else {
+ /* This node also falls within the new
+ * prefixlen. Merge the to-be-reinserted
+ * node and this one.
+ */
+ xfrm_policy_inexact_node_merge(net, node,
+ cached, family);
+ kfree_rcu(node, rcu);
+ }
+
+ /* restart */
+ p = &root->rb_node;
+ parent = NULL;
+ }
+ }
+
+ node = cached;
+ if (!node) {
+ node = xfrm_pol_inexact_node_alloc(addr, prefixlen);
+ if (!node)
+ return NULL;
+ }
+
+ rb_link_node_rcu(&node->node, parent, p);
+ rb_insert_color(&node->node, root);
+
+ return node;
+}
+
+static void xfrm_policy_inexact_gc_tree(struct rb_root *r, bool rm)
+{
+ struct xfrm_pol_inexact_node *node;
+ struct rb_node *rn = rb_first(r);
+
+ while (rn) {
+ node = rb_entry(rn, struct xfrm_pol_inexact_node, node);
+
+ rn = rb_next(rn);
+
+ if (!hlist_empty(&node->hhead)) {
+ WARN_ON_ONCE(rm);
+ continue;
+ }
+
+ rb_erase(&node->node, r);
+ kfree_rcu(node, rcu);
+ }
+}
+
static void __xfrm_policy_inexact_prune_bin(struct xfrm_pol_inexact_bin *b, bool net_exit)
{
- if (!hlist_empty(&b->hhead)) {
+ write_seqcount_begin(&b->count);
+ xfrm_policy_inexact_gc_tree(&b->root_d, net_exit);
+ write_seqcount_end(&b->count);
+
+ if (!RB_EMPTY_ROOT(&b->root_d) ||
+ !hlist_empty(&b->hhead)) {
WARN_ON_ONCE(net_exit);
return;
}
@@ -741,6 +982,37 @@ static void __xfrm_policy_inexact_flush(struct net *net)
__xfrm_policy_inexact_prune_bin(bin, false);
}
+static struct hlist_head *
+xfrm_policy_inexact_alloc_chain(struct xfrm_pol_inexact_bin *bin,
+ struct xfrm_policy *policy, u8 dir)
+{
+ struct xfrm_pol_inexact_node *n;
+ struct net *net;
+
+ net = xp_net(policy);
+ lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+
+ if (xfrm_policy_inexact_insert_use_any_list(policy))
+ return &bin->hhead;
+
+ if (xfrm_pol_inexact_addr_use_any_list(&policy->selector.daddr,
+ policy->family,
+ policy->selector.prefixlen_d))
+ return &bin->hhead;
+
+ /* daddr is fixed */
+ write_seqcount_begin(&bin->count);
+ n = xfrm_policy_inexact_insert_node(net,
+ &bin->root_d,
+ &policy->selector.daddr,
+ policy->family,
+ policy->selector.prefixlen_d, dir);
+ write_seqcount_end(&bin->count);
+ if (!n)
+ return NULL;
+ return &n->hhead;
+}
+
static struct xfrm_policy *
xfrm_policy_inexact_insert(struct xfrm_policy *policy, u8 dir, int excl)
{
@@ -756,13 +1028,12 @@ xfrm_policy_inexact_insert(struct xfrm_policy *policy, u8 dir, int excl)
net = xp_net(policy);
lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
- if (xfrm_policy_inexact_insert_use_any_list(policy)) {
- chain = &bin->hhead;
- goto insert_to_list;
+ chain = xfrm_policy_inexact_alloc_chain(bin, policy, dir);
+ if (!chain) {
+ __xfrm_policy_inexact_prune_bin(bin, false);
+ return ERR_PTR(-ENOMEM);
}
- chain = &bin->hhead;
-insert_to_list:
delpol = xfrm_policy_insert_list(chain, policy, excl);
if (delpol && excl) {
__xfrm_policy_inexact_prune_bin(bin, false);
@@ -843,6 +1114,9 @@ static void xfrm_hash_rebuild(struct work_struct *work)
bin = xfrm_policy_inexact_alloc_bin(policy, dir);
if (!bin)
goto out_unlock;
+
+ if (!xfrm_policy_inexact_alloc_chain(bin, policy, dir))
+ goto out_unlock;
}
/* reset the bydst and inexact table in all directions */
@@ -1462,17 +1736,64 @@ static int xfrm_policy_match(const struct xfrm_policy *pol,
return ret;
}
+static struct xfrm_pol_inexact_node *
+xfrm_policy_lookup_inexact_addr(const struct rb_root *r,
+ seqcount_t *count,
+ const xfrm_address_t *addr, u16 family)
+{
+ const struct rb_node *parent;
+ int seq;
+
+again:
+ seq = read_seqcount_begin(count);
+
+ parent = rcu_dereference_raw(r->rb_node);
+ while (parent) {
+ struct xfrm_pol_inexact_node *node;
+ int delta;
+
+ node = rb_entry(parent, struct xfrm_pol_inexact_node, node);
+
+ delta = xfrm_policy_addr_delta(addr, &node->addr,
+ node->prefixlen, family);
+ if (delta < 0) {
+ parent = rcu_dereference_raw(parent->rb_left);
+ continue;
+ } else if (delta > 0) {
+ parent = rcu_dereference_raw(parent->rb_right);
+ continue;
+ }
+
+ return node;
+ }
+
+ if (read_seqcount_retry(count, seq))
+ goto again;
+
+ return NULL;
+}
+
static bool
xfrm_policy_find_inexact_candidates(struct xfrm_pol_inexact_candidates *cand,
struct xfrm_pol_inexact_bin *b,
const xfrm_address_t *saddr,
const xfrm_address_t *daddr)
{
+ struct xfrm_pol_inexact_node *n;
+ u16 family;
+
if (!b)
return false;
+ family = b->k.family;
memset(cand, 0, sizeof(*cand));
cand->res[XFRM_POL_CAND_ANY] = &b->hhead;
+
+ n = xfrm_policy_lookup_inexact_addr(&b->root_d, &b->count, daddr,
+ family);
+ if (n)
+ cand->res[XFRM_POL_CAND_DADDR] = &n->hhead;
+
return true;
}
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 07/11] xfrm: policy: add inexact policy search tree infrastructure
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
At this time inexact policies are all searched in-order until the first
match is found. After removal of the flow cache, this resolution has
to be performed for every packetm resulting in major slowdown when
number of inexact policies is high.
This adds infrastructure to later sort inexact policies into a tree.
This only introduces a single class: any:any.
Next patch will add a search tree to pre-sort policies that
have a fixed daddr/prefixlen, so in this patch the any:any class
will still be used for all policies.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
include/net/xfrm.h | 1 +
net/xfrm/xfrm_policy.c | 301 +++++++++++++++++++++++++++++++++--------
2 files changed, 248 insertions(+), 54 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 870fa9b27f7e..9df6dca17155 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -577,6 +577,7 @@ struct xfrm_policy {
/* This lock only affects elements except for entry. */
rwlock_t lock;
refcount_t refcnt;
+ u32 pos;
struct timer_list timer;
atomic_t genid;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index dda27fd7b8a4..4eb12e9b40c2 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -46,6 +46,9 @@ struct xfrm_flo {
u8 flags;
};
+/* prefixes smaller than this are stored in lists, not trees. */
+#define INEXACT_PREFIXLEN_IPV4 16
+#define INEXACT_PREFIXLEN_IPV6 48
struct xfrm_pol_inexact_key {
possible_net_t net;
u32 if_id;
@@ -56,6 +59,7 @@ struct xfrm_pol_inexact_key {
struct xfrm_pol_inexact_bin {
struct xfrm_pol_inexact_key k;
struct rhash_head head;
+ /* list containing '*:*' policies */
struct hlist_head hhead;
/* slow path below */
@@ -63,6 +67,16 @@ struct xfrm_pol_inexact_bin {
struct rcu_head rcu;
};
+enum xfrm_pol_inexact_candidate_type {
+ XFRM_POL_CAND_ANY,
+
+ XFRM_POL_CAND_MAX,
+};
+
+struct xfrm_pol_inexact_candidates {
+ struct hlist_head *res[XFRM_POL_CAND_MAX];
+};
+
static DEFINE_SPINLOCK(xfrm_if_cb_lock);
static struct xfrm_if_cb const __rcu *xfrm_if_cb __read_mostly;
@@ -98,6 +112,12 @@ xfrm_policy_insert_list(struct hlist_head *chain, struct xfrm_policy *policy,
static void xfrm_policy_insert_inexact_list(struct hlist_head *chain,
struct xfrm_policy *policy);
+static bool
+xfrm_policy_find_inexact_candidates(struct xfrm_pol_inexact_candidates *cand,
+ struct xfrm_pol_inexact_bin *b,
+ const xfrm_address_t *saddr,
+ const xfrm_address_t *daddr);
+
static inline bool xfrm_pol_hold_rcu(struct xfrm_policy *policy)
{
return refcount_inc_not_zero(&policy->refcnt);
@@ -652,13 +672,48 @@ xfrm_policy_inexact_alloc_bin(const struct xfrm_policy *pol, u8 dir)
return IS_ERR(prev) ? NULL : prev;
}
-static void xfrm_policy_inexact_delete_bin(struct net *net,
- struct xfrm_pol_inexact_bin *b)
+static bool xfrm_pol_inexact_addr_use_any_list(const xfrm_address_t *addr,
+ int family, u8 prefixlen)
{
- lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+ if (xfrm_addr_any(addr, family))
+ return true;
+
+ if (family == AF_INET6 && prefixlen < INEXACT_PREFIXLEN_IPV6)
+ return true;
+
+ if (family == AF_INET && prefixlen < INEXACT_PREFIXLEN_IPV4)
+ return true;
+
+ return false;
+}
+
+static bool
+xfrm_policy_inexact_insert_use_any_list(const struct xfrm_policy *policy)
+{
+ const xfrm_address_t *addr;
+ bool saddr_any, daddr_any;
+ u8 prefixlen;
+
+ addr = &policy->selector.saddr;
+ prefixlen = policy->selector.prefixlen_s;
- if (!hlist_empty(&b->hhead))
+ saddr_any = xfrm_pol_inexact_addr_use_any_list(addr,
+ policy->family,
+ prefixlen);
+ addr = &policy->selector.daddr;
+ prefixlen = policy->selector.prefixlen_d;
+ daddr_any = xfrm_pol_inexact_addr_use_any_list(addr,
+ policy->family,
+ prefixlen);
+ return saddr_any && daddr_any;
+}
+
+static void __xfrm_policy_inexact_prune_bin(struct xfrm_pol_inexact_bin *b, bool net_exit)
+{
+ if (!hlist_empty(&b->hhead)) {
+ WARN_ON_ONCE(net_exit);
return;
+ }
if (rhashtable_remove_fast(&xfrm_policy_inexact_table, &b->head,
xfrm_pol_inexact_params) == 0) {
@@ -667,14 +722,23 @@ static void xfrm_policy_inexact_delete_bin(struct net *net,
}
}
+static void xfrm_policy_inexact_prune_bin(struct xfrm_pol_inexact_bin *b)
+{
+ struct net *net = read_pnet(&b->k.net);
+
+ spin_lock_bh(&net->xfrm.xfrm_policy_lock);
+ __xfrm_policy_inexact_prune_bin(b, false);
+ spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
+}
+
static void __xfrm_policy_inexact_flush(struct net *net)
{
- struct xfrm_pol_inexact_bin *bin;
+ struct xfrm_pol_inexact_bin *bin, *t;
lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
- list_for_each_entry(bin, &net->xfrm.inexact_bins, inexact_bins)
- xfrm_policy_inexact_delete_bin(net, bin);
+ list_for_each_entry_safe(bin, t, &net->xfrm.inexact_bins, inexact_bins)
+ __xfrm_policy_inexact_prune_bin(bin, false);
}
static struct xfrm_policy *
@@ -689,14 +753,28 @@ xfrm_policy_inexact_insert(struct xfrm_policy *policy, u8 dir, int excl)
if (!bin)
return ERR_PTR(-ENOMEM);
- delpol = xfrm_policy_insert_list(&bin->hhead, policy, excl);
- if (delpol && excl)
+ net = xp_net(policy);
+ lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+
+ if (xfrm_policy_inexact_insert_use_any_list(policy)) {
+ chain = &bin->hhead;
+ goto insert_to_list;
+ }
+
+ chain = &bin->hhead;
+insert_to_list:
+ delpol = xfrm_policy_insert_list(chain, policy, excl);
+ if (delpol && excl) {
+ __xfrm_policy_inexact_prune_bin(bin, false);
return ERR_PTR(-EEXIST);
+ }
- net = xp_net(policy);
chain = &net->xfrm.policy_inexact[dir];
xfrm_policy_insert_inexact_list(chain, policy);
+ if (delpol)
+ __xfrm_policy_inexact_prune_bin(bin, false);
+
return delpol;
}
@@ -733,6 +811,7 @@ static void xfrm_hash_rebuild(struct work_struct *work)
* we start with destructive action.
*/
list_for_each_entry(policy, &net->xfrm.policy_all, walk.all) {
+ struct xfrm_pol_inexact_bin *bin;
u8 dbits, sbits;
dir = xfrm_policy_id2dir(policy->index);
@@ -761,7 +840,8 @@ static void xfrm_hash_rebuild(struct work_struct *work)
policy->selector.prefixlen_s < sbits)
continue;
- if (!xfrm_policy_inexact_alloc_bin(policy, dir))
+ bin = xfrm_policy_inexact_alloc_bin(policy, dir);
+ if (!bin)
goto out_unlock;
}
@@ -820,6 +900,7 @@ static void xfrm_hash_rebuild(struct work_struct *work)
}
out_unlock:
+ __xfrm_policy_inexact_flush(net);
spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
mutex_unlock(&hash_resize_mutex);
@@ -977,6 +1058,7 @@ static void xfrm_policy_insert_inexact_list(struct hlist_head *chain,
{
struct xfrm_policy *pol, *delpol = NULL;
struct hlist_node *newpos = NULL;
+ int i = 0;
hlist_for_each_entry(pol, chain, bydst_inexact_list) {
if (pol->type == policy->type &&
@@ -1000,6 +1082,11 @@ static void xfrm_policy_insert_inexact_list(struct hlist_head *chain,
hlist_add_behind_rcu(&policy->bydst_inexact_list, newpos);
else
hlist_add_head_rcu(&policy->bydst_inexact_list, chain);
+
+ hlist_for_each_entry(pol, chain, bydst_inexact_list) {
+ pol->pos = i;
+ i++;
+ }
}
static struct xfrm_policy *xfrm_policy_insert_list(struct hlist_head *chain,
@@ -1083,6 +1170,29 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
}
EXPORT_SYMBOL(xfrm_policy_insert);
+static struct xfrm_policy *
+__xfrm_policy_bysel_ctx(struct hlist_head *chain, u32 mark, u32 if_id,
+ u8 type, int dir,
+ struct xfrm_selector *sel,
+ struct xfrm_sec_ctx *ctx)
+{
+ struct xfrm_policy *pol;
+
+ if (!chain)
+ return NULL;
+
+ hlist_for_each_entry(pol, chain, bydst) {
+ if (pol->type == type &&
+ pol->if_id == if_id &&
+ (mark & pol->mark.m) == pol->mark.v &&
+ !selector_cmp(sel, &pol->selector) &&
+ xfrm_sec_ctx_match(ctx, pol->security))
+ return pol;
+ }
+
+ return NULL;
+}
+
struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u32 if_id,
u8 type, int dir,
struct xfrm_selector *sel,
@@ -1097,6 +1207,9 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u32 if_id,
spin_lock_bh(&net->xfrm.xfrm_policy_lock);
chain = policy_hash_bysel(net, sel, sel->family, dir);
if (!chain) {
+ struct xfrm_pol_inexact_candidates cand;
+ int i;
+
bin = xfrm_policy_inexact_lookup(net, type,
sel->family, dir, if_id);
if (!bin) {
@@ -1104,35 +1217,46 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u32 if_id,
return NULL;
}
- chain = &bin->hhead;
+ if (!xfrm_policy_find_inexact_candidates(&cand, bin,
+ &sel->saddr,
+ &sel->daddr)) {
+ spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
+ return NULL;
+ }
+
+ pol = NULL;
+ for (i = 0; i < ARRAY_SIZE(cand.res); i++) {
+ struct xfrm_policy *tmp;
+
+ tmp = __xfrm_policy_bysel_ctx(cand.res[i], mark,
+ if_id, type, dir,
+ sel, ctx);
+ if (tmp && pol && tmp->pos < pol->pos)
+ pol = tmp;
+ }
+ } else {
+ pol = __xfrm_policy_bysel_ctx(chain, mark, if_id, type, dir,
+ sel, ctx);
}
- ret = NULL;
- hlist_for_each_entry(pol, chain, bydst) {
- if (pol->type == type &&
- pol->if_id == if_id &&
- (mark & pol->mark.m) == pol->mark.v &&
- !selector_cmp(sel, &pol->selector) &&
- xfrm_sec_ctx_match(ctx, pol->security)) {
- xfrm_pol_hold(pol);
- if (delete) {
- *err = security_xfrm_policy_delete(
- pol->security);
- if (*err) {
- spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
- return pol;
- }
- __xfrm_policy_unlink(pol, dir);
- xfrm_policy_inexact_delete_bin(net, bin);
+ if (pol) {
+ xfrm_pol_hold(pol);
+ if (delete) {
+ *err = security_xfrm_policy_delete(pol->security);
+ if (*err) {
+ spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
+ return pol;
}
- ret = pol;
- break;
+ __xfrm_policy_unlink(pol, dir);
}
+ ret = pol;
}
spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
if (ret && delete)
xfrm_policy_kill(ret);
+ if (bin && delete)
+ xfrm_policy_inexact_prune_bin(bin);
return ret;
}
EXPORT_SYMBOL(xfrm_policy_bysel_ctx);
@@ -1338,6 +1462,20 @@ static int xfrm_policy_match(const struct xfrm_policy *pol,
return ret;
}
+static bool
+xfrm_policy_find_inexact_candidates(struct xfrm_pol_inexact_candidates *cand,
+ struct xfrm_pol_inexact_bin *b,
+ const xfrm_address_t *saddr,
+ const xfrm_address_t *daddr)
+{
+ if (!b)
+ return false;
+
+ memset(cand, 0, sizeof(*cand));
+ cand->res[XFRM_POL_CAND_ANY] = &b->hhead;
+ return true;
+}
+
static struct xfrm_pol_inexact_bin *
xfrm_policy_inexact_lookup_rcu(struct net *net, u8 type, u16 family,
u8 dir, u32 if_id)
@@ -1370,11 +1508,76 @@ xfrm_policy_inexact_lookup(struct net *net, u8 type, u16 family,
return bin;
}
+static struct xfrm_policy *
+__xfrm_policy_eval_candidates(struct hlist_head *chain,
+ struct xfrm_policy *prefer,
+ const struct flowi *fl,
+ u8 type, u16 family, int dir, u32 if_id)
+{
+ u32 priority = prefer ? prefer->priority : ~0u;
+ struct xfrm_policy *pol;
+
+ if (!chain)
+ return NULL;
+
+ hlist_for_each_entry_rcu(pol, chain, bydst) {
+ int err;
+
+ if (pol->priority > priority)
+ break;
+
+ err = xfrm_policy_match(pol, fl, type, family, dir, if_id);
+ if (err) {
+ if (err != -ESRCH)
+ return ERR_PTR(err);
+
+ continue;
+ }
+
+ if (prefer) {
+ /* matches. Is it older than *prefer? */
+ if (pol->priority == priority &&
+ prefer->pos < pol->pos)
+ return prefer;
+ }
+
+ return pol;
+ }
+
+ return NULL;
+}
+
+static struct xfrm_policy *
+xfrm_policy_eval_candidates(struct xfrm_pol_inexact_candidates *cand,
+ struct xfrm_policy *prefer,
+ const struct flowi *fl,
+ u8 type, u16 family, int dir, u32 if_id)
+{
+ struct xfrm_policy *tmp;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(cand->res); i++) {
+ tmp = __xfrm_policy_eval_candidates(cand->res[i],
+ prefer,
+ fl, type, family, dir,
+ if_id);
+ if (!tmp)
+ continue;
+
+ if (IS_ERR(tmp))
+ return tmp;
+ prefer = tmp;
+ }
+
+ return prefer;
+}
+
static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
const struct flowi *fl,
u16 family, u8 dir,
u32 if_id)
{
+ struct xfrm_pol_inexact_candidates cand;
const xfrm_address_t *daddr, *saddr;
struct xfrm_pol_inexact_bin *bin;
struct xfrm_policy *pol, *ret;
@@ -1413,25 +1616,16 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
}
}
bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir, if_id);
- if (!bin)
+ if (!bin || !xfrm_policy_find_inexact_candidates(&cand, bin, saddr,
+ daddr))
goto skip_inexact;
- chain = &bin->hhead;
- hlist_for_each_entry_rcu(pol, chain, bydst) {
- if ((pol->priority >= priority) && ret)
- break;
- err = xfrm_policy_match(pol, fl, type, family, dir, if_id);
- if (err) {
- if (err == -ESRCH)
- continue;
- else {
- ret = ERR_PTR(err);
- goto fail;
- }
- } else {
- ret = pol;
- break;
- }
+ pol = xfrm_policy_eval_candidates(&cand, ret, fl, type,
+ family, dir, if_id);
+ if (pol) {
+ ret = pol;
+ if (IS_ERR(pol))
+ goto fail;
}
skip_inexact:
@@ -3168,7 +3362,7 @@ static int __net_init xfrm_policy_init(struct net *net)
static void xfrm_policy_fini(struct net *net)
{
- struct xfrm_pol_inexact_bin *bin, *tmp;
+ struct xfrm_pol_inexact_bin *b, *t;
unsigned int sz;
int dir;
@@ -3195,11 +3389,10 @@ static void xfrm_policy_fini(struct net *net)
WARN_ON(!hlist_empty(net->xfrm.policy_byidx));
xfrm_hash_free(net->xfrm.policy_byidx, sz);
- list_for_each_entry_safe(bin, tmp, &net->xfrm.inexact_bins,
- inexact_bins) {
- WARN_ON(!hlist_empty(&bin->hhead));
- xfrm_policy_inexact_delete_bin(net, bin);
- }
+ spin_lock_bh(&net->xfrm.xfrm_policy_lock);
+ list_for_each_entry_safe(b, t, &net->xfrm.inexact_bins, inexact_bins)
+ __xfrm_policy_inexact_prune_bin(b, true);
+ spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
}
static int __net_init xfrm_net_init(struct net *net)
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 06/11] xfrm: policy: consider if_id when hashing inexact policy
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
This avoids searches of polices that cannot match in the first
place due to different interface id by placing them in different bins.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/xfrm/xfrm_policy.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 5c7e7399323f..dda27fd7b8a4 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -48,6 +48,7 @@ struct xfrm_flo {
struct xfrm_pol_inexact_key {
possible_net_t net;
+ u32 if_id;
u16 family;
u8 dir, type;
};
@@ -85,11 +86,12 @@ static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
int dir);
static struct xfrm_pol_inexact_bin *
-xfrm_policy_inexact_lookup(struct net *net, u8 type, u16 family, u8 dir);
+xfrm_policy_inexact_lookup(struct net *net, u8 type, u16 family, u8 dir,
+ u32 if_id);
static struct xfrm_pol_inexact_bin *
xfrm_policy_inexact_lookup_rcu(struct net *net,
- u8 type, u16 family, u8 dir);
+ u8 type, u16 family, u8 dir, u32 if_id);
static struct xfrm_policy *
xfrm_policy_insert_list(struct hlist_head *chain, struct xfrm_policy *policy,
bool excl);
@@ -618,6 +620,7 @@ xfrm_policy_inexact_alloc_bin(const struct xfrm_policy *pol, u8 dir)
.family = pol->family,
.type = pol->type,
.dir = dir,
+ .if_id = pol->if_id,
};
struct net *net = xp_net(pol);
@@ -925,7 +928,8 @@ static u32 xfrm_pol_bin_key(const void *data, u32 len, u32 seed)
const struct xfrm_pol_inexact_key *k = data;
u32 a = k->type << 24 | k->dir << 16 | k->family;
- return jhash_2words(a, net_hash_mix(read_pnet(&k->net)), seed);
+ return jhash_3words(a, k->if_id, net_hash_mix(read_pnet(&k->net)),
+ seed);
}
static u32 xfrm_pol_bin_obj(const void *data, u32 len, u32 seed)
@@ -957,7 +961,7 @@ static int xfrm_pol_bin_cmp(struct rhashtable_compare_arg *arg,
if (ret)
return ret;
- return 0;
+ return b->k.if_id ^ key->if_id;
}
static const struct rhashtable_params xfrm_pol_inexact_params = {
@@ -1094,7 +1098,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u32 if_id,
chain = policy_hash_bysel(net, sel, sel->family, dir);
if (!chain) {
bin = xfrm_policy_inexact_lookup(net, type,
- sel->family, dir);
+ sel->family, dir, if_id);
if (!bin) {
spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
return NULL;
@@ -1335,12 +1339,14 @@ static int xfrm_policy_match(const struct xfrm_policy *pol,
}
static struct xfrm_pol_inexact_bin *
-xfrm_policy_inexact_lookup_rcu(struct net *net, u8 type, u16 family, u8 dir)
+xfrm_policy_inexact_lookup_rcu(struct net *net, u8 type, u16 family,
+ u8 dir, u32 if_id)
{
struct xfrm_pol_inexact_key k = {
.family = family,
.type = type,
.dir = dir,
+ .if_id = if_id,
};
write_pnet(&k.net, net);
@@ -1350,14 +1356,15 @@ xfrm_policy_inexact_lookup_rcu(struct net *net, u8 type, u16 family, u8 dir)
}
static struct xfrm_pol_inexact_bin *
-xfrm_policy_inexact_lookup(struct net *net, u8 type, u16 family, u8 dir)
+xfrm_policy_inexact_lookup(struct net *net, u8 type, u16 family,
+ u8 dir, u32 if_id)
{
struct xfrm_pol_inexact_bin *bin;
lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
rcu_read_lock();
- bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir);
+ bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir, if_id);
rcu_read_unlock();
return bin;
@@ -1405,7 +1412,7 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
break;
}
}
- bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir);
+ bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir, if_id);
if (!bin)
goto skip_inexact;
chain = &bin->hhead;
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 05/11] xfrm: policy: store inexact policies in an rhashtable
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
Switch packet-path lookups for inexact policies to rhashtable.
In this initial version, we now no longer need to search policies with
non-matching address family and type.
Next patch will add the if_id as well so lookups from the xfrm interface
driver only need to search inexact policies for that device.
Future patches will augment the hlist in each rhash bucket with a tree
and pre-sort policies according to daddr/prefix.
A single rhashtable is used. In order to avoid a full rhashtable walk on
netns exit, the bins get placed on a pernet list, i.e. we add almost no
cost for network namespaces that had no xfrm policies.
The inexact lists are kept in place, and policies are added to both the
per-rhash-inexact list and a pernet one.
The latter is needed for the control plane to handle migrate -- these
requests do not consider the if_id, so if we'd remove the inexact_list
now we would have to search all hash buckets and then figure
out which matching policy candidate is the most recent one -- this appears
a bit harder than just keeping the 'old' inexact list for this purpose.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
include/net/netns/xfrm.h | 2 +
include/net/xfrm.h | 1 +
net/xfrm/xfrm_policy.c | 350 +++++++++++++++++++++++++++++++++++++--
3 files changed, 335 insertions(+), 18 deletions(-)
diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index 9991e5ef52cc..59f45b1e9dac 100644
--- a/include/net/netns/xfrm.h
+++ b/include/net/netns/xfrm.h
@@ -5,6 +5,7 @@
#include <linux/list.h>
#include <linux/wait.h>
#include <linux/workqueue.h>
+#include <linux/rhashtable-types.h>
#include <linux/xfrm.h>
#include <net/dst_ops.h>
@@ -53,6 +54,7 @@ struct netns_xfrm {
unsigned int policy_count[XFRM_POLICY_MAX * 2];
struct work_struct policy_hash_work;
struct xfrm_policy_hthresh policy_hthresh;
+ struct list_head inexact_bins;
struct sock *nlsk;
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 0eb390c205af..870fa9b27f7e 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -596,6 +596,7 @@ struct xfrm_policy {
u16 family;
struct xfrm_sec_ctx *security;
struct xfrm_tmpl xfrm_vec[XFRM_MAX_DEPTH];
+ struct hlist_node bydst_inexact_list;
struct rcu_head rcu;
};
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index b00c265f6be3..5c7e7399323f 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -26,6 +26,7 @@
#include <linux/cache.h>
#include <linux/cpu.h>
#include <linux/audit.h>
+#include <linux/rhashtable.h>
#include <net/dst.h>
#include <net/flow.h>
#include <net/xfrm.h>
@@ -45,6 +46,22 @@ struct xfrm_flo {
u8 flags;
};
+struct xfrm_pol_inexact_key {
+ possible_net_t net;
+ u16 family;
+ u8 dir, type;
+};
+
+struct xfrm_pol_inexact_bin {
+ struct xfrm_pol_inexact_key k;
+ struct rhash_head head;
+ struct hlist_head hhead;
+
+ /* slow path below */
+ struct list_head inexact_bins;
+ struct rcu_head rcu;
+};
+
static DEFINE_SPINLOCK(xfrm_if_cb_lock);
static struct xfrm_if_cb const __rcu *xfrm_if_cb __read_mostly;
@@ -55,6 +72,9 @@ static struct xfrm_policy_afinfo const __rcu *xfrm_policy_afinfo[AF_INET6 + 1]
static struct kmem_cache *xfrm_dst_cache __ro_after_init;
static __read_mostly seqcount_t xfrm_policy_hash_generation;
+static struct rhashtable xfrm_policy_inexact_table;
+static const struct rhashtable_params xfrm_pol_inexact_params;
+
static void xfrm_init_pmtu(struct xfrm_dst **bundle, int nr);
static int stale_bundle(struct dst_entry *dst);
static int xfrm_bundle_ok(struct xfrm_dst *xdst);
@@ -64,6 +84,18 @@ static void __xfrm_policy_link(struct xfrm_policy *pol, int dir);
static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
int dir);
+static struct xfrm_pol_inexact_bin *
+xfrm_policy_inexact_lookup(struct net *net, u8 type, u16 family, u8 dir);
+
+static struct xfrm_pol_inexact_bin *
+xfrm_policy_inexact_lookup_rcu(struct net *net,
+ u8 type, u16 family, u8 dir);
+static struct xfrm_policy *
+xfrm_policy_insert_list(struct hlist_head *chain, struct xfrm_policy *policy,
+ bool excl);
+static void xfrm_policy_insert_inexact_list(struct hlist_head *chain,
+ struct xfrm_policy *policy);
+
static inline bool xfrm_pol_hold_rcu(struct xfrm_policy *policy)
{
return refcount_inc_not_zero(&policy->refcnt);
@@ -269,6 +301,7 @@ struct xfrm_policy *xfrm_policy_alloc(struct net *net, gfp_t gfp)
if (policy) {
write_pnet(&policy->xp_net, net);
INIT_LIST_HEAD(&policy->walk.all);
+ INIT_HLIST_NODE(&policy->bydst_inexact_list);
INIT_HLIST_NODE(&policy->bydst);
INIT_HLIST_NODE(&policy->byidx);
rwlock_init(&policy->lock);
@@ -563,6 +596,107 @@ static void xfrm_hash_resize(struct work_struct *work)
mutex_unlock(&hash_resize_mutex);
}
+static void xfrm_hash_reset_inexact_table(struct net *net)
+{
+ struct xfrm_pol_inexact_bin *b;
+
+ lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+
+ list_for_each_entry(b, &net->xfrm.inexact_bins, inexact_bins)
+ INIT_HLIST_HEAD(&b->hhead);
+}
+
+/* Make sure *pol can be inserted into fastbin.
+ * Useful to check that later insert requests will be sucessful
+ * (provided xfrm_policy_lock is held throughout).
+ */
+static struct xfrm_pol_inexact_bin *
+xfrm_policy_inexact_alloc_bin(const struct xfrm_policy *pol, u8 dir)
+{
+ struct xfrm_pol_inexact_bin *bin, *prev;
+ struct xfrm_pol_inexact_key k = {
+ .family = pol->family,
+ .type = pol->type,
+ .dir = dir,
+ };
+ struct net *net = xp_net(pol);
+
+ lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+
+ write_pnet(&k.net, net);
+ bin = rhashtable_lookup_fast(&xfrm_policy_inexact_table, &k,
+ xfrm_pol_inexact_params);
+ if (bin)
+ return bin;
+
+ bin = kzalloc(sizeof(*bin), GFP_ATOMIC);
+ if (!bin)
+ return NULL;
+
+ bin->k = k;
+ INIT_HLIST_HEAD(&bin->hhead);
+
+ prev = rhashtable_lookup_get_insert_key(&xfrm_policy_inexact_table,
+ &bin->k, &bin->head,
+ xfrm_pol_inexact_params);
+ if (!prev) {
+ list_add(&bin->inexact_bins, &net->xfrm.inexact_bins);
+ return bin;
+ }
+
+ kfree(bin);
+
+ return IS_ERR(prev) ? NULL : prev;
+}
+
+static void xfrm_policy_inexact_delete_bin(struct net *net,
+ struct xfrm_pol_inexact_bin *b)
+{
+ lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+
+ if (!hlist_empty(&b->hhead))
+ return;
+
+ if (rhashtable_remove_fast(&xfrm_policy_inexact_table, &b->head,
+ xfrm_pol_inexact_params) == 0) {
+ list_del(&b->inexact_bins);
+ kfree_rcu(b, rcu);
+ }
+}
+
+static void __xfrm_policy_inexact_flush(struct net *net)
+{
+ struct xfrm_pol_inexact_bin *bin;
+
+ lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+
+ list_for_each_entry(bin, &net->xfrm.inexact_bins, inexact_bins)
+ xfrm_policy_inexact_delete_bin(net, bin);
+}
+
+static struct xfrm_policy *
+xfrm_policy_inexact_insert(struct xfrm_policy *policy, u8 dir, int excl)
+{
+ struct xfrm_pol_inexact_bin *bin;
+ struct xfrm_policy *delpol;
+ struct hlist_head *chain;
+ struct net *net;
+
+ bin = xfrm_policy_inexact_alloc_bin(policy, dir);
+ if (!bin)
+ return ERR_PTR(-ENOMEM);
+
+ delpol = xfrm_policy_insert_list(&bin->hhead, policy, excl);
+ if (delpol && excl)
+ return ERR_PTR(-EEXIST);
+
+ net = xp_net(policy);
+ chain = &net->xfrm.policy_inexact[dir];
+ xfrm_policy_insert_inexact_list(chain, policy);
+
+ return delpol;
+}
+
static void xfrm_hash_rebuild(struct work_struct *work)
{
struct net *net = container_of(work, struct net,
@@ -592,7 +726,45 @@ static void xfrm_hash_rebuild(struct work_struct *work)
spin_lock_bh(&net->xfrm.xfrm_policy_lock);
+ /* make sure that we can insert the indirect policies again before
+ * we start with destructive action.
+ */
+ list_for_each_entry(policy, &net->xfrm.policy_all, walk.all) {
+ u8 dbits, sbits;
+
+ dir = xfrm_policy_id2dir(policy->index);
+ if (policy->walk.dead || dir >= XFRM_POLICY_MAX)
+ continue;
+
+ if ((dir & XFRM_POLICY_MASK) == XFRM_POLICY_OUT) {
+ if (policy->family == AF_INET) {
+ dbits = rbits4;
+ sbits = lbits4;
+ } else {
+ dbits = rbits6;
+ sbits = lbits6;
+ }
+ } else {
+ if (policy->family == AF_INET) {
+ dbits = lbits4;
+ sbits = rbits4;
+ } else {
+ dbits = lbits6;
+ sbits = rbits6;
+ }
+ }
+
+ if (policy->selector.prefixlen_d < dbits ||
+ policy->selector.prefixlen_s < sbits)
+ continue;
+
+ if (!xfrm_policy_inexact_alloc_bin(policy, dir))
+ goto out_unlock;
+ }
+
/* reset the bydst and inexact table in all directions */
+ xfrm_hash_reset_inexact_table(net);
+
for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
INIT_HLIST_HEAD(&net->xfrm.policy_inexact[dir]);
hmask = net->xfrm.policy_bydst[dir].hmask;
@@ -625,8 +797,13 @@ static void xfrm_hash_rebuild(struct work_struct *work)
chain = policy_hash_bysel(net, &policy->selector,
policy->family,
xfrm_policy_id2dir(policy->index));
- if (!chain)
- chain = &net->xfrm.policy_inexact[dir];
+ if (!chain) {
+ void *p = xfrm_policy_inexact_insert(policy, dir, 0);
+
+ WARN_ONCE(IS_ERR(p), "reinsert: %ld\n", PTR_ERR(p));
+ continue;
+ }
+
hlist_for_each_entry(pol, chain, bydst) {
if (policy->priority >= pol->priority)
newpos = &pol->bydst;
@@ -639,6 +816,7 @@ static void xfrm_hash_rebuild(struct work_struct *work)
hlist_add_head_rcu(&policy->bydst, chain);
}
+out_unlock:
spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
mutex_unlock(&hash_resize_mutex);
@@ -742,6 +920,84 @@ static bool xfrm_policy_mark_match(struct xfrm_policy *policy,
return false;
}
+static u32 xfrm_pol_bin_key(const void *data, u32 len, u32 seed)
+{
+ const struct xfrm_pol_inexact_key *k = data;
+ u32 a = k->type << 24 | k->dir << 16 | k->family;
+
+ return jhash_2words(a, net_hash_mix(read_pnet(&k->net)), seed);
+}
+
+static u32 xfrm_pol_bin_obj(const void *data, u32 len, u32 seed)
+{
+ const struct xfrm_pol_inexact_bin *b = data;
+
+ return xfrm_pol_bin_key(&b->k, 0, seed);
+}
+
+static int xfrm_pol_bin_cmp(struct rhashtable_compare_arg *arg,
+ const void *ptr)
+{
+ const struct xfrm_pol_inexact_key *key = arg->key;
+ const struct xfrm_pol_inexact_bin *b = ptr;
+ int ret;
+
+ if (!net_eq(read_pnet(&b->k.net), read_pnet(&key->net)))
+ return -1;
+
+ ret = b->k.dir ^ key->dir;
+ if (ret)
+ return ret;
+
+ ret = b->k.type ^ key->type;
+ if (ret)
+ return ret;
+
+ ret = b->k.family ^ key->family;
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct rhashtable_params xfrm_pol_inexact_params = {
+ .head_offset = offsetof(struct xfrm_pol_inexact_bin, head),
+ .hashfn = xfrm_pol_bin_key,
+ .obj_hashfn = xfrm_pol_bin_obj,
+ .obj_cmpfn = xfrm_pol_bin_cmp,
+ .automatic_shrinking = true,
+};
+
+static void xfrm_policy_insert_inexact_list(struct hlist_head *chain,
+ struct xfrm_policy *policy)
+{
+ struct xfrm_policy *pol, *delpol = NULL;
+ struct hlist_node *newpos = NULL;
+
+ hlist_for_each_entry(pol, chain, bydst_inexact_list) {
+ if (pol->type == policy->type &&
+ pol->if_id == policy->if_id &&
+ !selector_cmp(&pol->selector, &policy->selector) &&
+ xfrm_policy_mark_match(policy, pol) &&
+ xfrm_sec_ctx_match(pol->security, policy->security) &&
+ !WARN_ON(delpol)) {
+ delpol = pol;
+ if (policy->priority > pol->priority)
+ continue;
+ } else if (policy->priority >= pol->priority) {
+ newpos = &pol->bydst_inexact_list;
+ continue;
+ }
+ if (delpol)
+ break;
+ }
+
+ if (newpos)
+ hlist_add_behind_rcu(&policy->bydst_inexact_list, newpos);
+ else
+ hlist_add_head_rcu(&policy->bydst_inexact_list, chain);
+}
+
static struct xfrm_policy *xfrm_policy_insert_list(struct hlist_head *chain,
struct xfrm_policy *policy,
bool excl)
@@ -767,6 +1023,7 @@ static struct xfrm_policy *xfrm_policy_insert_list(struct hlist_head *chain,
if (delpol)
break;
}
+
if (newpos)
hlist_add_behind_rcu(&policy->bydst, &newpos->bydst);
else
@@ -783,12 +1040,10 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
spin_lock_bh(&net->xfrm.xfrm_policy_lock);
chain = policy_hash_bysel(net, &policy->selector, policy->family, dir);
- if (chain) {
+ if (chain)
delpol = xfrm_policy_insert_list(chain, policy, excl);
- } else {
- chain = &net->xfrm.policy_inexact[dir];
- delpol = xfrm_policy_insert_list(chain, policy, excl);
- }
+ else
+ delpol = xfrm_policy_inexact_insert(policy, dir, excl);
if (IS_ERR(delpol)) {
spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
@@ -830,14 +1085,24 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u32 if_id,
struct xfrm_sec_ctx *ctx, int delete,
int *err)
{
- struct xfrm_policy *pol, *ret;
+ struct xfrm_pol_inexact_bin *bin = NULL;
+ struct xfrm_policy *pol, *ret = NULL;
struct hlist_head *chain;
*err = 0;
spin_lock_bh(&net->xfrm.xfrm_policy_lock);
chain = policy_hash_bysel(net, sel, sel->family, dir);
- if (!chain)
- chain = &net->xfrm.policy_inexact[dir];
+ if (!chain) {
+ bin = xfrm_policy_inexact_lookup(net, type,
+ sel->family, dir);
+ if (!bin) {
+ spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
+ return NULL;
+ }
+
+ chain = &bin->hhead;
+ }
+
ret = NULL;
hlist_for_each_entry(pol, chain, bydst) {
if (pol->type == type &&
@@ -854,6 +1119,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u32 if_id,
return pol;
}
__xfrm_policy_unlink(pol, dir);
+ xfrm_policy_inexact_delete_bin(net, bin);
}
ret = pol;
break;
@@ -964,7 +1230,9 @@ int xfrm_policy_flush(struct net *net, u8 type, bool task_valid)
spin_lock_bh(&net->xfrm.xfrm_policy_lock);
goto again;
}
- if (!cnt)
+ if (cnt)
+ __xfrm_policy_inexact_flush(net);
+ else
err = -ESRCH;
out:
spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
@@ -1063,21 +1331,50 @@ static int xfrm_policy_match(const struct xfrm_policy *pol,
if (match)
ret = security_xfrm_policy_lookup(pol->security, fl->flowi_secid,
dir);
-
return ret;
}
+static struct xfrm_pol_inexact_bin *
+xfrm_policy_inexact_lookup_rcu(struct net *net, u8 type, u16 family, u8 dir)
+{
+ struct xfrm_pol_inexact_key k = {
+ .family = family,
+ .type = type,
+ .dir = dir,
+ };
+
+ write_pnet(&k.net, net);
+
+ return rhashtable_lookup(&xfrm_policy_inexact_table, &k,
+ xfrm_pol_inexact_params);
+}
+
+static struct xfrm_pol_inexact_bin *
+xfrm_policy_inexact_lookup(struct net *net, u8 type, u16 family, u8 dir)
+{
+ struct xfrm_pol_inexact_bin *bin;
+
+ lockdep_assert_held(&net->xfrm.xfrm_policy_lock);
+
+ rcu_read_lock();
+ bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir);
+ rcu_read_unlock();
+
+ return bin;
+}
+
static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
const struct flowi *fl,
u16 family, u8 dir,
u32 if_id)
{
- int err;
- struct xfrm_policy *pol, *ret;
const xfrm_address_t *daddr, *saddr;
+ struct xfrm_pol_inexact_bin *bin;
+ struct xfrm_policy *pol, *ret;
struct hlist_head *chain;
unsigned int sequence;
u32 priority;
+ int err;
daddr = xfrm_flowi_daddr(fl, family);
saddr = xfrm_flowi_saddr(fl, family);
@@ -1108,7 +1405,10 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
break;
}
}
- chain = &net->xfrm.policy_inexact[dir];
+ bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir);
+ if (!bin)
+ goto skip_inexact;
+ chain = &bin->hhead;
hlist_for_each_entry_rcu(pol, chain, bydst) {
if ((pol->priority >= priority) && ret)
break;
@@ -1127,6 +1427,7 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
}
}
+skip_inexact:
if (read_seqcount_retry(&xfrm_policy_hash_generation, sequence))
goto retry;
@@ -1218,6 +1519,7 @@ static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
/* Socket policies are not hashed. */
if (!hlist_unhashed(&pol->bydst)) {
hlist_del_rcu(&pol->bydst);
+ hlist_del_init(&pol->bydst_inexact_list);
hlist_del(&pol->byidx);
}
@@ -2795,13 +3097,17 @@ static void xfrm_statistics_fini(struct net *net)
static int __net_init xfrm_policy_init(struct net *net)
{
unsigned int hmask, sz;
- int dir;
+ int dir, err;
- if (net_eq(net, &init_net))
+ if (net_eq(net, &init_net)) {
xfrm_dst_cache = kmem_cache_create("xfrm_dst_cache",
sizeof(struct xfrm_dst),
0, SLAB_HWCACHE_ALIGN|SLAB_PANIC,
NULL);
+ err = rhashtable_init(&xfrm_policy_inexact_table,
+ &xfrm_pol_inexact_params);
+ BUG_ON(err);
+ }
hmask = 8 - 1;
sz = (hmask+1) * sizeof(struct hlist_head);
@@ -2836,6 +3142,7 @@ static int __net_init xfrm_policy_init(struct net *net)
seqlock_init(&net->xfrm.policy_hthresh.lock);
INIT_LIST_HEAD(&net->xfrm.policy_all);
+ INIT_LIST_HEAD(&net->xfrm.inexact_bins);
INIT_WORK(&net->xfrm.policy_hash_work, xfrm_hash_resize);
INIT_WORK(&net->xfrm.policy_hthresh.work, xfrm_hash_rebuild);
return 0;
@@ -2854,6 +3161,7 @@ static int __net_init xfrm_policy_init(struct net *net)
static void xfrm_policy_fini(struct net *net)
{
+ struct xfrm_pol_inexact_bin *bin, *tmp;
unsigned int sz;
int dir;
@@ -2879,6 +3187,12 @@ static void xfrm_policy_fini(struct net *net)
sz = (net->xfrm.policy_idx_hmask + 1) * sizeof(struct hlist_head);
WARN_ON(!hlist_empty(net->xfrm.policy_byidx));
xfrm_hash_free(net->xfrm.policy_byidx, sz);
+
+ list_for_each_entry_safe(bin, tmp, &net->xfrm.inexact_bins,
+ inexact_bins) {
+ WARN_ON(!hlist_empty(&bin->hhead));
+ xfrm_policy_inexact_delete_bin(net, bin);
+ }
}
static int __net_init xfrm_net_init(struct net *net)
@@ -3044,7 +3358,7 @@ static struct xfrm_policy *xfrm_migrate_policy_find(const struct xfrm_selector *
}
}
chain = &net->xfrm.policy_inexact[dir];
- hlist_for_each_entry(pol, chain, bydst) {
+ hlist_for_each_entry(pol, chain, bydst_inexact_list) {
if ((pol->priority >= priority) && ret)
break;
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 04/11] xfrm: policy: return NULL when inexact search needed
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
currently policy_hash_bysel() returns the hash bucket list
(for exact policies), or the inexact list (when policy uses a prefix).
Searching this inexact list is slow, so it might be better to pre-sort
inexact lists into a tree or another data structure for faster
searching.
However, due to 'any' policies, that need to be searched in any case,
doing so will require that 'inexact' policies need to be handled
specially to decide the best search strategy. So change hash_bysel()
and return NULL if the policy can't be handled via the policy hash
table.
Right now, we simply use the inexact list when this happens, but
future patch can then implement a different strategy.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/xfrm/xfrm_policy.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 20d6815be0d7..b00c265f6be3 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -365,7 +365,7 @@ static struct hlist_head *policy_hash_bysel(struct net *net,
hash = __sel_hash(sel, family, hmask, dbits, sbits);
if (hash == hmask + 1)
- return &net->xfrm.policy_inexact[dir];
+ return NULL;
return rcu_dereference_check(net->xfrm.policy_bydst[dir].table,
lockdep_is_held(&net->xfrm.xfrm_policy_lock)) + hash;
@@ -625,6 +625,8 @@ static void xfrm_hash_rebuild(struct work_struct *work)
chain = policy_hash_bysel(net, &policy->selector,
policy->family,
xfrm_policy_id2dir(policy->index));
+ if (!chain)
+ chain = &net->xfrm.policy_inexact[dir];
hlist_for_each_entry(pol, chain, bydst) {
if (policy->priority >= pol->priority)
newpos = &pol->bydst;
@@ -781,7 +783,12 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
spin_lock_bh(&net->xfrm.xfrm_policy_lock);
chain = policy_hash_bysel(net, &policy->selector, policy->family, dir);
- delpol = xfrm_policy_insert_list(chain, policy, excl);
+ if (chain) {
+ delpol = xfrm_policy_insert_list(chain, policy, excl);
+ } else {
+ chain = &net->xfrm.policy_inexact[dir];
+ delpol = xfrm_policy_insert_list(chain, policy, excl);
+ }
if (IS_ERR(delpol)) {
spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
@@ -829,6 +836,8 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u32 if_id,
*err = 0;
spin_lock_bh(&net->xfrm.xfrm_policy_lock);
chain = policy_hash_bysel(net, sel, sel->family, dir);
+ if (!chain)
+ chain = &net->xfrm.policy_inexact[dir];
ret = NULL;
hlist_for_each_entry(pol, chain, bydst) {
if (pol->type == type &&
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 03/11] xfrm: policy: split list insertion into a helper
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
... so we can reuse this later without code duplication when we add
policy to a second inexact list.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/xfrm/xfrm_policy.c | 43 ++++++++++++++++++++++++++----------------
1 file changed, 27 insertions(+), 16 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 39d0db2a50d9..20d6815be0d7 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -740,18 +740,12 @@ static bool xfrm_policy_mark_match(struct xfrm_policy *policy,
return false;
}
-int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
+static struct xfrm_policy *xfrm_policy_insert_list(struct hlist_head *chain,
+ struct xfrm_policy *policy,
+ bool excl)
{
- struct net *net = xp_net(policy);
- struct xfrm_policy *pol;
- struct xfrm_policy *delpol;
- struct hlist_head *chain;
- struct hlist_node *newpos;
+ struct xfrm_policy *pol, *newpos = NULL, *delpol = NULL;
- spin_lock_bh(&net->xfrm.xfrm_policy_lock);
- chain = policy_hash_bysel(net, &policy->selector, policy->family, dir);
- delpol = NULL;
- newpos = NULL;
hlist_for_each_entry(pol, chain, bydst) {
if (pol->type == policy->type &&
pol->if_id == policy->if_id &&
@@ -759,24 +753,41 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
xfrm_policy_mark_match(policy, pol) &&
xfrm_sec_ctx_match(pol->security, policy->security) &&
!WARN_ON(delpol)) {
- if (excl) {
- spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
- return -EEXIST;
- }
+ if (excl)
+ return ERR_PTR(-EEXIST);
delpol = pol;
if (policy->priority > pol->priority)
continue;
} else if (policy->priority >= pol->priority) {
- newpos = &pol->bydst;
+ newpos = pol;
continue;
}
if (delpol)
break;
}
if (newpos)
- hlist_add_behind_rcu(&policy->bydst, newpos);
+ hlist_add_behind_rcu(&policy->bydst, &newpos->bydst);
else
hlist_add_head_rcu(&policy->bydst, chain);
+
+ return delpol;
+}
+
+int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
+{
+ struct net *net = xp_net(policy);
+ struct xfrm_policy *delpol;
+ struct hlist_head *chain;
+
+ spin_lock_bh(&net->xfrm.xfrm_policy_lock);
+ chain = policy_hash_bysel(net, &policy->selector, policy->family, dir);
+ delpol = xfrm_policy_insert_list(chain, policy, excl);
+
+ if (IS_ERR(delpol)) {
+ spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
+ return PTR_ERR(delpol);
+ }
+
__xfrm_policy_link(policy, dir);
/* After previous checking, family can either be AF_INET or AF_INET6 */
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 02/11] xfrm: security: iterate all, not inexact lists
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
currently all non-socket policies are either hashed in the dst table,
or placed on the 'inexact list'. When flushing, we first walk the
table, then the (per-direction) inexact lists.
When we try and get rid of the inexact lists to having "n" inexact
lists (e.g. per-af inexact lists, or sorted into a tree), this walk
would become more complicated.
Simplify this: walk the 'all' list and skip socket policies during
traversal so we don't need to handle exact and inexact policies
separately anymore.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/xfrm/xfrm_policy.c | 93 ++++++++++++------------------------------
1 file changed, 26 insertions(+), 67 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 119a427d9b2b..39d0db2a50d9 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -892,36 +892,19 @@ EXPORT_SYMBOL(xfrm_policy_byid);
static inline int
xfrm_policy_flush_secctx_check(struct net *net, u8 type, bool task_valid)
{
- int dir, err = 0;
+ struct xfrm_policy *pol;
+ int err = 0;
- for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
- struct xfrm_policy *pol;
- int i;
+ list_for_each_entry(pol, &net->xfrm.policy_all, walk.all) {
+ if (pol->walk.dead ||
+ xfrm_policy_id2dir(pol->index) >= XFRM_POLICY_MAX ||
+ pol->type != type)
+ continue;
- hlist_for_each_entry(pol,
- &net->xfrm.policy_inexact[dir], bydst) {
- if (pol->type != type)
- continue;
- err = security_xfrm_policy_delete(pol->security);
- if (err) {
- xfrm_audit_policy_delete(pol, 0, task_valid);
- return err;
- }
- }
- for (i = net->xfrm.policy_bydst[dir].hmask; i >= 0; i--) {
- hlist_for_each_entry(pol,
- net->xfrm.policy_bydst[dir].table + i,
- bydst) {
- if (pol->type != type)
- continue;
- err = security_xfrm_policy_delete(
- pol->security);
- if (err) {
- xfrm_audit_policy_delete(pol, 0,
- task_valid);
- return err;
- }
- }
+ err = security_xfrm_policy_delete(pol->security);
+ if (err) {
+ xfrm_audit_policy_delete(pol, 0, task_valid);
+ return err;
}
}
return err;
@@ -937,6 +920,7 @@ xfrm_policy_flush_secctx_check(struct net *net, u8 type, bool task_valid)
int xfrm_policy_flush(struct net *net, u8 type, bool task_valid)
{
int dir, err = 0, cnt = 0;
+ struct xfrm_policy *pol;
spin_lock_bh(&net->xfrm.xfrm_policy_lock);
@@ -944,46 +928,21 @@ int xfrm_policy_flush(struct net *net, u8 type, bool task_valid)
if (err)
goto out;
- for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
- struct xfrm_policy *pol;
- int i;
-
- again1:
- hlist_for_each_entry(pol,
- &net->xfrm.policy_inexact[dir], bydst) {
- if (pol->type != type)
- continue;
- __xfrm_policy_unlink(pol, dir);
- spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
- cnt++;
-
- xfrm_audit_policy_delete(pol, 1, task_valid);
-
- xfrm_policy_kill(pol);
-
- spin_lock_bh(&net->xfrm.xfrm_policy_lock);
- goto again1;
- }
-
- for (i = net->xfrm.policy_bydst[dir].hmask; i >= 0; i--) {
- again2:
- hlist_for_each_entry(pol,
- net->xfrm.policy_bydst[dir].table + i,
- bydst) {
- if (pol->type != type)
- continue;
- __xfrm_policy_unlink(pol, dir);
- spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
- cnt++;
-
- xfrm_audit_policy_delete(pol, 1, task_valid);
- xfrm_policy_kill(pol);
-
- spin_lock_bh(&net->xfrm.xfrm_policy_lock);
- goto again2;
- }
- }
+again:
+ list_for_each_entry(pol, &net->xfrm.policy_all, walk.all) {
+ dir = xfrm_policy_id2dir(pol->index);
+ if (pol->walk.dead ||
+ dir >= XFRM_POLICY_MAX ||
+ pol->type != type)
+ continue;
+ __xfrm_policy_unlink(pol, dir);
+ spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
+ cnt++;
+ xfrm_audit_policy_delete(pol, 1, task_valid);
+ xfrm_policy_kill(pol);
+ spin_lock_bh(&net->xfrm.xfrm_policy_lock);
+ goto again;
}
if (!cnt)
err = -ESRCH;
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 01/11] selftests: add xfrm policy test script
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20181107220041.26205-1-fw@strlen.de>
add a script that adds a ipsec tunnel between two network
namespaces plus following policies:
.0/24 -> ipsec tunnel
.240/28 -> bypass
.253/32 -> ipsec tunnel
Then check that .254 bypasses tunnel (match /28 exception),
and .2 (match /24) and .253 (match direct policy) pass through the
tunnel.
Abuses iptables to check if ping did resolve an ipsec policy or not.
Also adds a bunch of 'block' rules that are not supposed to match.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
tools/testing/selftests/net/Makefile | 3 +-
tools/testing/selftests/net/xfrm_policy.sh | 276 +++++++++++++++++++++
2 files changed, 278 insertions(+), 1 deletion(-)
create mode 100755 tools/testing/selftests/net/xfrm_policy.sh
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 256d82d5fa87..4c7df17d4f42 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -4,7 +4,8 @@
CFLAGS = -Wall -Wl,--no-as-needed -O2 -g
CFLAGS += -I../../../../usr/include/
-TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh rtnetlink.sh
+TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh \
+ rtnetlink.sh xfrm_policy.sh
TEST_PROGS += fib_tests.sh fib-onlink-tests.sh pmtu.sh udpgso.sh ip_defrag.sh
TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh
TEST_PROGS_EXTENDED := in_netns.sh
diff --git a/tools/testing/selftests/net/xfrm_policy.sh b/tools/testing/selftests/net/xfrm_policy.sh
new file mode 100755
index 000000000000..72f9e3dfbea1
--- /dev/null
+++ b/tools/testing/selftests/net/xfrm_policy.sh
@@ -0,0 +1,276 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Check xfrm policy resolution. Topology:
+#
+# 1.2 1.1 3.1 3.10 2.1 2.2
+# eth1 eth1 veth0 veth0 eth1 eth1
+# ns1 ---- ns3 ----- ns4 ---- ns2
+#
+# ns3 and ns4 are connected via ipsec tunnel.
+# pings from ns1 to ns2 (and vice versa) are supposed to work like this:
+# ns1: ping 10.0.2.2: passes via ipsec tunnel.
+# ns2: ping 10.0.1.2: passes via ipsec tunnel.
+
+# ns1: ping 10.0.1.253: passes via ipsec tunnel (direct policy)
+# ns2: ping 10.0.2.253: passes via ipsec tunnel (direct policy)
+#
+# ns1: ping 10.0.2.254: does NOT pass via ipsec tunnel (exception)
+# ns2: ping 10.0.1.254: does NOT pass via ipsec tunnel (exception)
+
+# Kselftest framework requirement - SKIP code is 4.
+ksft_skip=4
+ret=0
+
+KEY_SHA=0xdeadbeef1234567890abcdefabcdefabcdefabcd
+KEY_AES=0x0123456789abcdef0123456789012345
+SPI1=0x1
+SPI2=0x2
+
+do_esp() {
+ local ns=$1
+ local me=$2
+ local remote=$3
+ local lnet=$4
+ local rnet=$5
+ local spi_out=$6
+ local spi_in=$7
+
+ ip -net $ns xfrm state add src $remote dst $me proto esp spi $spi_in enc aes $KEY_AES auth sha1 $KEY_SHA mode tunnel sel src $rnet dst $lnet
+ ip -net $ns xfrm state add src $me dst $remote proto esp spi $spi_out enc aes $KEY_AES auth sha1 $KEY_SHA mode tunnel sel src $lnet dst $rnet
+
+ # to encrypt packets as they go out (includes forwarded packets that need encapsulation)
+ ip -net $ns xfrm policy add src $lnet dst $rnet dir out tmpl src $me dst $remote proto esp mode tunnel priority 100 action allow
+ # to fwd decrypted packets after esp processing:
+ ip -net $ns xfrm policy add src $rnet dst $lnet dir fwd tmpl src $remote dst $me proto esp mode tunnel priority 100 action allow
+}
+
+do_exception() {
+ local ns=$1
+ local me=$2
+ local remote=$3
+ local encryptip=$4
+ local plain=$5
+
+ # network $plain passes without tunnel
+ ip -net $ns xfrm policy add dst $plain dir out priority 10 action allow
+
+ # direct policy for $encryptip, use tunnel, higher prio takes precedence
+ ip -net $ns xfrm policy add dst $encryptip dir out tmpl src $me dst $remote proto esp mode tunnel priority 1 action allow
+}
+
+# policies that are not supposed to match any packets generated in this test.
+do_dummies4() {
+ local ns=$1
+
+ for i in $(seq 10 16);do
+ # dummy policy with wildcard src/dst.
+ echo netns exec $ns ip xfrm policy add src 0.0.0.0/0 dst 10.$i.99.0/30 dir out action block
+ echo netns exec $ns ip xfrm policy add src 10.$i.99.0/30 dst 0.0.0.0/0 dir out action block
+ for j in $(seq 32 64);do
+ echo netns exec $ns ip xfrm policy add src 10.$i.1.0/30 dst 10.$i.$j.0/30 dir out action block
+ # silly, as it encompasses the one above too, but its allowed:
+ echo netns exec $ns ip xfrm policy add src 10.$i.1.0/29 dst 10.$i.$j.0/29 dir out action block
+ # and yet again, even more broad one.
+ echo netns exec $ns ip xfrm policy add src 10.$i.1.0/24 dst 10.$i.$j.0/24 dir out action block
+ echo netns exec $ns ip xfrm policy add src 10.$i.$j.0/24 dst 10.$i.1.0/24 dir fwd action block
+ done
+ done | ip -batch /dev/stdin
+}
+
+do_dummies6() {
+ local ns=$1
+
+ for i in $(seq 10 16);do
+ for j in $(seq 32 64);do
+ echo netns exec $ns ip xfrm policy add src dead:$i::/64 dst dead:$i:$j::/64 dir out action block
+ echo netns exec $ns ip xfrm policy add src dead:$i:$j::/64 dst dead:$i::/24 dir fwd action block
+ done
+ done | ip -batch /dev/stdin
+}
+
+check_ipt_policy_count()
+{
+ ns=$1
+
+ ip netns exec $ns iptables-save -c |grep policy | ( read c rest
+ ip netns exec $ns iptables -Z
+ if [ x"$c" = x'[0:0]' ]; then
+ exit 0
+ elif [ x"$c" = x ]; then
+ echo "ERROR: No counters"
+ ret=1
+ exit 111
+ else
+ exit 1
+ fi
+ )
+}
+
+check_xfrm() {
+ # 0: iptables -m policy rule count == 0
+ # 1: iptables -m policy rule count != 0
+ rval=$1
+ ip=$2
+ ret=0
+
+ ip netns exec ns1 ping -q -c 1 10.0.2.$ip > /dev/null
+
+ check_ipt_policy_count ns3
+ if [ $? -ne $rval ] ; then
+ ret=1
+ fi
+ check_ipt_policy_count ns4
+ if [ $? -ne $rval ] ; then
+ ret=1
+ fi
+
+ ip netns exec ns2 ping -q -c 1 10.0.1.$ip > /dev/null
+
+ check_ipt_policy_count ns3
+ if [ $? -ne $rval ] ; then
+ ret=1
+ fi
+ check_ipt_policy_count ns4
+ if [ $? -ne $rval ] ; then
+ ret=1
+ fi
+
+ return $ret
+}
+
+#check for needed privileges
+if [ "$(id -u)" -ne 0 ];then
+ echo "SKIP: Need root privileges"
+ exit $ksft_skip
+fi
+
+ip -Version 2>/dev/null >/dev/null
+if [ $? -ne 0 ];then
+ echo "SKIP: Could not run test without the ip tool"
+ exit $ksft_skip
+fi
+
+# needed to check if policy lookup got valid ipsec result
+iptables --version 2>/dev/null >/dev/null
+if [ $? -ne 0 ];then
+ echo "SKIP: Could not run test without iptables tool"
+ exit $ksft_skip
+fi
+
+for i in 1 2 3 4; do
+ ip netns add ns$i
+ ip -net ns$i link set lo up
+done
+
+DEV=veth0
+ip link add $DEV netns ns1 type veth peer name eth1 netns ns3
+ip link add $DEV netns ns2 type veth peer name eth1 netns ns4
+
+ip link add $DEV netns ns3 type veth peer name veth0 netns ns4
+
+DEV=veth0
+for i in 1 2; do
+ ip -net ns$i link set $DEV up
+ ip -net ns$i addr add 10.0.$i.2/24 dev $DEV
+ ip -net ns$i addr add dead:$i::2/64 dev $DEV
+
+ ip -net ns$i addr add 10.0.$i.253 dev $DEV
+ ip -net ns$i addr add 10.0.$i.254 dev $DEV
+ ip -net ns$i addr add dead:$i::fd dev $DEV
+ ip -net ns$i addr add dead:$i::fe dev $DEV
+done
+
+for i in 3 4; do
+ip -net ns$i link set eth1 up
+ip -net ns$i link set veth0 up
+done
+
+ip -net ns1 route add default via 10.0.1.1
+ip -net ns2 route add default via 10.0.2.1
+
+ip -net ns3 addr add 10.0.1.1/24 dev eth1
+ip -net ns3 addr add 10.0.3.1/24 dev veth0
+ip -net ns3 addr add 2001:1::1/64 dev eth1
+ip -net ns3 addr add 2001:3::1/64 dev veth0
+
+ip -net ns3 route add default via 10.0.3.10
+
+ip -net ns4 addr add 10.0.2.1/24 dev eth1
+ip -net ns4 addr add 10.0.3.10/24 dev veth0
+ip -net ns4 addr add 2001:2::1/64 dev eth1
+ip -net ns4 addr add 2001:3::10/64 dev veth0
+ip -net ns4 route add default via 10.0.3.1
+
+for j in 4 6; do
+ for i in 3 4;do
+ ip netns exec ns$i sysctl net.ipv$j.conf.eth1.forwarding=1 > /dev/null
+ ip netns exec ns$i sysctl net.ipv$j.conf.veth0.forwarding=1 > /dev/null
+ done
+done
+
+# abuse iptables rule counter to check if ping matches a policy
+ip netns exec ns3 iptables -p icmp -A FORWARD -m policy --dir out --pol ipsec
+ip netns exec ns4 iptables -p icmp -A FORWARD -m policy --dir out --pol ipsec
+if [ $? -ne 0 ];then
+ echo "SKIP: Could not insert iptables rule"
+ for i in 1 2 3 4;do ip netns del ns$i;done
+ exit $ksft_skip
+fi
+
+# localip remoteip localnet remotenet
+do_esp ns3 10.0.3.1 10.0.3.10 10.0.1.0/24 10.0.2.0/24 $SPI1 $SPI2
+do_esp ns3 dead:3::1 dead:3::10 dead:1::/64 dead:2::/64 $SPI1 $SPI2
+do_esp ns4 10.0.3.10 10.0.3.1 10.0.2.0/24 10.0.1.0/24 $SPI2 $SPI1
+do_esp ns4 dead:3::10 dead:3::1 dead:2::/64 dead:1::/64 $SPI2 $SPI1
+
+do_dummies4 ns3
+do_dummies6 ns4
+
+# ping to .254 should use ipsec, exception is not installed.
+check_xfrm 1 254
+if [ $? -ne 0 ]; then
+ echo "FAIL: expected ping to .254 to use ipsec tunnel"
+ ret=1
+else
+ echo "PASS: policy before exception matches"
+fi
+
+# installs exceptions
+# localip remoteip encryptdst plaindst
+do_exception ns3 10.0.3.1 10.0.3.10 10.0.2.253 10.0.2.240/28
+do_exception ns4 10.0.3.10 10.0.3.1 10.0.1.253 10.0.1.240/28
+
+do_exception ns3 dead:3::1 dead:3::10 dead:2::fd dead:2:f0::/96
+do_exception ns4 dead:3::10 dead:3::1 dead:1::fd dead:1:f0::/96
+
+# ping to .254 should now be excluded from the tunnel
+check_xfrm 0 254
+if [ $? -ne 0 ]; then
+ echo "FAIL: expected ping to .254 to fail"
+ ret=1
+else
+ echo "PASS: ping to .254 bypassed ipsec tunnel"
+fi
+
+# ping to .253 should use use ipsec due to direct policy exception.
+check_xfrm 1 253
+if [ $? -ne 0 ]; then
+ echo "FAIL: expected ping to .253 to use ipsec tunnel"
+ ret=1
+else
+ echo "PASS: direct policy matches"
+fi
+
+# ping to .2 should use ipsec.
+check_xfrm 1 2
+if [ $? -ne 0 ]; then
+ echo "FAIL: expected ping to .2 to use ipsec tunnel"
+ ret=1
+else
+ echo "PASS: policy matches"
+fi
+
+for i in 1 2 3 4;do ip netns del ns$i;done
+
+exit $ret
--
2.18.1
^ permalink raw reply related
* [PATCH ipsec-next 00/11] xfrm: policy: add inexact policy search tree
From: Florian Westphal @ 2018-11-07 22:00 UTC (permalink / raw)
To: netdev
This series attempts to improve xfrm policy lookup performance when
a lot of (several hundred or even thousands) inexact policies exist
on a system.
On insert, a policy is either placed in hash table (all direct (/32 for
ipv4, /128 policies, or all policies matching a user-configured threshold).
All other policies get inserted into inexact list as per priority.
Lookup then scans inexact list for first matching entry.
This series instead makes it so that inexact policy is added to exactly
one of four different search list classes.
1. "Any:Any" list, containing policies where both saddr and daddr are
wildcards or have very coarse prefixes, e.g. 10.0.0.0/8 and the like.
2. "saddr:any" list, containing policies with a fixed saddr/prefixlen,
but without destination restrictions.
These lists are stored in rbtree nodes; each node contains those
policies matching saddr/prefixlen.
3. "Any:daddr" list. Similar to 2), except for policies where only the
destinations are specified.
4. "saddr:daddr" lists, containing policies that match the given
source/destination network.
The root of the saddr/daddr tree is stored in the nodes of the
'daddr' tree.
This diagram illustrates the list classes, and their
placement in the lookup hierarchy:
xfrm_pol_inexact_bin = hash(dir,type,family,if_id);
|
+---- root_d: sorted by daddr:prefix
| |
| xfrm_pol_inexact_node
| |
| +- root: sorted by saddr/prefix
| | |
| | xfrm_pol_inexact_node
| | |
| | + root: unused
| | |
| | + hhead: saddr:daddr policies
| |
| +- coarse policies and all any:daddr policies
|
+---- root_s: sorted by saddr:prefix
| |
| xfrm_pol_inexact_node
| |
| + root: unused
| |
| + hhead: saddr:any policies
|
+---- coarse policies and all any:any policies
First we obtain inexact bin by hashing direction, family and other 'must
match' criteria.
Next, we search root_d using packets' destination ip address.
If we find a matching node, we search that nodes' source address tree
using packets src address.
We also search the bins root_s tree using packets source address.
This initial lookup now has created a 'candidate set' consisting
of up to four hlist heads to the four search classes.
Each list gets scanned for first matching entry.
Out of those up the one with the highest priority is chosen.
In case several policies had same priority, most recent one is used to
match old behaviour.
Locking rules:
insert requires net->xfrm.xfrm_policy_lock spinlock held + BH off
(no change to current kernel).
Insert/removal of a tree node needs increment of its sequence count.
lookup requires rcu read lock, lookups in a tree need to sample
the sequence count of the bin that tree is located in as well and
re-try in case it has changed.
Results on my kvm test setup, using 4 netns:
# 1.2 1.1 3.1 3.10 2.1 2.2
# eth1 eth1 veth0 veth0 eth1 eth1
# ns1 ---- ns3 ----- ns4 ---- ns2
ns3 and ns4 are connected via ipsec tunnel.
'Dummy policies' below means that i created 1k non-matching
policies in both ns3 and ns4.
20 parallel netperf (mbits, unidirectional TCP_STREAM):
normal:
993.518000 (no dummy policies)
796.32 (with dummy policies)
patched:
991.335 (no dummy policies)
989.684 (with dummy policies)
20 parallel TCP_RR, trans. per second:
normal:
32413.930000 (no dummy policies)
17725.970000 (with dummy policies)
patched:
32314.51 (no dummy policies)
32326.190000 (with dummy policies)
caveats:
1. Won't work unless large part of policies have no common saddr/daddr pairs.
Extreme example: adding 10k 0.0.0.0/0 -> 0.0.0.0/0
policies may require search of all policies, just like current kernel.
2. The policy add sequence
a daddr 10.0.0.0/26
b daddr 10.0.0.64/26
c daddr 10.0.0.0/24
When c gets added, search nodes for a and b have to be merged with
the new subnet, as it contains both.
This also means that a single policy rule with a small subnet value
can cause severe tree degradation and thus longer search lists.
3. Causes (small) performance degradation when setup only has few policies.
4. Policies need an additional hlist, as MIGRATE doesn't consider if_id,
but the patchset makes the if_id part of the initial hash lookup.
Could be avoided by not considering if_id as part of initial bin lookup,
but then increases list length a lot when we have lots of xfrm interfaces.
5. In oder to solve 'two matching candidates have same priority, which
takes precedence?' problem I added a number to xfrm_policy struct that
maps to its index in the complete inexact list.
This was necessary to make sure same sequence of policy-add commands
keeps on returning same lookup result compared to older kernel.
This work doesn't prevent us from re-adding a new flow caching scheme,
but so far i found no good solution (all have various DoS or degradation
issues).
Comments or questions welcome.
Florian Westphal (11):
selftests: add xfrm policy test script
xfrm: security: iterate all, not inexact lists
xfrm: policy: split list insertion into a helper
xfrm: policy: return NULL when inexact search needed
xfrm: policy: store inexact policies in an rhashtable
xfrm: policy: consider if_id when hashing inexact policy
xfrm: policy: add inexact policy search tree infrastructure
xfrm: policy: store inexact policies in a tree ordered by destination address
xfrm: policy: check reinserted policies match their node
xfrm: policy: store inexact policies in a tree ordered by source address
xfrm: policy: add 2nd-level saddr trees for inexact policies
include/net/netns/xfrm.h | 2
include/net/xfrm.h | 3
net/xfrm/xfrm_policy.c | 1234 ++++++++++++++++++++++++++---
tools/testing/selftests/net/Makefile | 3
tools/testing/selftests/net/xfrm_policy.sh | 276 ++++++
5 files changed, 1391 insertions(+), 127 deletions(-)
^ permalink raw reply
* Re: [PATCH bpf] bpf: Fix IPv6 dport byte order in bpf_sk_lookup_udp
From: Martin Lau @ 2018-11-07 21:55 UTC (permalink / raw)
To: Andrey Ignatov
Cc: netdev@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
joe@wand.net.nz, Kernel Team
In-Reply-To: <20181107213607.2658130-1-rdna@fb.com>
On Wed, Nov 07, 2018 at 01:36:07PM -0800, Andrey Ignatov wrote:
> Lookup functions in sk_lookup have different expectations about byte
> order of provided arguments.
>
> Specifically __inet_lookup, __udp4_lib_lookup and __udp6_lib_lookup
> expect dport to be in network byte order and do ntohs(dport) internally.
>
> At the same time __inet6_lookup expects dport to be in host byte order
> and correspondingly name the argument hnum.
>
> sk_lookup works correctly with __inet_lookup, __udp4_lib_lookup and
> __inet6_lookup with regard to dport. But in __udp6_lib_lookup case it
> uses host instead of expected network byte order. It makes result
> returned by bpf_sk_lookup_udp for IPv6 incorrect.
>
> The patch fixes byte order of dport passed to __udp6_lib_lookup.
>
> Originally sk_lookup properly handled UDPv6, but not TCPv6. 5ef0ae84f02a
> fixes TCPv6 but breaks UDPv6.
>
> Fixes: 5ef0ae84f02a ("bpf: Fix IPv6 dport byte-order in bpf_sk_lookup")
> Signed-off-by: Andrey Ignatov <rdna@fb.com>
Good catch.
Acked-by: Martin KaFai Lau <kafai@fb.com>
> ---
> net/core/filter.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index e521c5ebc7d1..9a1327eb25fa 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4852,18 +4852,17 @@ static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
> } else {
> struct in6_addr *src6 = (struct in6_addr *)&tuple->ipv6.saddr;
> struct in6_addr *dst6 = (struct in6_addr *)&tuple->ipv6.daddr;
> - u16 hnum = ntohs(tuple->ipv6.dport);
> int sdif = inet6_sdif(skb);
>
> if (proto == IPPROTO_TCP)
> sk = __inet6_lookup(net, &tcp_hashinfo, skb, 0,
> src6, tuple->ipv6.sport,
> - dst6, hnum,
> + dst6, ntohs(tuple->ipv6.dport),
> dif, sdif, &refcounted);
> else if (likely(ipv6_bpf_stub))
> sk = ipv6_bpf_stub->udp6_lib_lookup(net,
> src6, tuple->ipv6.sport,
> - dst6, hnum,
> + dst6, tuple->ipv6.dport,
> dif, sdif,
> &udp_table, skb);
> #endif
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH bpf] bpf: Fix IPv6 dport byte order in bpf_sk_lookup_udp
From: Joe Stringer @ 2018-11-07 21:51 UTC (permalink / raw)
To: rdna; +Cc: netdev, ast, daniel, Joe Stringer, kernel-team
In-Reply-To: <20181107213607.2658130-1-rdna@fb.com>
On Wed, 7 Nov 2018 at 13:37, Andrey Ignatov <rdna@fb.com> wrote:
>
> Lookup functions in sk_lookup have different expectations about byte
> order of provided arguments.
>
> Specifically __inet_lookup, __udp4_lib_lookup and __udp6_lib_lookup
> expect dport to be in network byte order and do ntohs(dport) internally.
>
> At the same time __inet6_lookup expects dport to be in host byte order
> and correspondingly name the argument hnum.
>
> sk_lookup works correctly with __inet_lookup, __udp4_lib_lookup and
> __inet6_lookup with regard to dport. But in __udp6_lib_lookup case it
> uses host instead of expected network byte order. It makes result
> returned by bpf_sk_lookup_udp for IPv6 incorrect.
>
> The patch fixes byte order of dport passed to __udp6_lib_lookup.
>
> Originally sk_lookup properly handled UDPv6, but not TCPv6. 5ef0ae84f02a
> fixes TCPv6 but breaks UDPv6.
>
> Fixes: 5ef0ae84f02a ("bpf: Fix IPv6 dport byte-order in bpf_sk_lookup")
> Signed-off-by: Andrey Ignatov <rdna@fb.com>
Thanks for the fix, makes sense.
Acked-by: Joe Stringer <joe@wand.net.nz>
^ permalink raw reply
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
From: Alexei Starovoitov @ 2018-11-07 21:49 UTC (permalink / raw)
To: Edward Cree
Cc: Martin Lau, Yonghong Song, Alexei Starovoitov,
daniel@iogearbox.net, netdev@vger.kernel.org, Kernel Team
In-Reply-To: <2ea1e869-5f28-6beb-9fb1-15ae5a396077@solarflare.com>
On Wed, Nov 07, 2018 at 07:29:31PM +0000, Edward Cree wrote:
> Whereas I don't, and I don't feel like my core criticisms have
> been addressed _at all_. The only answer I get to "BTF should
> store type and instance information in separate records" is
> "it's a debuginfo",
...
> I am just trying to organise
> BTF to consist of separate _parts_ for types and instances,
> rather than forcing both into the same Procrustean bed.
BTF does not have and should not have instances.
BTF is debug info only.
This is not negotiable.
Loading BTF into the kernel does not create an instance of anything.
The kernel verifies BTF and BTF blob sits there waiting to be pointed at
by the actual instance.
Today these instances are maps and programs.
A single program can have multiple instances of the functions
which debug info is described in BTF.
Multiple prograrms (with multiple functions) can point
to the same BTF blob description.
Consider that in single C file there will be multliple functions,
many maps, global variables (in the future) they will be created
by individual prog_load/map_create syscalls.
Before that the _single_ blob of BTF data for the whole C file
will be loaded, since functions, maps share most of
the type and name information.
In elf files the instances of functions have names.
These names go into symbol table of the elf. Dynamic linker
is using these symbols to connect different instances.
In BPF world we use IDs to connect instances.
There is btd if, map id, prog id.
You can argue that bpf_prog_load has prog_name attribute.
So the instance of the program sort of have name already,
but this name is for debug only. It doesn't have to be unique
and the kernel is not relying on the name to connect instances.
Example:
1: int foo(struct __sk_buff *skb, char arg2)
2: {
3: return skb->len;
4: }
Line 1 goes into BTF to describe the function (with names and types).
Line 1 also goes into line_info .BTF.ext elf section as a string.
Line 3 and 4 go into line_info as well as strings.
llvm compiles it into something:
.text
.globl foo
.type foo, @function
foo:
r0 = *(u32 *)(r1 + 0)
exit
.section .BTF
.byte X
.byte Y
.section .BTF.ext
.byte X
.byte Y
That 'foo' name goes into symbol table and together with the body
form the future instance of the function.
libbpf takes 'foo' from symbol table of elf and passes it
as bpf_attr.prog_name to the kernel along with bpf instructions
to create an instance.
The name 'foo' in symbol table doesn't have to match with
BTF 'foo' name and '.*foo.*' substring in line_info.
If you decide to use the sequence:
"""
.globl foo
.type foo, @function
foo:
""
in your ebpf_asm to parse and store into BTF as a function name
I would strongly argue against, since it would be incorrectly
conflating the instance of the function with debug purpose
of BTF.
Right now we do not have a concrete proposal for assembler text of BTF.
When LLVM emits BTF into .s it emits them as raw bytes.
So I'm looking forward to your ideas how to describe BTF in .s
Note such .s must have freedom to describe 'int bar(struct __sk_buff *a1, char a2)'
as debug info while having '.globl foo; foo:' as symbol name.
Your other 'criticism' was about libbpf's bpf_map_find_btf_info()
and ____btf_map_* hack. Yes. It is a hack and I'm open to change it
if there are better suggestions. It's a convention between
libbpf and program writers that produce elf. It's not a kernel abi.
Nothing to do with BTF and this instance vs debug info discussion.
> (I don't feel like we're making progress in understanding one
> another here; maybe we should have a telephone discussion?
> Sadly I'm not going to Plumbers, else that would be the
> perfect place to thrash this out.)
Happy to jump on the call to explain it again.
10:30am pacific time works for me tomorrow.
^ permalink raw reply
* Re: [PATCH bpf-next] xdp: sample code for redirecting vlan packets to specific cpus
From: Daniel Borkmann @ 2018-11-07 21:44 UTC (permalink / raw)
To: John Fastabend, Shannon Nelson, ast
Cc: netdev, eric.dumazet, silviu.smarandache, shannon.lee.nelson
In-Reply-To: <499cc6e8-f533-3b90-b053-b3a1c7fa3a15@gmail.com>
On 10/29/2018 11:11 PM, John Fastabend wrote:
> On 10/29/2018 02:19 PM, Shannon Nelson wrote:
>> This is an example of using XDP to redirect the processing of
>> particular vlan packets to specific CPUs. This is in response
>> to comments received on a kernel patch put forth previously
>> to do something similar using RPS.
>> https://www.spinics.net/lists/netdev/msg528210.html
>> [PATCH net-next] net: enable RPS on vlan devices
>>
>> This XDP application watches for inbound vlan-tagged packets
>> and redirects those packets to be processed on a specific CPU
>> as configured in a BPF map. The BPF map can be modified by
>> this user program, which can also load and unload the kernel
>> XDP code.
>>
>> One example use is for supporting VMs where we can't control the
>> OS being used: we'd like to separate the VM CPU processing from
>> the host's CPUs as a way to help mitigate L1TF related issues.
>> When running the VM's traffic on a vlan we can stick the host's
>> Rx processing on one set of CPUs separate from the VM's CPUs.
>>
>> This example currently uses a vlan key and cpu value in the
>> BPF map, so only can do one CPU per vlan. This could easily
>> be modified to use a bitpattern of CPUs rather than a CPU id
>> to allow multiple CPUs per vlan.
>
> Great, so does this solve your use case then? At least on drivers
> with XDP support?
>
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>> ---
>
> Some really small and trivial nits below.
>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
>
> [...]
>
>> + if (install) {
>> +
>
> new line probably not needed.
>
>> + /* check to see if already installed */
>> + errno = 0;
>> + access(pin_prog_name, R_OK);
>> + if (errno != ENOENT) {
>> + fprintf(stderr, "ERR: %s is already installed\n", argv[0]);
>> + return -1;
>> + }
>> +
>> + /* load the XDP program and maps with the convenient library */
>> + if (load_bpf_file(filename)) {
>> + fprintf(stderr, "ERR: load_bpf_file(%s): \n%s",
>> + filename, bpf_log_buf);
>> + return -1;
>> + }
>> + if (!prog_fd[0]) {
>> + fprintf(stderr, "ERR: load_bpf_file(%s): %d %s\n",
>> + filename, errno, strerror(errno));
>> + return -1;
>> + }
>> +
>> + /* pin the XDP program and maps */
>> + if (bpf_obj_pin(prog_fd[0], pin_prog_name) < 0) {
>> + fprintf(stderr, "ERR: bpf_obj_pin(%s): %d %s\n",
>> + pin_prog_name, errno, strerror(errno));
>> + if (errno == 2)
>> + fprintf(stderr, " (is the BPF fs mounted on /sys/fs/bpf?)\n");
>> + return -1;
>> + }
>> + if (bpf_obj_pin(map_fd[0], pin_vlanmap_name) < 0) {
>> + fprintf(stderr, "ERR: bpf_obj_pin(%s): %d %s\n",
>> + pin_vlanmap_name, errno, strerror(errno));
>> + return -1;
>> + }
>> + if (bpf_obj_pin(map_fd[2], pin_countermap_name) < 0) {
>> + fprintf(stderr, "ERR: bpf_obj_pin(%s): %d %s\n",
>> + pin_countermap_name, errno, strerror(errno));
>> + return -1;
>> + }
>> +
>> + /* prep the vlan map with "not used" values */
>> + c64 = UNDEF_CPU;
>> + for (v64 = 0; v64 < 4096; v64++) {
>
> maybe #define MAX_VLANS 4096 just to avoid constants.
>
>> + if (bpf_map_update_elem(map_fd[0], &v64, &c64, 0)) {
>> + fprintf(stderr, "ERR: preping vlan map failed on v=%llu: %d %s\n",
>> + v64, errno, strerror(errno));
>> + return -1;
>> + }
>> + }
>> +
>> + /* prep the cpumap with queue sizes */
>> + c64 = 128+64; /* see note in xdp_redirect_cpu_user.c */
>> + for (v64 = 0; v64 < MAX_CPUS; v64++) {
>> + if (bpf_map_update_elem(map_fd[1], &v64, &c64, 0)) {
>> + if (errno == ENODEV) {
>> + /* Save the last CPU number attempted
>> + * into the counters map
>> + */
>> + c64 = CPU_COUNT;
>> + ret = bpf_map_update_elem(map_fd[2], &c64, &v64, 0);
>> + break;
>> + }
>> +
>> + fprintf(stderr, "ERR: preping cpu map failed on v=%llu: %d %s\n",
>> + v64, errno, strerror(errno));
>> + return -1;
>> + }
>> + }
>> +
>> + /* wire the XDP program to the device */
>> + if (bpf_set_link_xdp_fd(ifindex, prog_fd[0], 0) < 0) {
>> + fprintf(stderr, "ERR: bpf_set_link_xdp_fd(): %d %s\n",
>> + errno, strerror(errno));
>> + return -1;
>> + }
>> +
>> + return 0;
>> + }
>> +
>> + if (remove) {
>> +
Ditto
>> + /* unlink the program from the device */
>> + if (bpf_set_link_xdp_fd(ifindex, -1, 0) < 0)
>> + fprintf(stderr, "ERR: bpf_set_link_xdp_fd(): %d %s\n",
>> + errno, strerror(errno));
>> +
>> + /* unlink pinned files */
>> + if (unlink(pin_prog_name))
>> + fprintf(stderr, "ERR: unlink(%s): %d %s\n",
>> + pin_prog_name, errno, strerror(errno));
>> + if (unlink(pin_vlanmap_name))
>> + fprintf(stderr, "ERR: unlink(%s): %d %s\n",
>> + pin_vlanmap_name, errno, strerror(errno));
>> + if (unlink(pin_countermap_name))
>> + fprintf(stderr, "ERR: unlink(%s): %d %s\n",
>> + pin_countermap_name, errno, strerror(errno));
>> +
>> + return 0;
>> + }
>> +
>> + if (vlan == 0) {
>> + fprintf(stderr, "ERR: required option --vlan missing\n");
>> + goto error;
>> + }
>> +
>> + if (cpu == MAX_CPUS && vlan > 0) {
>> + fprintf(stderr, "ERR: required option --cpu missing\n");
>> + goto error;
>> + }
>> +
>> + vfd = bpf_obj_get(pin_vlanmap_name);
>> + if (vfd < 0) {
>> + fprintf(stderr, "ERR: can't find pinned map %s: %d %s\n",
>> + pin_vlanmap_name, errno, strerror(errno));
>> + if (errno == ENOENT)
>> + fprintf(stderr, " (has %s been installed yet?)\n", argv[0]);
>> + return -1;
>> + }
>> +
>> + /* decode the requested action */
>> + if (vlan > 0) {
>> + /* check cpu against the max value found */
>> + cfd = bpf_obj_get(pin_countermap_name);
>> + if (cfd < 0) {
>> + fprintf(stderr, "ERR: can't find pinned map %s: %d %s\n",
>> + pin_countermap_name, errno, strerror(errno));
>> + return -1;
>> + }
>> + c64 = CPU_COUNT;
>> + ret = bpf_map_lookup_elem(cfd, &c64, &v64);
>> + if (cpu >= v64) {
>
> Need to check ret code?
Yeah, I expect this would have another respin, thanks!
>> + fprintf(stderr, "ERR: cpu %d greater than max %llu\n", cpu, v64);
>> + return -1;
>> + }
>> +
>> + /* Note that the value and key pointers really do need to be
>> + * pointers to 64-bit values, else things get a bit muddled.
>> + */
>> + v64 = vlan;
>> + c64 = cpu;
>> + ret = bpf_map_update_elem(vfd, &v64, &c64, 0);
>> + if (ret) {
>> + fprintf(stderr, "Adding vlan %d CPU %d failed: %d %s\n",
>> + vlan, cpu, errno, strerror(errno));
>> + return -1;
>> + }
>> +
>> + } else {
>> + v64 = -vlan;
>> + c64 = UNDEF_CPU;
>> +
>> + /* We can't actually delete from a TYPE_ARRAY map, so we
>> + * simply set it to an undefined value.
>> + */
>> + ret = bpf_map_update_elem(vfd, &v64, &c64, 0);
>> + if (ret) {
>> + fprintf(stderr, "Delete of vlan %llu failed: %d %s\n",
>> + v64, errno, strerror(errno));
>> + return -1;
>> + }
>> + }
>> +
>> + return 0;
>> +
>> +error:
>> + usage(argv);
>> + return -1;
>> +}
>>
>
^ permalink raw reply
* Re: [PATCH bpf-next] bpf_load: add map name to load_maps error message
From: Daniel Borkmann @ 2018-11-07 21:41 UTC (permalink / raw)
To: Song Liu, John Fastabend
Cc: shannon.nelson, Alexei Starovoitov, Networking,
shannon.lee.nelson
In-Reply-To: <CAPhsuW4-zJ=2QPUW7rz-rq3KHm_YJ3SnPTHVbddY5tZi3TjEwg@mail.gmail.com>
On 11/07/2018 01:26 AM, Song Liu wrote:
> On Mon, Oct 29, 2018 at 3:12 PM John Fastabend <john.fastabend@gmail.com> wrote:
>>
>> On 10/29/2018 02:14 PM, Shannon Nelson wrote:
>>> To help when debugging bpf/xdp load issues, have the load_map()
>>> error message include the number and name of the map that
>>> failed.
>>>
>>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>>> ---
>>> samples/bpf/bpf_load.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
>>> index 89161c9..5de0357 100644
>>> --- a/samples/bpf/bpf_load.c
>>> +++ b/samples/bpf/bpf_load.c
>>> @@ -282,8 +282,8 @@ static int load_maps(struct bpf_map_data *maps, int nr_maps,
>>> numa_node);
>>> }
>>> if (map_fd[i] < 0) {
>>> - printf("failed to create a map: %d %s\n",
>>> - errno, strerror(errno));
>>> + printf("failed to create map %d (%s): %d %s\n",
>>> + i, maps[i].name, errno, strerror(errno));
>>> return 1;
>>> }
>>> maps[i].fd = map_fd[i];
>>>
>>
>> LGTM
>>
>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>
> Acked-by: Song Liu <songliubraving@fb.com>
Applied to bpf-next, thanks!
^ permalink raw reply
* [PATCH bpf] bpf: Fix IPv6 dport byte order in bpf_sk_lookup_udp
From: Andrey Ignatov @ 2018-11-07 21:36 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, joe, kernel-team
Lookup functions in sk_lookup have different expectations about byte
order of provided arguments.
Specifically __inet_lookup, __udp4_lib_lookup and __udp6_lib_lookup
expect dport to be in network byte order and do ntohs(dport) internally.
At the same time __inet6_lookup expects dport to be in host byte order
and correspondingly name the argument hnum.
sk_lookup works correctly with __inet_lookup, __udp4_lib_lookup and
__inet6_lookup with regard to dport. But in __udp6_lib_lookup case it
uses host instead of expected network byte order. It makes result
returned by bpf_sk_lookup_udp for IPv6 incorrect.
The patch fixes byte order of dport passed to __udp6_lib_lookup.
Originally sk_lookup properly handled UDPv6, but not TCPv6. 5ef0ae84f02a
fixes TCPv6 but breaks UDPv6.
Fixes: 5ef0ae84f02a ("bpf: Fix IPv6 dport byte-order in bpf_sk_lookup")
Signed-off-by: Andrey Ignatov <rdna@fb.com>
---
net/core/filter.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index e521c5ebc7d1..9a1327eb25fa 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4852,18 +4852,17 @@ static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
} else {
struct in6_addr *src6 = (struct in6_addr *)&tuple->ipv6.saddr;
struct in6_addr *dst6 = (struct in6_addr *)&tuple->ipv6.daddr;
- u16 hnum = ntohs(tuple->ipv6.dport);
int sdif = inet6_sdif(skb);
if (proto == IPPROTO_TCP)
sk = __inet6_lookup(net, &tcp_hashinfo, skb, 0,
src6, tuple->ipv6.sport,
- dst6, hnum,
+ dst6, ntohs(tuple->ipv6.dport),
dif, sdif, &refcounted);
else if (likely(ipv6_bpf_stub))
sk = ipv6_bpf_stub->udp6_lib_lookup(net,
src6, tuple->ipv6.sport,
- dst6, hnum,
+ dst6, tuple->ipv6.dport,
dif, sdif,
&udp_table, skb);
#endif
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
From: Quentin Monnet @ 2018-11-07 21:34 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
guro, jiong.wang, bhole_prashant_q7, john.fastabend, jbenc,
treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107123231.6d2f4782@cakuba.netronome.com>
2018-11-07 12:32 UTC-0800 ~ Jakub Kicinski <jakub.kicinski@netronome.com>
> On Wed, 7 Nov 2018 20:08:53 +0000, Quentin Monnet wrote:
>>> + err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
>>> + if (err) {
>>> + p_err("failed to pin program %s",
>>> + bpf_program__title(prog, false));
>>> + goto err_close_obj;
>>> + }
>>
>> I don't have the same opinion as Jakub for pinning :). I was hoping we
>> could also load additional programs (for tail calls) for
>> non-flow_dissector programs. Could this be an occasion to update the
>> code in that direction?
>
> Do you mean having the bpftool construct an array for tail calling
> automatically when loading an object? Or do a "mass pin" of all
> programs in an object file?
>
> I'm not convinced about this strategy of auto assembling a tail call
> array by assuming that a flow dissector object carries programs for
> protocols in order (apart from the main program which doesn't have to
> be first, for some reason).
Not constructing the prog array, I don't think this should be the role
of bpftool either. Much more a "mass pin", so that you have a link to
each program loaded from the object file and can later add them to a
prog array map with subsequent calls to bpftool.
^ permalink raw reply
* Re: [PATCH bpf-next 2/2] selftests/bpf: add a test case for sock_ops perf-event notification
From: Daniel Borkmann @ 2018-11-07 21:31 UTC (permalink / raw)
To: Sowmini Varadhan, netdev, davem
In-Reply-To: <09992f2af846ab5186cf5d8c3e4e1c5d20d1b747.1541535172.git.sowmini.varadhan@oracle.com>
On 11/06/2018 09:28 PM, Sowmini Varadhan wrote:
> This patch provides a tcp_bpf based eBPF sample. The test
> - ncat(1) as the TCP client program to connect() to a port
> with the intention of triggerring SYN retransmissions: we
> first install an iptables DROP rule to make sure ncat SYNs are
> resent (instead of aborting instantly after a TCP RST)
> - has a bpf kernel module that sends a perf-event notification for
> each TCP retransmit, and also tracks the number of such notifications
> sent in the global_map
> The test passes when the number of event notifications intercepted
> in user-space matches the value in the global_map.
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
> tools/testing/selftests/bpf/Makefile | 4 +-
> tools/testing/selftests/bpf/perf-sys.h | 74 ++++++++
> tools/testing/selftests/bpf/test_tcpnotify.h | 19 ++
> tools/testing/selftests/bpf/test_tcpnotify_kern.c | 95 +++++++++++
> tools/testing/selftests/bpf/test_tcpnotify_user.c | 186 +++++++++++++++++++++
> 5 files changed, 377 insertions(+), 1 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/perf-sys.h
> create mode 100644 tools/testing/selftests/bpf/test_tcpnotify.h
> create mode 100644 tools/testing/selftests/bpf/test_tcpnotify_kern.c
> create mode 100644 tools/testing/selftests/bpf/test_tcpnotify_user.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index e39dfb4..6c94048 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -24,12 +24,13 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
> test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
> test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
> test_socket_cookie test_cgroup_storage test_select_reuseport test_section_names \
> - test_netcnt
> + test_netcnt test_tcpnotify_user
>
> TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
> test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
> sockmap_verdict_prog.o dev_cgroup.o sample_ret0.o test_tracepoint.o \
> test_l4lb_noinline.o test_xdp_noinline.o test_stacktrace_map.o \
> + test_tcpnotify_kern.o \
> sample_map_ret0.o test_tcpbpf_kern.o test_stacktrace_build_id.o \
> sockmap_tcp_msg_prog.o connect4_prog.o connect6_prog.o test_adjust_tail.o \
> test_btf_haskv.o test_btf_nokv.o test_sockmap_kern.o test_tunnel_kern.o \
> @@ -74,6 +75,7 @@ $(OUTPUT)/test_sock_addr: cgroup_helpers.c
> $(OUTPUT)/test_socket_cookie: cgroup_helpers.c
> $(OUTPUT)/test_sockmap: cgroup_helpers.c
> $(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
> +$(OUTPUT)/test_tcpnotify_user: cgroup_helpers.c trace_helpers.c
> $(OUTPUT)/test_progs: trace_helpers.c
> $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
> $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
> diff --git a/tools/testing/selftests/bpf/perf-sys.h b/tools/testing/selftests/bpf/perf-sys.h
> new file mode 100644
> index 0000000..3eb7a39
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/perf-sys.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _PERF_SYS_H
> +#define _PERF_SYS_H
> +
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/syscall.h>
> +#include <linux/types.h>
> +#include <linux/compiler.h>
> +#include <linux/perf_event.h>
> +#include <asm/barrier.h>
> +
> +#ifdef __powerpc__
> +#define CPUINFO_PROC {"cpu"}
> +#endif
> +
> +#ifdef __s390__
> +#define CPUINFO_PROC {"vendor_id"}
> +#endif
> +
> +#ifdef __sh__
> +#define CPUINFO_PROC {"cpu type"}
> +#endif
> +
> +#ifdef __hppa__
> +#define CPUINFO_PROC {"cpu"}
> +#endif
> +
> +#ifdef __sparc__
> +#define CPUINFO_PROC {"cpu"}
> +#endif
> +
> +#ifdef __alpha__
> +#define CPUINFO_PROC {"cpu model"}
> +#endif
> +
> +#ifdef __arm__
> +#define CPUINFO_PROC {"model name", "Processor"}
> +#endif
> +
> +#ifdef __mips__
> +#define CPUINFO_PROC {"cpu model"}
> +#endif
> +
> +#ifdef __arc__
> +#define CPUINFO_PROC {"Processor"}
> +#endif
> +
> +#ifdef __xtensa__
> +#define CPUINFO_PROC {"core ID"}
> +#endif
> +
> +#ifndef CPUINFO_PROC
> +#define CPUINFO_PROC { "model name", }
> +#endif
> +
> +static inline int
> +sys_perf_event_open(struct perf_event_attr *attr,
> + pid_t pid, int cpu, int group_fd,
> + unsigned long flags)
> +{
> + int fd;
> +
> + fd = syscall(__NR_perf_event_open, attr, pid, cpu,
> + group_fd, flags);
> +
> +#ifdef HAVE_ATTR_TEST
> + if (unlikely(test_attr__enabled))
> + test_attr__open(attr, pid, cpu, fd, group_fd, flags);
> +#endif
> + return fd;
> +}
I would prefer if we could avoid adding whole perf-sys duplicate right
into BPF kselftest directory. Agree it would be nice to have the mini
wrapper somewhere, but then lets make that a separate commit and place
the wrapper-only somewhere as tools/include/linux/perf.h that all the
remaining occurrences below can be replaced with.
$ git grep -n __NR_perf_event_open tools/
[...]
tools/testing/selftests/bpf/get_cgroup_id_user.c:112: pmu_fd = syscall(__NR_perf_event_open, &attr, getpid(), -1, -1, 0);
tools/testing/selftests/bpf/test_progs.c:799: pmu_fd[i] = syscall(__NR_perf_event_open, &attr, -1 /* pid */,
tools/testing/selftests/bpf/test_progs.c:977: pmu_fd = syscall(__NR_perf_event_open, &attr, -1 /* pid */,
tools/testing/selftests/bpf/test_progs.c:1163: pmu_fd = syscall(__NR_perf_event_open, &attr, -1 /* pid */,
tools/testing/selftests/bpf/test_progs.c:1297: pmu_fd = syscall(__NR_perf_event_open, &attr, -1 /* pid */,
tools/testing/selftests/bpf/test_progs.c:1510: pmu_fd = syscall(__NR_perf_event_open, &attr, getpid()/*pid*/, -1/*cpu*/,
tools/testing/selftests/bpf/test_progs.c:1653: pmu_fd = syscall(__NR_perf_event_open, &attr, -1 /* pid */,
tools/testing/selftests/powerpc/pmu/event.c:19: return syscall(__NR_perf_event_open, attr, pid, cpu,
tools/testing/selftests/powerpc/ptrace/perf-hwbreak.c:46: return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd, flags);
tools/testing/selftests/powerpc/utils.c:177: return syscall(__NR_perf_event_open, hw_event, pid, cpu,
> +#endif /* _PERF_SYS_H */
> diff --git a/tools/testing/selftests/bpf/test_tcpnotify.h b/tools/testing/selftests/bpf/test_tcpnotify.h
> new file mode 100644
> index 0000000..8b6cea0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_tcpnotify.h
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#ifndef _TEST_TCPBPF_H
> +#define _TEST_TCPBPF_H
> +
> +struct tcpnotify_globals {
> + __u32 total_retrans;
> + __u32 ncalls;
> +};
> +
> +struct tcp_notifier {
> + __u8 type;
> + __u8 subtype;
> + __u8 source;
> + __u8 hash;
> +};
> +
> +#define TESTPORT 12877
> +#endif
> diff --git a/tools/testing/selftests/bpf/test_tcpnotify_kern.c b/tools/testing/selftests/bpf/test_tcpnotify_kern.c
> new file mode 100644
> index 0000000..edbca20
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_tcpnotify_kern.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stddef.h>
> +#include <string.h>
> +#include <linux/bpf.h>
> +#include <linux/if_ether.h>
> +#include <linux/if_packet.h>
> +#include <linux/ip.h>
> +#include <linux/ipv6.h>
> +#include <linux/types.h>
> +#include <linux/socket.h>
> +#include <linux/tcp.h>
> +#include <netinet/in.h>
> +#include "bpf_helpers.h"
> +#include "bpf_endian.h"
> +#include "test_tcpnotify.h"
> +
> +struct bpf_map_def SEC("maps") global_map = {
> + .type = BPF_MAP_TYPE_ARRAY,
> + .key_size = sizeof(__u32),
> + .value_size = sizeof(struct tcpnotify_globals),
> + .max_entries = 4,
> +};
> +
> +struct bpf_map_def SEC("maps") perf_event_map = {
> + .type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> + .key_size = sizeof(int),
> + .value_size = sizeof(__u32),
> + .max_entries = 2,
> +};
> +
> +int _version SEC("version") = 1;
> +
> +SEC("sockops")
> +int bpf_testcb(struct bpf_sock_ops *skops)
> +{
> + int rv = -1;
> + int op;
> +
> + op = (int) skops->op;
> +
> + if (bpf_ntohl(skops->remote_port) != TESTPORT) {
> + skops->reply = -1;
> + return 0;
> + }
> +
> + switch (op) {
> + case BPF_SOCK_OPS_TIMEOUT_INIT:
> + case BPF_SOCK_OPS_RWND_INIT:
> + case BPF_SOCK_OPS_NEEDS_ECN:
> + case BPF_SOCK_OPS_BASE_RTT:
> + case BPF_SOCK_OPS_RTO_CB:
> + rv = 1;
> + break;
> +
> + case BPF_SOCK_OPS_TCP_CONNECT_CB:
> + case BPF_SOCK_OPS_TCP_LISTEN_CB:
> + case BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB:
> + case BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB:
> + bpf_sock_ops_cb_flags_set(skops, (BPF_SOCK_OPS_RETRANS_CB_FLAG|
> + BPF_SOCK_OPS_RTO_CB_FLAG));
> + rv = 1;
> + break;
> + case BPF_SOCK_OPS_RETRANS_CB: {
> + __u32 key = 0;
> + struct tcpnotify_globals g, *gp;
> + struct tcp_notifier msg = {
> + .type = 0xde,
> + .subtype = 0xad,
> + .source = 0xbe,
> + .hash = 0xef,
> + };
> +
> + rv = 1;
> +
> + /* Update results */
> + gp = bpf_map_lookup_elem(&global_map, &key);
> + if (!gp)
> + break;
> + g = *gp;
> + g.total_retrans = skops->total_retrans;
> + g.ncalls++;
> + bpf_map_update_elem(&global_map, &key, &g,
> + BPF_ANY);
> + bpf_perf_event_output(skops, &perf_event_map,
> + BPF_F_CURRENT_CPU,
> + &msg, sizeof(msg));
> + }
> + break;
> + default:
> + rv = -1;
> + }
> + skops->reply = rv;
> + return 1;
> +}
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_tcpnotify_user.c b/tools/testing/selftests/bpf/test_tcpnotify_user.c
> new file mode 100644
> index 0000000..8f88cb9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_tcpnotify_user.c
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <pthread.h>
> +#include <inttypes.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <asm/types.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <linux/bpf.h>
> +#include <sys/socket.h>
> +#include <bpf/bpf.h>
> +#include <bpf/libbpf.h>
> +#include <sys/ioctl.h>
> +#include <linux/rtnetlink.h>
> +#include <signal.h>
> +#include <linux/perf_event.h>
> +#include "perf-sys.h"
> +
> +#include "bpf_rlimit.h"
> +#include "bpf_util.h"
> +#include "cgroup_helpers.h"
> +
> +#include "test_tcpnotify.h"
> +#include "trace_helpers.h"
> +
> +#define SOCKET_BUFFER_SIZE (getpagesize() < 8192L ? getpagesize() : 8192L)
> +
> +pthread_t tid;
> +int rx_callbacks;
> +
> +static int dummyfn(void *data, int size)
> +{
> + struct tcp_notifier *t = data;
> +
> + if (t->type != 0xde || t->subtype != 0xad ||
> + t->source != 0xbe || t->hash != 0xef)
> + return 1;
> + rx_callbacks++;
> + return 0;
> +}
> +
> +void tcp_notifier_poller(int fd)
> +{
> + while (1)
> + perf_event_poller(fd, dummyfn);
> +}
> +
> +static void *poller_thread(void *arg)
> +{
> + int fd = *(int *)arg;
> +
> + tcp_notifier_poller(fd);
> + return arg;
> +}
> +
> +int verify_result(const struct tcpnotify_globals *result)
> +{
> + return (result->ncalls > 0 && result->ncalls == rx_callbacks ? 0 : 1);
> +}
> +
> +static int bpf_find_map(const char *test, struct bpf_object *obj,
> + const char *name)
> +{
> + struct bpf_map *map;
> +
> + map = bpf_object__find_map_by_name(obj, name);
> + if (!map) {
> + printf("%s:FAIL:map '%s' not found\n", test, name);
> + return -1;
> + }
> + return bpf_map__fd(map);
> +}
> +
> +static int setup_bpf_perf_event(int mapfd)
> +{
> + struct perf_event_attr attr = {
> + .sample_type = PERF_SAMPLE_RAW,
> + .type = PERF_TYPE_SOFTWARE,
> + .config = PERF_COUNT_SW_BPF_OUTPUT,
> + };
> + int key = 0;
> + int pmu_fd;
> +
> + pmu_fd = sys_perf_event_open(&attr, -1, 0, -1, 0);
> + if (pmu_fd < 0)
> + return pmu_fd;
> + bpf_map_update_elem(mapfd, &key, &pmu_fd, BPF_ANY);
> +
> + ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
> + return pmu_fd;
> +}
> +
> +int main(int argc, char **argv)
> +{
> + const char *file = "test_tcpnotify_kern.o";
> + int prog_fd, map_fd, perf_event_fd;
> + struct tcpnotify_globals g = {0};
> + const char *cg_path = "/foo";
> + int error = EXIT_FAILURE;
> + struct bpf_object *obj;
> + int cg_fd = -1;
> + __u32 key = 0;
> + int rv;
> + char test_script[80];
> + int pmu_fd;
> + cpu_set_t cpuset;
> +
> + CPU_ZERO(&cpuset);
> + CPU_SET(0, &cpuset);
> + pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t), &cpuset);
> +
> + if (setup_cgroup_environment())
> + goto err;
> +
> + cg_fd = create_and_get_cgroup(cg_path);
> + if (!cg_fd)
> + goto err;
> +
> + if (join_cgroup(cg_path))
> + goto err;
> +
> + if (bpf_prog_load(file, BPF_PROG_TYPE_SOCK_OPS, &obj, &prog_fd)) {
> + printf("FAILED: load_bpf_file failed for: %s\n", file);
> + goto err;
> + }
> +
> + rv = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_SOCK_OPS, 0);
> + if (rv) {
> + printf("FAILED: bpf_prog_attach: %d (%s)\n",
> + error, strerror(errno));
> + goto err;
> + }
> +
> + perf_event_fd = bpf_find_map(__func__, obj, "perf_event_map");
> + if (perf_event_fd < 0)
> + goto err;
> +
> + map_fd = bpf_find_map(__func__, obj, "global_map");
> + if (map_fd < 0)
> + goto err;
> +
> + pmu_fd = setup_bpf_perf_event(perf_event_fd);
> + if (pmu_fd < 0 || perf_event_mmap(pmu_fd) < 0)
> + goto err;
> +
> + pthread_create(&tid, NULL, poller_thread, (void *)&pmu_fd);
> +
> + sprintf(test_script,
> + "/usr/sbin/iptables -A INPUT -p tcp --dport %d -j DROP",
> + TESTPORT);
> + system(test_script);
> +
> + sprintf(test_script,
> + "/usr/bin/nc 127.0.0.1 %d < /etc/passwd > /dev/null 2>&1 ",
> + TESTPORT);
> + system(test_script);
> +
> + sprintf(test_script,
> + "/usr/sbin/iptables -D INPUT -p tcp --dport %d -j DROP",
> + TESTPORT);
> + system(test_script);
> +
> + rv = bpf_map_lookup_elem(map_fd, &key, &g);
> + if (rv != 0) {
> + printf("FAILED: bpf_map_lookup_elem returns %d\n", rv);
> + goto err;
> + }
> +
> + sleep(10);
> +
> + if (verify_result(&g)) {
> + printf("FAILED: Wrong stats Expected %d calls, got %d\n",
> + g.ncalls, rx_callbacks);
> + goto err;
> + }
> +
> + printf("PASSED!\n");
> + error = 0;
> +err:
> + bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
> + close(cg_fd);
> + cleanup_cgroup_environment();
> + return error;
> +}
>
^ permalink raw reply
* Re: [PATCH bpf-next] tools: bpftool: adjust rlimit RLIMIT_MEMLOCK when loading programs, maps
From: Daniel Borkmann @ 2018-11-07 21:24 UTC (permalink / raw)
To: Quentin Monnet, Alexei Starovoitov; +Cc: netdev, oss-drivers
In-Reply-To: <1541593770-28089-1-git-send-email-quentin.monnet@netronome.com>
On 11/07/2018 01:29 PM, Quentin Monnet wrote:
> The limit for memory locked in the kernel by a process is usually set to
> 64 bytes by default. This can be an issue when creating large BPF maps
> and/or loading many programs. A workaround is to raise this limit for
> the current process before trying to create a new BPF map. Changing the
> hard limit requires the CAP_SYS_RESOURCE and can usually only be done by
> root user (for non-root users, a call to setrlimit fails (and sets
> errno) and the program simply goes on with its rlimit unchanged).
>
> There is no API to get the current amount of memory locked for a user,
> therefore we cannot raise the limit only when required. One solution,
> used by bcc, is to try to create the map, and on getting a EPERM error,
> raising the limit to infinity before giving another try. Another
> approach, used in iproute2, is to raise the limit in all cases, before
> trying to create the map.
>
> Here we do the same as in iproute2: the rlimit is raised to infinity
> before trying to load programs or to create maps with bpftool.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Applied to bpf-next and fixed commit log, thanks!
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: enable (uncomment) all tests in test_libbpf.sh
From: Daniel Borkmann @ 2018-11-07 21:24 UTC (permalink / raw)
To: Quentin Monnet, Alexei Starovoitov
Cc: netdev, oss-drivers, Jesper Dangaard Brouer
In-Reply-To: <1541593725-27703-1-git-send-email-quentin.monnet@netronome.com>
On 11/07/2018 01:28 PM, Quentin Monnet wrote:
> libbpf is now able to load successfully test_l4lb_noinline.o and
> samples/bpf/tracex3_kern.o.
>
> For the test_l4lb_noinline, uncomment related tests from test_libbpf.c
> and remove the associated "TODO".
>
> For tracex3_kern.o, instead of loading a program from samples/bpf/ that
> might not have been compiled at this stage, try loading a program from
> BPF selftests. Since this test case is about loading a program compiled
> without the "-target bpf" flag, change the Makefile to compile one
> program accordingly (instead of passing the flag for compiling all
> programs).
>
> Regarding test_xdp_noinline.o: in its current shape the program fails to
> load because it provides no version section, but the loader needs one.
> The test was added to make sure that libbpf could load XDP programs even
> if they do not provide a version number in a dedicated section. But
> libbpf is already capable of doing that: in our case loading fails
> because the loader does not know that this is an XDP program (it does
> not need to, since it does not attach the program). So trying to load
> test_xdp_noinline.o does not bring much here: just delete this subtest.
>
> For the record, the error message obtained with tracex3_kern.o was
> fixed by commit e3d91b0ca523 ("tools/libbpf: handle issues with bpf ELF
> objects containing .eh_frames")
>
> I have not been abled to reproduce the "libbpf: incorrect bpf_call
> opcode" error for test_l4lb_noinline.o, even with the version of libbpf
> present at the time when test_libbpf.sh and test_libbpf_open.c were
> created.
>
> RFC -> v1:
> - Compile test_xdp without the "-target bpf" flag, and try to load it
> instead of ../../samples/bpf/tracex3_kern.o.
> - Delete test_xdp_noinline.o subtest.
>
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Applied to bpf-next, thanks!
^ permalink raw reply
* Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
From: Stanislav Fomichev @ 2018-11-07 21:17 UTC (permalink / raw)
To: Quentin Monnet
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
jakub.kicinski, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <484ea7f5-0d4c-b828-c5fe-1fd4d9bf847d@netronome.com>
On 11/07, Quentin Monnet wrote:
> Hi Stanislav,
>
> 2018-11-07 11:35 UTC-0800 ~ Stanislav Fomichev <sdf@google.com>
> > This commit adds support for loading/attaching/detaching flow
> > dissector program. The structure of the flow dissector program is
> > assumed to be the same as in the selftests:
> >
> > * flow_dissector section with the main entry point
> > * a bunch of tail call progs
> > * a jmp_table map that is populated with the tail call progs
> >
> > When `bpftool load` is called with a flow_dissector prog (i.e. when the
> > first section is flow_dissector of 'type flow_dissector' argument is
> > passed), we load and pin all the programs and build the jump table.
> >
> > The last argument of `bpftool attach` is made optional for this use
> > case.
> >
> > Example:
> > bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
> > /sys/fs/bpf/flow type flow_dissector
> > bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
> >
> > Tested by using the above two lines to load the prog in
> > the test_flow_dissector.sh selftest.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > .../bpftool/Documentation/bpftool-prog.rst | 16 ++-
> > tools/bpf/bpftool/common.c | 32 +++--
> > tools/bpf/bpftool/main.h | 1 +
> > tools/bpf/bpftool/prog.c | 135 +++++++++++++++---
> > 4 files changed, 141 insertions(+), 43 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > index ac4e904b10fb..3caa9153435b 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > @@ -25,8 +25,8 @@ MAP COMMANDS
> > | **bpftool** **prog dump jited** *PROG* [{**file** *FILE* | **opcodes**}]
> > | **bpftool** **prog pin** *PROG* *FILE*
> > | **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > -| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > -| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
> > +| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> > +| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> > | **bpftool** **prog help**
> > |
> > | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> > @@ -39,7 +39,9 @@ MAP COMMANDS
> > | **cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** | **cgroup/post_bind6** |
> > | **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4** | **cgroup/sendmsg6**
> > | }
> > -| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** | **skb_parse** }
> > +| *ATTACH_TYPE* := {
> > +| | **msg_verdict** | **skb_verdict** | **skb_parse** | **flow_dissector**
>
> ^
> Nitpick: Could you please remove the above pipe?
Ah, I missed that one, my bad, thanks.
> > +| }
> >
> >
> > DESCRIPTION
> > @@ -97,13 +99,13 @@ DESCRIPTION
> > contain a dot character ('.'), which is reserved for future
> > extensions of *bpffs*.
> >
> > - **bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > + **bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> > Attach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> > - to the map *MAP*.
> > + to the optional map *MAP*.
> >
> > - **bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
> > + **bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> > Detach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> > - from the map *MAP*.
> > + from the optional map *MAP*.
> >
> > **bpftool prog help**
> > Print short help message.
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 25af85304ebe..963881142dfb 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> > @@ -169,34 +169,24 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type)
> > return fd;
> > }
> >
> > -int do_pin_fd(int fd, const char *name)
> > +int mount_bpffs_for_pin(const char *name)
> > {
> > char err_str[ERR_MAX_LEN];
> > char *file;
> > char *dir;
> > int err = 0;
> >
> > - err = bpf_obj_pin(fd, name);
> > - if (!err)
> > - goto out;
> > -
> > file = malloc(strlen(name) + 1);
> > strcpy(file, name);
> > dir = dirname(file);
> >
> > - if (errno != EPERM || is_bpffs(dir)) {
> > - p_err("can't pin the object (%s): %s", name, strerror(errno));
> > + if (is_bpffs(dir)) {
> > + /* nothing to do if already mounted */
> > goto out_free;
> > }
> >
> > - /* Attempt to mount bpffs, then retry pinning. */
> > err = mnt_bpffs(dir, err_str, ERR_MAX_LEN);
> > - if (!err) {
> > - err = bpf_obj_pin(fd, name);
> > - if (err)
> > - p_err("can't pin the object (%s): %s", name,
> > - strerror(errno));
> > - } else {
> > + if (err) {
> > err_str[ERR_MAX_LEN - 1] = '\0';
> > p_err("can't mount BPF file system to pin the object (%s): %s",
> > name, err_str);
> > @@ -204,10 +194,22 @@ int do_pin_fd(int fd, const char *name)
> >
> > out_free:
> > free(file);
> > -out:
> > return err;
> > }
> >
> > +int do_pin_fd(int fd, const char *name)
> > +{
> > + int err = mount_bpffs_for_pin(name);
> > +
> > + if (err) {
> > + p_err("can't mount bpffs for pin %s: %s",
> > + name, strerror(errno));
> > + return err;
> > + }
> > +
> > + return bpf_obj_pin(fd, name);
> > +}
> > +
> > int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32))
> > {
> > unsigned int id;
> > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> > index 28322ace2856..1383824c9baf 100644
> > --- a/tools/bpf/bpftool/main.h
> > +++ b/tools/bpf/bpftool/main.h
> > @@ -129,6 +129,7 @@ const char *get_fd_type_name(enum bpf_obj_type type);
> > char *get_fdinfo(int fd, const char *key);
> > int open_obj_pinned(char *path);
> > int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type);
> > +int mount_bpffs_for_pin(const char *name);
> > int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32));
> > int do_pin_fd(int fd, const char *name);
> >
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index 5302ee282409..f3a07ec3a444 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -81,6 +81,7 @@ static const char * const attach_type_strings[] = {
> > [BPF_SK_SKB_STREAM_PARSER] = "stream_parser",
> > [BPF_SK_SKB_STREAM_VERDICT] = "stream_verdict",
> > [BPF_SK_MSG_VERDICT] = "msg_verdict",
> > + [BPF_FLOW_DISSECTOR] = "flow_dissector",
> > [__MAX_BPF_ATTACH_TYPE] = NULL,
> > };
> >
> > @@ -724,9 +725,10 @@ int map_replace_compar(const void *p1, const void *p2)
> > static int do_attach(int argc, char **argv)
> > {
> > enum bpf_attach_type attach_type;
> > - int err, mapfd, progfd;
> > + int err, progfd;
> > + int mapfd = 0;
> >
> > - if (!REQ_ARGS(5)) {
> > + if (!REQ_ARGS(3)) {
> > p_err("too few parameters for map attach");
> > return -EINVAL;
> > }
> > @@ -741,10 +743,11 @@ static int do_attach(int argc, char **argv)
> > return -EINVAL;
> > }
> > NEXT_ARG();
> > -
> > - mapfd = map_parse_fd(&argc, &argv);
> > - if (mapfd < 0)
> > - return mapfd;
> > + if (argc > 0) {
> > + mapfd = map_parse_fd(&argc, &argv);
> > + if (mapfd < 0)
> > + return mapfd;
> > + }
> >
> > err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
> > if (err) {
> > @@ -760,9 +763,10 @@ static int do_attach(int argc, char **argv)
> > static int do_detach(int argc, char **argv)
> > {
> > enum bpf_attach_type attach_type;
> > - int err, mapfd, progfd;
> > + int err, progfd;
> > + int mapfd = 0;
> >
> > - if (!REQ_ARGS(5)) {
> > + if (!REQ_ARGS(3)) {
> > p_err("too few parameters for map detach");
> > return -EINVAL;
> > }
> > @@ -777,10 +781,11 @@ static int do_detach(int argc, char **argv)
> > return -EINVAL;
> > }
> > NEXT_ARG();
> > -
> > - mapfd = map_parse_fd(&argc, &argv);
> > - if (mapfd < 0)
> > - return mapfd;
> > + if (argc > 0) {
> > + mapfd = map_parse_fd(&argc, &argv);
> > + if (mapfd < 0)
> > + return mapfd;
> > + }
> >
> > err = bpf_prog_detach2(progfd, mapfd, attach_type);
> > if (err) {
> > @@ -792,6 +797,56 @@ static int do_detach(int argc, char **argv)
> > jsonw_null(json_wtr);
> > return 0;
> > }
> > +
> > +/* Flow dissector consists of a main program and a jump table for each
> > + * supported protocol. The assumption here is that the first prog is the main
> > + * one and the other progs are used in the tail calls. In this routine we
> > + * build the jump table for the non-main progs.
> > + */
> > +static int build_flow_dissector_jmp_table(struct bpf_object *obj,
> > + struct bpf_program *prog,
> > + const char *jmp_table_map)
> > +{
> > + struct bpf_map *jmp_table;
> > + struct bpf_program *pos;
> > + int i = 0;
> > + int prog_fd, jmp_table_fd, fd;
> > +
> > + prog_fd = bpf_program__fd(prog);
> > + if (prog_fd < 0) {
> > + p_err("failed to get fd of main prog");
> > + return prog_fd;
> > + }
> > +
> > + jmp_table = bpf_object__find_map_by_name(obj, jmp_table_map);
> > + if (jmp_table == NULL) {
> > + p_err("failed to find '%s' map", jmp_table_map);
> > + return -1;
> > + }
> > +
> > + jmp_table_fd = bpf_map__fd(jmp_table);
> > + if (jmp_table_fd < 0) {
> > + p_err("failed to get fd of jmp_table");
> > + return jmp_table_fd;
> > + }
> > +
> > + bpf_object__for_each_program(pos, obj) {
> > + fd = bpf_program__fd(pos);
> > + if (fd < 0) {
> > + p_err("failed to get fd of '%s'",
> > + bpf_program__title(pos, false));
> > + return fd;
> > + }
> > +
> > + if (fd != prog_fd) {
> > + bpf_map_update_elem(jmp_table_fd, &i, &fd, BPF_ANY);
> > + ++i;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int do_load(int argc, char **argv)
> > {
> > enum bpf_attach_type expected_attach_type;
> > @@ -800,7 +855,7 @@ static int do_load(int argc, char **argv)
> > };
> > struct map_replace *map_replace = NULL;
> > unsigned int old_map_fds = 0;
> > - struct bpf_program *prog;
> > + struct bpf_program *prog, *pos;
> > struct bpf_object *obj;
> > struct bpf_map *map;
> > const char *pinfile;
> > @@ -918,13 +973,19 @@ static int do_load(int argc, char **argv)
> > goto err_free_reuse_maps;
> > }
> >
> > - prog = bpf_program__next(NULL, obj);
> > + if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> > + /* for the flow dissector type, the entry point is in the
> > + * section flow_dissector; other progs are tail calls
> > + */
> > + prog = bpf_object__find_program_by_title(obj, "flow_dissector");
>
> Is the section always called like this? Or could we use instead the same
> assumption as above, i.e. the main program is the first one?
Ideally yes, the main section should always be called
flow_dissector (to not break libbpf_prog_type_by_name assumptions).
We can call bpftool with and without the ATTACH_TYPE argument:
1. when 'type flow_dissector' is specified, we try to find section called
'flow_dissector' and assume, that's the main prog (libbpf_prog_type_by_name)
2. when 'type flow_dissector' is _not_ specified, we try to detect
program type from the first section
This check here is for the cases where first section is not
flow_dissector (but some tail call) and we want to force proper type.
> > + } else {
> > + prog = bpf_program__next(NULL, obj);
> > + }
> > if (!prog) {
> > p_err("object file doesn't contain any bpf program");
> > goto err_close_obj;
> > }
> >
> > - bpf_program__set_ifindex(prog, ifindex);
> > if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
> > const char *sec_name = bpf_program__title(prog, false);
> >
> > @@ -936,8 +997,13 @@ static int do_load(int argc, char **argv)
> > goto err_close_obj;
> > }
> > }
> > - bpf_program__set_type(prog, attr.prog_type);
> > - bpf_program__set_expected_attach_type(prog, expected_attach_type);
> > +
> > + bpf_object__for_each_program(pos, obj) {
> > + bpf_program__set_ifindex(pos, ifindex);
> > + bpf_program__set_type(pos, attr.prog_type);
> > + bpf_program__set_expected_attach_type(pos,
> > + expected_attach_type);
> > + }
>
> I believe it is possible to load program of several types or attach
> types from a same object file? If so, we would need to get individual
> prog types and attach types for each program with
> libbpf_prog_type_by_name().
I don't think it works actually. I also assumed that initially, but as
you can see from the patch, I had to loop over the programs and set
correct type. Current implementation will fail if you have two progs
in the same obj AFAICT.
> > qsort(map_replace, old_map_fds, sizeof(*map_replace),
> > map_replace_compar);
> > @@ -1001,8 +1067,34 @@ static int do_load(int argc, char **argv)
> > goto err_close_obj;
> > }
> >
> > - if (do_pin_fd(bpf_program__fd(prog), pinfile))
> > + err = mount_bpffs_for_pin(pinfile);
> > + if (err) {
> > + p_err("failed to mount bpffs for pin '%s'", pinfile);
> > goto err_close_obj;
> > + }
> > +
> > + if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> > + err = build_flow_dissector_jmp_table(obj, prog, "jmp_table");
> > + if (err) {
> > + p_err("failed to build flow dissector jump table");
> > + goto err_close_obj;
> > + }
> > + /* flow dissector consist of multiple programs,
> > + * we want to pin them all
> > + */
> > + err = bpf_object__pin(obj, pinfile);
> > + if (err) {
> > + p_err("failed to pin flow dissector object");
> > + goto err_close_obj;
> > + }
>
> What happens for the programs previously loaded and pinned in one of the
> program fails? Do they remain loaded and pinned even if all programs
> were not successfully loaded? Or should we consider removing all links
> we added instead and go back to a "clean" state?
They remain loaded. I agree, we should go back to the clean state;
that probably should be achieved by adding proper cleanup to libbpf's
bpf_object__pin when it fails. I can look into that for v2.
> > + } else {
> > + err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
> > + if (err) {
> > + p_err("failed to pin program %s",
> > + bpf_program__title(prog, false));
> > + goto err_close_obj;
> > + }
>
> I don't have the same opinion as Jakub for pinning :). I was hoping we
> could also load additional programs (for tail calls) for
> non-flow_dissector programs. Could this be an occasion to update the
> code in that direction?
(See above for loading multiple progs from the same object).
If we want to be able to load non-flow_dissector programs from the same file,
we should have some way to distinguish between flow_dissector and
non-flow_dissector ones; I don't see an easy way to do that.
> > + }
> >
> > if (json_output)
> > jsonw_null(json_wtr);
> > @@ -1037,8 +1129,8 @@ static int do_help(int argc, char **argv)
> > " %s %s pin PROG FILE\n"
> > " %s %s load OBJ FILE [type TYPE] [dev NAME] \\\n"
> > " [map { idx IDX | name NAME } MAP]\n"
> > - " %s %s attach PROG ATTACH_TYPE MAP\n"
> > - " %s %s detach PROG ATTACH_TYPE MAP\n"
> > + " %s %s attach PROG ATTACH_TYPE [MAP]\n"
> > + " %s %s detach PROG ATTACH_TYPE [MAP]\n"
> > " %s %s help\n"
> > "\n"
> > " " HELP_SPEC_MAP "\n"
> > @@ -1050,7 +1142,8 @@ static int do_help(int argc, char **argv)
> > " cgroup/bind4 | cgroup/bind6 | cgroup/post_bind4 |\n"
> > " cgroup/post_bind6 | cgroup/connect4 | cgroup/connect6 |\n"
> > " cgroup/sendmsg4 | cgroup/sendmsg6 }\n"
> > - " ATTACH_TYPE := { msg_verdict | skb_verdict | skb_parse }\n"
> > + " ATTACH_TYPE := { msg_verdict | skb_verdict | skb_parse |\n"
> > + " flow_dissector }\n"
> > " " HELP_SPEC_OPTIONS "\n"
> > "",
> > bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
> >
>
> Thanks a lot for the set!
Thank you for a review as well!
> Quentin
^ permalink raw reply
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