* Re: [PATCH v2 bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf
From: Andy Lutomirski @ 2019-06-27 23:42 UTC (permalink / raw)
To: Andy Lutomirski, Kees Cook, Linux API
Cc: Song Liu, Network Development, bpf, Alexei Starovoitov,
Daniel Borkmann, kernel-team, lmb, Jann Horn, Greg KH
In-Reply-To: <21894f45-70d8-dfca-8c02-044f776c5e05@kernel.org>
[sigh, I finally set up lore nntp, and I goofed some addresses. Hi
Kees and linux-api.]
On Thu, Jun 27, 2019 at 4:40 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On 6/27/19 1:19 PM, Song Liu wrote:
> > This patch introduce unprivileged BPF access. The access control is
> > achieved via device /dev/bpf. Users with write access to /dev/bpf are able
> > to call sys_bpf().
> >
> > Two ioctl command are added to /dev/bpf:
> >
> > The two commands enable/disable permission to call sys_bpf() for current
> > task. This permission is noted by bpf_permitted in task_struct. This
> > permission is inherited during clone(CLONE_THREAD).
> >
> > Helper function bpf_capable() is added to check whether the task has got
> > permission via /dev/bpf.
> >
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 0e079b2298f8..79dc4d641cf3 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -9134,7 +9134,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
> > env->insn_aux_data[i].orig_idx = i;
> > env->prog = *prog;
> > env->ops = bpf_verifier_ops[env->prog->type];
> > - is_priv = capable(CAP_SYS_ADMIN);
> > + is_priv = bpf_capable(CAP_SYS_ADMIN);
>
> Huh? This isn't a hardening measure -- the "is_priv" verifier mode
> allows straight-up leaks of private kernel state to user mode.
>
> (For that matter, the pending lockdown stuff should possibly consider
> this a "confidentiality" issue.)
>
>
> I have a bigger issue with this patch, though: it's a really awkward way
> to pretend to have capabilities. For bpf, it seems like you could make
> this be a *real* capability without too much pain since there's only one
> syscall there. Just find a way to pass an fd to /dev/bpf into the
> syscall. If this means you need a new bpf_with_cap() syscall that takes
> an extra argument, so be it. The old bpf() syscall can just translate
> to bpf_with_cap(..., -1).
>
> For a while, I've considered a scheme I call "implicit rights". There
> would be a directory in /dev called /dev/implicit_rights. This would
> either be part of devtmpfs or a whole new filesystem -- it would *not*
> be any other filesystem. The contents would be files that can't be read
> or written and exist only in memory. You create them with a privileged
> syscall. Certain actions that are sensitive but not at the level of
> CAP_SYS_ADMIN (use of large-attack-surface bpf stuff, creation of user
> namespaces, profiling the kernel, etc) could require an "implicit
> right". When you do them, if you don't have CAP_SYS_ADMIN, the kernel
> would do a path walk for, say, /dev/implicit_rights/bpf and, if the
> object exists, can be opened, and actually refers to the "bpf" rights
> object, then the action is allowed. Otherwise it's denied.
>
> This is extensible, and it doesn't require the rather ugly per-task
> state of whether it's enabled.
>
> For things like creation of user namespaces, there's an existing API,
> and the default is that it works without privilege. Switching it to an
> implicit right has the benefit of not requiring code changes to programs
> that already work as non-root.
>
> But, for BPF in particular, this type of compatibility issue doesn't
> exist now. You already can't use most eBPF functionality without
> privilege. New bpf-using programs meant to run without privilege are
> *new*, so they can use a new improved API. So, rather than adding this
> obnoxious ioctl, just make the API explicit, please.
>
> Also, please cc: linux-abi next time.
^ permalink raw reply
* Re: [PATCH v2 bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf
From: Andy Lutomirski @ 2019-06-27 23:40 UTC (permalink / raw)
To: Song Liu, netdev, bpf
Cc: ast, daniel, kernel-team, lmb, jannh, gregkh, linux-abi, kees
In-Reply-To: <20190627201923.2589391-2-songliubraving@fb.com>
On 6/27/19 1:19 PM, Song Liu wrote:
> This patch introduce unprivileged BPF access. The access control is
> achieved via device /dev/bpf. Users with write access to /dev/bpf are able
> to call sys_bpf().
>
> Two ioctl command are added to /dev/bpf:
>
> The two commands enable/disable permission to call sys_bpf() for current
> task. This permission is noted by bpf_permitted in task_struct. This
> permission is inherited during clone(CLONE_THREAD).
>
> Helper function bpf_capable() is added to check whether the task has got
> permission via /dev/bpf.
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0e079b2298f8..79dc4d641cf3 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9134,7 +9134,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
> env->insn_aux_data[i].orig_idx = i;
> env->prog = *prog;
> env->ops = bpf_verifier_ops[env->prog->type];
> - is_priv = capable(CAP_SYS_ADMIN);
> + is_priv = bpf_capable(CAP_SYS_ADMIN);
Huh? This isn't a hardening measure -- the "is_priv" verifier mode
allows straight-up leaks of private kernel state to user mode.
(For that matter, the pending lockdown stuff should possibly consider
this a "confidentiality" issue.)
I have a bigger issue with this patch, though: it's a really awkward way
to pretend to have capabilities. For bpf, it seems like you could make
this be a *real* capability without too much pain since there's only one
syscall there. Just find a way to pass an fd to /dev/bpf into the
syscall. If this means you need a new bpf_with_cap() syscall that takes
an extra argument, so be it. The old bpf() syscall can just translate
to bpf_with_cap(..., -1).
For a while, I've considered a scheme I call "implicit rights". There
would be a directory in /dev called /dev/implicit_rights. This would
either be part of devtmpfs or a whole new filesystem -- it would *not*
be any other filesystem. The contents would be files that can't be read
or written and exist only in memory. You create them with a privileged
syscall. Certain actions that are sensitive but not at the level of
CAP_SYS_ADMIN (use of large-attack-surface bpf stuff, creation of user
namespaces, profiling the kernel, etc) could require an "implicit
right". When you do them, if you don't have CAP_SYS_ADMIN, the kernel
would do a path walk for, say, /dev/implicit_rights/bpf and, if the
object exists, can be opened, and actually refers to the "bpf" rights
object, then the action is allowed. Otherwise it's denied.
This is extensible, and it doesn't require the rather ugly per-task
state of whether it's enabled.
For things like creation of user namespaces, there's an existing API,
and the default is that it works without privilege. Switching it to an
implicit right has the benefit of not requiring code changes to programs
that already work as non-root.
But, for BPF in particular, this type of compatibility issue doesn't
exist now. You already can't use most eBPF functionality without
privilege. New bpf-using programs meant to run without privilege are
*new*, so they can use a new improved API. So, rather than adding this
obnoxious ioctl, just make the API explicit, please.
Also, please cc: linux-abi next time.
^ permalink raw reply
* Re: [PATCH V33 24/30] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Andy Lutomirski @ 2019-06-27 23:27 UTC (permalink / raw)
To: Stephen Smalley
Cc: James Morris, Andy Lutomirski, Matthew Garrett, linux-security,
LKML, Linux API, David Howells, Alexei Starovoitov,
Matthew Garrett, Network Development, Chun-Yi Lee,
Daniel Borkmann, LSM List
In-Reply-To: <bce70c8b-9efd-6362-d536-cfbbcf70b0b7@tycho.nsa.gov>
On Thu, Jun 27, 2019 at 7:35 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 6/26/19 8:57 PM, Andy Lutomirski wrote:
> >
> >
> >> On Jun 26, 2019, at 1:22 PM, James Morris <jmorris@namei.org> wrote:
> >>
> >> [Adding the LSM mailing list: missed this patchset initially]
> >>
> >>> On Thu, 20 Jun 2019, Andy Lutomirski wrote:
> >>>
> >>> This patch exemplifies why I don't like this approach:
> >>>
> >>>> @@ -97,6 +97,7 @@ enum lockdown_reason {
> >>>> LOCKDOWN_INTEGRITY_MAX,
> >>>> LOCKDOWN_KCORE,
> >>>> LOCKDOWN_KPROBES,
> >>>> + LOCKDOWN_BPF,
> >>>> LOCKDOWN_CONFIDENTIALITY_MAX,
> >>>
> >>>> --- a/security/lockdown/lockdown.c
> >>>> +++ b/security/lockdown/lockdown.c
> >>>> @@ -33,6 +33,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> >>>> [LOCKDOWN_INTEGRITY_MAX] = "integrity",
> >>>> [LOCKDOWN_KCORE] = "/proc/kcore access",
> >>>> [LOCKDOWN_KPROBES] = "use of kprobes",
> >>>> + [LOCKDOWN_BPF] = "use of bpf",
> >>>> [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
> >>>
> >>> The text here says "use of bpf", but what this patch is *really* doing
> >>> is locking down use of BPF to read kernel memory. If the details
> >>> change, then every LSM needs to get updated, and we risk breaking user
> >>> policies that are based on LSMs that offer excessively fine
> >>> granularity.
> >>
> >> Can you give an example of how the details might change?
> >>
> >>> I'd be more comfortable if the LSM only got to see "confidentiality"
> >>> or "integrity".
> >>
> >> These are not sufficient for creating a useful policy for the SELinux
> >> case.
> >>
> >>
> >
> > I may have misunderstood, but I thought that SELinux mainly needed to allow certain privileged programs to bypass the policy. Is there a real example of what SELinux wants to do that can’t be done in the simplified model?
> >
> > The think that specifically makes me uneasy about exposing all of these precise actions to LSMs is that they will get exposed to userspace in a way that forces us to treat them as stable ABIs.
>
> There are two scenarios where finer-grained distinctions make sense:
>
> - Users may need to enable specific functionality that falls under the
> umbrella of "confidentiality" or "integrity" lockdown. Finer-grained
> lockdown reasons free them from having to make an all-or-nothing choice
> between lost functionality or no lockdown at all. This can be supported
> directly by the lockdown module without any help from SELinux or other
> security modules; we just need the ability to specify these
> finer-grained lockdown levels via the boot parameters and securityfs nodes.
>
> - Different processes/programs may need to use different sets of
> functionality restricted via lockdown confidentiality or integrity
> categories. If we have to allow all-or-none for the set of
> interfaces/functionality covered by the generic confidentiality or
> integrity categories, then we'll end up having to choose between lost
> functionality or overprivileged processes, neither of which is optimal.
>
> Is it truly the case that everything under the "confidentiality"
> category poses the same level of risk to kernel confidentiality, and
> similarly for everything under the "integrity" category? If not, then
> being able to distinguish them definitely has benefit.
>
They're really quite similar in my mind. Certainly some things in the
"integrity" category give absolutely trivial control over the kernel
(e.g. modules) while others make it quite challenging (ioperm), but
the end result is very similar. And quite a few "confidentiality"
things genuinely do allow all kernel memory to be read.
I agree that finer-grained distinctions could be useful. My concern is
that it's a tradeoff, and the other end of the tradeoff is an ABI
stability issue. If someone decides down the road that some feature
that is currently "integrity" can be split into a narrow "integrity"
feature and a "confidentiality" feature then, if the user policy knows
about the individual features, there's a risk of breaking people's
systems. If we keep the fine-grained control, do we have a clear
compatibility story?
^ permalink raw reply
* Re: [PATCH V33 24/30] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Andy Lutomirski @ 2019-06-27 23:23 UTC (permalink / raw)
To: Matthew Garrett
Cc: Stephen Smalley, James Morris, Andy Lutomirski, linux-security,
LKML, Linux API, David Howells, Alexei Starovoitov,
Network Development, Chun-Yi Lee, Daniel Borkmann, LSM List
In-Reply-To: <CACdnJuuG8cR7h9v3pNcBKsxyckAzpKuBJs1GQxsz77jk5DRoQA@mail.gmail.com>
On Thu, Jun 27, 2019 at 4:16 PM Matthew Garrett <mjg59@google.com> wrote:
>
> On Thu, Jun 27, 2019 at 1:16 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > That would only allow the LSM to further lock down the system above the
> > lockdown level set at boot, not grant exemptions for specific
> > functionality/interfaces required by the user or by a specific
> > process/program. You'd have to boot with lockdown=none (or your
> > lockdown=custom suggestion) in order for the LSM to allow anything
> > covered by the integrity or confidentiality levels. And then the kernel
> > would be unprotected prior to full initialization of the LSM, including
> > policy load.
> >
> > It seems like one would want to be able to boot with lockdown=integrity
> > to protect the kernel initially, then switch over to allowing the LSM to
> > selectively override it.
>
> One option would be to allow modules to be "unstacked" at runtime, but
> there's still something of a problem here - how do you ensure that
> your userland can be trusted to load a new policy before it does so?
> If you're able to assert that your early userland is trustworthy
> (perhaps because it's in an initramfs that's part of your signed boot
> payload), there's maybe an argument that most of the lockdown
> integrity guarantees are unnecessary before handoff - just using the
> lockdown LSM to protect against attacks via kernel parameters would be
> sufficient.
I think that, if you don't trust your system enough to avoid
compromising itself before policy load, then your MAC policy is more
or less dead in the water. It seems to be that it ought to be good
enough to boot with lockdown=none and then have a real policy loaded
along with the rest of the MAC policy. Or, for applications that need
to be stricter, you accept that MAC policy can't override lockdown.
^ permalink raw reply
* Re: [PATCH V33 24/30] bpf: Restrict bpf when kernel lockdown is in confidentiality mode
From: Matthew Garrett @ 2019-06-27 23:16 UTC (permalink / raw)
To: Stephen Smalley
Cc: James Morris, Andy Lutomirski, Andy Lutomirski, linux-security,
LKML, Linux API, David Howells, Alexei Starovoitov,
Network Development, Chun-Yi Lee, Daniel Borkmann, LSM List
In-Reply-To: <de8b15eb-ba6c-847a-7435-42742203d4a5@tycho.nsa.gov>
On Thu, Jun 27, 2019 at 1:16 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> That would only allow the LSM to further lock down the system above the
> lockdown level set at boot, not grant exemptions for specific
> functionality/interfaces required by the user or by a specific
> process/program. You'd have to boot with lockdown=none (or your
> lockdown=custom suggestion) in order for the LSM to allow anything
> covered by the integrity or confidentiality levels. And then the kernel
> would be unprotected prior to full initialization of the LSM, including
> policy load.
>
> It seems like one would want to be able to boot with lockdown=integrity
> to protect the kernel initially, then switch over to allowing the LSM to
> selectively override it.
One option would be to allow modules to be "unstacked" at runtime, but
there's still something of a problem here - how do you ensure that
your userland can be trusted to load a new policy before it does so?
If you're able to assert that your early userland is trustworthy
(perhaps because it's in an initramfs that's part of your signed boot
payload), there's maybe an argument that most of the lockdown
integrity guarantees are unnecessary before handoff - just using the
lockdown LSM to protect against attacks via kernel parameters would be
sufficient.
^ permalink raw reply
* [PATCH net-next 5/5] nfp: flower: add GRE encap action support
From: Jakub Kicinski @ 2019-06-27 23:12 UTC (permalink / raw)
To: davem
Cc: netdev, oss-drivers, Pieter Jansen van Vuuren, Jakub Kicinski,
John Hurley
In-Reply-To: <20190627231243.8323-1-jakub.kicinski@netronome.com>
From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Add new GRE encapsulation support, which allows offload of filters
using tunnel_key set action in combination with actions that egress
to GRE type ports.
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: John Hurley <john.hurley@netronome.com>
---
.../ethernet/netronome/nfp/flower/action.c | 33 ++++++++++++++++---
1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/action.c b/drivers/net/ethernet/netronome/nfp/flower/action.c
index 88fedb5ada97..b6bd31fe44b2 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/action.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/action.c
@@ -170,13 +170,36 @@ nfp_fl_output(struct nfp_app *app, struct nfp_fl_output *output,
return 0;
}
+static bool
+nfp_flower_tun_is_gre(struct tc_cls_flower_offload *flow, int start_idx)
+{
+ struct flow_action_entry *act = flow->rule->action.entries;
+ int num_act = flow->rule->action.num_entries;
+ int act_idx;
+
+ /* Preparse action list for next mirred or redirect action */
+ for (act_idx = start_idx + 1; act_idx < num_act; act_idx++)
+ if (act[act_idx].id == FLOW_ACTION_REDIRECT ||
+ act[act_idx].id == FLOW_ACTION_MIRRED)
+ return netif_is_gretap(act[act_idx].dev);
+
+ return false;
+}
+
static enum nfp_flower_tun_type
nfp_fl_get_tun_from_act(struct nfp_app *app,
- const struct flow_action_entry *act)
+ struct tc_cls_flower_offload *flow,
+ const struct flow_action_entry *act, int act_idx)
{
const struct ip_tunnel_info *tun = act->tunnel;
struct nfp_flower_priv *priv = app->priv;
+ /* Determine the tunnel type based on the egress netdev
+ * in the mirred action for tunnels without l4.
+ */
+ if (nfp_flower_tun_is_gre(flow, act_idx))
+ return NFP_FL_TUNNEL_GRE;
+
switch (tun->key.tp_dst) {
case htons(IANA_VXLAN_UDP_PORT):
return NFP_FL_TUNNEL_VXLAN;
@@ -841,7 +864,7 @@ nfp_flower_loop_action(struct nfp_app *app, const struct flow_action_entry *act,
enum nfp_flower_tun_type *tun_type, int *tun_out_cnt,
int *out_cnt, u32 *csum_updated,
struct nfp_flower_pedit_acts *set_act,
- struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack, int act_idx)
{
struct nfp_fl_set_ipv4_tun *set_tun;
struct nfp_fl_pre_tunnel *pre_tun;
@@ -896,7 +919,7 @@ nfp_flower_loop_action(struct nfp_app *app, const struct flow_action_entry *act,
case FLOW_ACTION_TUNNEL_ENCAP: {
const struct ip_tunnel_info *ip_tun = act->tunnel;
- *tun_type = nfp_fl_get_tun_from_act(app, act);
+ *tun_type = nfp_fl_get_tun_from_act(app, flow, act, act_idx);
if (*tun_type == NFP_FL_TUNNEL_NONE) {
NL_SET_ERR_MSG_MOD(extack, "unsupported offload: unsupported tunnel type in action list");
return -EOPNOTSUPP;
@@ -1022,8 +1045,8 @@ int nfp_flower_compile_action(struct nfp_app *app,
memset(&set_act, 0, sizeof(set_act));
err = nfp_flower_loop_action(app, act, flow, nfp_flow, &act_len,
netdev, &tun_type, &tun_out_cnt,
- &out_cnt, &csum_updated, &set_act,
- extack);
+ &out_cnt, &csum_updated,
+ &set_act, extack, i);
if (err)
return err;
act_cnt++;
--
2.21.0
^ permalink raw reply related
* [PATCH net-next 4/5] nfp: flower: add GRE decap classification support
From: Jakub Kicinski @ 2019-06-27 23:12 UTC (permalink / raw)
To: davem
Cc: netdev, oss-drivers, Pieter Jansen van Vuuren, Jakub Kicinski,
John Hurley
In-Reply-To: <20190627231243.8323-1-jakub.kicinski@netronome.com>
From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Extend the existing tunnel matching support to include GRE decap
classification. Specifically matching existing tunnel fields for
NVGRE (GRE with protocol field set to TEB).
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: John Hurley <john.hurley@netronome.com>
---
.../net/ethernet/netronome/nfp/flower/cmsg.h | 39 ++++++++++++
.../net/ethernet/netronome/nfp/flower/match.c | 44 ++++++++++++++
.../ethernet/netronome/nfp/flower/offload.c | 59 +++++++++++++------
3 files changed, 124 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
index d0d57d1ef750..0f1706ae5bfc 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
@@ -8,6 +8,7 @@
#include <linux/skbuff.h>
#include <linux/types.h>
#include <net/geneve.h>
+#include <net/gre.h>
#include <net/vxlan.h>
#include "../nfp_app.h"
@@ -22,6 +23,7 @@
#define NFP_FLOWER_LAYER_CT BIT(6)
#define NFP_FLOWER_LAYER_VXLAN BIT(7)
+#define NFP_FLOWER_LAYER2_GRE BIT(0)
#define NFP_FLOWER_LAYER2_GENEVE BIT(5)
#define NFP_FLOWER_LAYER2_GENEVE_OP BIT(6)
@@ -37,6 +39,9 @@
#define NFP_FL_IP_FRAG_FIRST BIT(7)
#define NFP_FL_IP_FRAGMENTED BIT(6)
+/* GRE Tunnel flags */
+#define NFP_FL_GRE_FLAG_KEY BIT(2)
+
/* Compressed HW representation of TCP Flags */
#define NFP_FL_TCP_FLAG_URG BIT(4)
#define NFP_FL_TCP_FLAG_PSH BIT(3)
@@ -107,6 +112,7 @@
enum nfp_flower_tun_type {
NFP_FL_TUNNEL_NONE = 0,
+ NFP_FL_TUNNEL_GRE = 1,
NFP_FL_TUNNEL_VXLAN = 2,
NFP_FL_TUNNEL_GENEVE = 4,
};
@@ -388,6 +394,35 @@ struct nfp_flower_ipv4_udp_tun {
__be32 tun_id;
};
+/* Flow Frame GRE TUNNEL --> Tunnel details (6W/24B)
+ * -----------------------------------------------------------------
+ * 3 2 1
+ * 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | ipv4_addr_src |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | ipv4_addr_dst |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | tun_flags | tos | ttl |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | Reserved | Ethertype |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | Key |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | Reserved |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+
+struct nfp_flower_ipv4_gre_tun {
+ struct nfp_flower_tun_ipv4 ipv4;
+ __be16 tun_flags;
+ struct nfp_flower_tun_ip_ext ip_ext;
+ __be16 reserved1;
+ __be16 ethertype;
+ __be32 tun_key;
+ __be32 reserved2;
+};
+
struct nfp_flower_geneve_options {
u8 data[NFP_FL_MAX_GENEVE_OPT_KEY];
};
@@ -538,6 +573,8 @@ nfp_fl_netdev_is_tunnel_type(struct net_device *netdev,
{
if (netif_is_vxlan(netdev))
return tun_type == NFP_FL_TUNNEL_VXLAN;
+ if (netif_is_gretap(netdev))
+ return tun_type == NFP_FL_TUNNEL_GRE;
if (netif_is_geneve(netdev))
return tun_type == NFP_FL_TUNNEL_GENEVE;
@@ -554,6 +591,8 @@ static inline bool nfp_fl_is_netdev_to_offload(struct net_device *netdev)
return true;
if (netif_is_geneve(netdev))
return true;
+ if (netif_is_gretap(netdev))
+ return true;
return false;
}
diff --git a/drivers/net/ethernet/netronome/nfp/flower/match.c b/drivers/net/ethernet/netronome/nfp/flower/match.c
index 9181611087c2..c1690de19172 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/match.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/match.c
@@ -316,6 +316,35 @@ nfp_flower_compile_tun_ip_ext(struct nfp_flower_tun_ip_ext *ext,
}
}
+static void
+nfp_flower_compile_ipv4_gre_tun(struct nfp_flower_ipv4_gre_tun *ext,
+ struct nfp_flower_ipv4_gre_tun *msk,
+ struct tc_cls_flower_offload *flow)
+{
+ struct flow_rule *rule = tc_cls_flower_offload_flow_rule(flow);
+
+ memset(ext, 0, sizeof(struct nfp_flower_ipv4_gre_tun));
+ memset(msk, 0, sizeof(struct nfp_flower_ipv4_gre_tun));
+
+ /* NVGRE is the only supported GRE tunnel type */
+ ext->ethertype = cpu_to_be16(ETH_P_TEB);
+ msk->ethertype = cpu_to_be16(~0);
+
+ if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
+ struct flow_match_enc_keyid match;
+
+ flow_rule_match_enc_keyid(rule, &match);
+ ext->tun_key = match.key->keyid;
+ msk->tun_key = match.mask->keyid;
+
+ ext->tun_flags = cpu_to_be16(NFP_FL_GRE_FLAG_KEY);
+ msk->tun_flags = cpu_to_be16(NFP_FL_GRE_FLAG_KEY);
+ }
+
+ nfp_flower_compile_tun_ipv4_addrs(&ext->ipv4, &msk->ipv4, flow);
+ nfp_flower_compile_tun_ip_ext(&ext->ip_ext, &msk->ip_ext, flow);
+}
+
static void
nfp_flower_compile_ipv4_udp_tun(struct nfp_flower_ipv4_udp_tun *ext,
struct nfp_flower_ipv4_udp_tun *msk,
@@ -425,6 +454,21 @@ int nfp_flower_compile_flow_match(struct nfp_app *app,
msk += sizeof(struct nfp_flower_ipv6);
}
+ if (key_ls->key_layer_two & NFP_FLOWER_LAYER2_GRE) {
+ __be32 tun_dst;
+
+ nfp_flower_compile_ipv4_gre_tun((void *)ext, (void *)msk, flow);
+ tun_dst = ((struct nfp_flower_ipv4_gre_tun *)ext)->ipv4.dst;
+ ext += sizeof(struct nfp_flower_ipv4_gre_tun);
+ msk += sizeof(struct nfp_flower_ipv4_gre_tun);
+
+ /* Store the tunnel destination in the rule data.
+ * This must be present and be an exact match.
+ */
+ nfp_flow->nfp_tun_ipv4_addr = tun_dst;
+ nfp_tunnel_add_ipv4_off(app, tun_dst);
+ }
+
if (key_ls->key_layer & NFP_FLOWER_LAYER_VXLAN ||
key_ls->key_layer_two & NFP_FLOWER_LAYER2_GENEVE) {
__be32 tun_dst;
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 6b28910442db..6dbe947269c3 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -52,8 +52,7 @@
#define NFP_FLOWER_WHITELIST_TUN_DISSECTOR_R \
(BIT(FLOW_DISSECTOR_KEY_ENC_CONTROL) | \
- BIT(FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) | \
- BIT(FLOW_DISSECTOR_KEY_ENC_PORTS))
+ BIT(FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS))
#define NFP_FLOWER_MERGE_FIELDS \
(NFP_FLOWER_LAYER_PORT | \
@@ -285,27 +284,51 @@ nfp_flower_calculate_key_layers(struct nfp_app *app,
return -EOPNOTSUPP;
}
- flow_rule_match_enc_ports(rule, &enc_ports);
- if (enc_ports.mask->dst != cpu_to_be16(~0)) {
- NL_SET_ERR_MSG_MOD(extack, "unsupported offload: only an exact match L4 destination port is supported");
- return -EOPNOTSUPP;
- }
-
if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_OPTS))
flow_rule_match_enc_opts(rule, &enc_op);
- err = nfp_flower_calc_udp_tun_layer(enc_ports.key, enc_op.key,
- &key_layer_two, &key_layer,
- &key_size, priv, tun_type,
- extack);
- if (err)
- return err;
+ if (!flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_PORTS)) {
+ /* check if GRE, which has no enc_ports */
+ if (netif_is_gretap(netdev)) {
+ *tun_type = NFP_FL_TUNNEL_GRE;
+ key_layer |= NFP_FLOWER_LAYER_EXT_META;
+ key_size += sizeof(struct nfp_flower_ext_meta);
+ key_layer_two |= NFP_FLOWER_LAYER2_GRE;
+ key_size +=
+ sizeof(struct nfp_flower_ipv4_gre_tun);
+
+ if (enc_op.key) {
+ NL_SET_ERR_MSG_MOD(extack, "unsupported offload: encap options not supported on GRE tunnels");
+ return -EOPNOTSUPP;
+ }
+ } else {
+ NL_SET_ERR_MSG_MOD(extack, "unsupported offload: an exact match on L4 destination port is required for non-GRE tunnels");
+ return -EOPNOTSUPP;
+ }
+ } else {
+ flow_rule_match_enc_ports(rule, &enc_ports);
+ if (enc_ports.mask->dst != cpu_to_be16(~0)) {
+ NL_SET_ERR_MSG_MOD(extack, "unsupported offload: only an exact match L4 destination port is supported");
+ return -EOPNOTSUPP;
+ }
- /* Ensure the ingress netdev matches the expected tun type. */
- if (!nfp_fl_netdev_is_tunnel_type(netdev, *tun_type)) {
- NL_SET_ERR_MSG_MOD(extack, "unsupported offload: ingress netdev does not match the expected tunnel type");
- return -EOPNOTSUPP;
+ err = nfp_flower_calc_udp_tun_layer(enc_ports.key,
+ enc_op.key,
+ &key_layer_two,
+ &key_layer,
+ &key_size, priv,
+ tun_type, extack);
+ if (err)
+ return err;
+
+ /* Ensure the ingress netdev matches the expected
+ * tun type.
+ */
+ if (!nfp_fl_netdev_is_tunnel_type(netdev, *tun_type)) {
+ NL_SET_ERR_MSG_MOD(extack, "unsupported offload: ingress netdev does not match the expected tunnel type");
+ return -EOPNOTSUPP;
+ }
}
}
--
2.21.0
^ permalink raw reply related
* [PATCH net-next 3/5] nfp: flower: rename tunnel related functions in action offload
From: Jakub Kicinski @ 2019-06-27 23:12 UTC (permalink / raw)
To: davem
Cc: netdev, oss-drivers, Pieter Jansen van Vuuren, Jakub Kicinski,
John Hurley
In-Reply-To: <20190627231243.8323-1-jakub.kicinski@netronome.com>
From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Previously tunnel related functions in action offload only applied
to UDP tunnels. Rename these functions in preparation for new
tunnel types.
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: John Hurley <john.hurley@netronome.com>
---
.../ethernet/netronome/nfp/flower/action.c | 30 +++++++++----------
.../net/ethernet/netronome/nfp/flower/cmsg.h | 2 +-
2 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/action.c b/drivers/net/ethernet/netronome/nfp/flower/action.c
index 8bea3004d66c..88fedb5ada97 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/action.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/action.c
@@ -171,8 +171,8 @@ nfp_fl_output(struct nfp_app *app, struct nfp_fl_output *output,
}
static enum nfp_flower_tun_type
-nfp_fl_get_tun_from_act_l4_port(struct nfp_app *app,
- const struct flow_action_entry *act)
+nfp_fl_get_tun_from_act(struct nfp_app *app,
+ const struct flow_action_entry *act)
{
const struct ip_tunnel_info *tun = act->tunnel;
struct nfp_flower_priv *priv = app->priv;
@@ -281,15 +281,13 @@ nfp_fl_push_geneve_options(struct nfp_fl_payload *nfp_fl, int *list_len,
}
static int
-nfp_fl_set_ipv4_udp_tun(struct nfp_app *app,
- struct nfp_fl_set_ipv4_udp_tun *set_tun,
- const struct flow_action_entry *act,
- struct nfp_fl_pre_tunnel *pre_tun,
- enum nfp_flower_tun_type tun_type,
- struct net_device *netdev,
- struct netlink_ext_ack *extack)
+nfp_fl_set_ipv4_tun(struct nfp_app *app, struct nfp_fl_set_ipv4_tun *set_tun,
+ const struct flow_action_entry *act,
+ struct nfp_fl_pre_tunnel *pre_tun,
+ enum nfp_flower_tun_type tun_type,
+ struct net_device *netdev, struct netlink_ext_ack *extack)
{
- size_t act_size = sizeof(struct nfp_fl_set_ipv4_udp_tun);
+ size_t act_size = sizeof(struct nfp_fl_set_ipv4_tun);
const struct ip_tunnel_info *ip_tun = act->tunnel;
struct nfp_flower_priv *priv = app->priv;
u32 tmp_set_ip_tun_type_index = 0;
@@ -845,7 +843,7 @@ nfp_flower_loop_action(struct nfp_app *app, const struct flow_action_entry *act,
struct nfp_flower_pedit_acts *set_act,
struct netlink_ext_ack *extack)
{
- struct nfp_fl_set_ipv4_udp_tun *set_tun;
+ struct nfp_fl_set_ipv4_tun *set_tun;
struct nfp_fl_pre_tunnel *pre_tun;
struct nfp_fl_push_vlan *psh_v;
struct nfp_fl_pop_vlan *pop_v;
@@ -898,7 +896,7 @@ nfp_flower_loop_action(struct nfp_app *app, const struct flow_action_entry *act,
case FLOW_ACTION_TUNNEL_ENCAP: {
const struct ip_tunnel_info *ip_tun = act->tunnel;
- *tun_type = nfp_fl_get_tun_from_act_l4_port(app, act);
+ *tun_type = nfp_fl_get_tun_from_act(app, act);
if (*tun_type == NFP_FL_TUNNEL_NONE) {
NL_SET_ERR_MSG_MOD(extack, "unsupported offload: unsupported tunnel type in action list");
return -EOPNOTSUPP;
@@ -914,7 +912,7 @@ nfp_flower_loop_action(struct nfp_app *app, const struct flow_action_entry *act,
* If none, the packet falls back before applying other actions.
*/
if (*a_len + sizeof(struct nfp_fl_pre_tunnel) +
- sizeof(struct nfp_fl_set_ipv4_udp_tun) > NFP_FL_MAX_A_SIZ) {
+ sizeof(struct nfp_fl_set_ipv4_tun) > NFP_FL_MAX_A_SIZ) {
NL_SET_ERR_MSG_MOD(extack, "unsupported offload: maximum allowed action list size exceeded at tunnel encap");
return -EOPNOTSUPP;
}
@@ -928,11 +926,11 @@ nfp_flower_loop_action(struct nfp_app *app, const struct flow_action_entry *act,
return err;
set_tun = (void *)&nfp_fl->action_data[*a_len];
- err = nfp_fl_set_ipv4_udp_tun(app, set_tun, act, pre_tun,
- *tun_type, netdev, extack);
+ err = nfp_fl_set_ipv4_tun(app, set_tun, act, pre_tun,
+ *tun_type, netdev, extack);
if (err)
return err;
- *a_len += sizeof(struct nfp_fl_set_ipv4_udp_tun);
+ *a_len += sizeof(struct nfp_fl_set_ipv4_tun);
}
break;
case FLOW_ACTION_TUNNEL_DECAP:
diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
index 0d3d1b68232c..d0d57d1ef750 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
@@ -203,7 +203,7 @@ struct nfp_fl_pre_tunnel {
__be32 extra[3];
};
-struct nfp_fl_set_ipv4_udp_tun {
+struct nfp_fl_set_ipv4_tun {
struct nfp_fl_act_head head;
__be16 reserved;
__be64 tun_id __packed;
--
2.21.0
^ permalink raw reply related
* [PATCH net-next 2/5] nfp: flower: add helper functions for tunnel classification
From: Jakub Kicinski @ 2019-06-27 23:12 UTC (permalink / raw)
To: davem
Cc: netdev, oss-drivers, Pieter Jansen van Vuuren, Jakub Kicinski,
John Hurley
In-Reply-To: <20190627231243.8323-1-jakub.kicinski@netronome.com>
From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Adds IPv4 address and TTL/TOS helper functions, which is done in
preparation for compiling new tunnel types.
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: John Hurley <john.hurley@netronome.com>
---
.../net/ethernet/netronome/nfp/flower/cmsg.h | 16 +++--
.../net/ethernet/netronome/nfp/flower/match.c | 59 ++++++++++++-------
2 files changed, 51 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
index 537f7fc19584..0d3d1b68232c 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
+++ b/drivers/net/ethernet/netronome/nfp/flower/cmsg.h
@@ -354,6 +354,16 @@ struct nfp_flower_ipv6 {
struct in6_addr ipv6_dst;
};
+struct nfp_flower_tun_ipv4 {
+ __be32 src;
+ __be32 dst;
+};
+
+struct nfp_flower_tun_ip_ext {
+ u8 tos;
+ u8 ttl;
+};
+
/* Flow Frame IPv4 UDP TUNNEL --> Tunnel details (4W/16B)
* -----------------------------------------------------------------
* 3 2 1
@@ -371,11 +381,9 @@ struct nfp_flower_ipv6 {
* +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
*/
struct nfp_flower_ipv4_udp_tun {
- __be32 ip_src;
- __be32 ip_dst;
+ struct nfp_flower_tun_ipv4 ipv4;
__be16 reserved1;
- u8 tos;
- u8 ttl;
+ struct nfp_flower_tun_ip_ext ip_ext;
__be32 reserved2;
__be32 tun_id;
};
diff --git a/drivers/net/ethernet/netronome/nfp/flower/match.c b/drivers/net/ethernet/netronome/nfp/flower/match.c
index 371b5be33dc7..9181611087c2 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/match.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/match.c
@@ -280,6 +280,42 @@ nfp_flower_compile_geneve_opt(void *ext, void *msk,
return 0;
}
+static void
+nfp_flower_compile_tun_ipv4_addrs(struct nfp_flower_tun_ipv4 *ext,
+ struct nfp_flower_tun_ipv4 *msk,
+ struct tc_cls_flower_offload *flow)
+{
+ struct flow_rule *rule = tc_cls_flower_offload_flow_rule(flow);
+
+ if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS)) {
+ struct flow_match_ipv4_addrs match;
+
+ flow_rule_match_enc_ipv4_addrs(rule, &match);
+ ext->src = match.key->src;
+ ext->dst = match.key->dst;
+ msk->src = match.mask->src;
+ msk->dst = match.mask->dst;
+ }
+}
+
+static void
+nfp_flower_compile_tun_ip_ext(struct nfp_flower_tun_ip_ext *ext,
+ struct nfp_flower_tun_ip_ext *msk,
+ struct tc_cls_flower_offload *flow)
+{
+ struct flow_rule *rule = tc_cls_flower_offload_flow_rule(flow);
+
+ if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_IP)) {
+ struct flow_match_ip match;
+
+ flow_rule_match_enc_ip(rule, &match);
+ ext->tos = match.key->tos;
+ ext->ttl = match.key->ttl;
+ msk->tos = match.mask->tos;
+ msk->ttl = match.mask->ttl;
+ }
+}
+
static void
nfp_flower_compile_ipv4_udp_tun(struct nfp_flower_ipv4_udp_tun *ext,
struct nfp_flower_ipv4_udp_tun *msk,
@@ -301,25 +337,8 @@ nfp_flower_compile_ipv4_udp_tun(struct nfp_flower_ipv4_udp_tun *ext,
msk->tun_id = cpu_to_be32(temp_vni);
}
- if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS)) {
- struct flow_match_ipv4_addrs match;
-
- flow_rule_match_enc_ipv4_addrs(rule, &match);
- ext->ip_src = match.key->src;
- ext->ip_dst = match.key->dst;
- msk->ip_src = match.mask->src;
- msk->ip_dst = match.mask->dst;
- }
-
- if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_IP)) {
- struct flow_match_ip match;
-
- flow_rule_match_enc_ip(rule, &match);
- ext->tos = match.key->tos;
- ext->ttl = match.key->ttl;
- msk->tos = match.mask->tos;
- msk->ttl = match.mask->ttl;
- }
+ nfp_flower_compile_tun_ipv4_addrs(&ext->ipv4, &msk->ipv4, flow);
+ nfp_flower_compile_tun_ip_ext(&ext->ip_ext, &msk->ip_ext, flow);
}
int nfp_flower_compile_flow_match(struct nfp_app *app,
@@ -411,7 +430,7 @@ int nfp_flower_compile_flow_match(struct nfp_app *app,
__be32 tun_dst;
nfp_flower_compile_ipv4_udp_tun((void *)ext, (void *)msk, flow);
- tun_dst = ((struct nfp_flower_ipv4_udp_tun *)ext)->ip_dst;
+ tun_dst = ((struct nfp_flower_ipv4_udp_tun *)ext)->ipv4.dst;
ext += sizeof(struct nfp_flower_ipv4_udp_tun);
msk += sizeof(struct nfp_flower_ipv4_udp_tun);
--
2.21.0
^ permalink raw reply related
* [PATCH net-next 1/5] nfp: flower: refactor tunnel key layer calculation
From: Jakub Kicinski @ 2019-06-27 23:12 UTC (permalink / raw)
To: davem
Cc: netdev, oss-drivers, Pieter Jansen van Vuuren, Jakub Kicinski,
John Hurley
In-Reply-To: <20190627231243.8323-1-jakub.kicinski@netronome.com>
From: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Refactor the key layer calculation function, in particular the tunnel
key layer calculation by introducing helper functions. This is done
in preparation for supporting GRE tunnel offloads.
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: John Hurley <john.hurley@netronome.com>
---
.../ethernet/netronome/nfp/flower/offload.c | 100 +++++++++++-------
1 file changed, 60 insertions(+), 40 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 39e6599f2bd7..6b28910442db 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -141,16 +141,16 @@ static bool nfp_flower_check_higher_than_l3(struct tc_cls_flower_offload *f)
}
static int
-nfp_flower_calc_opt_layer(struct flow_match_enc_opts *enc_opts,
+nfp_flower_calc_opt_layer(struct flow_dissector_key_enc_opts *enc_opts,
u32 *key_layer_two, int *key_size,
struct netlink_ext_ack *extack)
{
- if (enc_opts->key->len > NFP_FL_MAX_GENEVE_OPT_KEY) {
+ if (enc_opts->len > NFP_FL_MAX_GENEVE_OPT_KEY) {
NL_SET_ERR_MSG_MOD(extack, "unsupported offload: geneve options exceed maximum length");
return -EOPNOTSUPP;
}
- if (enc_opts->key->len > 0) {
+ if (enc_opts->len > 0) {
*key_layer_two |= NFP_FLOWER_LAYER2_GENEVE_OP;
*key_size += sizeof(struct nfp_flower_geneve_options);
}
@@ -158,6 +158,57 @@ nfp_flower_calc_opt_layer(struct flow_match_enc_opts *enc_opts,
return 0;
}
+static int
+nfp_flower_calc_udp_tun_layer(struct flow_dissector_key_ports *enc_ports,
+ struct flow_dissector_key_enc_opts *enc_op,
+ u32 *key_layer_two, u8 *key_layer, int *key_size,
+ struct nfp_flower_priv *priv,
+ enum nfp_flower_tun_type *tun_type,
+ struct netlink_ext_ack *extack)
+{
+ int err;
+
+ switch (enc_ports->dst) {
+ case htons(IANA_VXLAN_UDP_PORT):
+ *tun_type = NFP_FL_TUNNEL_VXLAN;
+ *key_layer |= NFP_FLOWER_LAYER_VXLAN;
+ *key_size += sizeof(struct nfp_flower_ipv4_udp_tun);
+
+ if (enc_op) {
+ NL_SET_ERR_MSG_MOD(extack, "unsupported offload: encap options not supported on vxlan tunnels");
+ return -EOPNOTSUPP;
+ }
+ break;
+ case htons(GENEVE_UDP_PORT):
+ if (!(priv->flower_ext_feats & NFP_FL_FEATS_GENEVE)) {
+ NL_SET_ERR_MSG_MOD(extack, "unsupported offload: loaded firmware does not support geneve offload");
+ return -EOPNOTSUPP;
+ }
+ *tun_type = NFP_FL_TUNNEL_GENEVE;
+ *key_layer |= NFP_FLOWER_LAYER_EXT_META;
+ *key_size += sizeof(struct nfp_flower_ext_meta);
+ *key_layer_two |= NFP_FLOWER_LAYER2_GENEVE;
+ *key_size += sizeof(struct nfp_flower_ipv4_udp_tun);
+
+ if (!enc_op)
+ break;
+ if (!(priv->flower_ext_feats & NFP_FL_FEATS_GENEVE_OPT)) {
+ NL_SET_ERR_MSG_MOD(extack, "unsupported offload: loaded firmware does not support geneve option offload");
+ return -EOPNOTSUPP;
+ }
+ err = nfp_flower_calc_opt_layer(enc_op, key_layer_two,
+ key_size, extack);
+ if (err)
+ return err;
+ break;
+ default:
+ NL_SET_ERR_MSG_MOD(extack, "unsupported offload: tunnel type unknown");
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
static int
nfp_flower_calculate_key_layers(struct nfp_app *app,
struct net_device *netdev,
@@ -243,44 +294,13 @@ nfp_flower_calculate_key_layers(struct nfp_app *app,
if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_OPTS))
flow_rule_match_enc_opts(rule, &enc_op);
- switch (enc_ports.key->dst) {
- case htons(IANA_VXLAN_UDP_PORT):
- *tun_type = NFP_FL_TUNNEL_VXLAN;
- key_layer |= NFP_FLOWER_LAYER_VXLAN;
- key_size += sizeof(struct nfp_flower_ipv4_udp_tun);
-
- if (enc_op.key) {
- NL_SET_ERR_MSG_MOD(extack, "unsupported offload: encap options not supported on vxlan tunnels");
- return -EOPNOTSUPP;
- }
- break;
- case htons(GENEVE_UDP_PORT):
- if (!(priv->flower_ext_feats & NFP_FL_FEATS_GENEVE)) {
- NL_SET_ERR_MSG_MOD(extack, "unsupported offload: loaded firmware does not support geneve offload");
- return -EOPNOTSUPP;
- }
- *tun_type = NFP_FL_TUNNEL_GENEVE;
- key_layer |= NFP_FLOWER_LAYER_EXT_META;
- key_size += sizeof(struct nfp_flower_ext_meta);
- key_layer_two |= NFP_FLOWER_LAYER2_GENEVE;
- key_size += sizeof(struct nfp_flower_ipv4_udp_tun);
- if (!enc_op.key)
- break;
- if (!(priv->flower_ext_feats &
- NFP_FL_FEATS_GENEVE_OPT)) {
- NL_SET_ERR_MSG_MOD(extack, "unsupported offload: loaded firmware does not support geneve option offload");
- return -EOPNOTSUPP;
- }
- err = nfp_flower_calc_opt_layer(&enc_op, &key_layer_two,
- &key_size, extack);
- if (err)
- return err;
- break;
- default:
- NL_SET_ERR_MSG_MOD(extack, "unsupported offload: tunnel type unknown");
- return -EOPNOTSUPP;
- }
+ err = nfp_flower_calc_udp_tun_layer(enc_ports.key, enc_op.key,
+ &key_layer_two, &key_layer,
+ &key_size, priv, tun_type,
+ extack);
+ if (err)
+ return err;
/* Ensure the ingress netdev matches the expected tun type. */
if (!nfp_fl_netdev_is_tunnel_type(netdev, *tun_type)) {
--
2.21.0
^ permalink raw reply related
* [PATCH net-next 0/5] nfp: extend flower capabilities for GRE tunnel offload
From: Jakub Kicinski @ 2019-06-27 23:12 UTC (permalink / raw)
To: davem; +Cc: netdev, oss-drivers, Jakub Kicinski
Pieter says:
This set extends the flower match and action components to offload
GRE decapsulation with classification and encapsulation actions. The
first 3 patches are refactor and cleanup patches for improving
readability and reusability. Patch 4 and 5 implement GRE decap and
encap functionality respectively.
Pieter Jansen van Vuuren (5):
nfp: flower: refactor tunnel key layer calculation
nfp: flower: add helper functions for tunnel classification
nfp: flower: rename tunnel related functions in action offload
nfp: flower: add GRE decap classification support
nfp: flower: add GRE encap action support
.../ethernet/netronome/nfp/flower/action.c | 59 +++++---
.../net/ethernet/netronome/nfp/flower/cmsg.h | 57 +++++++-
.../net/ethernet/netronome/nfp/flower/match.c | 103 +++++++++++---
.../ethernet/netronome/nfp/flower/offload.c | 133 ++++++++++++------
4 files changed, 263 insertions(+), 89 deletions(-)
--
2.21.0
^ permalink raw reply
* RE: [RFC net-next 1/5] net: stmmac: introduce IEEE 802.1Qbv configuration functionalities
From: Ong, Boon Leong @ 2019-06-27 23:08 UTC (permalink / raw)
To: Jose Abreu, Voon, Weifeng, David S. Miller, Maxime Coquelin
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Giuseppe Cavallaro, Andrew Lunn, Florian Fainelli,
Alexandre Torgue, Gomes, Vinicius
In-Reply-To: <BN8PR12MB32668CB3930DD0D9565D15B0D3FD0@BN8PR12MB3266.namprd12.prod.outlook.com>
>-----Original Message-----
>From: Jose Abreu [mailto:Jose.Abreu@synopsys.com]
>>From: Voon Weifeng <weifeng.voon@intel.com>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dw_tsn_lib.c
>b/drivers/net/ethernet/stmicro/stmmac/dw_tsn_lib.c
>> new file mode 100644
>> index 000000000000..cba27c604cb1
>> --- /dev/null
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dw_tsn_lib.c
>
>XGMAC also supports TSN features so I think more abstraction is needed
>on this because the XGMAC implementation is very similar (only reg
>offsets and bitfields changes).
>
>I would rather:
> - Implement HW specific handling in dwmac4_core.c / dwmac4_dma.c
>and
>add the callbacks in hwif table;
> - Let TSN logic in this file but call it stmmac_tsn.c.
OK. Thanks for above feedback.
>
>> @@ -3621,6 +3622,8 @@ static int stmmac_set_features(struct net_device
>*netdev,
>> */
>> stmmac_rx_ipc(priv, priv->hw);
>>
>> + netdev->features = features;
>
>Isn't this a fix ?
Yup. We will split this out as a patch and send separately.
^ permalink raw reply
* Re: linux-next: manual merge of the mlx5-next tree with the net-next tree
From: Saeed Mahameed @ 2019-06-27 22:59 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Leon Romanovsky, David Miller, Networking,
Linux Next Mailing List, Linux Kernel Mailing List,
Yevgeny Kliteynik, Saeed Mahameed, Eli Britstein, Jianbo Liu
In-Reply-To: <20190627140929.74ae7da6@canb.auug.org.au>
On Wed, Jun 26, 2019 at 9:09 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi all,
>
> Today's linux-next merge of the mlx5-next tree got a conflict in:
>
> drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
>
> between commits:
>
> 955858009708 ("net/mlx5e: Fix number of vports for ingress ACL configuration")
> d4a18e16c570 ("net/mlx5e: Enable setting multiple match criteria for flow group")
>
> from the net-next tree and commits:
>
> 7445cfb1169c ("net/mlx5: E-Switch, Tag packet with vport number in VF vports and uplink ingress ACLs")
> c01cfd0f1115 ("net/mlx5: E-Switch, Add match on vport metadata for rule in fast path")
>
> from the mlx5-next tree.
>
> I fixed it up (I basically used the latter versions) and can carry the
> fix as necessary. This is now fixed as far as linux-next is concerned,
> but any non trivial conflicts should be mentioned to your upstream
> maintainer when your tree is submitted for merging. You may also want
> to consider cooperating with the maintainer of the conflicting tree to
> minimise any particularly complex conflicts.
>
Thanks Stephen, this will be handled in my next pull request to net-next.
> --
> Cheers,
> Stephen Rothwell
^ permalink raw reply
* [PATCH net] sctp: fix error handling on stream scheduler initialization
From: Marcelo Ricardo Leitner @ 2019-06-27 22:48 UTC (permalink / raw)
To: netdev; +Cc: Xin Long, Neil Horman, linux-sctp, Hillf Danton
It allocates the extended area for outbound streams only on sendmsg
calls, if they are not yet allocated. When using the priority
stream scheduler, this initialization may imply into a subsequent
allocation, which may fail. In this case, it was aborting the stream
scheduler initialization but leaving the ->ext pointer (allocated) in
there, thus in a partially initialized state. On a subsequent call to
sendmsg, it would notice the ->ext pointer in there, and trip on
uninitialized stuff when trying to schedule the data chunk.
The fix is undo the ->ext initialization if the stream scheduler
initialization fails and avoid the partially initialized state.
Although syzkaller bisected this to commit 4ff40b86262b ("sctp: set
chunk transport correctly when it's a new asoc"), this bug was actually
introduced on the commit I marked below.
Reported-by: syzbot+c1a380d42b190ad1e559@syzkaller.appspotmail.com
Fixes: 5bbbbe32a431 ("sctp: introduce stream scheduler foundations")
Tested-by: Xin Long <lucien.xin@gmail.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
net/sctp/stream.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 93ed07877337eace4ef7f4775dda5868359ada37..25946604af85c09917e63e5c4a8d7d6fa2caebc4 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -153,13 +153,20 @@ int sctp_stream_init(struct sctp_stream *stream, __u16 outcnt, __u16 incnt,
int sctp_stream_init_ext(struct sctp_stream *stream, __u16 sid)
{
struct sctp_stream_out_ext *soute;
+ int ret;
soute = kzalloc(sizeof(*soute), GFP_KERNEL);
if (!soute)
return -ENOMEM;
SCTP_SO(stream, sid)->ext = soute;
- return sctp_sched_init_sid(stream, sid, GFP_KERNEL);
+ ret = sctp_sched_init_sid(stream, sid, GFP_KERNEL);
+ if (ret) {
+ kfree(SCTP_SO(stream, sid)->ext);
+ SCTP_SO(stream, sid)->ext = NULL;
+ }
+
+ return ret;
}
void sctp_stream_free(struct sctp_stream *stream)
--
2.21.0
^ permalink raw reply related
* Re: [PATCH bpf-next v9 0/9] bpf: getsockopt and setsockopt hooks
From: Alexei Starovoitov @ 2019-06-27 22:46 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, Network Development, bpf, David S. Miller,
Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <20190627224341.GE4866@mini-arch>
On Thu, Jun 27, 2019 at 3:43 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> > There is a build warning though:
> > test_sockopt_sk.c: In function ‘getsetsockopt’:
> > test_sockopt_sk.c:115:2: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
> > if (*(__u32 *)buf != 0x55AA*2) {
> > ^~
> > test_sockopt_sk.c:116:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
> > log_err("Unexpected getsockopt(SO_SNDBUF) 0x%x != 0x55AA*2",
> > ^~~~~~~
> >
> > Pls fix it in the follow up.
> Sure, but I can't reproduce it with gcc7 nor with clang9 :-/
I see it with gcc 6.3
^ permalink raw reply
* Re: linux-next: Fixes tag needs some work in the bpf tree
From: Roman Gushchin @ 2019-06-27 22:43 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Stephen Rothwell, Alexei Starovoitov, Alexei Starovoitov,
Networking, Linux Next Mailing List, Linux Kernel Mailing List
In-Reply-To: <134f90ff-13f8-b7c1-9693-2f2649245c38@iogearbox.net>
On Fri, Jun 28, 2019 at 12:25:54AM +0200, Daniel Borkmann wrote:
> On 06/27/2019 03:45 AM, Stephen Rothwell wrote:
> > Hi all,
> >
> > On Wed, 26 Jun 2019 16:36:50 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >>
> >> On Wed, Jun 26, 2019 at 3:14 PM Roman Gushchin <guro@fb.com> wrote:
> >>>
> >>> On Thu, Jun 27, 2019 at 08:05:21AM +1000, Stephen Rothwell wrote:
> >>>>
> >>>> In commit
> >>>>
> >>>> 12771345a467 ("bpf: fix cgroup bpf release synchronization")
> >>>>
> >>>> Fixes tag
> >>>>
> >>>> Fixes: 4bfc0bb2c60e ("bpf: decouple the lifetime of cgroup_bpf from
> >>>>
> >>>> has these problem(s):
> >>>>
> >>>> - Subject has leading but no trailing parentheses
> >>>> - Subject has leading but no trailing quotes
> >>>>
> >>>> Please don't split Fixes tags across more than one line.
> >>>
> >>> Oops, sorry.
> >>>
> >>> Alexei, can you fix this in place?
> >>> Or should I send an updated version?
> >>
> >> I cannot easily do it since -p and --signoff are incompatible flags.
> >> I need to use -p to preserve merge commits,
> >> but I also need to use --signoff to add my sob to all
> >> other commits that were committed by Daniel
> >> after your commit.
> >>
> >> Daniel, can you fix Roman's patch instead?
> >> you can do:
> >> git rebase -i -p 12771345a467^
> >> fix Roman's, add you sob only to that one
> >> and re-push the whole thing.
>
> (Fixed in bpf-next.)
Thank you, Daniel!
^ permalink raw reply
* Re: [PATCH bpf-next v9 0/9] bpf: getsockopt and setsockopt hooks
From: Stanislav Fomichev @ 2019-06-27 22:43 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: Stanislav Fomichev, netdev, bpf, davem, ast, daniel
In-Reply-To: <20190627223147.vkkmbtdcvjzas2ej@ast-mbp.dhcp.thefacebook.com>
On 06/27, Alexei Starovoitov wrote:
> On Thu, Jun 27, 2019 at 01:38:46PM -0700, Stanislav Fomichev wrote:
> > This series implements two new per-cgroup hooks: getsockopt and
> > setsockopt along with a new sockopt program type. The idea is pretty
> > similar to recently introduced cgroup sysctl hooks, but
> > implementation is simpler (no need to convert to/from strings).
> >
> > What this can be applied to:
> > * move business logic of what tos/priority/etc can be set by
> > containers (either pass or reject)
> > * handle existing options (or introduce new ones) differently by
> > propagating some information in cgroup/socket local storage
> >
> > Compared to a simple syscall/{g,s}etsockopt tracepoint, those
> > hooks are context aware. Meaning, they can access underlying socket
> > and use cgroup and socket local storage.
> >
> > v9:
> > * allow overwriting setsocktop arguments (Alexei Starovoitov)
> > (see individual changes for more changelog details)
>
> Applied. Thanks.
Great, thanks for all the reviews!
> There is a build warning though:
> test_sockopt_sk.c: In function ‘getsetsockopt’:
> test_sockopt_sk.c:115:2: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
> if (*(__u32 *)buf != 0x55AA*2) {
> ^~
> test_sockopt_sk.c:116:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
> log_err("Unexpected getsockopt(SO_SNDBUF) 0x%x != 0x55AA*2",
> ^~~~~~~
>
> Pls fix it in the follow up.
Sure, but I can't reproduce it with gcc7 nor with clang9 :-/
Presumably, a switch to __get_unaligned_cpu32 should help,
I'll try to play with compilers a bit before sending a fix.
^ permalink raw reply
* Re: [PATCH 4/6 bfp-next] Simplify AF_XDP umem allocation path for Intel drivers.
From: Jakub Kicinski @ 2019-06-27 22:42 UTC (permalink / raw)
To: Jonathan Lemon
Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
kernel-team
In-Reply-To: <20190627220836.2572684-5-jonathan.lemon@gmail.com>
On Thu, 27 Jun 2019 15:08:34 -0700, Jonathan Lemon wrote:
> Now that the recycle stack is always used for the driver umem path, the
> driver code path can be simplified.
>
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
I guess it's a question to Bjorn and Magnus whether they want Intel
drivers to always go through the reuse queue..
Could you be more explicit on the motivation? I'd call this patch set
"make all drivers use reuse queue" rather than "clean up".
Also when you're changing code please make sure you CC the author.
^ permalink raw reply
* Re: [PATCH 2/6 bpf-next] Clean up xsk reuseq API
From: Jakub Kicinski @ 2019-06-27 22:38 UTC (permalink / raw)
To: Jonathan Lemon
Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
kernel-team
In-Reply-To: <20190627220836.2572684-3-jonathan.lemon@gmail.com>
On Thu, 27 Jun 2019 15:08:32 -0700, Jonathan Lemon wrote:
> The reuseq is actually a recycle stack, only accessed from the kernel side.
> Also, the implementation details of the stack should belong to the umem
> object, and not exposed to the caller.
>
> Clean up and rename for consistency in preparation for the next patch.
>
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
Prepare/swap is to cater to how drivers should be written - being able
to allocate resources independently of those currently used. Allowing
for changing ring sizes and counts on the fly. This patch makes it
harder to write drivers in the way we are encouraging people to.
IOW no, please don't do this.
^ permalink raw reply
* Re: [PATCH net] sctp: not bind the socket in sctp_connect
From: Marcelo Ricardo Leitner @ 2019-06-27 22:32 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman, syzkaller-bugs
In-Reply-To: <35a0e4f6ca68185117c6e5517d8ac924cc2f9d05.1561537899.git.lucien.xin@gmail.com>
On Wed, Jun 26, 2019 at 04:31:39PM +0800, Xin Long wrote:
> Now when sctp_connect() is called with a wrong sa_family, it binds
> to a port but doesn't set bp->port, then sctp_get_af_specific will
> return NULL and sctp_connect() returns -EINVAL.
>
> Then if sctp_bind() is called to bind to another port, the last
> port it has bound will leak due to bp->port is NULL by then.
>
> sctp_connect() doesn't need to bind ports, as later __sctp_connect
> will do it if bp->port is NULL. So remove it from sctp_connect().
> While at it, remove the unnecessary sockaddr.sa_family len check
> as it's already done in sctp_inet_connect.
>
> Fixes: 644fbdeacf1d ("sctp: fix the issue that flags are ignored when using kernel_connect")
> Reported-by: syzbot+079bf326b38072f849d9@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/socket.c | 24 +++---------------------
> 1 file changed, 3 insertions(+), 21 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 39ea0a3..f33aa9e 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -4816,35 +4816,17 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
> static int sctp_connect(struct sock *sk, struct sockaddr *addr,
> int addr_len, int flags)
> {
> - struct inet_sock *inet = inet_sk(sk);
> struct sctp_af *af;
> - int err = 0;
> + int err = -EINVAL;
>
> lock_sock(sk);
> -
> pr_debug("%s: sk:%p, sockaddr:%p, addr_len:%d\n", __func__, sk,
> addr, addr_len);
>
> - /* We may need to bind the socket. */
> - if (!inet->inet_num) {
> - if (sk->sk_prot->get_port(sk, 0)) {
> - release_sock(sk);
> - return -EAGAIN;
> - }
> - inet->inet_sport = htons(inet->inet_num);
> - }
> -
> /* Validate addr_len before calling common connect/connectx routine. */
> - af = addr_len < offsetofend(struct sockaddr, sa_family) ? NULL :
> - sctp_get_af_specific(addr->sa_family);
> - if (!af || addr_len < af->sockaddr_len) {
> - err = -EINVAL;
> - } else {
> - /* Pass correct addr len to common routine (so it knows there
> - * is only one address being passed.
> - */
> + af = sctp_get_af_specific(addr->sa_family);
> + if (af && addr_len >= af->sockaddr_len)
> err = __sctp_connect(sk, addr, af->sockaddr_len, flags, NULL);
> - }
>
> release_sock(sk);
> return err;
> --
> 2.1.0
>
^ permalink raw reply
* Re: [PATCH bpf-next v9 0/9] bpf: getsockopt and setsockopt hooks
From: Alexei Starovoitov @ 2019-06-27 22:31 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: netdev, bpf, davem, ast, daniel
In-Reply-To: <20190627203855.10515-1-sdf@google.com>
On Thu, Jun 27, 2019 at 01:38:46PM -0700, Stanislav Fomichev wrote:
> This series implements two new per-cgroup hooks: getsockopt and
> setsockopt along with a new sockopt program type. The idea is pretty
> similar to recently introduced cgroup sysctl hooks, but
> implementation is simpler (no need to convert to/from strings).
>
> What this can be applied to:
> * move business logic of what tos/priority/etc can be set by
> containers (either pass or reject)
> * handle existing options (or introduce new ones) differently by
> propagating some information in cgroup/socket local storage
>
> Compared to a simple syscall/{g,s}etsockopt tracepoint, those
> hooks are context aware. Meaning, they can access underlying socket
> and use cgroup and socket local storage.
>
> v9:
> * allow overwriting setsocktop arguments (Alexei Starovoitov)
> (see individual changes for more changelog details)
Applied. Thanks.
There is a build warning though:
test_sockopt_sk.c: In function ‘getsetsockopt’:
test_sockopt_sk.c:115:2: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
if (*(__u32 *)buf != 0x55AA*2) {
^~
test_sockopt_sk.c:116:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
log_err("Unexpected getsockopt(SO_SNDBUF) 0x%x != 0x55AA*2",
^~~~~~~
Pls fix it in the follow up.
^ permalink raw reply
* Re: linux-next: Fixes tag needs some work in the bpf tree
From: Daniel Borkmann @ 2019-06-27 22:25 UTC (permalink / raw)
To: Stephen Rothwell, Alexei Starovoitov
Cc: Roman Gushchin, Alexei Starovoitov, Networking,
Linux Next Mailing List, Linux Kernel Mailing List
In-Reply-To: <20190627114536.09c08f5d@canb.auug.org.au>
On 06/27/2019 03:45 AM, Stephen Rothwell wrote:
> Hi all,
>
> On Wed, 26 Jun 2019 16:36:50 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>
>> On Wed, Jun 26, 2019 at 3:14 PM Roman Gushchin <guro@fb.com> wrote:
>>>
>>> On Thu, Jun 27, 2019 at 08:05:21AM +1000, Stephen Rothwell wrote:
>>>>
>>>> In commit
>>>>
>>>> 12771345a467 ("bpf: fix cgroup bpf release synchronization")
>>>>
>>>> Fixes tag
>>>>
>>>> Fixes: 4bfc0bb2c60e ("bpf: decouple the lifetime of cgroup_bpf from
>>>>
>>>> has these problem(s):
>>>>
>>>> - Subject has leading but no trailing parentheses
>>>> - Subject has leading but no trailing quotes
>>>>
>>>> Please don't split Fixes tags across more than one line.
>>>
>>> Oops, sorry.
>>>
>>> Alexei, can you fix this in place?
>>> Or should I send an updated version?
>>
>> I cannot easily do it since -p and --signoff are incompatible flags.
>> I need to use -p to preserve merge commits,
>> but I also need to use --signoff to add my sob to all
>> other commits that were committed by Daniel
>> after your commit.
>>
>> Daniel, can you fix Roman's patch instead?
>> you can do:
>> git rebase -i -p 12771345a467^
>> fix Roman's, add you sob only to that one
>> and re-push the whole thing.
(Fixed in bpf-next.)
^ permalink raw reply
* Re: [RFC PATCH bpf-next v2 0/6] bpf: add BPF_MAP_DUMP command to
From: Alexei Starovoitov @ 2019-06-27 22:14 UTC (permalink / raw)
To: Brian Vazquez
Cc: Brian Vazquez, Alexei Starovoitov, Daniel Borkmann,
David S . Miller, Stanislav Fomichev, Willem de Bruijn,
Petar Penkov, linux-kernel, netdev, bpf
In-Reply-To: <20190627202417.33370-1-brianvv@google.com>
On Thu, Jun 27, 2019 at 01:24:11PM -0700, Brian Vazquez wrote:
> This introduces a new command to retrieve a variable number of entries
> from a bpf map.
>
> This new command can be executed from the existing BPF syscall as
> follows:
>
> err = bpf(BPF_MAP_DUMP, union bpf_attr *attr, u32 size)
> using attr->dump.map_fd, attr->dump.prev_key, attr->dump.buf,
> attr->dump.buf_len
> returns zero or negative error, and populates buf and buf_len on
> succees
>
> This implementation is wrapping the existing bpf methods:
> map_get_next_key and map_lookup_elem
> the results show that even with a 1-elem_size buffer, it runs ~40 faster
> than the current implementation, improvements of ~85% are reported when
> the buffer size is increased, although, after the buffer size is around
> 5% of the total number of entries there's no huge difference in
> increasing
> it.
was it with kpti and retpoline mitigations?
^ permalink raw reply
* Re: [PATCH bpf-next v5 1/3] devmap/cpumap: Use flush list instead of bitmap
From: Daniel Borkmann @ 2019-06-27 22:14 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, netdev
Cc: Jesper Dangaard Brouer, Alexei Starovoitov, David Miller,
Jonathan Lemon
In-Reply-To: <156125626115.5209.3880071777007082264.stgit@alrua-x1>
On 06/23/2019 04:17 AM, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> The socket map uses a linked list instead of a bitmap to keep track of
> which entries to flush. Do the same for devmap and cpumap, as this means we
> don't have to care about the map index when enqueueing things into the
> map (and so we can cache the map lookup).
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
[...]
> +static int bq_flush_to_queue(struct xdp_bulk_queue *bq, bool in_napi_ctx)
> {
> + struct bpf_cpu_map_entry *rcpu = bq->obj;
> unsigned int processed = 0, drops = 0;
> const int to_cpu = rcpu->cpu;
> struct ptr_ring *q;
> @@ -621,6 +630,9 @@ static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
> bq->count = 0;
> spin_unlock(&q->producer_lock);
>
> + __list_del(bq->flush_node.prev, bq->flush_node.next);
> + bq->flush_node.prev = NULL;
Given this and below is a bit non-standard way of using list API, maybe add
these as inline helpers to include/linux/list.h to make sure anyone changing
list API semantics doesn't overlook these in future?
> /* Feedback loop via tracepoints */
> trace_xdp_cpumap_enqueue(rcpu->map_id, processed, drops, to_cpu);
> return 0;
[...]
> +
> + if (!bq->flush_node.prev)
> + list_add(&bq->flush_node, flush_list);
> +
> return 0;
> }
>
^ permalink raw reply
* Re: [RFC PATCH bpf-next v2 2/6] bpf: add BPF_MAP_DUMP command to access more than one entry per call
From: Alexei Starovoitov @ 2019-06-27 22:12 UTC (permalink / raw)
To: Brian Vazquez
Cc: Brian Vazquez, Alexei Starovoitov, Daniel Borkmann,
David S . Miller, Stanislav Fomichev, Willem de Bruijn,
Petar Penkov, linux-kernel, netdev, bpf
In-Reply-To: <20190627202417.33370-3-brianvv@google.com>
On Thu, Jun 27, 2019 at 01:24:13PM -0700, Brian Vazquez wrote:
> This introduces a new command to retrieve a variable number of entries
> from a bpf map wrapping the existing bpf methods:
> map_get_next_key and map_lookup_elem
>
> Note that map_dump doesn't guarantee that reading the entire table is
> consistent since this function is always racing with kernel and user code
> but the same behaviour is found when the entire table is walked using
> the current interfaces: map_get_next_key + map_lookup_elem.
> It is also important to note that when a locked map is provided it is
> consistent only for 1 entry at the time, meaning that the buf returned
> might or might not be consistent.
Please explain the api behavior and corner cases in the commit log
or in code comments.
Would it make sense to return last key back into prev_key,
so that next map_dump command doesn't need to copy it from the
buffer?
> Suggested-by: Stanislav Fomichev <sdf@google.com>
> Signed-off-by: Brian Vazquez <brianvv@google.com>
> ---
> include/uapi/linux/bpf.h | 9 ++++
> kernel/bpf/syscall.c | 108 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 117 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index b077507efa3f3..1d753958874df 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -106,6 +106,7 @@ enum bpf_cmd {
> BPF_TASK_FD_QUERY,
> BPF_MAP_LOOKUP_AND_DELETE_ELEM,
> BPF_MAP_FREEZE,
> + BPF_MAP_DUMP,
> };
>
> enum bpf_map_type {
> @@ -385,6 +386,14 @@ union bpf_attr {
> __u64 flags;
> };
>
> + struct { /* struct used by BPF_MAP_DUMP command */
> + __u32 map_fd;
> + __aligned_u64 prev_key;
> + __aligned_u64 buf;
> + __aligned_u64 buf_len; /* input/output: len of buf */
> + __u64 flags;
> + } dump;
> +
> struct { /* anonymous struct used by BPF_PROG_LOAD command */
> __u32 prog_type; /* one of enum bpf_prog_type */
> __u32 insn_cnt;
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index a1823a50f9be0..7653346b5cfd1 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1097,6 +1097,111 @@ static int map_get_next_key(union bpf_attr *attr)
> return err;
> }
>
> +/* last field in 'union bpf_attr' used by this command */
> +#define BPF_MAP_DUMP_LAST_FIELD dump.buf_len
> +
> +static int map_dump(union bpf_attr *attr)
> +{
> + void __user *ukey = u64_to_user_ptr(attr->dump.prev_key);
> + void __user *ubuf = u64_to_user_ptr(attr->dump.buf);
> + u32 __user *ubuf_len = u64_to_user_ptr(attr->dump.buf_len);
> + int ufd = attr->dump.map_fd;
> + struct bpf_map *map;
> + void *buf, *prev_key, *key, *value;
> + u32 value_size, elem_size, buf_len, cp_len;
> + struct fd f;
> + int err;
> +
> + if (CHECK_ATTR(BPF_MAP_DUMP))
> + return -EINVAL;
> +
> + attr->flags = 0;
> + if (attr->dump.flags & ~BPF_F_LOCK)
> + return -EINVAL;
> +
> + f = fdget(ufd);
> + map = __bpf_map_get(f);
> + if (IS_ERR(map))
> + return PTR_ERR(map);
> + if (!(map_get_sys_perms(map, f) & FMODE_CAN_READ)) {
> + err = -EPERM;
> + goto err_put;
> + }
> +
> + if ((attr->dump.flags & BPF_F_LOCK) &&
> + !map_value_has_spin_lock(map)) {
> + err = -EINVAL;
> + goto err_put;
> + }
> +
> + if (map->map_type == BPF_MAP_TYPE_QUEUE ||
> + map->map_type == BPF_MAP_TYPE_STACK) {
> + err = -ENOTSUPP;
> + goto err_put;
> + }
> +
> + value_size = bpf_map_value_size(map);
> +
> + err = get_user(buf_len, ubuf_len);
> + if (err)
> + goto err_put;
> +
> + elem_size = map->key_size + value_size;
> + if (buf_len < elem_size) {
> + err = -EINVAL;
> + goto err_put;
> + }
> +
> + if (ukey) {
> + prev_key = __bpf_copy_key(ukey, map->key_size);
> + if (IS_ERR(prev_key)) {
> + err = PTR_ERR(prev_key);
> + goto err_put;
> + }
> + } else {
> + prev_key = NULL;
> + }
> +
> + err = -ENOMEM;
> + buf = kmalloc(elem_size, GFP_USER | __GFP_NOWARN);
> + if (!buf)
> + goto err_put;
> +
> + key = buf;
> + value = key + map->key_size;
> + for (cp_len = 0; cp_len + elem_size <= buf_len ; cp_len += elem_size) {
checkpatch.pl please.
> +next:
> + if (signal_pending(current)) {
> + err = -EINTR;
> + break;
> + }
> +
> + rcu_read_lock();
> + err = map->ops->map_get_next_key(map, prev_key, key);
> + rcu_read_unlock();
> +
> + if (err)
> + break;
should probably be only for ENOENT case?
and other errors should be returned to user ?
> +
> + if (bpf_map_copy_value(map, key, value, attr->dump.flags))
> + goto next;
only for ENOENT as well?
and instead of goto use continue and move cp_len+= to the end after prev_key=key?
> +
> + if (copy_to_user(ubuf + cp_len, buf, elem_size))
> + break;
return error to user?
> +
> + prev_key = key;
> + }
> +
> + if (cp_len)
> + err = 0;
this will mask any above errors if there was at least one element copied.
> + if (copy_to_user(ubuf_len, &cp_len, sizeof(cp_len)))
> + err = -EFAULT;
> + kfree(buf);
> +err_put:
> + fdput(f);
> + return err;
> +}
> +
> #define BPF_MAP_LOOKUP_AND_DELETE_ELEM_LAST_FIELD value
>
> static int map_lookup_and_delete_elem(union bpf_attr *attr)
> @@ -2891,6 +2996,9 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
> case BPF_MAP_LOOKUP_AND_DELETE_ELEM:
> err = map_lookup_and_delete_elem(&attr);
> break;
> + case BPF_MAP_DUMP:
> + err = map_dump(&attr);
> + break;
> default:
> err = -EINVAL;
> break;
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
^ 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