* Re: [PATCH 1/2] net: Remove ndo_xmit_flush netdev operation, use signalling instead.
From: David Miller @ 2014-08-26 4:50 UTC (permalink / raw)
To: therbert
Cc: netdev, jhs, hannes, edumazet, jeffrey.t.kirsher, rusty, dborkman,
brouer
In-Reply-To: <CA+mtBx9kwbw=XNhQMRjHhnh8PRjy1A93MPPST6bwdMpXjz7YjA@mail.gmail.com>
From: Tom Herbert <therbert@google.com>
Date: Mon, 25 Aug 2014 20:42:59 -0700
> I would suggest the flush should be done if !skb->xmit_more or queue
> is being stopped. So maybe pull this code into it's own function e.g.
> igb_flush(tx_ring). Then do:
>
> if (igb_maybe_stop_tx(tx_ring, DESC_NEEDED) || !skb->more)
> igb_flush(tx_ring);
Agreed, and we will need to sort this out in all cases before we start
actually doing real deferral by setting skb->xmit_more.
^ permalink raw reply
* Re: [PATCH v6 net-next 4/6] bpf: enable bpf syscall on x64 and i386
From: Alexei Starovoitov @ 2014-08-26 4:24 UTC (permalink / raw)
To: Stephen Hemminger
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Brendan Gregg, Namhyung Kim, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <20140825205201.10ac5ee3@uryu.home.lan>
On Mon, Aug 25, 2014 at 8:52 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> Per discussion at Kernel Summit. Every new syscall requires
> a manual page and test programs. We have had too many new syscalls
> that are DOA.
There is verifier testsuite that is testing eBPF verifier from userspace
via bpf syscall. Also there are multiple examples and libbpf.
I think test coverage for bpf syscall is quite substantial already.
As far as manpage I already mentioned my plan in the other thread.
I will do it when stuff lands, since writing docs in english is the most
difficult part of these patches. I already rewrote Documentation/../filter.txt
several times. Even once Kees noticed discrepancies there when
interface changed from map_id to map_fd.
To do a manpage I'll use a help from tech writer, since I want it to
be better than docs I put in Documentation/...
^ permalink raw reply
* Re: [PATCH v6 net-next 1/6] net: filter: add "load 64-bit immediate" eBPF instruction
From: Alexei Starovoitov @ 2014-08-26 4:12 UTC (permalink / raw)
To: David Miller
Cc: Ingo Molnar, Linus Torvalds, Andy Lutomirski, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Brendan Gregg, Namhyung Kim, H. Peter Anvin, Andrew Morton,
Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <20140825.180621.1003932711320755950.davem@davemloft.net>
On Mon, Aug 25, 2014 at 6:06 PM, David Miller <davem@davemloft.net> wrote:
>
> Instead I would rather you look into a model like what the quake
> engine uses for it's VM.
Thanks for the tip! I wasn't aware of quake vm.
I've looked through several papers and slides.
I'm surely missing something in what they're doing, but
here is my comparison of eBPF vs QVM:
- QVM ISA is stack based vs eBPF registers
- pointer types are predefined by QVM ISA whereas eBPF relies
on static verifier which is more extensible, since verifier can get
progressively smarter with time without need to change interpreter,
llvm and JITs, whereas QVM would need changes through the
toolchain, interpreter, JITs to support new pointer type
- QVM calls with negative values invoke helper functions, which is
similar to eBPF calls. The difference is QVM keeps negative values
while interpreting and doing run-time checking of arguments whereas
eBPF is statically verifying all before interpreting
- access to QVM 'local' memory is bounds checked at run-time,
whereas eBPF does load/store bounds checking by static analysis
I may be wrong, but it seems possible to side step QVM run-time
checking, since their 'top of stack' is typeless and it seems possible
to push constant as a pointer there.
I'm biased, but eBPF seems like better architecture,
more flexible, likely faster to interpret, simple JITs, more powerful
compiler. The downside, of course, eBPF verifier is more complex
than QVM which is mainly relying on run-time checks.
^ permalink raw reply
* Re: [PATCH v6 net-next 4/6] bpf: enable bpf syscall on x64 and i386
From: Stephen Hemminger @ 2014-08-26 3:52 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Ingo Molnar, Linus Torvalds, Andy Lutomirski,
Steven Rostedt, Daniel Borkmann, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Brendan Gregg, Namhyung Kim, H. Peter Anvin,
Andrew Morton, Kees Cook, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1409014858-1410-5-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
Per discussion at Kernel Summit. Every new syscall requires
a manual page and test programs. We have had too many new syscalls
that are DOA.
^ permalink raw reply
* Re: [PATCH 1/2] net: Remove ndo_xmit_flush netdev operation, use signalling instead.
From: Tom Herbert @ 2014-08-26 3:42 UTC (permalink / raw)
To: David Miller
Cc: Linux Netdev List, Jamal Hadi Salim, Hannes Frederic Sowa,
Eric Dumazet, Jeff Kirsher, Rusty Russell, Daniel Borkmann,
brouer
In-Reply-To: <20140825.163502.973913220915588977.davem@davemloft.net>
On Mon, Aug 25, 2014 at 4:35 PM, David Miller <davem@davemloft.net> wrote:
>
> As reported by Jesper Dangaard Brouer, for high packet rates the
> overhead of having another indirect call in the TX path is
> non-trivial.
>
> There is the indirect call itself, and then there is all of the
> reloading of the state to refetch the tail pointer value and
> then write the device register.
>
> Move to a more passive scheme, which requires very light modifications
> to the device drivers.
>
> The signal is a new skb->xmit_more value, if it is non-zero it means
> that more SKBs are pending to be transmitted on the same queue as the
> current SKB. And therefore, the driver may elide the tail pointer
> update.
>
> Right now skb->xmit_more is always zero.
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 36 +++++++++++--------------------
> drivers/net/virtio_net.c | 12 +++--------
> include/linux/netdevice.h | 25 ++-------------------
> include/linux/skbuff.h | 2 ++
> 4 files changed, 19 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index b9c020a..89c29b4 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -136,7 +136,6 @@ static void igb_update_phy_info(unsigned long);
> static void igb_watchdog(unsigned long);
> static void igb_watchdog_task(struct work_struct *);
> static netdev_tx_t igb_xmit_frame(struct sk_buff *skb, struct net_device *);
> -static void igb_xmit_flush(struct net_device *netdev, u16 queue);
> static struct rtnl_link_stats64 *igb_get_stats64(struct net_device *dev,
> struct rtnl_link_stats64 *stats);
> static int igb_change_mtu(struct net_device *, int);
> @@ -2076,7 +2075,6 @@ static const struct net_device_ops igb_netdev_ops = {
> .ndo_open = igb_open,
> .ndo_stop = igb_close,
> .ndo_start_xmit = igb_xmit_frame,
> - .ndo_xmit_flush = igb_xmit_flush,
> .ndo_get_stats64 = igb_get_stats64,
> .ndo_set_rx_mode = igb_set_rx_mode,
> .ndo_set_mac_address = igb_set_mac,
> @@ -4917,6 +4915,14 @@ static void igb_tx_map(struct igb_ring *tx_ring,
>
> tx_ring->next_to_use = i;
>
> + if (!skb->xmit_more) {
> + writel(i, tx_ring->tail);
> +
> + /* we need this if more than one processor can write to our tail
> + * at a time, it synchronizes IO on IA64/Altix systems
> + */
> + mmiowb();
> + }
> return;
>
I would suggest the flush should be done if !skb->xmit_more or queue
is being stopped. So maybe pull this code into it's own function e.g.
igb_flush(tx_ring). Then do:
if (igb_maybe_stop_tx(tx_ring, DESC_NEEDED) || !skb->more)
igb_flush(tx_ring);
> dma_error:
> @@ -5052,20 +5058,17 @@ out_drop:
> return NETDEV_TX_OK;
> }
>
> -static struct igb_ring *__igb_tx_queue_mapping(struct igb_adapter *adapter, unsigned int r_idx)
> +static inline struct igb_ring *igb_tx_queue_mapping(struct igb_adapter *adapter,
> + struct sk_buff *skb)
> {
> + unsigned int r_idx = skb->queue_mapping;
> +
> if (r_idx >= adapter->num_tx_queues)
> r_idx = r_idx % adapter->num_tx_queues;
>
> return adapter->tx_ring[r_idx];
> }
>
> -static inline struct igb_ring *igb_tx_queue_mapping(struct igb_adapter *adapter,
> - struct sk_buff *skb)
> -{
> - return __igb_tx_queue_mapping(adapter, skb->queue_mapping);
> -}
> -
> static netdev_tx_t igb_xmit_frame(struct sk_buff *skb,
> struct net_device *netdev)
> {
> @@ -5094,21 +5097,6 @@ static netdev_tx_t igb_xmit_frame(struct sk_buff *skb,
> return igb_xmit_frame_ring(skb, igb_tx_queue_mapping(adapter, skb));
> }
>
> -static void igb_xmit_flush(struct net_device *netdev, u16 queue)
> -{
> - struct igb_adapter *adapter = netdev_priv(netdev);
> - struct igb_ring *tx_ring;
> -
> - tx_ring = __igb_tx_queue_mapping(adapter, queue);
> -
> - writel(tx_ring->next_to_use, tx_ring->tail);
> -
> - /* we need this if more than one processor can write to our tail
> - * at a time, it synchronizes IO on IA64/Altix systems
> - */
> - mmiowb();
> -}
> -
> /**
> * igb_tx_timeout - Respond to a Tx Hang
> * @netdev: network interface device structure
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6242108..f0c2824 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -953,15 +953,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> }
> }
>
> - return NETDEV_TX_OK;
> -}
> + if (!skb->xmit_more)
> + virtqueue_kick(sq->vq);
>
> -static void xmit_flush(struct net_device *dev, u16 qnum)
> -{
> - struct virtnet_info *vi = netdev_priv(dev);
> - struct send_queue *sq = &vi->sq[qnum];
> -
> - virtqueue_kick(sq->vq);
> + return NETDEV_TX_OK;
> }
>
> /*
> @@ -1393,7 +1388,6 @@ static const struct net_device_ops virtnet_netdev = {
> .ndo_open = virtnet_open,
> .ndo_stop = virtnet_close,
> .ndo_start_xmit = start_xmit,
> - .ndo_xmit_flush = xmit_flush,
> .ndo_validate_addr = eth_validate_addr,
> .ndo_set_mac_address = virtnet_set_mac_address,
> .ndo_set_rx_mode = virtnet_set_rx_mode,
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 220c509..039b237 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -782,19 +782,6 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
> * (can also return NETDEV_TX_LOCKED iff NETIF_F_LLTX)
> * Required can not be NULL.
> *
> - * void (*ndo_xmit_flush)(struct net_device *dev, u16 queue);
> - * A driver implements this function when it wishes to support
> - * deferred TX queue flushing. The idea is that the expensive
> - * operation to trigger TX queue processing can be done after
> - * N calls to ndo_start_xmit rather than being done every single
> - * time. In this regime ndo_start_xmit will be called one or more
> - * times, and then a final ndo_xmit_flush call will be made to
> - * have the driver tell the device about the new pending TX queue
> - * entries. The kernel keeps track of which queues need flushing
> - * by monitoring skb->queue_mapping of the packets it submits to
> - * ndo_start_xmit. This is the queue value that will be passed
> - * to ndo_xmit_flush.
> - *
> * u16 (*ndo_select_queue)(struct net_device *dev, struct sk_buff *skb,
> * void *accel_priv, select_queue_fallback_t fallback);
> * Called to decide which queue to when device supports multiple
> @@ -1018,7 +1005,6 @@ struct net_device_ops {
> int (*ndo_stop)(struct net_device *dev);
> netdev_tx_t (*ndo_start_xmit) (struct sk_buff *skb,
> struct net_device *dev);
> - void (*ndo_xmit_flush)(struct net_device *dev, u16 queue);
> u16 (*ndo_select_queue)(struct net_device *dev,
> struct sk_buff *skb,
> void *accel_priv,
> @@ -3447,15 +3433,8 @@ int __init dev_proc_init(void);
> static inline netdev_tx_t __netdev_start_xmit(const struct net_device_ops *ops,
> struct sk_buff *skb, struct net_device *dev)
> {
> - netdev_tx_t ret;
> - u16 q;
> -
> - q = skb->queue_mapping;
> - ret = ops->ndo_start_xmit(skb, dev);
> - if (dev_xmit_complete(ret) && ops->ndo_xmit_flush)
> - ops->ndo_xmit_flush(dev, q);
> -
> - return ret;
> + skb->xmit_more = 0;
> + return ops->ndo_start_xmit(skb, dev);
> }
>
> static inline netdev_tx_t netdev_start_xmit(struct sk_buff *skb, struct net_device *dev)
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 18ddf96..9b3802a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -452,6 +452,7 @@ static inline u32 skb_mstamp_us_delta(const struct skb_mstamp *t1,
> * @tc_verd: traffic control verdict
> * @hash: the packet hash
> * @queue_mapping: Queue mapping for multiqueue devices
> + * @xmit_more: More SKBs are pending for this queue
> * @ndisc_nodetype: router type (from link layer)
> * @ooo_okay: allow the mapping of a socket to a queue to be changed
> * @l4_hash: indicate hash is a canonical 4-tuple hash over transport
> @@ -558,6 +559,7 @@ struct sk_buff {
>
> __u16 queue_mapping;
> kmemcheck_bitfield_begin(flags2);
> + __u8 xmit_more:1;
> #ifdef CONFIG_IPV6_NDISC_NODETYPE
> __u8 ndisc_nodetype:2;
> #endif
> --
> 1.7.11.7
>
^ permalink raw reply
* Re: linux-next: build failure after merge of the net tree
From: Stephen Rothwell @ 2014-08-26 3:33 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-next, linux-kernel, vyasevich
In-Reply-To: <20140825.202222.2274651017865689445.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 322 bytes --]
Hi Dave,
On Mon, 25 Aug 2014 20:22:22 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
>
> Sorry about that, I just pushed the following to fix it:
>
> ====================
> [PATCH] mvneta: Add missing if_vlan.h include.
Thanks.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: linux-next: build failure after merge of the net tree
From: David Miller @ 2014-08-26 3:22 UTC (permalink / raw)
To: sfr; +Cc: netdev, linux-next, linux-kernel, vyasevich
In-Reply-To: <20140826120511.5b5dabef@canb.auug.org.au>
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 26 Aug 2014 12:05:11 +1000
> Hi all,
>
> After merging the net tree, today's linux-next build (arm
> multi_v7_defconfig) failed like this:
>
> drivers/net/ethernet/marvell/mvneta.c: In function 'mvneta_skb_tx_csum':
> drivers/net/ethernet/marvell/mvneta.c:1374:3: error: implicit declaration of function 'vlan_get_protocol' [-Werror=implicit-function-declaration]
> __be16 l3_proto = vlan_get_protocol(skb);
> ^
>
> Caused by commit 817dbfa5d1bc ("mvneta: Fix TSO and checksum for
> non-acceleration vlan traffic").
>
> I have reverted that commit for today.
Sorry about that, I just pushed the following to fix it:
====================
[PATCH] mvneta: Add missing if_vlan.h include.
drivers/net/ethernet/marvell/mvneta.c: In function 'mvneta_skb_tx_csum':
drivers/net/ethernet/marvell/mvneta.c:1374:3: error: implicit declaration of function 'vlan_get_protocol' [-Werror=implicit-function-declaration]
__be16 l3_proto = vlan_get_protocol(skb);
^
Reporeted-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
drivers/net/ethernet/marvell/mvneta.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 133f8c6..ade067d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -20,6 +20,7 @@
#include <linux/mbus.h>
#include <linux/module.h>
#include <linux/interrupt.h>
+#include <linux/if_vlan.h>
#include <net/ip.h>
#include <net/ipv6.h>
#include <linux/io.h>
--
1.7.11.7
^ permalink raw reply related
* [PATCH net-next] r8152: replace strncpy with strlcpy
From: Hayes Wang @ 2014-08-26 2:08 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb
In-Reply-To: <1394712342-15778-16-Taiwan-albertk@realtek.com>
Replace the strncpy with strlcpy, and use sizeof to determine the
length.
Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
drivers/net/usb/r8152.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 2470d9c..33dcc97 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -3151,8 +3151,8 @@ static void rtl8152_get_drvinfo(struct net_device *netdev,
{
struct r8152 *tp = netdev_priv(netdev);
- strncpy(info->driver, MODULENAME, ETHTOOL_BUSINFO_LEN);
- strncpy(info->version, DRIVER_VERSION, ETHTOOL_BUSINFO_LEN);
+ strlcpy(info->driver, MODULENAME, sizeof(info->driver));
+ strlcpy(info->version, DRIVER_VERSION, sizeof(info->version));
usb_make_path(tp->udev, info->bus_info, sizeof(info->bus_info));
}
--
1.9.3
^ permalink raw reply related
* linux-next: build failure after merge of the net tree
From: Stephen Rothwell @ 2014-08-26 2:05 UTC (permalink / raw)
To: David Miller, netdev; +Cc: linux-next, linux-kernel, Vlad Yasevich
[-- Attachment #1: Type: text/plain, Size: 600 bytes --]
Hi all,
After merging the net tree, today's linux-next build (arm
multi_v7_defconfig) failed like this:
drivers/net/ethernet/marvell/mvneta.c: In function 'mvneta_skb_tx_csum':
drivers/net/ethernet/marvell/mvneta.c:1374:3: error: implicit declaration of function 'vlan_get_protocol' [-Werror=implicit-function-declaration]
__be16 l3_proto = vlan_get_protocol(skb);
^
Caused by commit 817dbfa5d1bc ("mvneta: Fix TSO and checksum for
non-acceleration vlan traffic").
I have reverted that commit for today.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v6 net-next 1/6] net: filter: add "load 64-bit immediate" eBPF instruction
From: Alexei Starovoitov @ 2014-08-26 2:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Miller, Ingo Molnar, Linus Torvalds, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Brendan Gregg, Namhyung Kim, H. Peter Anvin, Andrew Morton,
Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CALCETrWLvjt_D2B2sYoQtXeU1_9-005BfPcuYDKp75GbPk68dQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, Aug 25, 2014 at 6:54 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Mon, Aug 25, 2014 at 6:53 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
>> On Mon, Aug 25, 2014 at 6:38 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>> On Mon, Aug 25, 2014 at 6:35 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
>>>> On Mon, Aug 25, 2014 at 6:06 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>>>>> From: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
>>>>> Date: Mon, 25 Aug 2014 18:00:53 -0700
>>>>>
>>>>>> add BPF_LD_IMM64 instruction to load 64-bit immediate value into a register.
>>>>>
>>>>> I think you need to rethink this.
>>>>>
>>>>> I understand that you want to be able to compile arbitrary C code into
>>>>> eBPF, but you have to restrict strongly what data the eBPF code can get
>>>>> to.
>>>>
>>>> I believe verifier already does restrict it. I don't see any holes in
>>>> the architecture. I'm probably not explaining it clearly though :(
>>>>
>>>>> Arbitrary pointer loads is asking for trouble.
>>>>
>>>> Of course.
>>>> There is no arbitrary pointer from user space.
>>>> Verifier checks all pointers.
>>>> I guess this commit log description is confusing.
>>>> It says:
>>>> BPF_LD_IMM64(R1, const_imm_map_ptr)
>>>> that's what appears in the program _after_ it goes through verifier.
>>>> User space cannot pass a pointer into the kernel.
>>>
>>> If you don't intend for userspace to load a program that contains this
>>> instruction, then why does it need to be an instruction that the
>>> verifier rewrites? Why not have an instruction "load immediate
>>
>> user space use _pseudo_ bpf_ld_imm64 instruction.
>> _pseudo_ stands for using 'map_fd' as imm instead of pointer.
>>
>>> relocated pointer" that contains a reference to a relocation table and
>>
>> Andy, I guess you missed explanation in:
>> https://lkml.org/lkml/2014/8/13/111
>> "
>> Obviously user space doesn't know what kernel map pointer is associated
>> with process-local map-FD.
>> So it's using pseudo BPF_LD_IMM64 instruction.
>> BPF_LD_IMM64 with src_reg == 0 -> generic move 64-bit immediate into dst_reg
>> BPF_LD_IMM64 with src_reg == BPF_PSEUDO_MAP_FD -> mov map_fd into dst_reg
>> Other values are reserved for now. (They will be used to implement
>> global variables, strings and other constants and per-cpu areas in the future)
>> So the programs look like:
>> BPF_LD_MAP_FD(BPF_REG_1, process_local_map_fd),
>> BPF_CALL(BPF_FUNC_map_lookup_elem),
>> eBPF verifier scans the program for such pseudo instructions, converts
>> process_local_map_fd -> in-kernel map pointer
>> and drops 'pseudo' flag of BPF_LD_IMM64 instruction.
>> "
>
> Will a program that uses BPF_LD_IMM64 w/o the FPG_REG_1 thing be accepted?
If you mean the program like:
BPF_LD_IMM64(BPF_REG_1, 0xdead),
BPF_CALL(BPF_FUNC_map_lookup_elem),
yes, it will be rejected, because type of R1 will not match
map_lookup() argument
constraints.
See check_ld_imm() in verifier.c where it assigns the type during verification.
There are 5 tests in verifier testsuite that test things around bpf_ld_imm64
and 2 tests around _pseudo_ bpf_ld_imm64.
^ permalink raw reply
* Re: [PATCH v6 net-next 1/6] net: filter: add "load 64-bit immediate" eBPF instruction
From: Andy Lutomirski @ 2014-08-26 1:54 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David Miller, Ingo Molnar, Linus Torvalds, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Brendan Gregg, Namhyung Kim, H. Peter Anvin, Andrew Morton,
Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CAMEtUuwz9MEei+tjWx4Fv8cK_zc9TKVbWxQEAE+yWvxRMa793g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, Aug 25, 2014 at 6:53 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> On Mon, Aug 25, 2014 at 6:38 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>> On Mon, Aug 25, 2014 at 6:35 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
>>> On Mon, Aug 25, 2014 at 6:06 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>>>> From: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
>>>> Date: Mon, 25 Aug 2014 18:00:53 -0700
>>>>
>>>>> add BPF_LD_IMM64 instruction to load 64-bit immediate value into a register.
>>>>
>>>> I think you need to rethink this.
>>>>
>>>> I understand that you want to be able to compile arbitrary C code into
>>>> eBPF, but you have to restrict strongly what data the eBPF code can get
>>>> to.
>>>
>>> I believe verifier already does restrict it. I don't see any holes in
>>> the architecture. I'm probably not explaining it clearly though :(
>>>
>>>> Arbitrary pointer loads is asking for trouble.
>>>
>>> Of course.
>>> There is no arbitrary pointer from user space.
>>> Verifier checks all pointers.
>>> I guess this commit log description is confusing.
>>> It says:
>>> BPF_LD_IMM64(R1, const_imm_map_ptr)
>>> that's what appears in the program _after_ it goes through verifier.
>>> User space cannot pass a pointer into the kernel.
>>
>> If you don't intend for userspace to load a program that contains this
>> instruction, then why does it need to be an instruction that the
>> verifier rewrites? Why not have an instruction "load immediate
>
> user space use _pseudo_ bpf_ld_imm64 instruction.
> _pseudo_ stands for using 'map_fd' as imm instead of pointer.
>
>> relocated pointer" that contains a reference to a relocation table and
>
> Andy, I guess you missed explanation in:
> https://lkml.org/lkml/2014/8/13/111
> "
> Obviously user space doesn't know what kernel map pointer is associated
> with process-local map-FD.
> So it's using pseudo BPF_LD_IMM64 instruction.
> BPF_LD_IMM64 with src_reg == 0 -> generic move 64-bit immediate into dst_reg
> BPF_LD_IMM64 with src_reg == BPF_PSEUDO_MAP_FD -> mov map_fd into dst_reg
> Other values are reserved for now. (They will be used to implement
> global variables, strings and other constants and per-cpu areas in the future)
> So the programs look like:
> BPF_LD_MAP_FD(BPF_REG_1, process_local_map_fd),
> BPF_CALL(BPF_FUNC_map_lookup_elem),
> eBPF verifier scans the program for such pseudo instructions, converts
> process_local_map_fd -> in-kernel map pointer
> and drops 'pseudo' flag of BPF_LD_IMM64 instruction.
> "
Will a program that uses BPF_LD_IMM64 w/o the FPG_REG_1 thing be accepted?
--Andy
^ permalink raw reply
* Re: [PATCH v6 net-next 1/6] net: filter: add "load 64-bit immediate" eBPF instruction
From: Alexei Starovoitov @ 2014-08-26 1:53 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Miller, Ingo Molnar, Linus Torvalds, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Brendan Gregg, Namhyung Kim, H. Peter Anvin, Andrew Morton,
Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CALCETrVgSXuRG_ZPXQt55ze7pHOCezU4Ev1KFUww6Yqk4JACgg@mail.gmail.com>
On Mon, Aug 25, 2014 at 6:38 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Aug 25, 2014 at 6:35 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> On Mon, Aug 25, 2014 at 6:06 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Alexei Starovoitov <ast@plumgrid.com>
>>> Date: Mon, 25 Aug 2014 18:00:53 -0700
>>>
>>>> add BPF_LD_IMM64 instruction to load 64-bit immediate value into a register.
>>>
>>> I think you need to rethink this.
>>>
>>> I understand that you want to be able to compile arbitrary C code into
>>> eBPF, but you have to restrict strongly what data the eBPF code can get
>>> to.
>>
>> I believe verifier already does restrict it. I don't see any holes in
>> the architecture. I'm probably not explaining it clearly though :(
>>
>>> Arbitrary pointer loads is asking for trouble.
>>
>> Of course.
>> There is no arbitrary pointer from user space.
>> Verifier checks all pointers.
>> I guess this commit log description is confusing.
>> It says:
>> BPF_LD_IMM64(R1, const_imm_map_ptr)
>> that's what appears in the program _after_ it goes through verifier.
>> User space cannot pass a pointer into the kernel.
>
> If you don't intend for userspace to load a program that contains this
> instruction, then why does it need to be an instruction that the
> verifier rewrites? Why not have an instruction "load immediate
user space use _pseudo_ bpf_ld_imm64 instruction.
_pseudo_ stands for using 'map_fd' as imm instead of pointer.
> relocated pointer" that contains a reference to a relocation table and
Andy, I guess you missed explanation in:
https://lkml.org/lkml/2014/8/13/111
"
Obviously user space doesn't know what kernel map pointer is associated
with process-local map-FD.
So it's using pseudo BPF_LD_IMM64 instruction.
BPF_LD_IMM64 with src_reg == 0 -> generic move 64-bit immediate into dst_reg
BPF_LD_IMM64 with src_reg == BPF_PSEUDO_MAP_FD -> mov map_fd into dst_reg
Other values are reserved for now. (They will be used to implement
global variables, strings and other constants and per-cpu areas in the future)
So the programs look like:
BPF_LD_MAP_FD(BPF_REG_1, process_local_map_fd),
BPF_CALL(BPF_FUNC_map_lookup_elem),
eBPF verifier scans the program for such pseudo instructions, converts
process_local_map_fd -> in-kernel map pointer
and drops 'pseudo' flag of BPF_LD_IMM64 instruction.
"
To rephrase it differently.
BPF_LD_MAP_FD(BPF_REG_1, process_local_map_fd),
is very much what you suggesting by "load immediate relocated pointer"
Right?
> have the JIT do it? That might be easier to understand than having
> the verifier do it, and it'll avoid committing to ABIs before we need
> them.
that part I don't understand.
The patch that handles pseudo_with_map_fd ->
-> normal_with_kernel_pointer conversion is only 147 lines:
https://git.kernel.org/cgit/linux/kernel/git/ast/bpf.git/commit/?id=d82d3daa20465dfdc6b2a0094ad27de9edbb328b
Cannot think of shorter version.
^ permalink raw reply
* Re: [PATCH net-next 1/8] net: Clarification of CHECKSUM_UNNECESSARY
From: Tom Herbert @ 2014-08-26 1:47 UTC (permalink / raw)
To: David Miller; +Cc: Linux Netdev List
In-Reply-To: <CA+mtBx8iv8tWU0mmEQEWxDfp614Hh=cQOpXqxSdhHaGsH6sBDA@mail.gmail.com>
On Mon, Aug 25, 2014 at 6:40 PM, Tom Herbert <therbert@google.com> wrote:
> On Mon, Aug 25, 2014 at 6:13 PM, David Miller <davem@davemloft.net> wrote:
>> From: Tom Herbert <therbert@google.com>
>> Date: Mon, 25 Aug 2014 17:55:56 -0700 (PDT)
>>
>>> @@ -569,16 +590,13 @@ struct sk_buff {
>>> __u8 wifi_acked:1;
>>> __u8 no_fcs:1;
>>> __u8 head_frag:1;
>>> - /* Encapsulation protocol and NIC drivers should use
>>> - * this flag to indicate to each other if the skb contains
>>> - * encapsulated packet or not and maybe use the inner packet
>>> - * headers if needed
>>> - */
>>> + /* Indicates the the inner headers are valid in the skbuff. */
>>> __u8 encapsulation:1;
>>> __u8 encap_hdr_csum:1;
>>> __u8 csum_valid:1;
>>> __u8 csum_complete_sw:1;
>>> - /* 2/4 bit hole (depending on ndisc_nodetype presence) */
>>> + __u8 csum_level:2;
>>> + /* 0/2 bit hole (depending on ndisc_nodetype presence) */
>>> kmemcheck_bitfield_end(flags2);
>>
>> Crap, with xmit_more, this actually bleeds us over into a new __u8.
>
> We'll undoubtably want to add more flags beyond that (they're quite
> useful). I'll try to find some more space in the existing fields so we
> don't increase skbuf size.
Actually, from inner_protocol through mac_header there are seven 16
bit fields, could put another 16 bit flags there.
^ permalink raw reply
* Re: [PATCH v6 net-next 4/6] bpf: enable bpf syscall on x64 and i386
From: Alexei Starovoitov @ 2014-08-26 1:43 UTC (permalink / raw)
To: David Miller
Cc: Ingo Molnar, Linus Torvalds, Andy Lutomirski, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Brendan Gregg, Namhyung Kim, H. Peter Anvin, Andrew Morton,
Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <20140825.180718.137768107010295086.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Mon, Aug 25, 2014 at 6:07 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
> Date: Mon, 25 Aug 2014 18:00:56 -0700
>
>> -
>> +asmlinkage long sys_bpf(int cmd, unsigned long arg2, unsigned long arg3,
>> + unsigned long arg4, unsigned long arg5);
>
> Please do not add interfaces with opaque types as arguments.
>
> It is impossible for the compiler to type check the args at
> compile time when userspace tries to use this stuff.
I share this concern. I went with single BPF syscall, because
alternative is 6 syscalls for every command and more
syscalls in the future when we'd need to add another command.
I think type casting is much lesser evil.
We already have similar muxing syscalls.
It feels to me that single mux/demux syscall is easier to support,
document, add new commands. Type casting, yeah, not pretty.
Most users will be using wrappers similar to those I've defined in libbpf.h
^ permalink raw reply
* Re: [PATCH net-next 1/8] net: Clarification of CHECKSUM_UNNECESSARY
From: Tom Herbert @ 2014-08-26 1:40 UTC (permalink / raw)
To: David Miller; +Cc: Linux Netdev List
In-Reply-To: <20140825.181341.304248903530349010.davem@davemloft.net>
On Mon, Aug 25, 2014 at 6:13 PM, David Miller <davem@davemloft.net> wrote:
> From: Tom Herbert <therbert@google.com>
> Date: Mon, 25 Aug 2014 17:55:56 -0700 (PDT)
>
>> @@ -569,16 +590,13 @@ struct sk_buff {
>> __u8 wifi_acked:1;
>> __u8 no_fcs:1;
>> __u8 head_frag:1;
>> - /* Encapsulation protocol and NIC drivers should use
>> - * this flag to indicate to each other if the skb contains
>> - * encapsulated packet or not and maybe use the inner packet
>> - * headers if needed
>> - */
>> + /* Indicates the the inner headers are valid in the skbuff. */
>> __u8 encapsulation:1;
>> __u8 encap_hdr_csum:1;
>> __u8 csum_valid:1;
>> __u8 csum_complete_sw:1;
>> - /* 2/4 bit hole (depending on ndisc_nodetype presence) */
>> + __u8 csum_level:2;
>> + /* 0/2 bit hole (depending on ndisc_nodetype presence) */
>> kmemcheck_bitfield_end(flags2);
>
> Crap, with xmit_more, this actually bleeds us over into a new __u8.
We'll undoubtably want to add more flags beyond that (they're quite
useful). I'll try to find some more space in the existing fields so we
don't increase skbuf size.
^ permalink raw reply
* Re: [PATCH v6 net-next 1/6] net: filter: add "load 64-bit immediate" eBPF instruction
From: Andy Lutomirski @ 2014-08-26 1:38 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David Miller, Ingo Molnar, Linus Torvalds, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Brendan Gregg, Namhyung Kim, H. Peter Anvin, Andrew Morton,
Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <CAMEtUuxdQpkX8t1_szde=Q1ALcp5t7rRyK+zEDafj27_J2LzVg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, Aug 25, 2014 at 6:35 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> On Mon, Aug 25, 2014 at 6:06 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>> From: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
>> Date: Mon, 25 Aug 2014 18:00:53 -0700
>>
>>> add BPF_LD_IMM64 instruction to load 64-bit immediate value into a register.
>>
>> I think you need to rethink this.
>>
>> I understand that you want to be able to compile arbitrary C code into
>> eBPF, but you have to restrict strongly what data the eBPF code can get
>> to.
>
> I believe verifier already does restrict it. I don't see any holes in
> the architecture. I'm probably not explaining it clearly though :(
>
>> Arbitrary pointer loads is asking for trouble.
>
> Of course.
> There is no arbitrary pointer from user space.
> Verifier checks all pointers.
> I guess this commit log description is confusing.
> It says:
> BPF_LD_IMM64(R1, const_imm_map_ptr)
> that's what appears in the program _after_ it goes through verifier.
> User space cannot pass a pointer into the kernel.
If you don't intend for userspace to load a program that contains this
instruction, then why does it need to be an instruction that the
verifier rewrites? Why not have an instruction "load immediate
relocated pointer" that contains a reference to a relocation table and
have the JIT do it? That might be easier to understand than having
the verifier do it, and it'll avoid committing to ABIs before we need
them.
--Andy
^ permalink raw reply
* Re: [PATCH v6 net-next 1/6] net: filter: add "load 64-bit immediate" eBPF instruction
From: Alexei Starovoitov @ 2014-08-26 1:35 UTC (permalink / raw)
To: David Miller
Cc: Ingo Molnar, Linus Torvalds, Andy Lutomirski, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Brendan Gregg, Namhyung Kim, H. Peter Anvin, Andrew Morton,
Kees Cook, Linux API, Network Development, LKML
In-Reply-To: <20140825.180621.1003932711320755950.davem@davemloft.net>
On Mon, Aug 25, 2014 at 6:06 PM, David Miller <davem@davemloft.net> wrote:
> From: Alexei Starovoitov <ast@plumgrid.com>
> Date: Mon, 25 Aug 2014 18:00:53 -0700
>
>> add BPF_LD_IMM64 instruction to load 64-bit immediate value into a register.
>
> I think you need to rethink this.
>
> I understand that you want to be able to compile arbitrary C code into
> eBPF, but you have to restrict strongly what data the eBPF code can get
> to.
I believe verifier already does restrict it. I don't see any holes in
the architecture. I'm probably not explaining it clearly though :(
> Arbitrary pointer loads is asking for trouble.
Of course.
There is no arbitrary pointer from user space.
Verifier checks all pointers.
I guess this commit log description is confusing.
It says:
BPF_LD_IMM64(R1, const_imm_map_ptr)
that's what appears in the program _after_ it goes through verifier.
User space cannot pass a pointer into the kernel.
> Instead I would rather you look into a model like what the quake
> engine uses for it's VM.
>
> Namely, the program can do loads and stores from/to a data section,
> but all of them are validated to be in the range of the VM program's
> image.
That's exactly what eBPF does as well. load and stores can only
be from three 'sections': pointer to stack, pointer to context, pointer
to map value.
All verifier logic including these pointer checks is described in
extensive verifier documentation. Please take a look at it.
Andy said that it's good doc :)
Programs like:
BPF_LD_IMM64(R1, some_64bit_constant)
*(u8*)(R1+ off) = imm;
will be rejected by verifier.
I even have a test case for that later in the patches.
> eBPF programs should only be able to access things using either:
>
> 1) Well defined entry/exit points for control transfer
>
> 2) Load/Store within a private limited data segment range used
> only by the eBPF program
>
> I don't want the eBPF program to be able to get "out of it's box"
> in any way shape or form.
Agree 100%.
And I believe I already achieved that with verifier.
> And besides, you're only making this thing as an optimization right?
not quite. This instruction is needed to get rid of IDR for maps,
speeded up lookup, made verifier much simpler and most important
it cleaned up program<->maps interaction.
I've described it better in V4 set:
https://lkml.org/lkml/2014/8/13/111
Should I resend this patch together with all of the verifier patches?
I included it here as #1, since patch #2 moves all eBPF macros
into uapi/linux/bpf.h and this instruction by itself is completely
harmless. It's verifier that needs to do proper checking.
^ permalink raw reply
* [PATCH net-next 8/8] qlcnic: Set skb->csum_level for encapsulated checksum
From: Tom Herbert @ 2014-08-26 0:56 UTC (permalink / raw)
To: davem, netdev
Set skb->csum_level instead of skb->encapsulation when indicating
CHECKSUM_UNNECESSARY for an encapsulated checksum.
Signed-off-by: Tom Herbert <therbert@google.com>
---
drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
index e45bf09..18e5de7 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
@@ -1753,7 +1753,7 @@ qlcnic_83xx_process_rcv(struct qlcnic_adapter *adapter,
if (qlcnic_encap_length(sts_data[1]) &&
skb->ip_summed == CHECKSUM_UNNECESSARY) {
- skb->encapsulation = 1;
+ skb->csum_level = 1;
adapter->stats.encap_rx_csummed++;
}
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* Re: linux/if_bridge.h no longer self contained (again)
From: David Miller @ 2014-08-26 1:18 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: f.fainelli, netdev, gregory.0xf0, carlos, eblake, galak
In-Reply-To: <CAM_iQpV6EvvoYE17oi2XN0cbBkssQv0j=VXVR1-uCtMoJrSHwQ@mail.gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 30 Jul 2014 16:20:07 -0700
> On Wed, Jul 30, 2014 at 3:05 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> What would be the recommended ways to move forward?
>>
>> Should we keep a local change that includes netinet/in6.h and just
>> hope it does not cause any re-definitions?
>>
>
> I think we should just #include <linux/in6.h> since the redefinition error
> was already fixed.
Someone please submit a patch to do so, and please include all of the context
such as the commits references in Florian's initial email of this thread.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next 1/8] net: Clarification of CHECKSUM_UNNECESSARY
From: David Miller @ 2014-08-26 1:13 UTC (permalink / raw)
To: therbert; +Cc: netdev
In-Reply-To: <alpine.DEB.2.02.1408251738110.5541@tomh.mtv.corp.google.com>
From: Tom Herbert <therbert@google.com>
Date: Mon, 25 Aug 2014 17:55:56 -0700 (PDT)
> @@ -569,16 +590,13 @@ struct sk_buff {
> __u8 wifi_acked:1;
> __u8 no_fcs:1;
> __u8 head_frag:1;
> - /* Encapsulation protocol and NIC drivers should use
> - * this flag to indicate to each other if the skb contains
> - * encapsulated packet or not and maybe use the inner packet
> - * headers if needed
> - */
> + /* Indicates the the inner headers are valid in the skbuff. */
> __u8 encapsulation:1;
> __u8 encap_hdr_csum:1;
> __u8 csum_valid:1;
> __u8 csum_complete_sw:1;
> - /* 2/4 bit hole (depending on ndisc_nodetype presence) */
> + __u8 csum_level:2;
> + /* 0/2 bit hole (depending on ndisc_nodetype presence) */
> kmemcheck_bitfield_end(flags2);
Crap, with xmit_more, this actually bleeds us over into a new __u8.
^ permalink raw reply
* Re: [PATCH v6 net-next 4/6] bpf: enable bpf syscall on x64 and i386
From: David Miller @ 2014-08-26 1:07 UTC (permalink / raw)
To: ast-uqk4Ao+rVK5Wk0Htik3J/w
Cc: mingo-DgEjT+Ai2ygdnm+yROfE0A,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
luto-kltTT9wpgjJwATOyAt5JVQ, rostedt-nx8X9YLhiw1AfugRpC6u6w,
dborkman-H+wXaHxf7aLQT0dZR+AlfA, chema-hpIqsD4AKlfQT0dZR+AlfA,
edumazet-hpIqsD4AKlfQT0dZR+AlfA,
a.p.zijlstra-/NLkJaSkS4VmR6Xm/wNWPw,
brendan.d.gregg-Re5JQEeQqe8AvxtiuMwx3w,
namhyung-DgEjT+Ai2ygdnm+yROfE0A, hpa-YMNOUZJC4hwAvxtiuMwx3w,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
keescook-F7+t8E8rja9g9hUCZPvPmw, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1409014858-1410-5-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
From: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
Date: Mon, 25 Aug 2014 18:00:56 -0700
> -
> +asmlinkage long sys_bpf(int cmd, unsigned long arg2, unsigned long arg3,
> + unsigned long arg4, unsigned long arg5);
Please do not add interfaces with opaque types as arguments.
It is impossible for the compiler to type check the args at
compile time when userspace tries to use this stuff.
^ permalink raw reply
* Re: [PATCH v6 net-next 1/6] net: filter: add "load 64-bit immediate" eBPF instruction
From: David Miller @ 2014-08-26 1:06 UTC (permalink / raw)
To: ast
Cc: mingo, torvalds, luto, rostedt, dborkman, chema, edumazet,
a.p.zijlstra, brendan.d.gregg, namhyung, hpa, akpm, keescook,
linux-api, netdev, linux-kernel
In-Reply-To: <1409014858-1410-2-git-send-email-ast@plumgrid.com>
From: Alexei Starovoitov <ast@plumgrid.com>
Date: Mon, 25 Aug 2014 18:00:53 -0700
> add BPF_LD_IMM64 instruction to load 64-bit immediate value into a register.
I think you need to rethink this.
I understand that you want to be able to compile arbitrary C code into
eBPF, but you have to restrict strongly what data the eBPF code can get
to.
Arbitrary pointer loads is asking for trouble.
Instead I would rather you look into a model like what the quake
engine uses for it's VM.
Namely, the program can do loads and stores from/to a data section,
but all of them are validated to be in the range of the VM program's
image.
eBPF programs should only be able to access things using either:
1) Well defined entry/exit points for control transfer
2) Load/Store within a private limited data segment range used
only by the eBPF program
I don't want the eBPF program to be able to get "out of it's box"
in any way shape or form.
And besides, you're only making this thing as an optimization right?
^ permalink raw reply
* [PATCH net-next 5/8] i40e: Set skb->csum_level for encapsulated checksum
From: Tom Herbert @ 2014-08-26 0:56 UTC (permalink / raw)
To: davem, netdev
Set skb->csum_level instead of skb->encapsulation when indicating
CHECKSUM_UNNECESSARY for an encapsulated checksum.
Signed-off-by: Tom Herbert <therbert@google.com>
---
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index a51aa37..4dec093 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -1213,7 +1213,6 @@ static inline void i40e_rx_checksum(struct i40e_vsi *vsi,
ipv6_tunnel = (rx_ptype > I40E_RX_PTYPE_GRENAT6_MAC_PAY3) &&
(rx_ptype < I40E_RX_PTYPE_GRENAT6_MACVLAN_IPV6_ICMP_PAY4);
- skb->encapsulation = ipv4_tunnel || ipv6_tunnel;
skb->ip_summed = CHECKSUM_NONE;
/* Rx csum enabled and ip headers found? */
@@ -1287,6 +1286,7 @@ static inline void i40e_rx_checksum(struct i40e_vsi *vsi,
}
skb->ip_summed = CHECKSUM_UNNECESSARY;
+ skb->csum_level = ipv4_tunnel || ipv6_tunnel;
return;
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH net-next 3/8] sctp: Change sctp to implement csum_levels
From: Tom Herbert @ 2014-08-26 0:56 UTC (permalink / raw)
To: davem, netdev
CHECKSUM_UNNECESSARY may be applied to the SCTP CRC so we need to
appropriate account for this by decrementing csum_level. This is
done by calling __skb_dec_checksum_unnecessary.
Signed-off-by: Tom Herbert <therbert@google.com>
---
net/sctp/input.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/sctp/input.c b/net/sctp/input.c
index c1b9912..c86b86c 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -133,9 +133,13 @@ int sctp_rcv(struct sk_buff *skb)
__skb_pull(skb, skb_transport_offset(skb));
if (skb->len < sizeof(struct sctphdr))
goto discard_it;
- if (!sctp_checksum_disable && !skb_csum_unnecessary(skb) &&
- sctp_rcv_checksum(net, skb) < 0)
+
+ skb->csum_valid = 0; /* Previous value not applicable */
+ if (skb_csum_unnecessary(skb))
+ __skb_dec_checksum_unnecessary(skb);
+ else if (!sctp_checksum_disable && sctp_rcv_checksum(net, skb) < 0)
goto discard_it;
+ skb->csum_valid = 1;
skb_pull(skb, sizeof(struct sctphdr));
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* [PATCH v6 net-next 6/6] bpf: add hashtable type of BPF maps
From: Alexei Starovoitov @ 2014-08-26 1:00 UTC (permalink / raw)
To: David S. Miller
Cc: Ingo Molnar, Linus Torvalds, Andy Lutomirski, Steven Rostedt,
Daniel Borkmann, Chema Gonzalez, Eric Dumazet, Peter Zijlstra,
Brendan Gregg, Namhyung Kim, H. Peter Anvin, Andrew Morton,
Kees Cook, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1409014858-1410-1-git-send-email-ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
add new map type BPF_MAP_TYPE_HASH and its implementation
- key/value are opaque range of bytes
- user space provides 3 configuration attributes via BPF syscall:
key_size, value_size, max_entries
- if value_size == 0, the map is used as a set
- map_update_elem() must fail to insert new element when max_entries
limit is reached
- map takes care of allocating/freeing key/value pairs
- update/lookup/delete methods may be called from eBPF program attached
to kprobes, so use spin_lock_irqsave() mechanism for concurrent updates
- optimized for speed of lookup() which can be called multiple times from
eBPF program which itself is triggered by high volume of events
- in the future JIT compiler may recognize lookup() call and optimize it
further, since key_size is constant for life of eBPF program
Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
---
Note, lib/rhashtable.c couldn't be reused here, since it's not managing the
objects, shrink/expand is not supported out of irq context, overhead of
generic key/head_offsets and hashfn is too high.
In the future if/when rhashtable gains shrink/expand as a separate thread,
we may add another BPF_MAP_TYPE_ for it, but security implications of
dynamically resizeable maps would need to be analyzed.
include/uapi/linux/bpf.h | 1 +
kernel/bpf/Makefile | 2 +-
kernel/bpf/hashtab.c | 373 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 375 insertions(+), 1 deletion(-)
create mode 100644 kernel/bpf/hashtab.c
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f68edb2681f8..8069ab7b64cf 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -365,6 +365,7 @@ enum bpf_map_attributes {
enum bpf_map_type {
BPF_MAP_TYPE_UNSPEC,
+ BPF_MAP_TYPE_HASH,
};
#endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index e9f7334ed07a..558e12712ebc 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -1 +1 @@
-obj-y := core.o syscall.o
+obj-y := core.o syscall.o hashtab.o
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
new file mode 100644
index 000000000000..3178a6746b92
--- /dev/null
+++ b/kernel/bpf/hashtab.c
@@ -0,0 +1,373 @@
+/* Copyright (c) 2011-2014 PLUMgrid, http://plumgrid.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+#include <linux/bpf.h>
+#include <net/netlink.h>
+#include <linux/jhash.h>
+
+struct bpf_htab {
+ struct bpf_map map;
+ struct hlist_head *buckets;
+ struct kmem_cache *elem_cache;
+ spinlock_t lock;
+ u32 count; /* number of elements in this hashtable */
+ u32 n_buckets; /* number of hash buckets */
+ u32 elem_size; /* size of each element in bytes */
+};
+
+/* each htab element is struct htab_elem + key + value */
+struct htab_elem {
+ struct hlist_node hash_node;
+ struct rcu_head rcu;
+ struct bpf_htab *htab;
+ u32 hash;
+ u32 pad;
+ char key[0];
+};
+
+#define BPF_MAP_MAX_KEY_SIZE 256
+static struct bpf_map *htab_map_alloc(struct nlattr *attr[BPF_MAP_ATTR_MAX + 1])
+{
+ struct bpf_htab *htab;
+ int err, i;
+
+ htab = kzalloc(sizeof(*htab), GFP_USER);
+ if (!htab)
+ return ERR_PTR(-ENOMEM);
+
+ /* look for mandatory map attributes */
+ err = -EINVAL;
+ if (!attr[BPF_MAP_KEY_SIZE])
+ goto free_htab;
+ htab->map.key_size = nla_get_u32(attr[BPF_MAP_KEY_SIZE]);
+
+ if (!attr[BPF_MAP_VALUE_SIZE])
+ goto free_htab;
+ htab->map.value_size = nla_get_u32(attr[BPF_MAP_VALUE_SIZE]);
+
+ if (!attr[BPF_MAP_MAX_ENTRIES])
+ goto free_htab;
+ htab->map.max_entries = nla_get_u32(attr[BPF_MAP_MAX_ENTRIES]);
+
+ /* check sanity of attributes.
+ * value_size == 0 is allowed, in this case map is used as a set
+ */
+ if (htab->map.max_entries == 0 || htab->map.key_size == 0)
+ goto free_htab;
+
+ /* hash table size must be power of 2 */
+ htab->n_buckets = roundup_pow_of_two(htab->map.max_entries);
+
+ err = -E2BIG;
+ if (htab->map.key_size > BPF_MAP_MAX_KEY_SIZE)
+ goto free_htab;
+
+ err = -ENOMEM;
+ htab->buckets = kmalloc_array(htab->n_buckets,
+ sizeof(struct hlist_head), GFP_USER);
+
+ if (!htab->buckets)
+ goto free_htab;
+
+ for (i = 0; i < htab->n_buckets; i++)
+ INIT_HLIST_HEAD(&htab->buckets[i]);
+
+ spin_lock_init(&htab->lock);
+ htab->count = 0;
+
+ htab->elem_size = sizeof(struct htab_elem) +
+ round_up(htab->map.key_size, 8) +
+ htab->map.value_size;
+
+ htab->elem_cache = kmem_cache_create("bpf_htab", htab->elem_size, 0, 0,
+ NULL);
+ if (!htab->elem_cache)
+ goto free_buckets;
+
+ return &htab->map;
+
+free_buckets:
+ kfree(htab->buckets);
+free_htab:
+ kfree(htab);
+ return ERR_PTR(err);
+}
+
+static inline u32 htab_map_hash(const void *key, u32 key_len)
+{
+ return jhash(key, key_len, 0);
+}
+
+static inline struct hlist_head *select_bucket(struct bpf_htab *htab, u32 hash)
+{
+ return &htab->buckets[hash & (htab->n_buckets - 1)];
+}
+
+static struct htab_elem *lookup_elem_raw(struct hlist_head *head, u32 hash,
+ void *key, u32 key_size)
+{
+ struct htab_elem *l;
+
+ hlist_for_each_entry_rcu(l, head, hash_node) {
+ if (l->hash == hash && !memcmp(&l->key, key, key_size))
+ return l;
+ }
+ return NULL;
+}
+
+/* Must be called with rcu_read_lock. */
+static void *htab_map_lookup_elem(struct bpf_map *map, void *key)
+{
+ struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
+ struct hlist_head *head;
+ struct htab_elem *l;
+ u32 hash, key_size;
+
+ WARN_ON_ONCE(!rcu_read_lock_held());
+
+ key_size = map->key_size;
+
+ hash = htab_map_hash(key, key_size);
+
+ head = select_bucket(htab, hash);
+
+ l = lookup_elem_raw(head, hash, key, key_size);
+
+ if (l)
+ return l->key + round_up(map->key_size, 8);
+
+ return NULL;
+}
+
+/* Must be called with rcu_read_lock. */
+static int htab_map_get_next_key(struct bpf_map *map, void *key, void *next_key)
+{
+ struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
+ struct hlist_head *head;
+ struct htab_elem *l, *next_l;
+ u32 hash, key_size;
+ int i;
+
+ WARN_ON_ONCE(!rcu_read_lock_held());
+
+ key_size = map->key_size;
+
+ hash = htab_map_hash(key, key_size);
+
+ head = select_bucket(htab, hash);
+
+ /* lookup the key */
+ l = lookup_elem_raw(head, hash, key, key_size);
+
+ if (!l) {
+ i = 0;
+ goto find_first_elem;
+ }
+
+ /* key was found, get next key in the same bucket */
+ next_l = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(&l->hash_node)),
+ struct htab_elem, hash_node);
+
+ if (next_l) {
+ /* if next elem in this hash list is non-zero, just return it */
+ memcpy(next_key, next_l->key, key_size);
+ return 0;
+ }
+
+ /* no more elements in this hash list, go to the next bucket */
+ i = hash & (htab->n_buckets - 1);
+ i++;
+
+find_first_elem:
+ /* iterate over buckets */
+ for (; i < htab->n_buckets; i++) {
+ head = select_bucket(htab, i);
+
+ /* pick first element in the bucket */
+ next_l = hlist_entry_safe(rcu_dereference_raw(hlist_first_rcu(head)),
+ struct htab_elem, hash_node);
+ if (next_l) {
+ /* if it's not empty, just return it */
+ memcpy(next_key, next_l->key, key_size);
+ return 0;
+ }
+ }
+
+ /* itereated over all buckets and all elements */
+ return -ENOENT;
+}
+
+static struct htab_elem *htab_alloc_elem(struct bpf_htab *htab)
+{
+ void *l;
+
+ l = kmem_cache_alloc(htab->elem_cache, GFP_ATOMIC);
+ if (!l)
+ return ERR_PTR(-ENOMEM);
+ return l;
+}
+
+static void free_htab_elem_rcu(struct rcu_head *rcu)
+{
+ struct htab_elem *l = container_of(rcu, struct htab_elem, rcu);
+
+ kmem_cache_free(l->htab->elem_cache, l);
+}
+
+static void release_htab_elem(struct bpf_htab *htab, struct htab_elem *l)
+{
+ l->htab = htab;
+ call_rcu(&l->rcu, free_htab_elem_rcu);
+}
+
+/* Must be called with rcu_read_lock. */
+static int htab_map_update_elem(struct bpf_map *map, void *key, void *value)
+{
+ struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
+ struct htab_elem *l_new, *l_old;
+ struct hlist_head *head;
+ unsigned long flags;
+ u32 key_size;
+
+ WARN_ON_ONCE(!rcu_read_lock_held());
+
+ l_new = htab_alloc_elem(htab);
+ if (IS_ERR(l_new))
+ return -ENOMEM;
+
+ key_size = map->key_size;
+
+ memcpy(l_new->key, key, key_size);
+ memcpy(l_new->key + round_up(key_size, 8), value, map->value_size);
+
+ l_new->hash = htab_map_hash(l_new->key, key_size);
+
+ /* bpf_map_update_elem() can be called in_irq() as well, so
+ * spin_lock() or spin_lock_bh() cannot be used
+ */
+ spin_lock_irqsave(&htab->lock, flags);
+
+ head = select_bucket(htab, l_new->hash);
+
+ l_old = lookup_elem_raw(head, l_new->hash, key, key_size);
+
+ if (!l_old && unlikely(htab->count >= map->max_entries)) {
+ /* if elem with this 'key' doesn't exist and we've reached
+ * max_entries limit, fail insertion of new elem
+ */
+ spin_unlock_irqrestore(&htab->lock, flags);
+ kmem_cache_free(htab->elem_cache, l_new);
+ return -EFBIG;
+ }
+
+ /* add new element to the head of the list, so that concurrent
+ * search will find it before old elem
+ */
+ hlist_add_head_rcu(&l_new->hash_node, head);
+ if (l_old) {
+ hlist_del_rcu(&l_old->hash_node);
+ release_htab_elem(htab, l_old);
+ } else {
+ htab->count++;
+ }
+ spin_unlock_irqrestore(&htab->lock, flags);
+
+ return 0;
+}
+
+/* Must be called with rcu_read_lock. */
+static int htab_map_delete_elem(struct bpf_map *map, void *key)
+{
+ struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
+ struct hlist_head *head;
+ struct htab_elem *l;
+ unsigned long flags;
+ u32 hash, key_size;
+ int ret = -ESRCH;
+
+ WARN_ON_ONCE(!rcu_read_lock_held());
+
+ key_size = map->key_size;
+
+ hash = htab_map_hash(key, key_size);
+
+ spin_lock_irqsave(&htab->lock, flags);
+
+ head = select_bucket(htab, hash);
+
+ l = lookup_elem_raw(head, hash, key, key_size);
+
+ if (l) {
+ hlist_del_rcu(&l->hash_node);
+ htab->count--;
+ release_htab_elem(htab, l);
+ ret = 0;
+ }
+
+ spin_unlock_irqrestore(&htab->lock, flags);
+ return ret;
+}
+
+static void delete_all_elements(struct bpf_htab *htab)
+{
+ int i;
+
+ for (i = 0; i < htab->n_buckets; i++) {
+ struct hlist_head *head = select_bucket(htab, i);
+ struct hlist_node *n;
+ struct htab_elem *l;
+
+ hlist_for_each_entry_safe(l, n, head, hash_node) {
+ hlist_del_rcu(&l->hash_node);
+ htab->count--;
+ kmem_cache_free(htab->elem_cache, l);
+ }
+ }
+}
+
+/* called when map->refcnt goes to zero */
+static void htab_map_free(struct bpf_map *map)
+{
+ struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
+
+ /* wait for all outstanding updates to complete */
+ synchronize_rcu();
+
+ /* kmem_cache_free all htab elements */
+ delete_all_elements(htab);
+
+ /* and destroy cache, which might sleep */
+ kmem_cache_destroy(htab->elem_cache);
+
+ kfree(htab->buckets);
+ kfree(htab);
+}
+
+static struct bpf_map_ops htab_ops = {
+ .map_alloc = htab_map_alloc,
+ .map_free = htab_map_free,
+ .map_get_next_key = htab_map_get_next_key,
+ .map_lookup_elem = htab_map_lookup_elem,
+ .map_update_elem = htab_map_update_elem,
+ .map_delete_elem = htab_map_delete_elem,
+};
+
+static struct bpf_map_type_list tl = {
+ .ops = &htab_ops,
+ .type = BPF_MAP_TYPE_HASH,
+};
+
+static int __init register_htab_map(void)
+{
+ bpf_register_map_type(&tl);
+ return 0;
+}
+late_initcall(register_htab_map);
--
1.7.9.5
^ 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