* Re: Kernel sends ICMP unreachable for GRE packets even if there is a listening socket
From: Steinar H. Gunderson @ 2015-02-10 16:16 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1423584596.28434.30.camel@edumazet-glaptop2.roam.corp.google.com>
On Tue, Feb 10, 2015 at 08:09:56AM -0800, Eric Dumazet wrote:
>> Can this be suppressed when I have such a socket giong? ip6tables -A OUTPUT can
>> stop them, but it seems very hacky and kludgy.
> Thats because you loaded ip6_gre maybe ?
I have loaded ip6_gre, yes. But I might need the kernel variant for other
tunnels, so unloading it is a suboptimal workaround.
/* Steinar */
--
Homepage: http://www.sesse.net/
^ permalink raw reply
* Re: Kernel sends ICMP unreachable for GRE packets even if there is a listening socket
From: Eric Dumazet @ 2015-02-10 16:18 UTC (permalink / raw)
To: Steinar H. Gunderson; +Cc: netdev
In-Reply-To: <1423584596.28434.30.camel@edumazet-glaptop2.roam.corp.google.com>
On Tue, 2015-02-10 at 08:09 -0800, Eric Dumazet wrote:
> On Tue, 2015-02-10 at 16:24 +0100, Steinar H. Gunderson wrote:
> > [Resent from Bugzilla]
> >
> > Hi,
> >
> > I have a userspace GRE listener, which opens a raw socket (error handling removed):
> >
> > int gresock = socket(AF_INET6, SOCK_RAW, IPPROTO_GRE);
> > bind(gresock, (sockaddr *)&my_addr, sizeof(my_addr));
> >
> > and then select()s and recvfrom()s on it, as well as sendto().
> >
> > This works great. I can send and receive GRE packets. However, Linux _also_
> > sends ICMPv6 unreachables when the other end sends to me. I assume this is
> > because I don't have a corresponding kernel GRE tunnel interface. (I don't use
> > the kernel GRE because I want, among others, stronger reordering functionality
> > and possibly error correction, which is not in any standard.)
> >
> > Can this be suppressed when I have such a socket giong? ip6tables -A OUTPUT can
> > stop them, but it seems very hacky and kludgy.
>
> Thats because you loaded ip6_gre maybe ?
Not clear why we even bother sending ICMP in ipv6, while we do nothing
in ipv4 similar case
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index bc28b7d42a6dab05abee80d4fa84c102d92ca91f..8ff364b93bc2909253a6a0ef7268fc2dfb92c77c 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -572,7 +572,6 @@ static int ip6gre_rcv(struct sk_buff *skb)
return 0;
}
- icmpv6_send(skb, ICMPV6_DEST_UNREACH, ICMPV6_PORT_UNREACH, 0);
drop:
kfree_skb(skb);
^ permalink raw reply related
* Re: [PATCH] ipv6: Partial checksum only UDP packets
From: Sabrina Dubroca @ 2015-02-10 16:23 UTC (permalink / raw)
To: Vladislav Yasevich; +Cc: netdev, Vladislav Yasevich
In-Reply-To: <1423583752-31932-1-git-send-email-vyasevic@redhat.com>
2015-02-10, 10:55:52 -0500, Vladislav Yasevich wrote:
> ip6_append_data is used by other protocols and some of them can't
> be partially checksummed. Only partially checksum UDP protocol.
>
> Fixes: 32dce968dd987a (ipv6: Allow for partial checksums on non-ufo packets)
> Reported-by: Sabrian Dubroca <sd@queasysnail.net>
not a big deal, but since there's a small problem with this version
(see below):
s/Sabrian/Sabrina/
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
> Hi Sabrina
>
> Can you try this patch. Thanks.
>
> net/ipv6/ip6_output.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index d33df4c..8f2d558 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1273,7 +1273,7 @@ emsgsize:
> /* If this is the first and only packet and device
> * supports checksum offloading, let's use it.
> */
> - if (!skb &&
> + if (!skb && sk->protocol == IPPROTO_UDP &&
^^^
should be sk->sk_protocol
And you can add:
Tested-by: Sabrina Dubroca <sd@queasysnail.net>
Thanks Vlad!
> length + fragheaderlen < mtu &&
> rt->dst.dev->features & NETIF_F_V6_CSUM &&
> !exthdrlen)
> --
> 1.9.3
--
Sabrina
^ permalink raw reply
* Re: [GIT] Networking
From: Linus Torvalds @ 2015-02-10 16:33 UTC (permalink / raw)
To: David Miller
Cc: Al Viro, Andrew Morton, Network Development,
Linux Kernel Mailing List
In-Reply-To: <20150209.205209.1524645061817000265.davem@davemloft.net>
On Mon, Feb 9, 2015 at 8:52 PM, David Miller <davem@davemloft.net> wrote:
>
> I think the handling of the 'used' local variable for function
> skcipher_recvmsg() in that commit is suspect.
Yeah, skcipher_wait_for_data() waits for it to be non-zero, but it's
not reloaded.
Will try your suggestion,
Linus
^ permalink raw reply
* [PATCH v2 net-next] ipv6: Partial checksum only UDP packets
From: Vladislav Yasevich @ 2015-02-10 16:37 UTC (permalink / raw)
To: netdev; +Cc: sd, Vladislav Yasevich
ip6_append_data is used by other protocols and some of them can't
be partially checksummed. Only partially checksum UDP protocol.
Fixes: 32dce968dd987a (ipv6: Allow for partial checksums on non-ufo packets)
Reported-by: Sabrina Dubroca <sd@queasysnail.net>
Tested-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
v2: added tested-by tag and fixed compile issue with prior patch.
net/ipv6/ip6_output.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index d33df4c..7deebf1 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1273,7 +1273,7 @@ emsgsize:
/* If this is the first and only packet and device
* supports checksum offloading, let's use it.
*/
- if (!skb &&
+ if (!skb && sk->sk_protocol == IPPROTO_UDP &&
length + fragheaderlen < mtu &&
rt->dst.dev->features & NETIF_F_V6_CSUM &&
!exthdrlen)
--
1.9.3
^ permalink raw reply related
* Re: [PATCH] rds: rds_cong_queue_updates needs to defer the congestion update transmission
From: Chuck Lever @ 2015-02-10 16:50 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: Chien Yen, davem, rds-devel, netdev, linux-kernel
In-Reply-To: <20150210142214.GO337@oracle.com>
On Feb 10, 2015, at 9:22 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote:
>
> This patch fixes a sock_lock deadlock in the rds_cong_queue_update path.
Note that the deadlock appears to exist only with TCP transports.
> We cannot inline the call to rds_send_xmit from rds_cong_queue_update
> because
> (a) we are already holding the sock_lock in the recv path, and
> will deadlock when tcp_setsockopt/tcp_sendmsg try to get the sock
> lock
> (b) cong_queue_update does an irqsave on the rds_cong_lock, and this
> will trigger warnings (for a good reason) from functions called
> out of sock_lock.
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
> net/rds/cong.c | 16 +++++++++++++++-
> 1 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/net/rds/cong.c b/net/rds/cong.c
> index e5b65ac..765d18f 100644
> --- a/net/rds/cong.c
> +++ b/net/rds/cong.c
> @@ -221,7 +221,21 @@ void rds_cong_queue_updates(struct rds_cong_map *map)
> list_for_each_entry(conn, &map->m_conn_list, c_map_item) {
> if (!test_and_set_bit(0, &conn->c_map_queued)) {
> rds_stats_inc(s_cong_update_queued);
> - rds_send_xmit(conn);
> + /* We cannot inline the call to rds_send_xmit() here
> + * for two reasons:
> + * 1. When we get here from the receive path, we
> + * are already holding the sock_lock (held by
> + * tcp_v4_rcv()). So inlining calls to
> + * tcp_setsockopt and/or tcp_sendmsg will deadlock
> + * when it tries to get the sock_lock())
> + * 2. Interrupts are masked so that we can mark the
> + * the port congested from both send and recv paths.
> + * (See comment around declaration of rds_cong_lock).
> + * An attempt to get the sock_lock() here will
> + * therefore trigger warnings.
> + * Defer the xmit to rds_send_worker() instead.
> + */
> + queue_delayed_work(rds_wq, &conn->c_send_w, 0);
> }
> }
>
> --
> 1.7.1
>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply
* Re: [PATCH] rds: rds_cong_queue_updates needs to defer the congestion update transmission
From: Sowmini Varadhan @ 2015-02-10 16:56 UTC (permalink / raw)
To: Chuck Lever; +Cc: Chien Yen, davem, rds-devel, netdev, linux-kernel
In-Reply-To: <E0CB7D17-5ACB-4860-847E-BB0C01CBB65A@oracle.com>
On (02/10/15 11:50), Chuck Lever wrote:
> >
> > This patch fixes a sock_lock deadlock in the rds_cong_queue_update path.
>
> Note that the deadlock appears to exist only with TCP transports.
>
True, but the patch does no harm to IB: this actually only reverts
the change from commit 2fa57129d.
I could update the comment to note that this is only true for TCP
transports. Do you think that would help?
--Sowmini
^ permalink raw reply
* Re: [GIT] Networking
From: Linus Torvalds @ 2015-02-10 16:58 UTC (permalink / raw)
To: David Miller
Cc: Al Viro, Andrew Morton, Network Development,
Linux Kernel Mailing List
In-Reply-To: <CA+55aFxrGRjL_d=aCZQ0sNC+pfW8nYDLb4KkUVAme5oHmjBjTw@mail.gmail.com>
On Tue, Feb 10, 2015 at 8:33 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Will try your suggestion,
Nope, that wasn't it. It still hangs in the same place (I forgot to
get rid of the red hat graphical boot screen so I didn't see the oops,
but ..)
Looking more closely at the generated code, and the fact that the oops
was an access at offset 0x18 from a NULL pointer, it would *look* like
it's this instruction:
call *24(%rax) # MEM[(struct ablkcipher_tfm *)_48 + 8B].decrypt
in the asm. Very odd. That part doesn't seem to have changed at all
(it's the crypto_ablkcipher_decrypt() part inside the
af_alg_wait_for_completion call).
Ignoring whitespace changes and doing a
git show -w 1d10eb2f156f crypto/algif_skcipher.c
does make it clear that apart from the "iter" changes, the only real
difference seems to be to the usage of "used". But I can't judge the
iter changes. And maybe something else changed elsewhere.
Any other ideas? Al?
Linus
^ permalink raw reply
* Re: [PATCH 1/3] rhashtable: require max_shift definition
From: Daniel Borkmann @ 2015-02-10 17:06 UTC (permalink / raw)
To: Josh Hunt
Cc: Thomas Graf, Pablo Neira Ayuso, Patrick McHardy, netfilter-devel,
netdev
In-Reply-To: <54DA2A13.5010204@akamai.com>
On 02/10/2015 04:56 PM, Josh Hunt wrote:
> On 02/10/2015 02:30 AM, Daniel Borkmann wrote:
>> On 02/10/2015 01:58 AM, Thomas Graf wrote:
>>> On 02/09/15 at 07:48pm, Josh Hunt wrote:
>>>> if ((params->key_len && !params->hashfn) ||
>>>> - (!params->key_len && !params->obj_hashfn))
>>>> + (!params->key_len && !params->obj_hashfn) ||
>>>> + (!params->max_shift))
>>>> return -EINVAL;
>>>
>>> You can drop the parenthesis around the new max_shift check.
>>
>> Also, I think the test should be expanded to check if there's
>> a grow_decision given and only in that case require max_shift
>> to be non-zero, otherwise we would require users who don't
>> want to expand their table to give a upper expansion limit.
>
> This is a good point. I'll make this change.
>
> max_shift restricts the # of buckets, but should there be an optional parameter, maxelems, to set a ceiling on the # of elements in a table also? If not, I believe users will be able to add an "unlimited" # of entries to the existing buckets, whether or not a grow_decision fn is defined.
Hm, given that min_shift/max_shift are parameters that directly
concern internals of rhashtable i.e. are tightly coupled to expand
and shrink functionality, I'd say that depending on the use case,
a maxelem limit should rather be handled outside of it, if it's
truly an issue/concern.
^ permalink raw reply
* udp v6 early demux?
From: Vlad Yasevich @ 2015-02-10 17:14 UTC (permalink / raw)
To: netdev@vger.kernel.org
Hi
While testing udpv6 lockless path, I noticed that there is no
support for udpv6 early demux? Is there a technical reason why,
or was this just something that got forgotten (kind of like
lockeless sendmsg)?
Thanks
-vlad
^ permalink raw reply
* Re: [PATCH 1/3] rhashtable: require max_shift definition
From: Thomas Graf @ 2015-02-10 17:22 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Josh Hunt, Pablo Neira Ayuso, Patrick McHardy, netfilter-devel,
netdev
In-Reply-To: <54DA3A9F.9090608@iogearbox.net>
On 02/10/15 at 06:06pm, Daniel Borkmann wrote:
> Hm, given that min_shift/max_shift are parameters that directly
> concern internals of rhashtable i.e. are tightly coupled to expand
> and shrink functionality, I'd say that depending on the use case,
> a maxelem limit should rather be handled outside of it, if it's
> truly an issue/concern.
Agreed, Netlink already uses the atomic counter of rhashtable to
enforce upper limit of table entries:
err = -ENOMEM;
if (BITS_PER_LONG > 32 &&
unlikely(atomic_read(&table->hash.nelems) >= UINT_MAX))
goto err;
^ permalink raw reply
* Re: udp v6 early demux?
From: Eric Dumazet @ 2015-02-10 17:32 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev@vger.kernel.org
In-Reply-To: <54DA3C64.1060309@gmail.com>
On Tue, 2015-02-10 at 12:14 -0500, Vlad Yasevich wrote:
> Hi
>
> While testing udpv6 lockless path, I noticed that there is no
> support for udpv6 early demux? Is there a technical reason why,
> or was this just something that got forgotten (kind of like
> lockeless sendmsg)?
If you have a use case for connected UDP ipv6 flows with performance
issues, then you might add early demux I guess.
IPv4 udp early demux has been a bit of a hack actually,
as Shawn main usage was multicast AFAIK
63c6f81cdde5 udp: ipv4: do not waste time in __udp4_lib_mcast_demux_lookup
610438b74496 udp: ipv4: fix potential use after free in udp_v4_early_demux()
f69b923a758f udp: fix a typo in __udp4_lib_mcast_demux_lookup
421b3885bf6d udp: ipv4: Add udp early demux
^ permalink raw reply
* Re: [PATCH 1/3] rhashtable: require max_shift definition
From: Patrick McHardy @ 2015-02-10 17:44 UTC (permalink / raw)
To: Thomas Graf, Daniel Borkmann
Cc: Josh Hunt, Pablo Neira Ayuso, netfilter-devel, netdev
In-Reply-To: <20150210172241.GB9301@casper.infradead.org>
Am 10. Februar 2015 18:22:41 MEZ, schrieb Thomas Graf <tgraf@suug.ch>:
>On 02/10/15 at 06:06pm, Daniel Borkmann wrote:
>> Hm, given that min_shift/max_shift are parameters that directly
>> concern internals of rhashtable i.e. are tightly coupled to expand
>> and shrink functionality, I'd say that depending on the use case,
>> a maxelem limit should rather be handled outside of it, if it's
>> truly an issue/concern.
>
>Agreed, Netlink already uses the atomic counter of rhashtable to
>enforce upper limit of table entries:
>
> err = -ENOMEM;
> if (BITS_PER_LONG > 32 &&
> unlikely(atomic_read(&table->hash.nelems) >= UINT_MAX))
> goto err;
I would tend to agree with Pablo, now we're handling half (shift) internally and half (max) externally, using internal values.
^ permalink raw reply
* [PATCHv2] rds: rds_cong_queue_updates needs to defer the congestion update transmission
From: Sowmini Varadhan @ 2015-02-10 18:33 UTC (permalink / raw)
To: chien.yen, davem; +Cc: rds-devel, netdev, linux-kernel, sowmini.varadhan
When the RDS transport is TCP, we cannot inline the call to rds_send_xmit
from rds_cong_queue_update because
(a) we are already holding the sock_lock in the recv path, and
will deadlock when tcp_setsockopt/tcp_sendmsg try to get the sock
lock
(b) cong_queue_update does an irqsave on the rds_cong_lock, and this
will trigger warnings (for a good reason) from functions called
out of sock_lock.
This patch reverts the change introduced by
2fa57129d ("RDS: Bypass workqueue when queueing cong updates").
The patch has been verified for both RDS/TCP as well as RDS/RDMA
to ensure that there are not regressions for either transport:
- for verification of RDS/TCP a client-server unit-test was used,
with the server blocked in gdb and thus unable to drain its rcvbuf,
eventually triggering a RDS congestion update.
- for RDS/RDMA, the standard IB regression tests were used
Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
changes from v1: incorporate feedback from Chuck Lever
net/rds/cong.c | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/net/rds/cong.c b/net/rds/cong.c
index e5b65ac..765d18f 100644
--- a/net/rds/cong.c
+++ b/net/rds/cong.c
@@ -221,7 +221,21 @@ void rds_cong_queue_updates(struct rds_cong_map *map)
list_for_each_entry(conn, &map->m_conn_list, c_map_item) {
if (!test_and_set_bit(0, &conn->c_map_queued)) {
rds_stats_inc(s_cong_update_queued);
- rds_send_xmit(conn);
+ /* We cannot inline the call to rds_send_xmit() here
+ * for two reasons (both pertaining to a TCP transport):
+ * 1. When we get here from the receive path, we
+ * are already holding the sock_lock (held by
+ * tcp_v4_rcv()). So inlining calls to
+ * tcp_setsockopt and/or tcp_sendmsg will deadlock
+ * when it tries to get the sock_lock())
+ * 2. Interrupts are masked so that we can mark the
+ * the port congested from both send and recv paths.
+ * (See comment around declaration of rdc_cong_lock).
+ * An attempt to get the sock_lock() here will
+ * therefore trigger warnings.
+ * Defer the xmit to rds_send_worker() instead.
+ */
+ queue_delayed_work(rds_wq, &conn->c_send_w, 0);
}
}
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v3 linux-trace 1/8] tracing: attach eBPF programs to tracepoints and syscalls
From: Alexei Starovoitov @ 2015-02-10 19:53 UTC (permalink / raw)
To: Steven Rostedt
Cc: Ingo Molnar, Namhyung Kim, Arnaldo Carvalho de Melo, Jiri Olsa,
Masami Hiramatsu, Linux API, Network Development, LKML,
Linus Torvalds
On Tue, Feb 10, 2015 at 5:05 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 9 Feb 2015 22:10:45 -0800
> Alexei Starovoitov <ast@plumgrid.com> wrote:
>
>> One can argue that current TP_printk format is already an ABI,
>> because somebody might be parsing the text output.
>
> If somebody does, then it is an ABI. Luckily, it's not that useful to
> parse, thus it hasn't been an issue. As Linus has stated in the past,
> it's not that we can't change ABI interfaces, its just that we can not
> change them if there's a user space application that depends on it.
there are already tools that parse trace_pipe:
https://github.com/brendangregg/perf-tools
> and expect some events to have specific fields. Now we can add new
> fields, or even remove fields that no user space tool is using. This is
> because today, tools use libtraceevent to parse the event data.
not all tools use libtraceevent.
gdb calls perf_event_open directly:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/nat/linux-btrace.c
and parses PERF_RECORD_SAMPLE as a binary.
In this case it's branch records, but I think we never said anywhere
that PERF_SAMPLE_IP | PERF_SAMPLE_ADDR should come
in this particular order.
> This is why I'm nervous about exporting the parameters of the trace
> event call. Right now, those parameters can always change, because the
> only way to know they exist is by looking at the code. And currently,
> there's no way to interact with those parameters. Once we have eBPF in
> mainline, we now have a way to interact with the parameters and if
> those parameters change, then the eBPF program will break, and if eBPF
> can be part of a user space tool, that will break that tool and
> whatever change in the trace point that caused this breakage would have
> to be reverted. IOW, this can limit development in the kernel.
it can limit development unless we say that bpf programs
that attach to tracepoints are not part of ABI.
Easy enough to add large comment similar to perf_event.h
> Al Viro currently does not let any tracepoint in VFS because he doesn't
> want the internals of that code locked to an ABI. He's right to be
> worried.
Same with networking bits. We don't want tracepoints to limit
kernel development, but we want debuggability and kernel
analytics.
All existing tracepoints defined via DEFINE_EVENT should
not be an ABI.
But some maintainers think of them as ABI, whereas others
are using them freely. imo it's time to remove ambiguity.
The idea for new style of tracepoints is the following:
introduce new macro: DEFINE_EVENT_USER
and let programs attach to them.
These tracepoint will receive one or two pointers to important
structs only. They will not have TP_printk, assign and fields.
The placement and arguments to these new tracepoints
will be an ABI.
All existing tracepoints are not.
The main reason to attach to tracepoint is that they are
accessible without debug info (unlike kprobe)
Another reason is speed. tracepoints are much faster than
optimized kprobes and for real-time analytics the speed
is critical.
The position of new tracepoints and their arguments
will be an ABI and the programs can be both.
If program is using bpf_fetch*() helpers it obviously
wants to access internal data structures, so
it's really nothing more, but 'safe kernel module'
and kernel layout specific.
Both old and new tracepoints + programs will be used
for live kernel debugging.
If program is accessing user-ized data structures then
it is portable and will run on any kernel.
In uapi header we can define:
struct task_struct_user {
int pid;
int prio;
};
and let bpf programs access it via real 'struct task_struct *'
pointer passed into tracepoint.
bpf loader will translate offsets and sizes used inside
the program into real task_struct's offsets and loads.
(all structs are read-only of course)
programs will be fast and kernel independent.
They will be used for analytics (latency, etc)
>> so in some cases we cannot change tracepoints without
>> somebody complaining that his tool broke.
>> In other cases tracepoints are used for debugging only
>> and no one will notice when they change...
>> It was and still a grey area.
>
> Not really. If a tool uses the tracepoint, it can lock that tracepoint
> down. This is exactly what latencytop did. It happened, it's not a
> hypothetical situation.
correct.
>> bpf doesn't change any of that.
>> It actually makes addition of new tracepoints easier.
>
> I totally disagree. It adds more ways to see inside the kernel, and if
> user space depends on this, it adds more ways the kernel can not change.
>
> It comes down to how robust eBPF is with the internals of the kernel
> changing. If we limit eBPF to system call tracepoints only, that's
> fine because those have the same ABI as the system call itself. I'm
> worried about the internal tracepoints for scheduling, irqs, file
> systems, etc.
agree. we need to make it clear that existing tracepoints
+ programs is not ABI.
>> In the future we might add a tracepoint and pass a single
>> pointer to interesting data struct to it. bpf programs will walk
>> data structures 'as safe modules' via bpf_fetch*() methods
>> without exposing it as ABI.
>
> Will this work if that structure changes? When the field we are looking
> for no longer exists?
bpf_fetch*() is the same mechanism as perf probe.
If there is a mistake by user space tools, the program
will be reading some junk, but it won't be crashing.
To be able to debug live kernel we need to see everywhere.
Same way as systemtap loads kernel modules to walk
things inside kernel, bpf programs walk pointers with
bpf_fetch*().
I'm saying that if program is using bpf_fetch*()
it wants to see kernel internals and obviously depends
on particular kernel layout.
>> whereas today we pass a lot of fields to tracepoints and
>> make all of these fields immutable.
>
> The parameters passed to the tracepoint are not shown to userspace and
> can change at will. Now, we present the final parsing of the parameters
> that convert to fields. As all currently known tools uses
> libtraceevent.a, and parse the format files, those fields can move
> around and even change in size. The structures are not immutable. The
> fields are locked down if user space relies on them. But they can move
> about within the tracepoint, because the parsing allows for it.
>
> Remember, these are processed fields. The result of TP_fast_assign()
> and what gets put into the ring buffer. Now what is passed to the
> actual tracepoint is not visible by userspace, and in lots of cases, it
> is just a pointer to some structure. What eBPF brings to the table is a
> way to access this structure from user space. What keeps a structured
> passed to a tracepoint from becoming immutable if there's a eBPF
> program that expects it to have a specific field?
agree. that's fair.
I'm proposing to treat bpf programs that attach to existing
tracepoints as kernel modules that carry no ABI claims.
>> and bpf programs like live debugger that examine things.
>
> If bpf programs only dealt with kprobes, I may agree. But tracepoints
> have already been proven to be a type of ABI. If we open another window
> into the kernel, this can screw us later. It's better to solve this now
> than when we are fighting with Linus over user space breakage.
I'm not sure what's more needed other than adding
large comments into documentation, man pages and sample
code that bpf+existing tracepoint is not an ABI.
> What we need is to know if eBPF programs are modules or a user space
> interface. If they are a user interface then we need to be extremely
> careful here. If they are treated the same as modules, then it would
> not add any API. But that hasn't been settled yet, even if we have a
> comment in the kernel.
>
> Maybe what we should do is to make eBPF pass the kernel version it was
> made for (with all the mod version checks). If it doesn't match, fail
> to load it. Perhaps the more eBPF is limited like modules are, the
> better chance we have that no eBPF program creates a new ABI.
it's easy to add kernel version check and it will be equally
easy for user space to hack it.
imo comments in documentation and samples is good enough.
also not all bpf programs are equal.
bpf+existing tracepoint is not ABI
bpf+new tracepoint is ABI if programs are not using bpf_fetch
bpf+syscall is ABI if programs are not using bpf_fetch
bpf+kprobe is not ABI
bpf+sockets is ABI
At the end we want most of the programs to be written
without assuming anything about kernel internals.
But for live kernel debugging we will write programs
very specific to given kernel layout.
We can categorize the above in non-ambigous via
bpf program type.
Programs with:
BPF_PROG_TYPE_TRACEPOINT - not ABI
BPF_PROG_TYPE_KPROBE - not ABI
BPF_PROG_TYPE_SOCKET_FILTER - ABI
for my proposed 'new tracepoints' we can add type:
BPF_PROG_TYPE_TRACEPOINT_USER - ABI
and disallow calls to bpf_fetch*() for them.
To make it more strict we can do kernel version check
for all prog types that are 'not ABI', but is it really necessary?
To summarize and consolidate other threads:
- I will remove reading of PERF_SAMPLE_RAW in tracex1 example.
it's really orthogonal to this whole discussion.
- will add more comments through out that just because
programs can read tracepoint arguments, they shouldn't
make any assumptions that args stays as-is from version to version
- will work on a patch to demonstrate how few in-kernel
structures can be user-ized and how programs can access
them in version-indepedent way
btw the concept of user-ized data structures already exists
with classic bpf, since 'A = load -0x1000' is translated into
'A = skb->protocol'. I'm thinking of something similar
but more generic and less obscure.
^ permalink raw reply
* Re: [RFC PATCH 00/29] net: VRF support
From: David Ahern @ 2015-02-10 20:54 UTC (permalink / raw)
To: Thomas Graf; +Cc: netdev, ebiederm
In-Reply-To: <20150210005344.GA6293@casper.infradead.org>
On 2/9/15 5:53 PM, Thomas Graf wrote:
> On 02/04/15 at 06:34pm, David Ahern wrote:
>> Namespaces provide excellent separation of the networking stack from the
>> netdevices and up. The intent of VRFs is to provide an additional,
>> logical separation at the L3 layer within a namespace.
>
> What you ask for seems to be L3 micro segmentation inside netns. I
I would not label it 'micro' but yes a L3 separation within a L1 separation.
> would argue that we already support this through multiple routing
> tables. I would prefer improving the existing architecture to cover
> your use cases: Increase the number of supported tables, extend
> routing rules as needed, ...
I've seen that response for VRFs as well. I have not personally tried
it, but from what I have read it does not work well. I think Roopa
responded that Cumulus has spent time on that path and has hit some
roadblocks.
>
>> The VRF id of tasks defaults to 1 and is inherited parent to child. It can
>> be read via the file '/proc/<pid>/vrf' and can be changed anytime by writing
>> to this file (if preferred this can be made a prctl to change the VRF id).
>> This allows services to be launched in a VRF context using ip, similar to
>> what is done for network namespaces.
>> e.g., ip vrf exec 99 /usr/sbin/sshd
>
> I think such as classification should occur through cgroups instead
> of touching PIDs directly.
That is an interesting idea -- using cgroups for task labeling. It
presents a creation / deletion event for VRFs which I was trying to
avoid, and there will be some amount of overhead with a cgroup. I'll
take a look at that option when I get some time.
As for as the current proposal I am treating VRF as part of a network
context. Today 'ip netns' is used to run a command in a specific network
namespace; the proposal with the VRF layering is to add a vrf context
within a namespace so in keeping with how 'ip netns' works the above
syntax allows a user to supply both a network namespace + VRF for
running a command.
>
>> Network devices belong to a single VRF context which defaults to VRF 1.
>> They can be assigned to another VRF using IFLA_VRF attribute in link
>> messages. Similarly the VRF assignment is returned in the IFLA_VRF
>> attribute. The ip command has been modified to display the VRF id of a
>> device. L2 applications like lldp are not VRF aware and still work through
>> through all network devices within the namespace.
>
> I believe that binding net_devices to VRFs is misleading and the
> concept by itself is non-scalable. You do not want to create 10k
> net_devices for your overlay of choice just to tie them to a
> particular VRF. You want to store the VRF identifier as metadata and
> have a stateless classifier included it in the VRF decision. See the
> recent VXLAN-GBP work.
I'll take a look when I get time.
I have not seen scalability issues creating 1,000+ net_devices.
Certainly the 40k'ish memory per net_device is noticeable but I believe
that can be improved (e.g., a number of entries can be moved under
proper CONFIG_ checks). I do need to repeat the tests on newer kernels.
>
> You could either map whatever selects the VRF to the mark or support it
> natively in the routing rules classifier.
>
> An obvious alternative is OVS. What you describe can be implemented in
> a scalable matter using OVS and mark. I understand that OVS is not for
> everybody but it gets a fundamental principle right: Scalability
> demands for programmability.
>
> I don’t think we should be adding a new single purpose metadata field
> to arbitrary structures for every new use case that comes up. We
> should work on programmability which increases flexibility and allows
> decoupling application interest from networking details.
>
>> On RX skbs get their VRF context from the netdevice the packet is received
>> on. For TX the VRF context for an skb is taken from the socket. The
>> intention is for L3/raw sockets to be able to set the VRF context for a
>> packet TX using cmsg (not coded in this patch set).
>
> Specyfing L3 context in cmsg seems very broken to me. We do not want
> to bind applications any closer to underlying networking infrastructure.
> In fact, we should do the opposite and decouple this completely.
That suggestion is inline with what is done today for other L3
parameters -- TOS, TTL, and a few others.
>
>> The 'any' context applies to listen sockets only; connected sockets are in
>> a VRF context. Child sockets accepted by the daemon acquire the VRF context
>> of the network device the connection originated on.
>
> Linux considers an address local regardless of the interface the packet
> was received on. So you would accept the packet on any interface and
> then bind it to the VRF of that interface even though the route for it
> might be on a different interface.
>
> This really belongs into routing rules from my perspective which takes
> mark and the cgroup context into account.
Expanding the current network namespace checks to a networking context
is a very simple and clean way of implementing VRFs versus cobbling
together a 'VRF like' capability using marks, multiple tables, etc (ie.,
the existing capabilities). Further, the VRF tagging of net_devices
seems to readily fit into the hardware offload and switchdev
capabilities (e.g., add a ndo operation for setting the VRF tag on a
device which passes it to the driver).
Big picture wise where is OCP and switchdev headed? Top-of-rack switches
seem to be the first target, but after that? Will the kernel ever
support MPLS? Will the kernel attain the richer feature set of high-end
routers? If so, how does VRF support fit into the design? As I
understand it a scalable VRF solution is a fundamental building block.
Will a cobbled together solution of cgroups, marks, rules, multiple
tables really work versus the simplicity of an expanded network context?
David
^ permalink raw reply
* [trivial PATCH] treewide: Remove unnecessary BCMA_CORETABLE_END macro
From: Joe Perches @ 2015-02-10 20:55 UTC (permalink / raw)
To: LKML; +Cc: Andrew Morton, netdev, brcm80211-dev-list, linux-spi, linux-usb
Use the normal {} instead of a macro to terminate an array.
Remove the macro too.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/ethernet/broadcom/bgmac.c | 2 +-
drivers/net/wireless/b43/main.c | 2 +-
drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c | 2 +-
drivers/spi/spi-bcm53xx.c | 2 +-
drivers/usb/host/bcma-hcd.c | 2 +-
include/linux/mod_devicetable.h | 2 --
6 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 3007d95..ea63cb0 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -21,7 +21,7 @@
static const struct bcma_device_id bgmac_bcma_tbl[] = {
BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_4706_MAC_GBIT, BCMA_ANY_REV, BCMA_ANY_CLASS),
BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_MAC_GBIT, BCMA_ANY_REV, BCMA_ANY_CLASS),
- BCMA_CORETABLE_END
+ {},
};
MODULE_DEVICE_TABLE(bcma, bgmac_bcma_tbl);
diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index 2c90886..6acb2b5 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -127,7 +127,7 @@ static const struct bcma_device_id b43_bcma_tbl[] = {
BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_80211, 0x1E, BCMA_ANY_CLASS),
BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_80211, 0x28, BCMA_ANY_CLASS),
BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_80211, 0x2A, BCMA_ANY_CLASS),
- BCMA_CORETABLE_END
+ {},
};
MODULE_DEVICE_TABLE(bcma, b43_bcma_tbl);
#endif
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
index f95b524..4813506 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
@@ -99,7 +99,7 @@ static struct bcma_device_id brcms_coreid_table[] = {
BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_80211, 17, BCMA_ANY_CLASS),
BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_80211, 23, BCMA_ANY_CLASS),
BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_80211, 24, BCMA_ANY_CLASS),
- BCMA_CORETABLE_END
+ {},
};
MODULE_DEVICE_TABLE(bcma, brcms_coreid_table);
diff --git a/drivers/spi/spi-bcm53xx.c b/drivers/spi/spi-bcm53xx.c
index 17b34cb..3fb91c8 100644
--- a/drivers/spi/spi-bcm53xx.c
+++ b/drivers/spi/spi-bcm53xx.c
@@ -216,7 +216,7 @@ static struct spi_board_info bcm53xx_info = {
static const struct bcma_device_id bcm53xxspi_bcma_tbl[] = {
BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_NS_QSPI, BCMA_ANY_REV, BCMA_ANY_CLASS),
- BCMA_CORETABLE_END
+ {},
};
MODULE_DEVICE_TABLE(bcma, bcm53xxspi_bcma_tbl);
diff --git a/drivers/usb/host/bcma-hcd.c b/drivers/usb/host/bcma-hcd.c
index cd6d0af..526cfab 100644
--- a/drivers/usb/host/bcma-hcd.c
+++ b/drivers/usb/host/bcma-hcd.c
@@ -306,7 +306,7 @@ static int bcma_hcd_resume(struct bcma_device *dev)
static const struct bcma_device_id bcma_hcd_table[] = {
BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_USB20_HOST, BCMA_ANY_REV, BCMA_ANY_CLASS),
- BCMA_CORETABLE_END
+ {},
};
MODULE_DEVICE_TABLE(bcma, bcma_hcd_table);
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 745def8..6711d57 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -380,8 +380,6 @@ struct bcma_device_id {
} __attribute__((packed,aligned(2)));
#define BCMA_CORE(_manuf, _id, _rev, _class) \
{ .manuf = _manuf, .id = _id, .rev = _rev, .class = _class, }
-#define BCMA_CORETABLE_END \
- { 0, },
#define BCMA_ANY_MANUF 0xFFFF
#define BCMA_ANY_ID 0xFFFF
^ permalink raw reply related
* Re: [PATCH 1/3] rhashtable: require max_shift definition
From: Josh Hunt @ 2015-02-10 21:18 UTC (permalink / raw)
To: Patrick McHardy, Thomas Graf, Daniel Borkmann
Cc: Pablo Neira Ayuso, netfilter-devel, netdev
In-Reply-To: <FB9AAD93-7F9C-440E-8EB2-44D50B8AA22A@trash.net>
On 02/10/2015 11:44 AM, Patrick McHardy wrote:
> Am 10. Februar 2015 18:22:41 MEZ, schrieb Thomas Graf <tgraf@suug.ch>:
>> On 02/10/15 at 06:06pm, Daniel Borkmann wrote:
>>> Hm, given that min_shift/max_shift are parameters that directly
>>> concern internals of rhashtable i.e. are tightly coupled to expand
>>> and shrink functionality, I'd say that depending on the use case,
>>> a maxelem limit should rather be handled outside of it, if it's
>>> truly an issue/concern.
>>
>> Agreed, Netlink already uses the atomic counter of rhashtable to
>> enforce upper limit of table entries:
>>
>> err = -ENOMEM;
>> if (BITS_PER_LONG > 32 &&
>> unlikely(atomic_read(&table->hash.nelems) >= UINT_MAX))
>> goto err;
>
> I would tend to agree with Pablo, now we're handling half (shift) internally and half (max) externally, using internal values.
OK. Thanks for all the feedback. I will send a v2 once the other 2
patches get reviewed.
Thanks
Josh
^ permalink raw reply
* [trivial PATCH] treewide: Remove unnecessary SSB_DEVTABLE_END macro
From: Joe Perches @ 2015-02-10 21:19 UTC (permalink / raw)
To: LKML; +Cc: Andrew Morton, netdev, b43-dev, linux-usb
In-Reply-To: <1423601703.15343.3.camel@perches.com>
Use the normal {} instead of a macro to terminate an array.
Remove the macro too.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/ethernet/broadcom/b44.c | 2 +-
drivers/net/wireless/b43/main.c | 2 +-
drivers/net/wireless/b43legacy/main.c | 2 +-
drivers/ssb/driver_gige.c | 2 +-
drivers/usb/host/ssb-hcd.c | 2 +-
include/linux/mod_devicetable.h | 2 --
6 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index d86d6ba..bd5916a 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -121,7 +121,7 @@ static struct pci_driver b44_pci_driver = {
static const struct ssb_device_id b44_ssb_tbl[] = {
SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_ETHERNET, SSB_ANY_REV),
- SSB_DEVTABLE_END
+ {},
};
MODULE_DEVICE_TABLE(ssb, b44_ssb_tbl);
diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index 6acb2b5..ccbdb05 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -144,7 +144,7 @@ static const struct ssb_device_id b43_ssb_tbl[] = {
SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_80211, 13),
SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_80211, 15),
SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_80211, 16),
- SSB_DEVTABLE_END
+ {},
};
MODULE_DEVICE_TABLE(ssb, b43_ssb_tbl);
#endif
diff --git a/drivers/net/wireless/b43legacy/main.c b/drivers/net/wireless/b43legacy/main.c
index 1aec214..4e58c00 100644
--- a/drivers/net/wireless/b43legacy/main.c
+++ b/drivers/net/wireless/b43legacy/main.c
@@ -86,7 +86,7 @@ MODULE_PARM_DESC(fwpostfix, "Postfix for the firmware files to load.");
static const struct ssb_device_id b43legacy_ssb_tbl[] = {
SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_80211, 2),
SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_80211, 4),
- SSB_DEVTABLE_END
+ {},
};
MODULE_DEVICE_TABLE(ssb, b43legacy_ssb_tbl);
diff --git a/drivers/ssb/driver_gige.c b/drivers/ssb/driver_gige.c
index 21f71a1..e973405 100644
--- a/drivers/ssb/driver_gige.c
+++ b/drivers/ssb/driver_gige.c
@@ -24,7 +24,7 @@ MODULE_LICENSE("GPL");
static const struct ssb_device_id ssb_gige_tbl[] = {
SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_ETHERNET_GBIT, SSB_ANY_REV),
- SSB_DEVTABLE_END
+ {},
};
/* MODULE_DEVICE_TABLE(ssb, ssb_gige_tbl); */
diff --git a/drivers/usb/host/ssb-hcd.c b/drivers/usb/host/ssb-hcd.c
index 0196f76..ffc32f4 100644
--- a/drivers/usb/host/ssb-hcd.c
+++ b/drivers/usb/host/ssb-hcd.c
@@ -251,7 +251,7 @@ static const struct ssb_device_id ssb_hcd_table[] = {
SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_USB11_HOSTDEV, SSB_ANY_REV),
SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_USB11_HOST, SSB_ANY_REV),
SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_USB20_HOST, SSB_ANY_REV),
- SSB_DEVTABLE_END
+ {},
};
MODULE_DEVICE_TABLE(ssb, ssb_hcd_table);
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 72f24b6..e530533 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -364,8 +364,6 @@ struct ssb_device_id {
} __attribute__((packed, aligned(2)));
#define SSB_DEVICE(_vendor, _coreid, _revision) \
{ .vendor = _vendor, .coreid = _coreid, .revision = _revision, }
-#define SSB_DEVTABLE_END \
- { 0, },
#define SSB_ANY_VENDOR 0xFFFF
#define SSB_ANY_ID 0xFFFF
^ permalink raw reply related
* Re: [GIT] Networking
From: Linus Torvalds @ 2015-02-10 21:26 UTC (permalink / raw)
To: David Miller
Cc: Al Viro, Andrew Morton, Network Development,
Linux Kernel Mailing List
In-Reply-To: <CA+55aFwxuMcNPU-C5p2hSWuNhJW+DOi3Py0NQ--Rw9_bv3md2w@mail.gmail.com>
On Tue, Feb 10, 2015 at 8:58 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Feb 10, 2015 at 8:33 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Will try your suggestion,
>
> Nope, that wasn't it. It still hangs in the same place (I forgot to
> get rid of the red hat graphical boot screen so I didn't see the oops,
> but ..)
Just to confirm that yes, it's that particular commit 1d10eb2f156f.
I reverted it and things work again. So it's not the miscalculation of
"used" , but it's certainly *something* in that commit.
Oh well. I have a ton of other trees to pull, so I'll just drop this for now.
> Looking more closely at the generated code, and the fact that the oops
> was an access at offset 0x18 from a NULL pointer, it would *look* like
> it's this instruction:
>
> call *24(%rax) # MEM[(struct ablkcipher_tfm *)_48 + 8B].decrypt
Double-checked, and yes, "skcipher_recvmsg+0x360/0x410" is that call
to the decrypt routine.
So it looks like for some reason that
struct ablkcipher_tfm *crt =
crypto_ablkcipher_crt(crypto_ablkcipher_reqtfm(req));;
ends up being NULL in
crypto_ablkcipher_decrypt(&ctx->req)
causing the oops. I just don't see what the heck in that patch would
have changed any of that.
Linus
^ permalink raw reply
* [PATCH net] openvswitch: Reset key metadata for packet execution.
From: Pravin B Shelar @ 2015-02-10 21:35 UTC (permalink / raw)
To: davem; +Cc: netdev, Pravin B Shelar
Userspace packet execute command pass down flow key for given
packet. But userspace can skip some parameter with zero value.
Therefore kernel needs to initialize key metadata to zero.
Fixes: 0714812134 ("openvswitch: Eliminate memset() from flow_extract.")
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
net/openvswitch/flow.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index da2fae0..5e41bf2 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -717,6 +717,8 @@ int ovs_flow_key_extract_userspace(const struct nlattr *attr,
{
int err;
+ memset(key, 0, OVS_SW_FLOW_KEY_METADATA_SIZE);
+
/* Extract metadata from netlink attributes. */
err = ovs_nla_get_flow_metadata(attr, key, log);
if (err)
--
1.7.1
^ permalink raw reply related
* Re: [GIT] Networking
From: Linus Torvalds @ 2015-02-10 21:50 UTC (permalink / raw)
To: David Miller
Cc: Al Viro, Andrew Morton, Network Development,
Linux Kernel Mailing List
In-Reply-To: <CA+55aFzJNNDi0YKXnNYF=ccYvXALRxzngiV1tARd6CdCimVRYg@mail.gmail.com>
On Tue, Feb 10, 2015 at 1:26 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Just to confirm that yes, it's that particular commit 1d10eb2f156f.
>
> I reverted it and things work again. So it's not the miscalculation of
> "used" , but it's certainly *something* in that commit.
How about this?
- npages = (off + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
+ npages = PAGE_ALIGN(off + n);
The two are not even *remotely* the same thing.
It's in af_alg_make_sg(), and it would seem to cause sg_init_table()
and the loop that does sg_set_page() do insane things. Possibly
including random memory scribbles etc. Which could explain anything.
Anyway, I'm now working on the other pulls I have, so I'll leave this
for now, but I think that might be it.
Linus
^ permalink raw reply
* Re: [PATCH v3 linux-trace 1/8] tracing: attach eBPF programs to tracepoints and syscalls
From: Steven Rostedt @ 2015-02-10 21:53 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Ingo Molnar, Namhyung Kim, Arnaldo Carvalho de Melo, Jiri Olsa,
Masami Hiramatsu, Linux API, Network Development, LKML,
Linus Torvalds, Peter Zijlstra, ebiederm-aS9lmoZGLiVWk0Htik3J/w
In-Reply-To: <CAMEtUuwAf4AFeWVEdJB4VQz_=2fWY=q8=rbkFg38rtVtNVCHxg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, 10 Feb 2015 11:53:22 -0800
Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> On Tue, Feb 10, 2015 at 5:05 AM, Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org> wrote:
> > On Mon, 9 Feb 2015 22:10:45 -0800
> > Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> >
> >> One can argue that current TP_printk format is already an ABI,
> >> because somebody might be parsing the text output.
> >
> > If somebody does, then it is an ABI. Luckily, it's not that useful to
> > parse, thus it hasn't been an issue. As Linus has stated in the past,
> > it's not that we can't change ABI interfaces, its just that we can not
> > change them if there's a user space application that depends on it.
>
> there are already tools that parse trace_pipe:
> https://github.com/brendangregg/perf-tools
Yep, and if this becomes a standard, then any change that makes
trace_pipe different will be reverted.
>
> > and expect some events to have specific fields. Now we can add new
> > fields, or even remove fields that no user space tool is using. This is
> > because today, tools use libtraceevent to parse the event data.
>
> not all tools use libtraceevent.
> gdb calls perf_event_open directly:
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/nat/linux-btrace.c
> and parses PERF_RECORD_SAMPLE as a binary.
> In this case it's branch records, but I think we never said anywhere
> that PERF_SAMPLE_IP | PERF_SAMPLE_ADDR should come
> in this particular order.
What particular order? Note, that's a hardware event, not a software
one.
>
> > This is why I'm nervous about exporting the parameters of the trace
> > event call. Right now, those parameters can always change, because the
> > only way to know they exist is by looking at the code. And currently,
> > there's no way to interact with those parameters. Once we have eBPF in
> > mainline, we now have a way to interact with the parameters and if
> > those parameters change, then the eBPF program will break, and if eBPF
> > can be part of a user space tool, that will break that tool and
> > whatever change in the trace point that caused this breakage would have
> > to be reverted. IOW, this can limit development in the kernel.
>
> it can limit development unless we say that bpf programs
> that attach to tracepoints are not part of ABI.
> Easy enough to add large comment similar to perf_event.h
Again, it doesn't matter what we say. Linus made that very clear. He
stated if you provide an interface, and someone uses that interface for
a user space application, and they complain if it breaks, that is just
reason to revert whatever broke it.
>
> > Al Viro currently does not let any tracepoint in VFS because he doesn't
> > want the internals of that code locked to an ABI. He's right to be
> > worried.
>
> Same with networking bits. We don't want tracepoints to limit
> kernel development, but we want debuggability and kernel
> analytics.
> All existing tracepoints defined via DEFINE_EVENT should
> not be an ABI.
I agree, but that doesn't make it so :-/
> But some maintainers think of them as ABI, whereas others
> are using them freely. imo it's time to remove ambiguity.
I would love to, and have brought this up at Kernel Summit more than
once with no solution out of it.
>
> The idea for new style of tracepoints is the following:
> introduce new macro: DEFINE_EVENT_USER
> and let programs attach to them.
We actually have that today. But it's TRACE_EVENT_FLAGS(), although
that should be cleaned up a bit. Frederic added it to label events that
are safe for perf non root. It seems to be used currently only for
syscalls.
> These tracepoint will receive one or two pointers to important
> structs only. They will not have TP_printk, assign and fields.
> The placement and arguments to these new tracepoints
> will be an ABI.
> All existing tracepoints are not.
TP_printk() is not really an issue.
>
> The main reason to attach to tracepoint is that they are
> accessible without debug info (unlike kprobe)
That is, if you have a special bpf call to access variables, right? How
else do you access part of a data structure.
> Another reason is speed. tracepoints are much faster than
> optimized kprobes and for real-time analytics the speed
> is critical.
>
> The position of new tracepoints and their arguments
> will be an ABI and the programs can be both.
You means "special tracepoints" one that does export the arguments?
Question is, how many maintainers will add these, knowing that they
will have to be forever maintained as is.
> If program is using bpf_fetch*() helpers it obviously
> wants to access internal data structures, so
> it's really nothing more, but 'safe kernel module'
> and kernel layout specific.
> Both old and new tracepoints + programs will be used
> for live kernel debugging.
>
> If program is accessing user-ized data structures then
Technically, the TP_struct__entry is a user-ized structure.
> it is portable and will run on any kernel.
> In uapi header we can define:
> struct task_struct_user {
> int pid;
> int prio;
Here's a perfect example of something that looks stable to show to
user space, but is really a pimple that is hiding cancer.
Lets start with pid. We have name spaces. What pid will be put there?
We have to show the pid of the name space it is under.
Then we have prio. What is prio in the DEADLINE scheduler. It is rather
meaningless. Also, it is meaningless in SCHED_OTHER.
Also note that even for SCHED_FIFO, the prio is used differently in the
kernel than it is in userspace. For the kernel, lower is higher.
> };
> and let bpf programs access it via real 'struct task_struct *'
> pointer passed into tracepoint.
> bpf loader will translate offsets and sizes used inside
> the program into real task_struct's offsets and loads.
It would need to do more that that. It may have to calculate the value
that it returns, as the internal value may be different with different
kernels.
> (all structs are read-only of course)
> programs will be fast and kernel independent.
> They will be used for analytics (latency, etc)
>
> >> so in some cases we cannot change tracepoints without
> >> somebody complaining that his tool broke.
> >> In other cases tracepoints are used for debugging only
> >> and no one will notice when they change...
> >> It was and still a grey area.
> >
> > Not really. If a tool uses the tracepoint, it can lock that tracepoint
> > down. This is exactly what latencytop did. It happened, it's not a
> > hypothetical situation.
>
> correct.
>
> >> bpf doesn't change any of that.
> >> It actually makes addition of new tracepoints easier.
> >
> > I totally disagree. It adds more ways to see inside the kernel, and if
> > user space depends on this, it adds more ways the kernel can not change.
> >
> > It comes down to how robust eBPF is with the internals of the kernel
> > changing. If we limit eBPF to system call tracepoints only, that's
> > fine because those have the same ABI as the system call itself. I'm
> > worried about the internal tracepoints for scheduling, irqs, file
> > systems, etc.
>
> agree. we need to make it clear that existing tracepoints
> + programs is not ABI.
The question is, how do we do that. Linus pointed out that comments and
documentation is not enough. We need to have an interface that users
would use before they use one that we do not like them to use.
>
> >> In the future we might add a tracepoint and pass a single
> >> pointer to interesting data struct to it. bpf programs will walk
> >> data structures 'as safe modules' via bpf_fetch*() methods
> >> without exposing it as ABI.
> >
> > Will this work if that structure changes? When the field we are looking
> > for no longer exists?
>
> bpf_fetch*() is the same mechanism as perf probe.
> If there is a mistake by user space tools, the program
> will be reading some junk, but it won't be crashing.
But what if the userspace tool depends on that value returning
something meaningful. If it was meaningful in the past, it will have to
be meaningful in the future, even if the internals of the kernel make
it otherwise.
> To be able to debug live kernel we need to see everywhere.
> Same way as systemtap loads kernel modules to walk
> things inside kernel, bpf programs walk pointers with
> bpf_fetch*().
I would agree here too. But again, we really need to be careful about
the interface we expose.
> I'm saying that if program is using bpf_fetch*()
> it wants to see kernel internals and obviously depends
> on particular kernel layout.
Right, but if there's something specific in that kernel layout that it
can decide to do something with, otherwise it breaks, then we need to
worry about it.
eBPF is very flexible, which means it is bound to have someone use it
in a way you never dreamed of, and that will be what bites you in the
end (pun intended).
>
> >> whereas today we pass a lot of fields to tracepoints and
> >> make all of these fields immutable.
> >
> > The parameters passed to the tracepoint are not shown to userspace and
> > can change at will. Now, we present the final parsing of the parameters
> > that convert to fields. As all currently known tools uses
> > libtraceevent.a, and parse the format files, those fields can move
> > around and even change in size. The structures are not immutable. The
> > fields are locked down if user space relies on them. But they can move
> > about within the tracepoint, because the parsing allows for it.
> >
> > Remember, these are processed fields. The result of TP_fast_assign()
> > and what gets put into the ring buffer. Now what is passed to the
> > actual tracepoint is not visible by userspace, and in lots of cases, it
> > is just a pointer to some structure. What eBPF brings to the table is a
> > way to access this structure from user space. What keeps a structured
> > passed to a tracepoint from becoming immutable if there's a eBPF
> > program that expects it to have a specific field?
>
> agree. that's fair.
> I'm proposing to treat bpf programs that attach to existing
> tracepoints as kernel modules that carry no ABI claims.
Yeah, we may need to find a way to guarantee that.
>
> >> and bpf programs like live debugger that examine things.
> >
> > If bpf programs only dealt with kprobes, I may agree. But tracepoints
> > have already been proven to be a type of ABI. If we open another window
> > into the kernel, this can screw us later. It's better to solve this now
> > than when we are fighting with Linus over user space breakage.
>
> I'm not sure what's more needed other than adding
> large comments into documentation, man pages and sample
> code that bpf+existing tracepoint is not an ABI.
Making it break as soon as a config or kernel version changes ;-)
That may be the only way to guarantee that users do not rely on it.
>
> > What we need is to know if eBPF programs are modules or a user space
> > interface. If they are a user interface then we need to be extremely
> > careful here. If they are treated the same as modules, then it would
> > not add any API. But that hasn't been settled yet, even if we have a
> > comment in the kernel.
> >
> > Maybe what we should do is to make eBPF pass the kernel version it was
> > made for (with all the mod version checks). If it doesn't match, fail
> > to load it. Perhaps the more eBPF is limited like modules are, the
> > better chance we have that no eBPF program creates a new ABI.
>
> it's easy to add kernel version check and it will be equally
> easy for user space to hack it.
Well, if the bpf program says it is built for kernel 3.19, but the
user tool fudges it to say it was built for kernel 3.20, but it breaks,
than how can they complain about a regression? The tool itself says it
was built for 3.20 but doesn't work. You need to show that same program
worked on 3.19, but it wont because 3.20 wont work there.
> imo comments in documentation and samples is good enough.
Again, your and my opinion do not matter. It comes down to what Linus
thinks. And if we expose an interface that applications decide to use,
and it breaks when a design in the kernel changes, Linus may revert
that change. The author of that change will not be too happy with us.
>
> also not all bpf programs are equal.
> bpf+existing tracepoint is not ABI
Why not?
> bpf+new tracepoint is ABI if programs are not using bpf_fetch
How is this different?
> bpf+syscall is ABI if programs are not using bpf_fetch
Well, this is easy. As syscalls are ABI, and the tracepoints for them
match the ABI, it by default becomes an ABI.
> bpf+kprobe is not ABI
Right.
> bpf+sockets is ABI
Right, because sockets themselves are ABI.
> At the end we want most of the programs to be written
> without assuming anything about kernel internals.
> But for live kernel debugging we will write programs
> very specific to given kernel layout.
And here lies the trick. How do we differentiate applications for
everyday use from debugging tools? There's been times when debugging
tools have shown themselves as being so useful they become everyday
use tools.
>
> We can categorize the above in non-ambigous via
> bpf program type.
> Programs with:
> BPF_PROG_TYPE_TRACEPOINT - not ABI
> BPF_PROG_TYPE_KPROBE - not ABI
> BPF_PROG_TYPE_SOCKET_FILTER - ABI
Again, what enforces this? (hint, it's Linus)
>
> for my proposed 'new tracepoints' we can add type:
> BPF_PROG_TYPE_TRACEPOINT_USER - ABI
> and disallow calls to bpf_fetch*() for them.
> To make it more strict we can do kernel version check
> for all prog types that are 'not ABI', but is it really necessary?
If we have something that makes it difficult for a tool to work from
one kernel to the next, or ever with different configs, where that tool
will never become a standard, then that should be good enough to keep
it from dictating user ABI.
To give you an example, we thought about scrambling the trace event
field locations from boot to boot to keep tools from hard coding the
event layout. This may sound crazy, but developers out there are crazy.
And if you want to keep them from abusing interfaces, you just need to
be a bit more crazy than they are.
>
> To summarize and consolidate other threads:
> - I will remove reading of PERF_SAMPLE_RAW in tracex1 example.
> it's really orthogonal to this whole discussion.
Or yous libtraceevent ;-) We really need to finish that and package it
up for distros.
> - will add more comments through out that just because
> programs can read tracepoint arguments, they shouldn't
> make any assumptions that args stays as-is from version to version
We may need to find a way to actually keep it from being as is from
version to version even if the users do not change.
> - will work on a patch to demonstrate how few in-kernel
> structures can be user-ized and how programs can access
> them in version-indepedent way
It will be interesting to see what kernel structures can be user-ized
that are not already used by system calls.
>
> btw the concept of user-ized data structures already exists
> with classic bpf, since 'A = load -0x1000' is translated into
> 'A = skb->protocol'. I'm thinking of something similar
> but more generic and less obscure.
I have to try to wrap my head around understanding the classic bpf, and
how "load -0x1000" translates to "skb->protocol". Is that documented
somewhere?
-- Steve
^ permalink raw reply
* Re: [PATCH RFC v5 net-next 1/6] virtio_ring: fix virtqueue_enable_cb() when only 1 buffers were pending
From: Rusty Russell @ 2015-02-10 23:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: pagupta, netdev, linux-kernel, virtualization
In-Reply-To: <20150210101839.GA9505@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Tue, Feb 10, 2015 at 11:33:52AM +1030, Rusty Russell wrote:
>> Jason Wang <jasowang@redhat.com> writes:
>> > We currently does:
>> >
>> > bufs = (avail->idx - last_used_idx) * 3 / 4;
>> >
>> > This is ok now since we only try to enable the delayed callbacks when
>> > the queue is about to be full. This may not work well when there is
>> > only one pending buffer in the virtqueue (this may be the case after
>> > tx interrupt was enabled). Since virtqueue_enable_cb() will return
>> > false which may cause unnecessary triggering of napis. This patch
>> > correct this by only calculate the four thirds when bufs is not one.
>>
>> I mildly prefer to avoid the branch, by changing the calculation like
>> so:
>>
>> /* Set bufs >= 1, even if there's only one pending buffer */
>> bufs = (bufs + 1) * 3 / 4;
>
> Or bus * 3/4 + 1
>
>> But it's not clear to me how much this happens. I'm happy with the
>> patch though, as currently virtqueue_enable_cb_delayed() is the same
>> as virtqueue_enable_cb() if there's only been one buffer added.
>>
>> Cheers,
>> Rusty.
>
> But isn't this by design?
> The documentation says:
>
> * This re-enables callbacks but hints to the other side to delay
> * interrupts until most of the available buffers have been processed;
>
> Clearly, this implies that when there's one buffer it must behave
> exactly the same.
Yes, my mistake. We could hit the "never gets notified" case with this
change, and that's a much bigger problem.
So I don't think we can accept this patch...
Rusty.
^ permalink raw reply
* Re: [PATCH 3/5] virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined.
From: Rusty Russell @ 2015-02-11 0:06 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: lkml, netdev@vger.kernel.org
In-Reply-To: <87fvag3qv3.fsf@rustcorp.com.au>
Rusty Russell <rusty@rustcorp.com.au> writes:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> On Fri, Feb 06, 2015 at 03:36:54PM +1030, Rusty Russell wrote:
>>> In particular, the virtio header always has the u16 num_buffers field.
>>> We define a new 'struct virtio_net_modern_hdr' for this (rather than
>>> simply calling it 'struct virtio_net_hdr', to avoid nasty type errors
>>> if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't.
>>>
>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>>> ---
>>> include/uapi/linux/virtio_net.h | 30 ++++++++++++++++++++++++++++--
>>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>> index b5f1677b291c..32754f3000e8 100644
>>> --- a/include/uapi/linux/virtio_net.h
>>> +++ b/include/uapi/linux/virtio_net.h
>>> @@ -35,7 +35,6 @@
>>> #define VIRTIO_NET_F_CSUM 0 /* Host handles pkts w/ partial csum */
>>> #define VIRTIO_NET_F_GUEST_CSUM 1 /* Guest handles pkts w/ partial csum */
>>> #define VIRTIO_NET_F_MAC 5 /* Host has given MAC address. */
>>> -#define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
>>> #define VIRTIO_NET_F_GUEST_TSO4 7 /* Guest can handle TSOv4 in. */
>>> #define VIRTIO_NET_F_GUEST_TSO6 8 /* Guest can handle TSOv6 in. */
>>> #define VIRTIO_NET_F_GUEST_ECN 9 /* Guest can handle TSO[6] w/ ECN in. */
>>> @@ -56,6 +55,10 @@
>>> * Steering */
>>> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>>>
>>> +#ifndef VIRTIO_NET_NO_LEGACY
>>> +#define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
>>> +#endif /* VIRTIO_NET_NO_LEGACY */
>>> +
>>> #define VIRTIO_NET_S_LINK_UP 1 /* Link is up */
>>> #define VIRTIO_NET_S_ANNOUNCE 2 /* Announcement is needed */
>>>
>>> @@ -71,8 +74,9 @@ struct virtio_net_config {
>>> __u16 max_virtqueue_pairs;
>>> } __attribute__((packed));
>>>
>>> +#ifndef VIRTIO_NET_NO_LEGACY
>>> /* This header comes first in the scatter-gather list.
>>> - * If VIRTIO_F_ANY_LAYOUT is not negotiated, it must
>>> + * For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated, it must
>>> * be the first element of the scatter-gather list. If you don't
>>> * specify GSO or CSUM features, you can simply ignore the header. */
>>> struct virtio_net_hdr {
>>> @@ -97,6 +101,28 @@ struct virtio_net_hdr_mrg_rxbuf {
>>> struct virtio_net_hdr hdr;
>>> __virtio16 num_buffers; /* Number of merged rx buffers */
>>> };
>>> +#else /* ... VIRTIO_NET_NO_LEGACY */
>>> +/*
>>> + * This header comes first in the scatter-gather list. If you don't
>>> + * specify GSO or CSUM features, you can simply ignore the header.
>>> + */
>>> +struct virtio_net_modern_hdr {
>>> +#define VIRTIO_NET_HDR_F_NEEDS_CSUM 1 /* Use csum_start, csum_offset */
>>> +#define VIRTIO_NET_HDR_F_DATA_VALID 2 /* Csum is valid */
>>> + __u8 flags;
>>> +#define VIRTIO_NET_HDR_GSO_NONE 0 /* Not a GSO frame */
>>> +#define VIRTIO_NET_HDR_GSO_TCPV4 1 /* GSO frame, IPv4 TCP (TSO) */
>>> +#define VIRTIO_NET_HDR_GSO_UDP 3 /* GSO frame, IPv4 UDP (UFO) */
>>> +#define VIRTIO_NET_HDR_GSO_TCPV6 4 /* GSO frame, IPv6 TCP */
>>> +#define VIRTIO_NET_HDR_GSO_ECN 0x80 /* TCP has ECN set */
>>> + __u8 gso_type;
>>> + __virtio16 hdr_len; /* Ethernet + IP + tcp/udp hdrs */
>>> + __virtio16 gso_size; /* Bytes to append to hdr_len per frame */
>>> + __virtio16 csum_start; /* Position to start checksumming from */
>>> + __virtio16 csum_offset; /* Offset after that to place checksum */
>>> + __virtio16 num_buffers; /* Number of merged rx buffers */
>>> +};
>>> +#endif /* ...VIRTIO_NET_NO_LEGACY */
>>
>> This kind of masks the fact that it's the same as
>> virtio_net_hdr_mrg_rxbuf. So it's forcing people to duplicate
>> code for transitional devices.
>
> It's a hard problem to solve; someone suffers. My preference is to make
> non-legacy implementations as "clean" as possible, even if transitional
> devices suffer (since their suffering should be shorter-lived).
>
> Transitional devices can't define VIRTIO_NET_NO_LEGACY, so they'll
> have to use virtio_net_hdr_mrg_rxbuf anyway.
Here's what I ended up with. In effect, it's just the name change
and a comment that it's the same as the legacy virtio_net_hdr_mrg_rxbuf:
Subject: virtio: Don't expose legacy net features when VIRTIO_NET_NO_LEGACY defined.
In particular, the virtio header always has the u16 num_buffers field.
We define a new 'struct virtio_net_hdr_v1' for this (rather than
simply calling it 'struct virtio_net_hdr', to avoid nasty type errors
if some parts of a project define VIRTIO_NET_NO_LEGACY and some don't.
Transitional devices (which can't define VIRTIO_NET_NO_LEGACY) will
have to keep using struct virtio_net_hdr_mrg_rxbuf, which has the same
byte layout as struct virtio_net_hdr_v1.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index b5f1677b291c..4a9b58113d6e 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -35,7 +35,6 @@
#define VIRTIO_NET_F_CSUM 0 /* Host handles pkts w/ partial csum */
#define VIRTIO_NET_F_GUEST_CSUM 1 /* Guest handles pkts w/ partial csum */
#define VIRTIO_NET_F_MAC 5 /* Host has given MAC address. */
-#define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
#define VIRTIO_NET_F_GUEST_TSO4 7 /* Guest can handle TSOv4 in. */
#define VIRTIO_NET_F_GUEST_TSO6 8 /* Guest can handle TSOv6 in. */
#define VIRTIO_NET_F_GUEST_ECN 9 /* Guest can handle TSO[6] w/ ECN in. */
@@ -56,6 +55,10 @@
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#ifndef VIRTIO_NET_NO_LEGACY
+#define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */
+#endif /* VIRTIO_NET_NO_LEGACY */
+
#define VIRTIO_NET_S_LINK_UP 1 /* Link is up */
#define VIRTIO_NET_S_ANNOUNCE 2 /* Announcement is needed */
@@ -71,8 +74,9 @@ struct virtio_net_config {
__u16 max_virtqueue_pairs;
} __attribute__((packed));
+#ifndef VIRTIO_NET_NO_LEGACY
/* This header comes first in the scatter-gather list.
- * If VIRTIO_F_ANY_LAYOUT is not negotiated, it must
+ * For legacy virtio, if VIRTIO_F_ANY_LAYOUT is not negotiated, it must
* be the first element of the scatter-gather list. If you don't
* specify GSO or CSUM features, you can simply ignore the header. */
struct virtio_net_hdr {
@@ -97,6 +101,30 @@ struct virtio_net_hdr_mrg_rxbuf {
struct virtio_net_hdr hdr;
__virtio16 num_buffers; /* Number of merged rx buffers */
};
+#else /* ... VIRTIO_NET_NO_LEGACY */
+/*
+ * This header comes first in the scatter-gather list. If you don't
+ * specify GSO or CSUM features, you can simply ignore the header.
+ *
+ * This is bitwise-equivalent to the legacy struct virtio_net_hdr_mrg_rxbuf.
+ */
+struct virtio_net_hdr_v1 {
+#define VIRTIO_NET_HDR_F_NEEDS_CSUM 1 /* Use csum_start, csum_offset */
+#define VIRTIO_NET_HDR_F_DATA_VALID 2 /* Csum is valid */
+ __u8 flags;
+#define VIRTIO_NET_HDR_GSO_NONE 0 /* Not a GSO frame */
+#define VIRTIO_NET_HDR_GSO_TCPV4 1 /* GSO frame, IPv4 TCP (TSO) */
+#define VIRTIO_NET_HDR_GSO_UDP 3 /* GSO frame, IPv4 UDP (UFO) */
+#define VIRTIO_NET_HDR_GSO_TCPV6 4 /* GSO frame, IPv6 TCP */
+#define VIRTIO_NET_HDR_GSO_ECN 0x80 /* TCP has ECN set */
+ __u8 gso_type;
+ __virtio16 hdr_len; /* Ethernet + IP + tcp/udp hdrs */
+ __virtio16 gso_size; /* Bytes to append to hdr_len per frame */
+ __virtio16 csum_start; /* Position to start checksumming from */
+ __virtio16 csum_offset; /* Offset after that to place checksum */
+ __virtio16 num_buffers; /* Number of merged rx buffers */
+};
+#endif /* ...VIRTIO_NET_NO_LEGACY */
/*
* Control virtqueue data structures
^ 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