* [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 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
* 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
* 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: 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 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 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 1/2] tls: remove close callback sock unlock/lock and flush_sync
From: Jakub Kicinski @ 2019-06-27 23:44 UTC (permalink / raw)
To: John Fastabend; +Cc: daniel, ast, netdev, edumazet, bpf
In-Reply-To: <156165700197.32598.17496423044615153967.stgit@john-XPS-13-9370>
On Thu, 27 Jun 2019 10:36:42 -0700, John Fastabend wrote:
> The tls close() callback currently drops the sock lock, makes a
> cancel_delayed_work_sync() call, and then relocks the sock. This
> seems suspect at best. The lock_sock() is applied to stop concurrent
> operations on the socket while tearing the sock down. Further we
> will need to add support for unhash() shortly and this complicates
> matters because the lock may or may not be held then.
>
> So to fix the above situation and simplify the next patch to add
> unhash this patch creates a function tls_sk_proto_cleanup() that
> tears down the socket without calling lock_sock/release_sock. In
> order to flush the workqueue then we do the following,
>
> - Add a new bit to ctx, BIT_TX_CLOSING that is set when the
> tls resources are being removed.
> - Check this bit before scheduling any new work. This way we
> avoid queueing new work after tear down has started.
> - With the BIT_TX_CLOSING ensuring no new work is being added
> convert the cancel_delayed_work_sync to flush_delayed_work()
> - Finally call tlx_tx_records() to complete any available records
> before,
> - releasing and removing tls ctx.
>
> The above is implemented for the software case namely any of
> the following configurations from build_protos,
>
> prot[TLS_SW][TLS_BASE]
> prot[TLS_BASE][TLS_SW]
> prot[TLS_SW][TLS_SW]
>
> The implication is a follow up patch is needed to resolve the
> hardware offload case.
>
> Tested with net selftests and bpf selftests.
>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
> include/net/tls.h | 4 ++--
> net/tls/tls_main.c | 54 ++++++++++++++++++++++++++--------------------------
> net/tls/tls_sw.c | 50 ++++++++++++++++++++++++++++++++----------------
> 3 files changed, 62 insertions(+), 46 deletions(-)
>
> diff --git a/include/net/tls.h b/include/net/tls.h
> index 4a55ce6a303f..6fe1f5c96f4a 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -105,9 +105,7 @@ struct tls_device {
> enum {
> TLS_BASE,
> TLS_SW,
> -#ifdef CONFIG_TLS_DEVICE
> TLS_HW,
> -#endif
> TLS_HW_RECORD,
> TLS_NUM_CONFIG,
> };
> @@ -160,6 +158,7 @@ struct tls_sw_context_tx {
> int async_capable;
>
> #define BIT_TX_SCHEDULED 0
BTW do you understand why we track this bit separately? Just to avoid
the irq operations in the workqueue code?
> +#define BIT_TX_CLOSING 1
But since we do have the above, and I think it's tested everywhere,
wouldn't setting SCHEDULED without accentually scheduling have
effectively the same result?
> unsigned long tx_bitmask;
> };
>
> @@ -327,6 +326,7 @@ void tls_sw_close(struct sock *sk, long timeout);
> void tls_sw_free_resources_tx(struct sock *sk);
> void tls_sw_free_resources_rx(struct sock *sk);
> void tls_sw_release_resources_rx(struct sock *sk);
> +void tls_sw_release_strp_rx(struct tls_context *tls_ctx);
> int tls_sw_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> int nonblock, int flags, int *addr_len);
> bool tls_sw_stream_read(const struct sock *sk);
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index fc81ae18cc44..51cb19e24dd9 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -261,24 +261,9 @@ static void tls_ctx_free(struct tls_context *ctx)
> kfree(ctx);
> }
>
> -static void tls_sk_proto_close(struct sock *sk, long timeout)
> +static void tls_sk_proto_cleanup(struct sock *sk,
> + struct tls_context *ctx, long timeo)
> {
> - struct tls_context *ctx = tls_get_ctx(sk);
> - long timeo = sock_sndtimeo(sk, 0);
> - void (*sk_proto_close)(struct sock *sk, long timeout);
> - bool free_ctx = false;
> -
> - lock_sock(sk);
> - sk_proto_close = ctx->sk_proto_close;
> -
> - if (ctx->tx_conf == TLS_HW_RECORD && ctx->rx_conf == TLS_HW_RECORD)
> - goto skip_tx_cleanup;
> -
> - if (ctx->tx_conf == TLS_BASE && ctx->rx_conf == TLS_BASE) {
> - free_ctx = true;
> - goto skip_tx_cleanup;
> - }
> -
> if (!tls_complete_pending_work(sk, ctx, 0, &timeo))
> tls_handle_open_record(sk, 0);
>
> @@ -299,22 +284,37 @@ static void tls_sk_proto_close(struct sock *sk, long timeout)
> #ifdef CONFIG_TLS_DEVICE
> if (ctx->rx_conf == TLS_HW)
> tls_device_offload_cleanup_rx(sk);
> -
> - if (ctx->tx_conf != TLS_HW && ctx->rx_conf != TLS_HW) {
> -#else
> - {
> #endif
> - tls_ctx_free(ctx);
> - ctx = NULL;
> +}
> +
> +static void tls_sk_proto_close(struct sock *sk, long timeout)
> +{
> + struct tls_context *ctx = tls_get_ctx(sk);
> + long timeo = sock_sndtimeo(sk, 0);
> + void (*sk_proto_close)(struct sock *sk, long timeout);
> + bool free_ctx = false;
Set but not used?
> +
> + lock_sock(sk);
> + sk_proto_close = ctx->sk_proto_close;
> +
> + if (ctx->tx_conf == TLS_HW_RECORD && ctx->rx_conf == TLS_HW_RECORD)
> + goto skip_tx_cleanup;
> +
> + if (ctx->tx_conf == TLS_BASE && ctx->rx_conf == TLS_BASE) {
> + free_ctx = true;
> + goto skip_tx_cleanup;
> }
>
> + tls_sk_proto_cleanup(sk, ctx, timeo);
> +
> skip_tx_cleanup:
> release_sock(sk);
> + if (ctx->rx_conf == TLS_SW)
> + tls_sw_release_strp_rx(ctx);
> sk_proto_close(sk, timeout);
> - /* free ctx for TLS_HW_RECORD, used by tcp_set_state
> - * for sk->sk_prot->unhash [tls_hw_unhash]
> - */
> - if (free_ctx)
> +
> + if (ctx->tx_conf != TLS_HW && ctx->rx_conf != TLS_HW &&
> + ctx->tx_conf != TLS_HW_RECORD && ctx->rx_conf != TLS_HW_RECORD)
> tls_ctx_free(ctx);
> }
>
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 455a782c7658..d234a6b818e6 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -473,7 +473,8 @@ static void tls_encrypt_done(struct crypto_async_request *req, int err)
> return;
>
> /* Schedule the transmission */
> - if (!test_and_set_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask))
> + if (!test_and_set_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask) &&
> + !test_bit(BIT_TX_CLOSING, &ctx->tx_bitmask))
Probably doesn't matter but seems like CLOSING test should be before
the test_and_set().
> schedule_delayed_work(&ctx->tx_work.work, 1);
> }
>
> @@ -2058,16 +2059,26 @@ void tls_sw_free_resources_tx(struct sock *sk)
> struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> struct tls_rec *rec, *tmp;
>
> + /* Set TX CLOSING bit to stop tx_work from being scheduled
> + * while tearing down TX context. We will flush any pending
> + * work before free'ing ctx anyways. If already set then
> + * another call is already free'ing resources.
> + */
Oh, can we get multiple calls here? Is this prep for unhash?
> + if (test_and_set_bit(BIT_TX_CLOSING, &ctx->tx_bitmask))
> + return;
> +
> /* Wait for any pending async encryptions to complete */
> smp_store_mb(ctx->async_notify, true);
> if (atomic_read(&ctx->encrypt_pending))
> crypto_wait_req(-EINPROGRESS, &ctx->async_wait);
>
> - release_sock(sk);
> - cancel_delayed_work_sync(&ctx->tx_work.work);
> - lock_sock(sk);
> -
> - /* Tx whatever records we can transmit and abandon the rest */
> + /* Flush work queue and then Tx whatever records we can
> + * transmit and abandon the rest, lock_sock(sk) must be
> + * held here. We ensure no further work is enqueue by
> + * checking CLOSING bit before queueing new work and
> + * setting it above.
> + */
> + flush_delayed_work(&ctx->tx_work.work);
> tls_tx_records(sk, -1);
>
> /* Free up un-sent records in tx_list. First, free
> @@ -2111,22 +2122,22 @@ void tls_sw_release_resources_rx(struct sock *sk)
> write_lock_bh(&sk->sk_callback_lock);
> sk->sk_data_ready = ctx->saved_data_ready;
> write_unlock_bh(&sk->sk_callback_lock);
> - release_sock(sk);
> - strp_done(&ctx->strp);
> - lock_sock(sk);
> }
> }
>
> -void tls_sw_free_resources_rx(struct sock *sk)
> +void tls_sw_release_strp_rx(struct tls_context *tls_ctx)
> {
> - struct tls_context *tls_ctx = tls_get_ctx(sk);
> struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
>
> - tls_sw_release_resources_rx(sk);
> -
> + strp_done(&ctx->strp);
> kfree(ctx);
> }
>
> +void tls_sw_free_resources_rx(struct sock *sk)
> +{
> + tls_sw_release_resources_rx(sk);
> +}
I don't understand the RX side well enough, but perhaps a separate
patch would make sense here?
> /* The work handler to transmitt the encrypted records in tx_list */
> static void tx_work_handler(struct work_struct *work)
> {
> @@ -2140,9 +2151,14 @@ static void tx_work_handler(struct work_struct *work)
> if (!test_and_clear_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask))
> return;
>
> - lock_sock(sk);
> + /* If we are running from a socket close operation then the
> + * lock is already held so we do not need to hold it.
> + */
> + if (likely(!test_bit(BIT_TX_CLOSING, &ctx->tx_bitmask)))
> + lock_sock(sk);
CPU 0 (free) CPU 1 (wq)
test_bit()
lock(sk)
set_bit()
lock(sk)
flush_work()
No?
> tls_tx_records(sk, -1);
> - release_sock(sk);
> + if (likely(!test_bit(BIT_TX_CLOSING, &ctx->tx_bitmask)))
> + release_sock(sk);
> }
>
> void tls_sw_write_space(struct sock *sk, struct tls_context *ctx)
> @@ -2152,8 +2168,8 @@ void tls_sw_write_space(struct sock *sk, struct tls_context *ctx)
> /* Schedule the transmission if tx list is ready */
> if (is_tx_ready(tx_ctx) && !sk->sk_write_pending) {
> /* Schedule the transmission */
> - if (!test_and_set_bit(BIT_TX_SCHEDULED,
> - &tx_ctx->tx_bitmask))
> + if (!test_and_set_bit(BIT_TX_SCHEDULED, &tx_ctx->tx_bitmask) &&
> + !test_bit(BIT_TX_CLOSING, &tx_ctx->tx_bitmask))
> schedule_delayed_work(&tx_ctx->tx_work.work, 0);
> }
> }
>
^ permalink raw reply
* Re: [GIT] Networking
From: pr-tracker-bot @ 2019-06-28 0:30 UTC (permalink / raw)
To: David Miller; +Cc: torvalds, akpm, netdev, linux-kernel
In-Reply-To: <20190626.195006.2073691861982062351.davem@davemloft.net>
The pull request you sent on Wed, 26 Jun 2019 19:50:06 -0700 (PDT):
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net refs/heads/master
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/c84afab02c311b08b5cb8ea758cc177f81c95d11
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH bpf-next v3 1/2] xsk: remove AF_XDP socket from map when the socket is released
From: Daniel Borkmann @ 2019-06-28 0:33 UTC (permalink / raw)
To: Björn Töpel, ast, netdev
Cc: Björn Töpel, magnus.karlsson, bruce.richardson,
songliubraving, bpf
In-Reply-To: <20190620100652.31283-2-bjorn.topel@gmail.com>
On 06/20/2019 12:06 PM, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> When an AF_XDP socket is released/closed the XSKMAP still holds a
> reference to the socket in a "released" state. The socket will still
> use the netdev queue resource, and block newly created sockets from
> attaching to that queue, but no user application can access the
> fill/complete/rx/tx queues. This results in that all applications need
> to explicitly clear the map entry from the old "zombie state"
> socket. This should be done automatically.
>
> After this patch, when a socket is released, it will remove itself
> from all the XSKMAPs it resides in, allowing the socket application to
> remove the code that cleans the XSKMAP entry.
>
> This behavior is also closer to that of SOCKMAP, making the two socket
> maps more consistent.
>
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
Sorry for the bit of delay in reviewing, few comments inline:
> ---
> include/net/xdp_sock.h | 3 ++
> kernel/bpf/xskmap.c | 101 +++++++++++++++++++++++++++++++++++------
> net/xdp/xsk.c | 25 ++++++++++
> 3 files changed, 116 insertions(+), 13 deletions(-)
>
> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index ae0f368a62bb..011a1b08d7c9 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -68,6 +68,8 @@ struct xdp_sock {
> */
> spinlock_t tx_completion_lock;
> u64 rx_dropped;
> + struct list_head map_list;
> + spinlock_t map_list_lock;
> };
>
> struct xdp_buff;
> @@ -87,6 +89,7 @@ struct xdp_umem_fq_reuse *xsk_reuseq_swap(struct xdp_umem *umem,
> struct xdp_umem_fq_reuse *newq);
> void xsk_reuseq_free(struct xdp_umem_fq_reuse *rq);
> struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev, u16 queue_id);
> +void xsk_map_delete_from_node(struct xdp_sock *xs, struct list_head *node);
>
> static inline char *xdp_umem_get_data(struct xdp_umem *umem, u64 addr)
> {
> diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
> index ef7338cebd18..af802c89ebab 100644
> --- a/kernel/bpf/xskmap.c
> +++ b/kernel/bpf/xskmap.c
> @@ -13,8 +13,58 @@ struct xsk_map {
> struct bpf_map map;
> struct xdp_sock **xsk_map;
> struct list_head __percpu *flush_list;
> + spinlock_t lock;
> };
>
> +/* Nodes are linked in the struct xdp_sock map_list field, and used to
> + * track which maps a certain socket reside in.
> + */
> +struct xsk_map_node {
> + struct list_head node;
> + struct xsk_map *map;
> + struct xdp_sock **map_entry;
> +};
> +
> +static struct xsk_map_node *xsk_map_node_alloc(void)
> +{
> + return kzalloc(sizeof(struct xsk_map_node), GFP_ATOMIC | __GFP_NOWARN);
> +}
> +
> +static void xsk_map_node_free(struct xsk_map_node *node)
> +{
> + kfree(node);
> +}
> +
> +static void xsk_map_node_init(struct xsk_map_node *node,
> + struct xsk_map *map,
> + struct xdp_sock **map_entry)
> +{
> + node->map = map;
> + node->map_entry = map_entry;
> +}
> +
> +static void xsk_map_add_node(struct xdp_sock *xs, struct xsk_map_node *node)
> +{
> + spin_lock_bh(&xs->map_list_lock);
> + list_add_tail(&node->node, &xs->map_list);
> + spin_unlock_bh(&xs->map_list_lock);
> +}
> +
> +static void xsk_map_del_node(struct xdp_sock *xs, struct xdp_sock **map_entry)
> +{
> + struct xsk_map_node *n, *tmp;
> +
> + spin_lock_bh(&xs->map_list_lock);
> + list_for_each_entry_safe(n, tmp, &xs->map_list, node) {
> + if (map_entry == n->map_entry) {
> + list_del(&n->node);
> + xsk_map_node_free(n);
> + }
> + }
> + spin_unlock_bh(&xs->map_list_lock);
> +
> +}
> +
> static struct bpf_map *xsk_map_alloc(union bpf_attr *attr)
> {
> struct xsk_map *m;
> @@ -34,6 +84,7 @@ static struct bpf_map *xsk_map_alloc(union bpf_attr *attr)
> return ERR_PTR(-ENOMEM);
>
> bpf_map_init_from_attr(&m->map, attr);
> + spin_lock_init(&m->lock);
>
> cost = (u64)m->map.max_entries * sizeof(struct xdp_sock *);
> cost += sizeof(struct list_head) * num_possible_cpus();
> @@ -76,15 +127,16 @@ static void xsk_map_free(struct bpf_map *map)
> bpf_clear_redirect_map(map);
> synchronize_net();
>
> + spin_lock_bh(&m->lock);
> for (i = 0; i < map->max_entries; i++) {
> - struct xdp_sock *xs;
> -
> - xs = m->xsk_map[i];
> - if (!xs)
> - continue;
> + struct xdp_sock **map_entry = &m->xsk_map[i];
> + struct xdp_sock *old_xs;
>
> - sock_put((struct sock *)xs);
> + old_xs = xchg(map_entry, NULL);
> + if (old_xs)
> + xsk_map_del_node(old_xs, map_entry);
> }
> + spin_unlock_bh(&m->lock);
>
> free_percpu(m->flush_list);
> bpf_map_area_free(m->xsk_map);
> @@ -166,7 +218,8 @@ static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value,
> {
> struct xsk_map *m = container_of(map, struct xsk_map, map);
> u32 i = *(u32 *)key, fd = *(u32 *)value;
> - struct xdp_sock *xs, *old_xs;
> + struct xdp_sock *xs, *old_xs, **entry;
> + struct xsk_map_node *node;
> struct socket *sock;
> int err;
>
> @@ -193,11 +246,20 @@ static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value,
> return -EOPNOTSUPP;
> }
>
> - sock_hold(sock->sk);
> + node = xsk_map_node_alloc();
> + if (!node) {
> + sockfd_put(sock);
> + return -ENOMEM;
> + }
>
> - old_xs = xchg(&m->xsk_map[i], xs);
> + spin_lock_bh(&m->lock);
> + entry = &m->xsk_map[i];
> + xsk_map_node_init(node, m, entry);
> + xsk_map_add_node(xs, node);
> + old_xs = xchg(entry, xs);
> if (old_xs)
> - sock_put((struct sock *)old_xs);
> + xsk_map_del_node(old_xs, entry);
> + spin_unlock_bh(&m->lock);
>
> sockfd_put(sock);
> return 0;
> @@ -206,19 +268,32 @@ static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value,
> static int xsk_map_delete_elem(struct bpf_map *map, void *key)
> {
> struct xsk_map *m = container_of(map, struct xsk_map, map);
> - struct xdp_sock *old_xs;
> + struct xdp_sock *old_xs, **map_entry;
> int k = *(u32 *)key;
>
> if (k >= map->max_entries)
> return -EINVAL;
>
> - old_xs = xchg(&m->xsk_map[k], NULL);
> + spin_lock_bh(&m->lock);
> + map_entry = &m->xsk_map[k];
> + old_xs = xchg(map_entry, NULL);
> if (old_xs)
> - sock_put((struct sock *)old_xs);
> + xsk_map_del_node(old_xs, map_entry);
> + spin_unlock_bh(&m->lock);
>
> return 0;
> }
>
> +void xsk_map_delete_from_node(struct xdp_sock *xs, struct list_head *node)
> +{
> + struct xsk_map_node *n = list_entry(node, struct xsk_map_node, node);
> +
> + spin_lock_bh(&n->map->lock);
> + *n->map_entry = NULL;
> + spin_unlock_bh(&n->map->lock);
> + xsk_map_node_free(n);
> +}
> +
> const struct bpf_map_ops xsk_map_ops = {
> .map_alloc = xsk_map_alloc,
> .map_free = xsk_map_free,
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a14e8864e4fa..1931d98a7754 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -335,6 +335,27 @@ static int xsk_init_queue(u32 entries, struct xsk_queue **queue,
> return 0;
> }
>
> +static struct list_head *xsk_map_list_pop(struct xdp_sock *xs)
> +{
> + struct list_head *node = NULL;
> +
> + spin_lock_bh(&xs->map_list_lock);
> + if (!list_empty(&xs->map_list)) {
> + node = xs->map_list.next;
> + list_del(node);
> + }
> + spin_unlock_bh(&xs->map_list_lock);
> + return node;
> +}
> +
> +static void xsk_delete_from_maps(struct xdp_sock *xs)
> +{
> + struct list_head *node;
> +
> + while ((node = xsk_map_list_pop(xs)))
> + xsk_map_delete_from_node(xs, node);
> +}
> +
I stared at this set for a while and I think there are still two
issues in the design unless I'm missing something obvious.
1) xs teardown and parallel map update:
- CPU0 is in xsk_release(), calls xsk_delete_from_maps().
- CPU1 is in xsk_map_update_elem(), both access the same map slot.
- CPU0 does the xsk_map_list_pop() for that given slot, gets
interrupted before calling into xsk_map_delete_from_node().
- CPU1 takes m->lock in updates, *entry = xs to the new sock,
does xsk_map_del_node() to check on the xs (which CPU0 tears
down). Given this was popped off the list, it doesn't do
anything here, all good. It unlocks m->lock and succeeds.
- CPU0 now continues in xsk_map_delete_from_node(), takes
m->lock, zeroes *n->map_entry, releases m->lock, and frees
n. However, at this point *n->map_entry contains the xs that
we've just updated on CPU1. So zero'ing it will 1) remove
the wrong entry, and ii) leak it since it goes out of reach.
2) Inconsistent use of xchg() and friends:
- AF_XDP fast-path is doing READ_ONCE(m->xsk_map[key]) without
taking m->lock. This is also why you have xchg() for example
inside m->lock region since both protect different things (should
probably be commented). However, this is not consistently used.
E.g. xsk_map_delete_from_node() or xsk_map_update_elem() have
plain assignment, so compiler could in theory happily perform
store tearing and the READ_ONCE() would see garbage. This needs
to be consistently paired.
> static int xsk_release(struct socket *sock)
> {
> struct sock *sk = sock->sk;
> @@ -354,6 +375,7 @@ static int xsk_release(struct socket *sock)
> sock_prot_inuse_add(net, sk->sk_prot, -1);
> local_bh_enable();
>
> + xsk_delete_from_maps(xs);
> if (xs->dev) {
> struct net_device *dev = xs->dev;
>
> @@ -767,6 +789,9 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
> mutex_init(&xs->mutex);
> spin_lock_init(&xs->tx_completion_lock);
>
> + INIT_LIST_HEAD(&xs->map_list);
> + spin_lock_init(&xs->map_list_lock);
> +
> mutex_lock(&net->xdp.lock);
> sk_add_node_rcu(sk, &net->xdp.list);
> mutex_unlock(&net->xdp.lock);
>
^ permalink raw reply
* Re: [PATCH 1/5] net: dsa: microchip: Replace ad-hoc polling with regmap
From: Andrew Lunn @ 2019-06-28 0:38 UTC (permalink / raw)
To: Marek Vasut; +Cc: netdev, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <20190627215556.23768-2-marex@denx.de>
On Thu, Jun 27, 2019 at 11:55:52PM +0200, Marek Vasut wrote:
> Regmap provides polling function to poll for bits in a register,
> use in instead of reimplementing it.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 2/5] net: dsa: microchip: Replace ksz9477_wait_vlan_ctrl_ready polling with regmap
From: Andrew Lunn @ 2019-06-28 0:39 UTC (permalink / raw)
To: Marek Vasut; +Cc: netdev, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <20190627215556.23768-3-marex@denx.de>
On Thu, Jun 27, 2019 at 11:55:53PM +0200, Marek Vasut wrote:
> Regmap provides polling function to poll for bits in a register. This
> function is another reimplementation of polling for bit being clear in
> a register. Replace this with regmap polling function. Moreover, inline
> the function parameters, as the function is never called with any other
> parameter values than this one.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 3/5] net: dsa: microchip: Replace ksz9477_wait_alu_ready polling with regmap
From: Andrew Lunn @ 2019-06-28 0:40 UTC (permalink / raw)
To: Marek Vasut; +Cc: netdev, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <20190627215556.23768-4-marex@denx.de>
On Thu, Jun 27, 2019 at 11:55:54PM +0200, Marek Vasut wrote:
> Regmap provides polling function to poll for bits in a register. This
> function is another reimplementation of polling for bit being clear in
> a register. Replace this with regmap polling function. Moreover, inline
> the function parameters, as the function is never called with any other
> parameter values than this one.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 4/5] net: dsa: microchip: Replace ksz9477_wait_alu_sta_ready polling with regmap
From: Andrew Lunn @ 2019-06-28 0:41 UTC (permalink / raw)
To: Marek Vasut; +Cc: netdev, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <20190627215556.23768-5-marex@denx.de>
On Thu, Jun 27, 2019 at 11:55:55PM +0200, Marek Vasut wrote:
> Regmap provides polling function to poll for bits in a register. This
> function is another reimplementation of polling for bit being clear in
> a register. Replace this with regmap polling function. Moreover, inline
> the function parameters, as the function is never called with any other
> parameter values than this one.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 5/5] net: dsa: microchip: Replace bit RMW with regmap
From: Andrew Lunn @ 2019-06-28 0:42 UTC (permalink / raw)
To: Marek Vasut; +Cc: netdev, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <20190627215556.23768-6-marex@denx.de>
On Thu, Jun 27, 2019 at 11:55:56PM +0200, Marek Vasut wrote:
> Regmap provides read-modify-write function to update bitfields in
> registers. Replace ad-hoc read-modify-write with regmap_update_bits()
> where applicable.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* [PATCH 1/2 nf-next v3] netfilter: nft_meta: Add NFT_META_BRI_IIFVPROTO support
From: wenxu @ 2019-06-28 0:49 UTC (permalink / raw)
To: pablo, fw; +Cc: netfilter-devel, netdev
From: wenxu <wenxu@ucloud.cn>
This patch provide a meta to get the bridge vlan proto
nft add rule bridge firewall zones counter meta br_vlan_proto 0x8100
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
include/uapi/linux/netfilter/nf_tables.h | 2 ++
net/netfilter/nft_meta.c | 9 +++++++++
2 files changed, 11 insertions(+)
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 8859535..0b18646 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -796,6 +796,7 @@ enum nft_exthdr_attributes {
* @NFT_META_IIFKIND: packet input interface kind name (dev->rtnl_link_ops->kind)
* @NFT_META_OIFKIND: packet output interface kind name (dev->rtnl_link_ops->kind)
* @NFT_META_BRI_PVID: packet input bridge port pvid
+ * @NFT_META_BRI_IIFVPROTO: packet input bridge vlan proto
*/
enum nft_meta_keys {
NFT_META_LEN,
@@ -827,6 +828,7 @@ enum nft_meta_keys {
NFT_META_IIFKIND,
NFT_META_OIFKIND,
NFT_META_BRI_PVID,
+ NFT_META_BRI_IIFVPROTO,
};
/**
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 4f8116d..e3adf6a 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -248,6 +248,14 @@ void nft_meta_get_eval(const struct nft_expr *expr,
return;
}
goto err;
+ case NFT_META_BRI_IIFVPROTO:
+ if (in == NULL || (p = br_port_get_rtnl_rcu(in)) == NULL)
+ goto err;
+ if (br_opt_get(p->br, BROPT_VLAN_ENABLED)) {
+ nft_reg_store16(dest, p->br->vlan_proto);
+ return;
+ }
+ goto err;
#endif
case NFT_META_IIFKIND:
if (in == NULL || in->rtnl_link_ops == NULL)
@@ -376,6 +384,7 @@ static int nft_meta_get_init(const struct nft_ctx *ctx,
len = IFNAMSIZ;
break;
case NFT_META_BRI_PVID:
+ case NFT_META_BRI_IIFVPROTO:
if (ctx->family != NFPROTO_BRIDGE)
return -EOPNOTSUPP;
len = sizeof(u16);
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/2 nf-next v3] netfilter:nft_meta: Add NFT_META_VLAN support
From: wenxu @ 2019-06-28 0:49 UTC (permalink / raw)
To: pablo, fw; +Cc: netfilter-devel, netdev
In-Reply-To: <1561682975-21790-1-git-send-email-wenxu@ucloud.cn>
From: wenxu <wenxu@ucloud.cn>
This patch provide a meta vlan to set the vlan tag of the packet.
for q-in-q outer vlan id 20:
meta vlan set 0x88a8:20
set the default 0x8100 vlan type with vlan id 20
meta vlan set 20
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
include/uapi/linux/netfilter/nf_tables.h | 4 ++++
net/netfilter/nft_meta.c | 27 ++++++++++++++++++++++++++-
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 0b18646..cf037f2 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -797,6 +797,7 @@ enum nft_exthdr_attributes {
* @NFT_META_OIFKIND: packet output interface kind name (dev->rtnl_link_ops->kind)
* @NFT_META_BRI_PVID: packet input bridge port pvid
* @NFT_META_BRI_IIFVPROTO: packet input bridge vlan proto
+ * @NFT_META_VLAN: packet vlan metadata
*/
enum nft_meta_keys {
NFT_META_LEN,
@@ -829,6 +830,7 @@ enum nft_meta_keys {
NFT_META_OIFKIND,
NFT_META_BRI_PVID,
NFT_META_BRI_IIFVPROTO,
+ NFT_META_VLAN,
};
/**
@@ -895,12 +897,14 @@ enum nft_hash_attributes {
* @NFTA_META_DREG: destination register (NLA_U32)
* @NFTA_META_KEY: meta data item to load (NLA_U32: nft_meta_keys)
* @NFTA_META_SREG: source register (NLA_U32)
+ * @NFTA_META_SREG2: source register (NLA_U32)
*/
enum nft_meta_attributes {
NFTA_META_UNSPEC,
NFTA_META_DREG,
NFTA_META_KEY,
NFTA_META_SREG,
+ NFTA_META_SREG2,
__NFTA_META_MAX
};
#define NFTA_META_MAX (__NFTA_META_MAX - 1)
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index e3adf6a..29a6679 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -28,7 +28,10 @@ struct nft_meta {
enum nft_meta_keys key:8;
union {
enum nft_registers dreg:8;
- enum nft_registers sreg:8;
+ struct {
+ enum nft_registers sreg:8;
+ enum nft_registers sreg2:8;
+ };
};
};
@@ -312,6 +315,17 @@ static void nft_meta_set_eval(const struct nft_expr *expr,
skb->secmark = value;
break;
#endif
+ case NFT_META_VLAN: {
+ u32 *sreg2 = ®s->data[meta->sreg2];
+ __be16 vlan_proto;
+ u16 vlan_tci;
+
+ vlan_tci = nft_reg_load16(sreg);
+ vlan_proto = nft_reg_load16(sreg2);
+
+ __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
+ break;
+ }
default:
WARN_ON(1);
}
@@ -321,6 +335,7 @@ static void nft_meta_set_eval(const struct nft_expr *expr,
[NFTA_META_DREG] = { .type = NLA_U32 },
[NFTA_META_KEY] = { .type = NLA_U32 },
[NFTA_META_SREG] = { .type = NLA_U32 },
+ [NFTA_META_SREG2] = { .type = NLA_U32 },
};
static int nft_meta_get_init(const struct nft_ctx *ctx,
@@ -483,6 +498,13 @@ static int nft_meta_set_init(const struct nft_ctx *ctx,
case NFT_META_PKTTYPE:
len = sizeof(u8);
break;
+ case NFT_META_VLAN:
+ len = sizeof(u16);
+ priv->sreg2 = nft_parse_register(tb[NFTA_META_SREG2]);
+ err = nft_validate_register_load(priv->sreg2, len);
+ if (err < 0)
+ return err;
+ break;
default:
return -EOPNOTSUPP;
}
@@ -521,6 +543,9 @@ static int nft_meta_set_dump(struct sk_buff *skb, const struct nft_expr *expr)
goto nla_put_failure;
if (nft_dump_register(skb, NFTA_META_SREG, priv->sreg))
goto nla_put_failure;
+ if (priv->key == NFT_META_VLAN &&
+ nft_dump_register(skb, NFTA_META_SREG2, priv->sreg2))
+ goto nla_put_failure;
return 0;
--
1.8.3.1
^ permalink raw reply related
* [PATCH bpf-next] selftests/bpf: fix -Wstrict-aliasing in test_sockopt_sk.c
From: Stanislav Fomichev @ 2019-06-28 1:12 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
Let's use union with u8[4] and u32 members for sockopt buffer,
that should fix any possible aliasing issues.
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",
^~~~~~~
Fixes: 8a027dc0d8f5 ("selftests/bpf: add sockopt test that exercises sk helpers")
Reported-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/test_sockopt_sk.c | 51 +++++++++----------
1 file changed, 24 insertions(+), 27 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_sockopt_sk.c b/tools/testing/selftests/bpf/test_sockopt_sk.c
index 12e79ed075ce..036b652e5ca9 100644
--- a/tools/testing/selftests/bpf/test_sockopt_sk.c
+++ b/tools/testing/selftests/bpf/test_sockopt_sk.c
@@ -22,7 +22,10 @@
static int getsetsockopt(void)
{
int fd, err;
- char buf[4] = {};
+ union {
+ char u8[4];
+ __u32 u32;
+ } buf = {};
socklen_t optlen;
fd = socket(AF_INET, SOCK_STREAM, 0);
@@ -33,31 +36,31 @@ static int getsetsockopt(void)
/* IP_TOS - BPF bypass */
- buf[0] = 0x08;
- err = setsockopt(fd, SOL_IP, IP_TOS, buf, 1);
+ buf.u8[0] = 0x08;
+ err = setsockopt(fd, SOL_IP, IP_TOS, &buf, 1);
if (err) {
log_err("Failed to call setsockopt(IP_TOS)");
goto err;
}
- buf[0] = 0x00;
+ buf.u8[0] = 0x00;
optlen = 1;
- err = getsockopt(fd, SOL_IP, IP_TOS, buf, &optlen);
+ err = getsockopt(fd, SOL_IP, IP_TOS, &buf, &optlen);
if (err) {
log_err("Failed to call getsockopt(IP_TOS)");
goto err;
}
- if (buf[0] != 0x08) {
+ if (buf.u8[0] != 0x08) {
log_err("Unexpected getsockopt(IP_TOS) buf[0] 0x%02x != 0x08",
- buf[0]);
+ buf.u8[0]);
goto err;
}
/* IP_TTL - EPERM */
- buf[0] = 1;
- err = setsockopt(fd, SOL_IP, IP_TTL, buf, 1);
+ buf.u8[0] = 1;
+ err = setsockopt(fd, SOL_IP, IP_TTL, &buf, 1);
if (!err || errno != EPERM) {
log_err("Unexpected success from setsockopt(IP_TTL)");
goto err;
@@ -65,16 +68,16 @@ static int getsetsockopt(void)
/* SOL_CUSTOM - handled by BPF */
- buf[0] = 0x01;
- err = setsockopt(fd, SOL_CUSTOM, 0, buf, 1);
+ buf.u8[0] = 0x01;
+ err = setsockopt(fd, SOL_CUSTOM, 0, &buf, 1);
if (err) {
log_err("Failed to call setsockopt");
goto err;
}
- buf[0] = 0x00;
+ buf.u32 = 0x00;
optlen = 4;
- err = getsockopt(fd, SOL_CUSTOM, 0, buf, &optlen);
+ err = getsockopt(fd, SOL_CUSTOM, 0, &buf, &optlen);
if (err) {
log_err("Failed to call getsockopt");
goto err;
@@ -84,37 +87,31 @@ static int getsetsockopt(void)
log_err("Unexpected optlen %d != 1", optlen);
goto err;
}
- if (buf[0] != 0x01) {
- log_err("Unexpected buf[0] 0x%02x != 0x01", buf[0]);
+ if (buf.u8[0] != 0x01) {
+ log_err("Unexpected buf[0] 0x%02x != 0x01", buf.u8[0]);
goto err;
}
/* SO_SNDBUF is overwritten */
- buf[0] = 0x01;
- buf[1] = 0x01;
- buf[2] = 0x01;
- buf[3] = 0x01;
- err = setsockopt(fd, SOL_SOCKET, SO_SNDBUF, buf, 4);
+ buf.u32 = 0x01010101;
+ err = setsockopt(fd, SOL_SOCKET, SO_SNDBUF, &buf, 4);
if (err) {
log_err("Failed to call setsockopt(SO_SNDBUF)");
goto err;
}
- buf[0] = 0x00;
- buf[1] = 0x00;
- buf[2] = 0x00;
- buf[3] = 0x00;
+ buf.u32 = 0x00;
optlen = 4;
- err = getsockopt(fd, SOL_SOCKET, SO_SNDBUF, buf, &optlen);
+ err = getsockopt(fd, SOL_SOCKET, SO_SNDBUF, &buf, &optlen);
if (err) {
log_err("Failed to call getsockopt(SO_SNDBUF)");
goto err;
}
- if (*(__u32 *)buf != 0x55AA*2) {
+ if (buf.u32 != 0x55AA*2) {
log_err("Unexpected getsockopt(SO_SNDBUF) 0x%x != 0x55AA*2",
- *(__u32 *)buf);
+ buf.u32);
goto err;
}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v3 net-next] net: link_watch: prevent starvation when processing linkwatch wq
From: Yunsheng Lin @ 2019-06-28 1:13 UTC (permalink / raw)
To: davem
Cc: hkallweit1, gregkh, tglx, netdev, linux-kernel, pbonzini, rkrcmar,
kvm
When user has configured a large number of virtual netdev, such
as 4K vlans, the carrier on/off operation of the real netdev
will also cause it's virtual netdev's link state to be processed
in linkwatch. Currently, the processing is done in a work queue,
which may cause rtnl locking starvation problem and worker
starvation problem for other work queue, such as irqfd_inject wq.
This patch releases the cpu when link watch worker has processed
a fixed number of netdev' link watch event, and schedule the
work queue again when there is still link watch event remaining.
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
V2: use cond_resched and rtnl_unlock after processing a fixed
number of events
V3: fall back to v1 and change commit log to reflect that.
---
---
net/core/link_watch.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index 04fdc95..f153e06 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -163,9 +163,16 @@ static void linkwatch_do_dev(struct net_device *dev)
static void __linkwatch_run_queue(int urgent_only)
{
+#define MAX_DO_DEV_PER_LOOP 100
+
+ int do_dev = MAX_DO_DEV_PER_LOOP;
struct net_device *dev;
LIST_HEAD(wrk);
+ /* Give urgent case more budget */
+ if (urgent_only)
+ do_dev += MAX_DO_DEV_PER_LOOP;
+
/*
* Limit the number of linkwatch events to one
* per second so that a runaway driver does not
@@ -184,7 +191,7 @@ static void __linkwatch_run_queue(int urgent_only)
spin_lock_irq(&lweventlist_lock);
list_splice_init(&lweventlist, &wrk);
- while (!list_empty(&wrk)) {
+ while (!list_empty(&wrk) && do_dev > 0) {
dev = list_first_entry(&wrk, struct net_device, link_watch_list);
list_del_init(&dev->link_watch_list);
@@ -195,9 +202,13 @@ static void __linkwatch_run_queue(int urgent_only)
}
spin_unlock_irq(&lweventlist_lock);
linkwatch_do_dev(dev);
+ do_dev--;
spin_lock_irq(&lweventlist_lock);
}
+ /* Add the remaining work back to lweventlist */
+ list_splice_init(&wrk, &lweventlist);
+
if (!list_empty(&lweventlist))
linkwatch_schedule_work(0);
spin_unlock_irq(&lweventlist_lock);
--
2.8.1
^ permalink raw reply related
* Re: [PATCH v4 02/13] dt-bindings: net: Add a YAML schemas for the generic PHY options
From: Rob Herring @ 2019-06-28 1:16 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mark Rutland, Frank Rowand, David S . Miller, Chen-Yu Tsai,
Maxime Coquelin, Alexandre Torgue, netdev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
devicetree, linux-stm32, Maxime Chevallier, Antoine Ténart,
Andrew Lunn, Florian Fainelli, Heiner Kallweit
In-Reply-To: <945e54f02cc36a543b4c0bfd960475147359f7ff.1561649505.git-series.maxime.ripard@bootlin.com>
On Thu, Jun 27, 2019 at 9:32 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> The networking PHYs have a number of available device tree properties that
> can be used in their device tree node. Add a YAML schemas for those.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
> Documentation/devicetree/bindings/net/ethernet-phy.yaml | 179 +++++++++-
> Documentation/devicetree/bindings/net/phy.txt | 80 +----
> 2 files changed, 180 insertions(+), 79 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/net/ethernet-phy.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> new file mode 100644
> index 000000000000..81d2016d7232
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -0,0 +1,179 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/ethernet-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ethernet PHY Generic Binding
> +
> +maintainers:
> + - Andrew Lunn <andrew@lunn.ch>
> + - Florian Fainelli <f.fainelli@gmail.com>
> + - Heiner Kallweit <hkallweit1@gmail.com>
> +
> +# The dt-schema tools will generate a select statement first by using
> +# the compatible, and second by using the node name if any. In our
> +# case, the node name is the one we want to match on, while the
> +# compatible is optional.
> +select:
> + properties:
> + $nodename:
> + pattern: "^ethernet-phy(@[a-f0-9]+)?$"
> +
> + required:
> + - $nodename
> +
> +properties:
> + $nodename:
> + pattern: "^ethernet-phy(@[a-f0-9]+)?$"
> +
> + compatible:
> + oneOf:
> + - const: ethernet-phy-ieee802.3-c22
> + description: PHYs that implement IEEE802.3 clause 22
> + - const: ethernet-phy-ieee802.3-c45
> + description: PHYs that implement IEEE802.3 clause 45
> + - pattern: "^ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$"
> + description:
> + If the PHY reports an incorrect ID (or none at all) then the
> + compatible list may contain an entry with the correct PHY ID
> + in the above form.
> + The first group of digits is the 16 bit Phy Identifier 1
> + register, this is the chip vendor OUI bits 3:18. The
> + second group of digits is the Phy Identifier 2 register,
> + this is the chip vendor OUI bits 19:24, followed by 10
> + bits of a vendor specific ID.
> + - items:
> + - pattern: "^ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$"
> + - const: ethernet-phy-ieee802.3-c45
> +
> + reg:
> + maxItems: 1
> + minimum: 0
> + maximum: 31
> + description:
> + The ID number for the PHY.
Mixing array and scalar properties is something we shouldn't be doing,
so I dropped maxItems as that is implied. I have a meta-schema check
for this once a couple of occurrences are fixed in the tree.
Rob
^ permalink raw reply
* Re: [PATCH net] net/sched: flower: fix infinite loop in fl_walk()
From: Cong Wang @ 2019-06-28 1:24 UTC (permalink / raw)
To: Davide Caratti
Cc: Vlad Buslov, David S. Miller, Linux Kernel Network Developers,
Lucas Bates
In-Reply-To: <94260c8898834cfa8e5421933a2b5ea59680b970.camel@redhat.com>
On Thu, Jun 27, 2019 at 3:10 PM Davide Caratti <dcaratti@redhat.com> wrote:
>
> On Wed, 2019-06-26 at 14:15 -0700, Cong Wang wrote:
> > Hi, Davide
> >
> > On Tue, Jun 25, 2019 at 12:29 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > It should handle this overflow case more gracefully, I hope.
> > >
> >
> > Please try this attached one and let me know if it works.
> > Hope I get it right this time.
> >
> > Thanks!
>
> hello Cong, and thanks a lot for the patch!
> I see it uses
>
> (tmp <= id)
>
> as the condition to detect the overflow, and at each iteration it does
>
> tmp = id, ++id
>
> so that 'tmp' contains the last IDR found in the tree and 'id' is the next
> tentative value to be searched for. When 'id' overflows, (tmp <= id)
> becomes false, and the 'for' loop exits.
Yes, thanks for testing it with tc actions.
> I tested it successfully with TC actions having the highest possible
> index: 'tc actions show' doesn't loop anymore. But with cls_flower (that
> uses idr_for_each_entry_continue_ul() ) I still see the infinite loop:
> even when idr_for_each_entry_continue_ul() is used, fl_get_next_filter()
> never returns NULL, because
>
> (tmp <= id) && (((entry) = idr_get_next_ul(idr, &(id))) != NULL)
>
> calls idr_get_next_ul(idr, &(id)) at least once. So, even if
> idr_for_each_entry_continue_ul() detected the overflow of 'id' after the
> first iteration, and bailouts the for loop, fl_get_next_filter()
> repeatedly returns a pointer to the idr slot with index equal to
> 0xffffffff. Because of that, the while() loop in fl_walk() keeps dumping
> the same rule.
Good catch, it is actually the arg->cookie++ which causes the trouble
here.
> In my original patch I found easier to check for the overflow of
> arg->cookie in fl_walk(), before the self-increment, so I was sure that
>
> arg->fn(tp, f, arg)
>
> was already called once when 'f' was the slot having the highest possible
> IDR. Now, I didn't check it, but I guess
>
> refcount_inc_not_zero(&f->refcnt))
>
> in fl_get_next_filter() is always true during my test, so the inner
> while() loop is not endless, even when the idr has a slot with id equal to
> ULONG_MAX. Probably, to stay on the safe side, cls_flower needs both tests
> to be in place, what do you think?
I think we can just fold the nested loops into one for cls_flower and remove
the arg->cookie++.
What's more, arg->cookie could overflow too,seems we have to switch back to
arg->skip. I am not sure, if this is really a problem we can fix it separately.
Thanks.
^ permalink raw reply
* linux-next: manual merge of the net-next tree with the net tree
From: Stephen Rothwell @ 2019-06-28 1:45 UTC (permalink / raw)
To: David Miller, Networking
Cc: Linux Next Mailing List, Linux Kernel Mailing List,
Dmitry Bogdanov, Igor Russkikh
[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]
Hi all,
Today's linux-next merge of the net-next tree got conflicts in:
drivers/net/ethernet/aquantia/atlantic/aq_nic.c
drivers/net/ethernet/aquantia/atlantic/aq_nic.h
between commit:
48dd73d08d4d ("net: aquantia: fix vlans not working over bridged network")
from the net tree and commit:
d3ed7c5cf79b ("net: aquantia: adding fields and device features for vlan offload")
from the net-next tree.
I fixed it up (see below) 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.
--
Cheers,
Stephen Rothwell
diff --cc drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 41172fbebddd,746f85e6de13..000000000000
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@@ -126,7 -126,8 +126,9 @@@ void aq_nic_cfg_start(struct aq_nic_s *
cfg->link_speed_msk &= cfg->aq_hw_caps->link_speed_msk;
cfg->features = cfg->aq_hw_caps->hw_features;
+ cfg->is_vlan_force_promisc = true;
+ cfg->is_vlan_rx_strip = !!(cfg->features & NETIF_F_HW_VLAN_CTAG_RX);
+ cfg->is_vlan_tx_insert = !!(cfg->features & NETIF_F_HW_VLAN_CTAG_TX);
}
static int aq_nic_update_link_status(struct aq_nic_s *self)
diff --cc drivers/net/ethernet/aquantia/atlantic/aq_nic.h
index 0f22f5d5691b,26c72f298684..000000000000
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.h
@@@ -35,7 -35,8 +35,9 @@@ struct aq_nic_cfg_s
u32 flow_control;
u32 link_speed_msk;
u32 wol;
+ bool is_vlan_force_promisc;
+ u8 is_vlan_rx_strip;
+ u8 is_vlan_tx_insert;
u16 is_mc_list_enabled;
u16 mc_list_count;
bool is_autoneg;
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 0/5] net: dsa: microchip: Further regmap cleanups
From: David Miller @ 2019-06-28 2:25 UTC (permalink / raw)
To: marex; +Cc: netdev, andrew, f.fainelli, Tristram.Ha, Woojung.Huh
In-Reply-To: <20190627215556.23768-1-marex@denx.de>
From: Marek Vasut <marex@denx.de>
Date: Thu, 27 Jun 2019 23:55:51 +0200
> This patchset cleans up KSZ9477 switch driver by replacing various
> ad-hoc polling implementations and register RMW with regmap functions.
>
> Each polling function is replaced separately to make it easier to review
> and possibly bisect, but maybe the patches can be squashed.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
Series applied, thanks.
^ permalink raw reply
* RE: [EXT] [PATCH 73/87] ethernet: freescale: Remove memset after dma_alloc_coherent
From: Andy Duan @ 2019-06-28 2:26 UTC (permalink / raw)
To: Fuqian Huang
Cc: David S. Miller, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190627174641.6474-1-huangfq.daxian@gmail.com>
From: Fuqian Huang <huangfq.daxian@gmail.com> Sent: Friday, June 28, 2019 1:47 AM
> In commit af7ddd8a627c
> ("Merge tag 'dma-mapping-4.21' of
> git://git.infradead.org/users/hch/dma-mapping"),
> dma_alloc_coherent has already zeroed the memory.
> So memset is not needed.
>
> Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
Acked-by: Fugang Duan <fugang.duan@nxp.com>
> ---
> drivers/net/ethernet/freescale/fec_main.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 38f10f7dcbc3..ec87b8b78d21 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3143,8 +3143,6 @@ static int fec_enet_init(struct net_device *ndev)
> return -ENOMEM;
> }
>
> - memset(cbd_base, 0, bd_size);
> -
> /* Get the Ethernet address */
> fec_get_mac(ndev);
> /* make sure MAC we just acquired is programmed into the hw */
> --
> 2.11.0
^ permalink raw reply
* Re: [PATCH 2/6 bpf-next] Clean up xsk reuseq API
From: Jonathan Lemon @ 2019-06-28 2:31 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
kernel-team
In-Reply-To: <20190627153856.1f4d4709@cakuba.netronome.com>
On 27 Jun 2019, at 15:38, Jakub Kicinski wrote:
> 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.
The main reason I rewrote this was to provide the same type
of functionality as realloc() - no need to allocate/initialize a new
array if the old one would still end up being used. This would seem
to be a win for the typical case of having the interface go up/down.
Perhaps I should have named the function differently?
--
Jonathan
^ 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