* [PATCH net-next 1/5] bpf: simplify __is_valid_access test on cb
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
The __is_valid_access() test for cb[] from 62c7989b24db ("bpf: allow
b/h/w/dw access for bpf's cb in ctx") was done unnecessarily complex,
we can just simplify it the same way as recent fix from 2d071c643f1c
("bpf, trace: make ctx access checks more robust") did. Overflow can
never happen as size is 1/2/4/8 depending on access.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
net/core/filter.c | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 9038386..883975f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2784,19 +2784,8 @@ static bool __is_valid_access(int off, int size)
switch (off) {
case offsetof(struct __sk_buff, cb[0]) ...
offsetof(struct __sk_buff, cb[4]) + sizeof(__u32) - 1:
- if (size == sizeof(__u16) &&
- off > offsetof(struct __sk_buff, cb[4]) + sizeof(__u16))
- return false;
- if (size == sizeof(__u32) &&
- off > offsetof(struct __sk_buff, cb[4]))
- return false;
- if (size == sizeof(__u64) &&
- off > offsetof(struct __sk_buff, cb[2]))
- return false;
- if (size != sizeof(__u8) &&
- size != sizeof(__u16) &&
- size != sizeof(__u32) &&
- size != sizeof(__u64))
+ if (off + size >
+ offsetof(struct __sk_buff, cb[4]) + sizeof(__u32))
return false;
break;
default:
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 3/5] bpf: allow option for setting bpf_l4_csum_replace from scratch
From: Daniel Borkmann @ 2017-01-24 0:06 UTC (permalink / raw)
To: davem; +Cc: netdev, ast, Daniel Borkmann
In-Reply-To: <cover.1485214851.git.daniel@iogearbox.net>
When programs need to calculate the csum from scratch for small UDP
packets and use bpf_l4_csum_replace() to feed the result from helpers
like bpf_csum_diff(), then we need a flag besides BPF_F_MARK_MANGLED_0
that would ignore the case of current csum being 0, and which would
still allow for the helper to set the csum and transform when needed
to CSUM_MANGLED_0.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
include/uapi/linux/bpf.h | 1 +
net/core/filter.c | 7 ++++---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bd30684..e07fd5a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -522,6 +522,7 @@ enum bpf_func_id {
/* BPF_FUNC_l4_csum_replace flags. */
#define BPF_F_PSEUDO_HDR (1ULL << 4)
#define BPF_F_MARK_MANGLED_0 (1ULL << 5)
+#define BPF_F_MARK_ENFORCE (1ULL << 6)
/* BPF_FUNC_clone_redirect and BPF_FUNC_redirect flags. */
#define BPF_F_INGRESS (1ULL << 0)
diff --git a/net/core/filter.c b/net/core/filter.c
index e2263da..1e00737 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1522,10 +1522,11 @@ static inline void bpf_pull_mac_rcsum(struct sk_buff *skb)
{
bool is_pseudo = flags & BPF_F_PSEUDO_HDR;
bool is_mmzero = flags & BPF_F_MARK_MANGLED_0;
+ bool do_mforce = flags & BPF_F_MARK_ENFORCE;
__sum16 *ptr;
- if (unlikely(flags & ~(BPF_F_MARK_MANGLED_0 | BPF_F_PSEUDO_HDR |
- BPF_F_HDR_FIELD_MASK)))
+ if (unlikely(flags & ~(BPF_F_MARK_MANGLED_0 | BPF_F_MARK_ENFORCE |
+ BPF_F_PSEUDO_HDR | BPF_F_HDR_FIELD_MASK)))
return -EINVAL;
if (unlikely(offset > 0xffff || offset & 1))
return -EFAULT;
@@ -1533,7 +1534,7 @@ static inline void bpf_pull_mac_rcsum(struct sk_buff *skb)
return -EFAULT;
ptr = (__sum16 *)(skb->data + offset);
- if (is_mmzero && !*ptr)
+ if (is_mmzero && !do_mforce && !*ptr)
return 0;
switch (flags & BPF_F_HDR_FIELD_MASK) {
--
1.9.3
^ permalink raw reply related
* Re: [RFC PATCH net-next 0/5] bridge: per vlan lwt and dst_metadata support
From: Roopa Prabhu @ 2017-01-24 0:09 UTC (permalink / raw)
To: Or Gerlitz
Cc: Jiri Benc, Jiri Pirko, Linux Netdev List, David Miller,
Stephen Hemminger, Nikolay Aleksandrov, Thomas Graf,
Hannes Frederic Sowa, pravin shelar, David Ahern,
Jamal Hadi Salim
In-Reply-To: <CAJ3xEMiC5xJ+rex8xMnyuGj5QKj+sYA9A6JjOM0xQaZraFSHig@mail.gmail.com>
On 1/23/17, 9:03 AM, Or Gerlitz wrote:
> On Mon, Jan 23, 2017 at 6:13 PM, Roopa Prabhu <roopa@cumulusnetworks.com>
> wrote:
>
>> Also, the goal is to reduce the number of vxlan devices from say 4k to 1.
>> I don't think replacing it with 8k (egress + ingress) rules is going in the
>> right direction.
>>
> Can't you take advantage of the shared vxlan device configuration
> introduced throughout the LWT work such that you have single device dealing
> with many tunnels? why?
>
I tried to cover this in my initial paragraph in the cover letter:
"lwt and dst_metadata/collect_metadata have enabled vxlan l3 deployments to use a 'single vxlan
netdev for multiple vnis' eliminating the scalability problem with using a 'single vxlan netdev per vni'.
This series tries to do the same for vxlan netdevs in pure l2 bridged networks. Use-case/deployment and
details are below." there is more in the cover letter on this.
There is no route pointing to the vxlan device here. vxlan device is a bridged port. And it bridges local host ports to remote vxlan tunnels
vlan-to-vxlan.
^ permalink raw reply
* Re: [PATCH cumulus-4.1.y 1/5] vxlan: flush fdb entries on oper down
From: Roopa Prabhu @ 2017-01-24 0:11 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: davem, netdev, ramanb, jbenc, pshelar
In-Reply-To: <20170123085945.36404cf0@xeon-e3>
On 1/23/17, 8:59 AM, Stephen Hemminger wrote:
> On Fri, 20 Jan 2017 23:40:06 -0800
> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index 19b1653..15b1c23 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -3276,6 +3276,12 @@ static int vxlan_netdevice_event(struct notifier_block *unused,
>> vxlan_handle_lowerdev_unregister(vn, dev);
>> else if (event == NETDEV_UDP_TUNNEL_PUSH_INFO)
>> vxlan_push_rx_ports(dev);
>> + else if (event == NETDEV_CHANGE) {
>> + if (dev->netdev_ops == &vxlan_netdev_ops) {
>> + if (netif_running(dev) && !netif_oper_up(dev))
>> + vxlan_flush(netdev_priv(dev));
>> + }
>> + }
> Looks correct.
> Maybe logic would be clearer with a switch() statement here.
ack. this was an internal patch i accidentally sent. I have refined it a bit for upstream since then.
will post an update..
thanks
^ permalink raw reply
* Re: [PATCH net-next 1/2] vxlan: don't flush static fdb entries on admin down
From: Roopa Prabhu @ 2017-01-24 0:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev, ramanb, stephen, jbenc, pshelar
In-Reply-To: <20170123.160741.1558621534864494201.davem@davemloft.net>
On 1/23/17, 1:07 PM, David Miller wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> Date: Fri, 20 Jan 2017 23:43:18 -0800
>
>> /* Purge the forwarding table */
>> -static void vxlan_flush(struct vxlan_dev *vxlan)
>> +static void vxlan_flush(struct vxlan_dev *vxlan, int do_all)
> Please use 'bool' and true/false for this new argument.
>
> FWIW, I am fine with changing the default behavior in this way.
ack, will fix and post a v2, thanks.
^ permalink raw reply
* Re: [RFC PATCH] mlx5: Fix page rfcnt issue
From: David Miller @ 2017-01-24 0:16 UTC (permalink / raw)
To: tom; +Cc: netdev, tariqt, saeedm, kernel-team
In-Reply-To: <20170123235658.3872770-1-tom@herbertland.com>
From: Tom Herbert <tom@herbertland.com>
Date: Mon, 23 Jan 2017 15:56:58 -0800
> One other potential problem in the driver is the use of put_page in
> release pages. Comparing how the allocation is done in other drivers
> (for instance comparing to ixgbe) some seem to use __free_pages instead.
> I don't know which is correct to use, but somehow it doesn't seem like
> they can both be right.
The only difference I can see is that put_page() does a
__page_cache_release() which shouldn't be necessary for driver RX
pages, so it could be unnecessary overhead.
^ permalink raw reply
* Re: [PATCH cumulus-4.1.y 2/5] vxlan: don't replace fdb entry if nothing changed
From: Roopa Prabhu @ 2017-01-24 0:14 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: davem, netdev, ramanb, jbenc, pshelar
In-Reply-To: <20170123090211.6bcd2afc@xeon-e3>
On 1/23/17, 9:02 AM, Stephen Hemminger wrote:
> On Fri, 20 Jan 2017 23:40:07 -0800
> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
>> + if (!vxlan_addr_equal(&rd->remote_ip, ip) ||
>> + rd->remote_port != port ||
>> + rd->remote_vni != vni ||
>> + rd->remote_ifindex != ifindex) {
>> + dst_cache_reset(&rd->dst_cache);
>> + rd->remote_ip = *ip;
>> + rd->remote_port = port;
>> + rd->remote_vni = vni;
>> + rd->remote_ifindex = ifindex;
>> + return 1;
>> + }
>> +
> I think it would be clearer if negative logic was avoided.
>
> if (vxlan_addr_equal(&rd->remote_ip, ip) &&
> rd->remote_port == port &&
> rd->remote_vni == vni &&
> rd->ermote_ifindex == ifndex)
> return 1;
>
> dst_cache_reset ...
ack, this was an accidental hit on send as well.
It is on my upstream patch stack..but i think this patch is not really needed upstream because
a previous call to vxlan_fdb_find_rdst in vxlan_fdb_replace does the same thing.
I will test again and repost if needed, thanks.
^ permalink raw reply
* [PATCH net-next] bpf, lpm: fix kfree of im_node in trie_update_elem
From: Daniel Borkmann @ 2017-01-24 0:26 UTC (permalink / raw)
To: davem; +Cc: ast, daniel, netdev, Daniel Borkmann
We need to initialize im_node to NULL, otherwise in case of error path
it gets passed to kfree() as uninitialized pointer.
Fixes: b95a5c4db09b ("bpf: add a longest prefix match trie map implementation")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
Mentioned it in http://patchwork.ozlabs.org/patch/718070/, but
was probably overlooked.
kernel/bpf/lpm_trie.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
index ba19241d..144e976 100644
--- a/kernel/bpf/lpm_trie.c
+++ b/kernel/bpf/lpm_trie.c
@@ -262,7 +262,7 @@ static int trie_update_elem(struct bpf_map *map,
void *_key, void *value, u64 flags)
{
struct lpm_trie *trie = container_of(map, struct lpm_trie, map);
- struct lpm_trie_node *node, *im_node, *new_node = NULL;
+ struct lpm_trie_node *node, *im_node = NULL, *new_node = NULL;
struct lpm_trie_node __rcu **slot;
struct bpf_lpm_trie_key *key = _key;
unsigned long irq_flags;
--
1.9.3
^ permalink raw reply related
* Re: [PATCH net-next] bpf, lpm: fix kfree of im_node in trie_update_elem
From: Alexei Starovoitov @ 2017-01-24 0:37 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, ast, daniel, netdev
In-Reply-To: <6052894aa1bd4df5a5c7aaa05f5b63a980cbe371.1485217298.git.daniel@iogearbox.net>
On Tue, Jan 24, 2017 at 01:26:46AM +0100, Daniel Borkmann wrote:
> We need to initialize im_node to NULL, otherwise in case of error path
> it gets passed to kfree() as uninitialized pointer.
>
> Fixes: b95a5c4db09b ("bpf: add a longest prefix match trie map implementation")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Great catch.
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* linux-next: manual merge of the net-next tree with the net tree
From: Stephen Rothwell @ 2017-01-24 0:38 UTC (permalink / raw)
To: David Miller, Networking
Cc: linux-next, linux-kernel, David Ahern, Robert Shearman
Hi all,
Today's linux-next merge of the net-next tree got a conflict in:
net/mpls/af_mpls.c
between commit:
9f427a0e474a ("net: mpls: Fix multipath selection for LSR use case")
from the net tree and commit:
27d691056bde ("mpls: Packet stats")
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 net/mpls/af_mpls.c
index 5b77377e5a15,4dc81963af8f..000000000000
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@@ -98,10 -94,35 +94,35 @@@ bool mpls_pkt_too_big(const struct sk_b
}
EXPORT_SYMBOL_GPL(mpls_pkt_too_big);
+ void mpls_stats_inc_outucastpkts(struct net_device *dev,
+ const struct sk_buff *skb)
+ {
+ struct mpls_dev *mdev;
+
+ if (skb->protocol == htons(ETH_P_MPLS_UC)) {
+ mdev = mpls_dev_get(dev);
+ if (mdev)
+ MPLS_INC_STATS_LEN(mdev, skb->len,
+ tx_packets,
+ tx_bytes);
+ } else if (skb->protocol == htons(ETH_P_IP)) {
+ IP_UPD_PO_STATS(dev_net(dev), IPSTATS_MIB_OUT, skb->len);
+ #if IS_ENABLED(CONFIG_IPV6)
+ } else if (skb->protocol == htons(ETH_P_IPV6)) {
+ struct inet6_dev *in6dev = __in6_dev_get(dev);
+
+ if (in6dev)
+ IP6_UPD_PO_STATS(dev_net(dev), in6dev,
+ IPSTATS_MIB_OUT, skb->len);
+ #endif
+ }
+ }
+ EXPORT_SYMBOL_GPL(mpls_stats_inc_outucastpkts);
+
-static u32 mpls_multipath_hash(struct mpls_route *rt,
- struct sk_buff *skb, bool bos)
+static u32 mpls_multipath_hash(struct mpls_route *rt, struct sk_buff *skb)
{
struct mpls_entry_decoded dec;
+ unsigned int mpls_hdr_len = 0;
struct mpls_shim_hdr *hdr;
bool eli_seen = false;
int label_index;
@@@ -280,27 -308,24 +310,24 @@@ static int mpls_forward(struct sk_buff
hdr = mpls_hdr(skb);
dec = mpls_entry_decode(hdr);
- /* Pop the label */
- skb_pull(skb, sizeof(*hdr));
- skb_reset_network_header(skb);
-
- skb_orphan(skb);
-
rt = mpls_route_input_rcu(net, dec.label);
- if (!rt)
+ if (!rt) {
+ MPLS_INC_STATS(mdev, rx_noroute);
goto drop;
+ }
- nh = mpls_select_multipath(rt, skb, dec.bos);
+ nh = mpls_select_multipath(rt, skb);
if (!nh)
- goto drop;
-
- /* Find the output device */
- out_dev = rcu_dereference(nh->nh_dev);
- if (!mpls_output_possible(out_dev))
- goto drop;
+ goto err;
+ /* Pop the label */
+ skb_pull(skb, sizeof(*hdr));
+ skb_reset_network_header(skb);
+
+ skb_orphan(skb);
+
if (skb_warn_if_lro(skb))
- goto drop;
+ goto err;
skb_forward_csum(skb);
^ permalink raw reply
* [PATCH v2 net 0/2] ipv6: fix ip6_tnl_parse_tlv_enc_lim() issues
From: Eric Dumazet @ 2017-01-24 0:43 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet
First patch fixes ip6_tnl_parse_tlv_enc_lim() callers,
bug added in linux-3.7
Second patch fixes ip6_tnl_parse_tlv_enc_lim() itself,
bug predates linux-2.6.12
Based on a report from Dmitry Vyukov, thanks to KASAN.
Eric Dumazet (2):
ip6_tunnel: must reload ipv6h in ip6ip6_tnl_xmit()
ipv6: fix ip6_tnl_parse_tlv_enc_lim()
net/ipv6/ip6_gre.c | 3 +++
net/ipv6/ip6_tunnel.c | 36 ++++++++++++++++++++++++------------
2 files changed, 27 insertions(+), 12 deletions(-)
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply
* [PATCH v2 net 1/2] ip6_tunnel: must reload ipv6h in ip6ip6_tnl_xmit()
From: Eric Dumazet @ 2017-01-24 0:43 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet, Dmitry Kozlov
In-Reply-To: <20170124004306.19236-1-edumazet@google.com>
Since ip6_tnl_parse_tlv_enc_lim() can call pskb_may_pull(),
we must reload any pointer that was related to skb->head
(or skb->data), or risk use after free.
Fixes: c12b395a4664 ("gre: Support GRE over IPv6")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Dmitry Kozlov <xeb@mail.ru>
---
net/ipv6/ip6_gre.c | 3 +++
net/ipv6/ip6_tunnel.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 75b6108234dd05a54af0ae51c7c11eaf1ca2..558631860d91bbcb1321a4b566b6e92ddcaf 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -582,6 +582,9 @@ static inline int ip6gre_xmit_ipv6(struct sk_buff *skb, struct net_device *dev)
return -1;
offset = ip6_tnl_parse_tlv_enc_lim(skb, skb_network_header(skb));
+ /* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head */
+ ipv6h = ipv6_hdr(skb);
+
if (offset > 0) {
struct ipv6_tlv_tnl_enc_lim *tel;
tel = (struct ipv6_tlv_tnl_enc_lim *)&skb_network_header(skb)[offset];
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 753d6d0860fb14c100ab8b20799782ab8160..02923f956ac8ba5f3270e8d5282fd9637c2d 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1303,6 +1303,8 @@ ip6ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
fl6.flowlabel = key->label;
} else {
offset = ip6_tnl_parse_tlv_enc_lim(skb, skb_network_header(skb));
+ /* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head */
+ ipv6h = ipv6_hdr(skb);
if (offset > 0) {
struct ipv6_tlv_tnl_enc_lim *tel;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 net 2/2] ipv6: fix ip6_tnl_parse_tlv_enc_lim()
From: Eric Dumazet @ 2017-01-24 0:43 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Willem de Bruijn, Alexei Starovoitov,
Eric Dumazet
In-Reply-To: <20170124004306.19236-1-edumazet@google.com>
This function suffers from multiple issues.
First one is that pskb_may_pull() may reallocate skb->head,
so the 'raw' pointer needs either to be reloaded or not used at all.
Second issue is that NEXTHDR_DEST handling does not validate
that the options are present in skb->data, so we might read
garbage or access non existent memory.
With help from Willem de Bruijn.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Willem de Bruijn <willemb@google.com>
---
net/ipv6/ip6_tunnel.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 02923f956ac8ba5f3270e8d5282fd9637c2d..ff8ee06491c335d209e86bb15f2526ab1915 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -400,18 +400,19 @@ ip6_tnl_dev_uninit(struct net_device *dev)
__u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
{
- const struct ipv6hdr *ipv6h = (const struct ipv6hdr *) raw;
- __u8 nexthdr = ipv6h->nexthdr;
- __u16 off = sizeof(*ipv6h);
+ const struct ipv6hdr *ipv6h = (const struct ipv6hdr *)raw;
+ unsigned int nhoff = raw - skb->data;
+ unsigned int off = nhoff + sizeof(*ipv6h);
+ u8 next, nexthdr = ipv6h->nexthdr;
while (ipv6_ext_hdr(nexthdr) && nexthdr != NEXTHDR_NONE) {
- __u16 optlen = 0;
struct ipv6_opt_hdr *hdr;
- if (raw + off + sizeof(*hdr) > skb->data &&
- !pskb_may_pull(skb, raw - skb->data + off + sizeof (*hdr)))
+ u16 optlen;
+
+ if (!pskb_may_pull(skb, off + sizeof(*hdr)))
break;
- hdr = (struct ipv6_opt_hdr *) (raw + off);
+ hdr = (struct ipv6_opt_hdr *)(skb->data + off);
if (nexthdr == NEXTHDR_FRAGMENT) {
struct frag_hdr *frag_hdr = (struct frag_hdr *) hdr;
if (frag_hdr->frag_off)
@@ -422,20 +423,29 @@ __u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
} else {
optlen = ipv6_optlen(hdr);
}
+ /* cache hdr->nexthdr, since pskb_may_pull() might
+ * invalidate hdr
+ */
+ next = hdr->nexthdr;
if (nexthdr == NEXTHDR_DEST) {
- __u16 i = off + 2;
+ u16 i = 2;
+
+ /* Remember : hdr is no longer valid at this point. */
+ if (!pskb_may_pull(skb, off + optlen))
+ break;
+
while (1) {
struct ipv6_tlv_tnl_enc_lim *tel;
/* No more room for encapsulation limit */
- if (i + sizeof (*tel) > off + optlen)
+ if (i + sizeof(*tel) > optlen)
break;
- tel = (struct ipv6_tlv_tnl_enc_lim *) &raw[i];
+ tel = (struct ipv6_tlv_tnl_enc_lim *) skb->data + off + i;
/* return index of option if found and valid */
if (tel->type == IPV6_TLV_TNL_ENCAP_LIMIT &&
tel->length == 1)
- return i;
+ return i + off - nhoff;
/* else jump to next option */
if (tel->type)
i += tel->length + 2;
@@ -443,7 +453,7 @@ __u16 ip6_tnl_parse_tlv_enc_lim(struct sk_buff *skb, __u8 *raw)
i++;
}
}
- nexthdr = hdr->nexthdr;
+ nexthdr = next;
off += optlen;
}
return 0;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: XDP offload to hypervisor
From: Alexei Starovoitov @ 2017-01-24 1:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: John Fastabend, jasowang, john.r.fastabend, netdev, daniel
In-Reply-To: <20170123230727-mutt-send-email-mst@kernel.org>
On Mon, Jan 23, 2017 at 11:40:29PM +0200, Michael S. Tsirkin wrote:
> I've been thinking about passing XDP programs from guest to the
> hypervisor. Basically, after getting an incoming packet, we could run
> an XDP program in host kernel.
>
> If the result is XDP_DROP or XDP_TX we don't need to wake up the guest at all!
that's an interesting idea!
Long term 'xdp offload' needs to be defined, since NICs become smarter
and can accelerate xdp programs.
So pushing the xdp program down from virtio in the guest into host
and from x86 into nic cpu should probably be handled through the same api.
> When using tun for networking - especially with adjust_head - this
> unfortunately probably means we need to do a data copy unless there is
> enough headroom. How much is enough though?
Frankly I don't understand the whole virtio nit picking that was happening.
imo virtio+xdp by itself is only useful for debugging, development and testing
of xdp programs in a VM. The discussion about performance of virtio+xdp
will only be meaningful when corresponding host part is done.
Likely in the form of vhost extensions and may be driver changes.
Trying to optimize virtio+xdp when host is doing traditional skb+vhost
isn't going to be impactful.
But when host can do xdp in phyiscal NIC that can deliver raw
pages into vhost that gets picked up by guest virtio, then we hopefully
will be around 10G line rate. page pool is likely needed in such scenario.
Some new xdp action like xdp_tx_into_vhost or whatever.
And guest will be seeing full pages that host nic provided and discussion
about headroom will be automatically solved.
Arguing that skb has 64-byte headroom and therefore we need to
reduce XDP_PACKET_HEADROOM is really upside down.
> Another issue is around host/guest ABI. Guest BPF could add new features
> at any point. What if hypervisor can not support it all? I guess we
> could try loading program into hypervisor and run it within guest on
> failure to load, but this ignores question of cross-version
> compatibility - someone might start guest on a new host
> then try to move to an old one. So we will need an option
> "behave like an older host" such that guest can start and then
> move to an older host later. This will likely mean
> implementing this validation of programs in qemu userspace unless linux
> can supply something like this. Is this (disabling some features)
> something that might be of interest to larger bpf community?
In case of x86->nic offload not all xdp features will be supported
by the nic and that is expected. The user will request 'offload of xdp prog'
in some form and if it cannot be done, then xdp programs will run
on x86 as before. Same thing, I imagine, is applicable to virtio->host
offload. Therefore I don't see a need for user space visible
feature negotiation.
> With a device such as macvtap there exist configurations where a single
> guest is in control of the device (aka passthrough mode) in that case
> there's a potential to run xdp on host before host skb is built, unless
> host already has an xdp program attached. If it does we could run the
> program within guest, but what if a guest program got attached first?
> Maybe we should pass a flag in the packet "xdp passed on this packet in
> host". Then, guest can skip running it. Unless we do a full reset
> there's always a potential for packets to slip through, e.g. on xdp
> program changes. Maybe a flush command is needed, or force queue or
> device reset to make sure nothing is going on. Does this make sense?
All valid questions and concerns.
Since there is still no xdp_adjust_head support in virtio,
it feels kinda early to get into detailed 'virtio offload' discussion.
^ permalink raw reply
* [PATCH] net: ks8851: Drop eeprom_size structure member
From: Stephen Boyd @ 2017-01-24 1:49 UTC (permalink / raw)
To: David S. Miller; +Cc: linux-kernel, netdev
After commit 51b7b1c34e19 (KSZ8851-SNL: Add ethtool support for
EEPROM via eeprom_93cx6, 2011-11-21) this structure member is
unused. Delete it.
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---
Found while debugging an eeprom issue.
drivers/net/ethernet/micrel/ks8851.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index e7e1aff40bd9..955d69a8e8d3 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -84,7 +84,6 @@ union ks8851_tx_hdr {
* @rc_ier: Cached copy of KS_IER.
* @rc_ccr: Cached copy of KS_CCR.
* @rc_rxqcr: Cached copy of KS_RXQCR.
- * @eeprom_size: Companion eeprom size in Bytes, 0 if no eeprom
* @eeprom: 93CX6 EEPROM state for accessing on-board EEPROM.
* @vdd_reg: Optional regulator supplying the chip
* @vdd_io: Optional digital power supply for IO
@@ -120,7 +119,6 @@ struct ks8851_net {
u16 rc_ier;
u16 rc_rxqcr;
u16 rc_ccr;
- u16 eeprom_size;
struct mii_if_info mii;
struct ks8851_rxctrl rxctrl;
@@ -1533,11 +1531,6 @@ static int ks8851_probe(struct spi_device *spi)
/* cache the contents of the CCR register for EEPROM, etc. */
ks->rc_ccr = ks8851_rdreg16(ks, KS_CCR);
- if (ks->rc_ccr & CCR_EEPROM)
- ks->eeprom_size = 128;
- else
- ks->eeprom_size = 0;
-
ks8851_read_selftest(ks);
ks8851_init_mac(ks);
--
2.10.0.297.gf6727b0
^ permalink raw reply related
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Alexei Starovoitov @ 2017-01-24 2:09 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Network Development, David S. Miller, Daniel Borkmann,
Alexei Starovoitov, David Ahern
In-Reply-To: <ddd9e097f54e894197a2925731de02d1d3cae137.1485203708.git.luto@kernel.org>
On Mon, Jan 23, 2017 at 12:36:08PM -0800, Andy Lutomirski wrote:
> To see how cgroup+bpf interacts with network namespaces, I wrote a
> little program called show_bind that calls getsockopt(...,
> SO_BINDTODEVICE, ...) and prints the result. It did this:
>
> # ./ip link add dev vrf0 type vrf table 10
> # ./ip vrf exec vrf0 ./show_bind
> Default binding is "vrf0"
> # ./ip vrf exec vrf0 unshare -n ./show_bind
> show_bind: getsockopt: No such device
>
> What's happening here is that "ip vrf" looks up vrf0's ifindex in
> the init netns and installs a hook that binds sockets to that
> ifindex. When the hook runs in a different netns, it sets
> sk_bound_dev_if to an ifindex from the wrong netns, resulting in
> incorrect behavior. In this particular example, the ifindex was 4
> and there was no ifindex 4 in the new netns. If there had been,
> this test would have malfunctioned differently
>
> Since it's rather late in the release cycle, let's punt. This patch
> makes it impossible to install cgroup+bpf hooks outside the init
> netns and makes them not run on sockets that aren't in the init
> netns.
>
> In a future release, it should be relatively straightforward to make
> these hooks be local to a netns and, if needed, to add a flag so
> that hooks can be made global if necessary. Global hooks should
> presumably be constrained so that they can't write to any ifindex
> fields.
>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: David Ahern <dsa@cumulusnetworks.com>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>
> DaveM, this mitigates a bug in a feature that's new in 4.10, and the
> bug can be hit using current iproute2 -git. please consider this for
> -net.
>
> Changes from v1:
> - Fix the commit message. 'git commit' was very clever and thought that
> all the interesting bits of the test case were intended to be comments
> and stripped them. Whoops!
>
> kernel/bpf/cgroup.c | 21 +++++++++++++++++++++
> kernel/bpf/syscall.c | 11 +++++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index a515f7b007c6..a824f543de69 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -143,6 +143,17 @@ int __cgroup_bpf_run_filter_skb(struct sock *sk,
> if (!sk || !sk_fullsock(sk))
> return 0;
>
> + /*
> + * For now, socket bpf hooks attached to cgroups can only be
> + * installed in the init netns and only affect the init netns.
> + * This could be relaxed in the future once some semantic issues
> + * are resolved. For example, ifindexes belonging to one netns
> + * should probably not be visible to hooks installed by programs
> + * running in a different netns.
> + */
> + if (sock_net(sk) != &init_net)
> + return 0;
Nack.
Such check will create absolutely broken abi that we won't be able to fix later.
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index e89acea22ecf..c0bbc55e244d 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -902,6 +902,17 @@ static int bpf_prog_attach(const union bpf_attr *attr)
> struct cgroup *cgrp;
> enum bpf_prog_type ptype;
>
> + /*
> + * For now, socket bpf hooks attached to cgroups can only be
> + * installed in the init netns and only affect the init netns.
> + * This could be relaxed in the future once some semantic issues
> + * are resolved. For example, ifindexes belonging to one netns
> + * should probably not be visible to hooks installed by programs
> + * running in a different netns.
the comment is bogus and shows complete lack of understanding
how bpf is working and how it's being used.
See SKF_AD_IFINDEX that was in classic bpf forever.
> + */
> + if (current->nsproxy->net_ns != &init_net)
> + return -EINVAL;
this restriction I actually don't mind, since it indeed can be
relaxed later, but please make it proper with net_eq()
^ permalink raw reply
* Re: [PATCH net-next] bpf, lpm: fix kfree of im_node in trie_update_elem
From: David Miller @ 2017-01-24 2:18 UTC (permalink / raw)
To: daniel; +Cc: ast, daniel, netdev
In-Reply-To: <6052894aa1bd4df5a5c7aaa05f5b63a980cbe371.1485217298.git.daniel@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Tue, 24 Jan 2017 01:26:46 +0100
> We need to initialize im_node to NULL, otherwise in case of error path
> it gets passed to kfree() as uninitialized pointer.
>
> Fixes: b95a5c4db09b ("bpf: add a longest prefix match trie map implementation")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> Mentioned it in http://patchwork.ozlabs.org/patch/718070/, but
> was probably overlooked.
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] Bluetooth: hidp: might sleep error in hidp_session_thread
From: Brian Norris @ 2017-01-24 2:31 UTC (permalink / raw)
To: Jeffy Chen
Cc: linux-bluetooth, Douglas Anderson, Johan Hedberg, netdev,
linux-kernel, David S. Miller, Marcel Holtmann, Gustavo Padovan,
Peter Hurley, Peter Zijlstra, Guenter Roeck
In-Reply-To: <1484920328-20522-1-git-send-email-jeffy.chen@rock-chips.com>
Hi Jeffy,
On Fri, Jan 20, 2017 at 09:52:08PM +0800, Jeffy Chen wrote:
> [ 39.044329] do not call blocking ops when !TASK_RUNNING; state=1 set
> at [<ffffffbffc290358>] hidp_session_thread+0x110/0x568 [hidp]
> ...
> [ 40.159664] Call trace:
> [ 40.162122] [<ffffffc00024ae08>] __might_sleep+0x64/0x90
> [ 40.167443] [<ffffffc00080568c>] lock_sock_nested+0x30/0x78
> [ 40.173047] [<ffffffbffc1b3ca0>] l2cap_sock_sendmsg+0x90/0xf0
> [bluetooth]
> [ 40.179842] [<ffffffc0008012c4>] sock_sendmsg+0x4c/0x68
> [ 40.185072] [<ffffffc000801414>] kernel_sendmsg+0x54/0x68
> [ 40.190477] [<ffffffbffc28f4d0>] hidp_send_frame+0x78/0xa0 [hidp]
> [ 40.196574] [<ffffffbffc28f53c>] hidp_process_transmit+0x44/0x98
> [hidp]
> [ 40.203191] [<ffffffbffc2905ac>] hidp_session_thread+0x364/0x568
> [hidp]
Am I crazy, or are several other protocols broken like this too? I see a
similar structure in net/bluetooth/bnep/core.c and
net/bluetooth/cmtp/core.c, at least, each of which also call
kernel_sendmsg(), which might be an l2cap socket (...I think? I'm not a
bluetooth expert really).
>
> Following (https://lwn.net/Articles/628628/).
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> net/bluetooth/hidp/core.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 0bec458..bfd3fb8 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -1180,7 +1180,9 @@ static void hidp_session_run(struct hidp_session *session)
> struct sock *ctrl_sk = session->ctrl_sock->sk;
> struct sock *intr_sk = session->intr_sock->sk;
> struct sk_buff *skb;
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
>
> + add_wait_queue(sk_sleep(intr_sk), &wait);
> for (;;) {
> /*
> * This thread can be woken up two ways:
> @@ -1188,12 +1190,10 @@ static void hidp_session_run(struct hidp_session *session)
> * session->terminate flag and wakes this thread up.
> * - Via modifying the socket state of ctrl/intr_sock. This
> * thread is woken up by ->sk_state_changed().
> - *
> - * Note: set_current_state() performs any necessary
> - * memory-barriers for us.
> */
> - set_current_state(TASK_INTERRUPTIBLE);
>
> + /* Ensure session->terminate is updated */
> + smp_mb__before_atomic();
> if (atomic_read(&session->terminate))
> break;
>
> @@ -1227,11 +1227,14 @@ static void hidp_session_run(struct hidp_session *session)
> hidp_process_transmit(session, &session->ctrl_transmit,
> session->ctrl_sock);
>
> - schedule();
> + wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
I think this looks mostly good, except what about the
hidp_session_terminate() condition? In that case, you're running
wake_up_process() -- which won't set WQ_FLAG_WOKEN for us. So what
happens if we see a hidp_session_terminate() call in between the check
for the ->terminate count, but before we call wait_woken()? IIUC, I
think we'll just ignore the call and keep waiting for the next wake
signal.
I think you might need to rework hidp_session_terminate() too, to
actually target the wait queue and not just the processes.
IIUC, of course. I could be wrong...
Brian
> }
> + remove_wait_queue(sk_sleep(intr_sk), &wait);
>
> atomic_inc(&session->terminate);
> - set_current_state(TASK_RUNNING);
> +
> + /* Ensure session->terminate is updated */
> + smp_mb__after_atomic();
> }
>
> /*
> --
> 2.1.4
>
>
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: David Ahern @ 2017-01-24 2:31 UTC (permalink / raw)
To: Alexei Starovoitov, Andy Lutomirski
Cc: Network Development, David S. Miller, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <20170124020950.GA72992@ast-mbp.thefacebook.com>
On 1/23/17 7:09 PM, Alexei Starovoitov wrote:
>> + */
>> + if (current->nsproxy->net_ns != &init_net)
>> + return -EINVAL;
>
> this restriction I actually don't mind, since it indeed can be
> relaxed later, but please make it proper with net_eq()
>
I do mind. Why have different restrictions for the skb and sk filters?
I have already shown that ip can alleviate the management aspects of the implementation -- just like ip netns does.
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Andy Lutomirski @ 2017-01-24 2:39 UTC (permalink / raw)
To: David Ahern
Cc: Alexei Starovoitov, Andy Lutomirski, Network Development,
David S. Miller, Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <678f4d6f-e2f8-eb37-207a-c77269507f9d@cumulusnetworks.com>
On Mon, Jan 23, 2017 at 6:31 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 1/23/17 7:09 PM, Alexei Starovoitov wrote:
>>> + */
>>> + if (current->nsproxy->net_ns != &init_net)
>>> + return -EINVAL;
>>
>> this restriction I actually don't mind, since it indeed can be
>> relaxed later, but please make it proper with net_eq()
>>
>
> I do mind. Why have different restrictions for the skb and sk filters?
>
> I have already shown that ip can alleviate the management aspects of the implementation -- just like ip netns does.
I'm not sure I followed what you meant. If I understood right (which
is a big if) you're saying that ip vrf when run in a netns works
correctly in that netns. I agree, but I think that it would continue
to work (even more reliably) if the hooks only executed in the netns
in which they were created. I think that would probably be a good
improvement, and it could be done on top of my patch, but I'm not
about to write that code for 4.10.
--Andy
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Andy Lutomirski @ 2017-01-24 2:42 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andy Lutomirski, Network Development, David S. Miller,
Daniel Borkmann, Alexei Starovoitov, David Ahern
In-Reply-To: <20170124020950.GA72992@ast-mbp.thefacebook.com>
On Mon, Jan 23, 2017 at 6:09 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Mon, Jan 23, 2017 at 12:36:08PM -0800, Andy Lutomirski wrote:
>> To see how cgroup+bpf interacts with network namespaces, I wrote a
>> little program called show_bind that calls getsockopt(...,
>> SO_BINDTODEVICE, ...) and prints the result. It did this:
>>
>> # ./ip link add dev vrf0 type vrf table 10
>> # ./ip vrf exec vrf0 ./show_bind
>> Default binding is "vrf0"
>> # ./ip vrf exec vrf0 unshare -n ./show_bind
>> show_bind: getsockopt: No such device
>>
>> What's happening here is that "ip vrf" looks up vrf0's ifindex in
>> the init netns and installs a hook that binds sockets to that
>> ifindex. When the hook runs in a different netns, it sets
>> sk_bound_dev_if to an ifindex from the wrong netns, resulting in
>> incorrect behavior. In this particular example, the ifindex was 4
>> and there was no ifindex 4 in the new netns. If there had been,
>> this test would have malfunctioned differently
>>
>> Since it's rather late in the release cycle, let's punt. This patch
>> makes it impossible to install cgroup+bpf hooks outside the init
>> netns and makes them not run on sockets that aren't in the init
>> netns.
>>
>> In a future release, it should be relatively straightforward to make
>> these hooks be local to a netns and, if needed, to add a flag so
>> that hooks can be made global if necessary. Global hooks should
>> presumably be constrained so that they can't write to any ifindex
>> fields.
>>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: David Ahern <dsa@cumulusnetworks.com>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>
>> DaveM, this mitigates a bug in a feature that's new in 4.10, and the
>> bug can be hit using current iproute2 -git. please consider this for
>> -net.
>>
>> Changes from v1:
>> - Fix the commit message. 'git commit' was very clever and thought that
>> all the interesting bits of the test case were intended to be comments
>> and stripped them. Whoops!
>>
>> kernel/bpf/cgroup.c | 21 +++++++++++++++++++++
>> kernel/bpf/syscall.c | 11 +++++++++++
>> 2 files changed, 32 insertions(+)
>>
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index a515f7b007c6..a824f543de69 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -143,6 +143,17 @@ int __cgroup_bpf_run_filter_skb(struct sock *sk,
>> if (!sk || !sk_fullsock(sk))
>> return 0;
>>
>> + /*
>> + * For now, socket bpf hooks attached to cgroups can only be
>> + * installed in the init netns and only affect the init netns.
>> + * This could be relaxed in the future once some semantic issues
>> + * are resolved. For example, ifindexes belonging to one netns
>> + * should probably not be visible to hooks installed by programs
>> + * running in a different netns.
>> + */
>> + if (sock_net(sk) != &init_net)
>> + return 0;
>
> Nack.
> Such check will create absolutely broken abi that we won't be able to fix later.
Please explain how the change results in a broken ABI and how the
current ABI is better. I gave a fully worked out example of how the
current ABI misbehaves using only standard Linux networking tools.
>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index e89acea22ecf..c0bbc55e244d 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -902,6 +902,17 @@ static int bpf_prog_attach(const union bpf_attr *attr)
>> struct cgroup *cgrp;
>> enum bpf_prog_type ptype;
>>
>> + /*
>> + * For now, socket bpf hooks attached to cgroups can only be
>> + * installed in the init netns and only affect the init netns.
>> + * This could be relaxed in the future once some semantic issues
>> + * are resolved. For example, ifindexes belonging to one netns
>> + * should probably not be visible to hooks installed by programs
>> + * running in a different netns.
>
> the comment is bogus and shows complete lack of understanding
> how bpf is working and how it's being used.
> See SKF_AD_IFINDEX that was in classic bpf forever.
>
I think I disagree completely.
With classic BPF, a program creates a socket and binds a bpf program
to it. That program can see the ifindex, which is fine because that
ifindex is scoped to the socket's netns, which is (unless the caller
uses setns() or unshare()) the same as the caller's netns, so the
ifindex means exactly what the caller thinks it means.
With cgroup+bpf, the program installing the hook can be completely
unrelated to the program whose sockets are subject to the hook, and,
if they're using different namespaces, it breaks.
--Andy
^ permalink raw reply
* Re: [PULL] vhost: cleanups and fixes
From: Michael S. Tsirkin @ 2017-01-24 2:45 UTC (permalink / raw)
To: Linus Torvalds
Cc: gcampana, KVM list, Network Development, pmorel,
Linux Kernel Mailing List, virtualization, Dan Carpenter,
Colin Ian King, bhumirks, silbe
In-Reply-To: <CA+55aFypi-Vg6CGtDX7tv_DfuN5ztrtSoQxexZiPKxDtmo9ZmQ@mail.gmail.com>
On Mon, Jan 23, 2017 at 01:50:32PM -0800, Linus Torvalds wrote:
> On Mon, Jan 23, 2017 at 7:05 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > virtio, vhost: fixes, cleanups
>
> Was there a reason why you sent this twice?
>
> Or was this *supposed* to be the ARM DMA fix pull request? Because it wasn't.
>
> Linus
Ouch sorry. Was tired and forgot I already sent it. Should have
checked.
--
MST
^ permalink raw reply
* Re: XDP offload to hypervisor
From: Michael S. Tsirkin @ 2017-01-24 2:47 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: John Fastabend, jasowang, john.r.fastabend, netdev, daniel
In-Reply-To: <20170124010201.GB60699@ast-mbp.thefacebook.com>
On Mon, Jan 23, 2017 at 05:02:02PM -0800, Alexei Starovoitov wrote:
> > Another issue is around host/guest ABI. Guest BPF could add new features
> > at any point. What if hypervisor can not support it all? I guess we
> > could try loading program into hypervisor and run it within guest on
> > failure to load, but this ignores question of cross-version
> > compatibility - someone might start guest on a new host
> > then try to move to an old one. So we will need an option
> > "behave like an older host" such that guest can start and then
> > move to an older host later. This will likely mean
> > implementing this validation of programs in qemu userspace unless linux
> > can supply something like this. Is this (disabling some features)
> > something that might be of interest to larger bpf community?
>
> In case of x86->nic offload not all xdp features will be supported
> by the nic and that is expected. The user will request 'offload of xdp prog'
> in some form and if it cannot be done, then xdp programs will run
> on x86 as before. Same thing, I imagine, is applicable to virtio->host
> offload. Therefore I don't see a need for user space visible
> feature negotiation.
Not userspace visible - guest visible. As guests move between hosts,
you need to make sure source host does not commit to a feature that
destination host does not support.
--
MST
^ permalink raw reply
* Re: [PATCH v2] bpf: Restrict cgroup bpf hooks to the init netns
From: Alexei Starovoitov @ 2017-01-24 3:13 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Lutomirski, Network Development, David S. Miller,
Daniel Borkmann, Alexei Starovoitov, David Ahern
In-Reply-To: <CALCETrUq_ZSso_252ixzjt+WbFOv_K57zbePUXneLv-Tgi5=jA@mail.gmail.com>
On Mon, Jan 23, 2017 at 06:42:27PM -0800, Andy Lutomirski wrote:
> On Mon, Jan 23, 2017 at 6:09 PM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Mon, Jan 23, 2017 at 12:36:08PM -0800, Andy Lutomirski wrote:
> >> To see how cgroup+bpf interacts with network namespaces, I wrote a
> >> little program called show_bind that calls getsockopt(...,
> >> SO_BINDTODEVICE, ...) and prints the result. It did this:
> >>
> >> # ./ip link add dev vrf0 type vrf table 10
> >> # ./ip vrf exec vrf0 ./show_bind
> >> Default binding is "vrf0"
> >> # ./ip vrf exec vrf0 unshare -n ./show_bind
> >> show_bind: getsockopt: No such device
> >>
> >> What's happening here is that "ip vrf" looks up vrf0's ifindex in
> >> the init netns and installs a hook that binds sockets to that
> >> ifindex. When the hook runs in a different netns, it sets
> >> sk_bound_dev_if to an ifindex from the wrong netns, resulting in
> >> incorrect behavior. In this particular example, the ifindex was 4
> >> and there was no ifindex 4 in the new netns. If there had been,
> >> this test would have malfunctioned differently
> >>
> >> Since it's rather late in the release cycle, let's punt. This patch
> >> makes it impossible to install cgroup+bpf hooks outside the init
> >> netns and makes them not run on sockets that aren't in the init
> >> netns.
> >>
> >> In a future release, it should be relatively straightforward to make
> >> these hooks be local to a netns and, if needed, to add a flag so
> >> that hooks can be made global if necessary. Global hooks should
> >> presumably be constrained so that they can't write to any ifindex
> >> fields.
> >>
> >> Cc: Daniel Borkmann <daniel@iogearbox.net>
> >> Cc: Alexei Starovoitov <ast@kernel.org>
> >> Cc: David Ahern <dsa@cumulusnetworks.com>
> >> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> >> ---
> >>
> >> DaveM, this mitigates a bug in a feature that's new in 4.10, and the
> >> bug can be hit using current iproute2 -git. please consider this for
> >> -net.
> >>
> >> Changes from v1:
> >> - Fix the commit message. 'git commit' was very clever and thought that
> >> all the interesting bits of the test case were intended to be comments
> >> and stripped them. Whoops!
> >>
> >> kernel/bpf/cgroup.c | 21 +++++++++++++++++++++
> >> kernel/bpf/syscall.c | 11 +++++++++++
> >> 2 files changed, 32 insertions(+)
> >>
> >> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> >> index a515f7b007c6..a824f543de69 100644
> >> --- a/kernel/bpf/cgroup.c
> >> +++ b/kernel/bpf/cgroup.c
> >> @@ -143,6 +143,17 @@ int __cgroup_bpf_run_filter_skb(struct sock *sk,
> >> if (!sk || !sk_fullsock(sk))
> >> return 0;
> >>
> >> + /*
> >> + * For now, socket bpf hooks attached to cgroups can only be
> >> + * installed in the init netns and only affect the init netns.
> >> + * This could be relaxed in the future once some semantic issues
> >> + * are resolved. For example, ifindexes belonging to one netns
> >> + * should probably not be visible to hooks installed by programs
> >> + * running in a different netns.
> >> + */
> >> + if (sock_net(sk) != &init_net)
> >> + return 0;
> >
> > Nack.
> > Such check will create absolutely broken abi that we won't be able to fix later.
>
> Please explain how the change results in a broken ABI and how the
> current ABI is better. I gave a fully worked out example of how the
> current ABI misbehaves using only standard Linux networking tools.
I gave an example in the other thread that demonstrated
cgroup escape by the appliction when it changes netns.
If application became part of cgroup, it has to stay there,
no matter setns() calls afterwards.
> >
> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >> index e89acea22ecf..c0bbc55e244d 100644
> >> --- a/kernel/bpf/syscall.c
> >> +++ b/kernel/bpf/syscall.c
> >> @@ -902,6 +902,17 @@ static int bpf_prog_attach(const union bpf_attr *attr)
> >> struct cgroup *cgrp;
> >> enum bpf_prog_type ptype;
> >>
> >> + /*
> >> + * For now, socket bpf hooks attached to cgroups can only be
> >> + * installed in the init netns and only affect the init netns.
> >> + * This could be relaxed in the future once some semantic issues
> >> + * are resolved. For example, ifindexes belonging to one netns
> >> + * should probably not be visible to hooks installed by programs
> >> + * running in a different netns.
> >
> > the comment is bogus and shows complete lack of understanding
> > how bpf is working and how it's being used.
> > See SKF_AD_IFINDEX that was in classic bpf forever.
> >
>
> I think I disagree completely.
>
> With classic BPF, a program creates a socket and binds a bpf program
> to it. That program can see the ifindex, which is fine because that
> ifindex is scoped to the socket's netns, which is (unless the caller
> uses setns() or unshare()) the same as the caller's netns, so the
> ifindex means exactly what the caller thinks it means.
>
> With cgroup+bpf, the program installing the hook can be completely
> unrelated to the program whose sockets are subject to the hook, and,
> if they're using different namespaces, it breaks.
Please also see ingress_ifindex, ifindex, bpf_redirect(), bpf_clone_redirect()
that all operate on the ifindex while the program can be detached from
the application. In general bpf program and application that loaded and
attached it are mostly disjoint in case of networking. We have tail_call
mechanism and master bpf prog may call programs installed by other apps
that may have exited already.
cls_bpf is scoped by netdev that belongs to some netns.
If after attaching a program with hardcoded if(ifindex==3) check
to such netdev, this netdev is moved into different netns, this 'if' check
and the program become bogus and won't do what program author
expected. It is expected behavior. The same thing with current 'ip vrf'
implementation that Dave is adjusting. It's not a bug in cgroup+bpf behavior.
^ permalink raw reply
* [PATCH net-next] net: dsa: Drop WARN() in tag_brcm.c
From: Florian Fainelli @ 2017-01-24 3:19 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, David S. Miller,
open list
We may be able to see invalid Broadcom tags when the hardware and drivers are
misconfigured, or just while exercising the error path. Instead of flooding
the console with messages, flat out drop the packet.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
net/dsa/tag_brcm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index af82927674e0..cb5a2b7a0118 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -121,7 +121,8 @@ static int brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
/* We should never see a reserved reason code without knowing how to
* handle it
*/
- WARN_ON(brcm_tag[2] & BRCM_EG_RC_RSVD);
+ if (unlikely(brcm_tag[2] & BRCM_EG_RC_RSVD))
+ goto out_drop;
/* Locate which port this is coming from */
source_port = brcm_tag[3] & BRCM_EG_PID_MASK;
--
2.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox