* [PATCH v2 1/2] bpf: Allow bpf_skb_event_output for a few prog types
From: allanzhang @ 2019-06-24 23:53 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
Cc: allanzhang
In-Reply-To: <20190624235334.163625-1-allanzhang@google.com>
Software event output is only enabled by a few prog types right now (TC,
LWT out, XDP, sockops). Many other skb based prog types need
bpf_skb_event_output to produce software event.
Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
Test bpf code is generated from code snippet:
struct TMP {
uint64_t tmp;
} tt;
tt.tmp = 5;
bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
&tt, sizeof(tt));
return 1;
the bpf assembly from llvm is:
0: b7 02 00 00 05 00 00 00 r2 = 5
1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
2: bf a4 00 00 00 00 00 00 r4 = r10
3: 07 04 00 00 f8 ff ff ff r4 += -8
4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
6: b7 03 00 00 00 00 00 00 r3 = 0
7: b7 05 00 00 08 00 00 00 r5 = 8
8: 85 00 00 00 19 00 00 00 call 25
9: b7 00 00 00 01 00 00 00 r0 = 1
10: 95 00 00 00 00 00 00 00 exit
Signed-off-by: allanzhang <allanzhang@google.com>
---
net/core/filter.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/core/filter.c b/net/core/filter.c
index 2014d76e0d2a..b75fcf412628 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5958,6 +5958,8 @@ sk_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_get_socket_cookie_proto;
case BPF_FUNC_get_socket_uid:
return &bpf_get_socket_uid_proto;
+ case BPF_FUNC_perf_event_output:
+ return &bpf_skb_event_output_proto;
default:
return bpf_base_func_proto(func_id);
}
@@ -5978,6 +5980,8 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_storage_get_proto;
case BPF_FUNC_sk_storage_delete:
return &bpf_sk_storage_delete_proto;
+ case BPF_FUNC_perf_event_output:
+ return &bpf_skb_event_output_proto;
#ifdef CONFIG_SOCK_CGROUP_DATA
case BPF_FUNC_skb_cgroup_id:
return &bpf_skb_cgroup_id_proto;
@@ -6226,6 +6230,8 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_redirect_map_proto;
case BPF_FUNC_sk_redirect_hash:
return &bpf_sk_redirect_hash_proto;
+ case BPF_FUNC_perf_event_output:
+ return &bpf_skb_event_output_proto;
#ifdef CONFIG_INET
case BPF_FUNC_sk_lookup_tcp:
return &bpf_sk_lookup_tcp_proto;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v2 0/2] bpf: Allow bpf_skb_event_output for a few prog types
From: allanzhang @ 2019-06-24 23:53 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
Cc: allanzhang
Software event output is only enabled by a few prog types right now (TC,
LWT out, XDP, sockops). Many other skb based prog types need
bpf_skb_event_output to produce software event.
Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
*** BLURB HERE ***
allanzhang (2):
bpf: Allow bpf_skb_event_output for a few prog types
bpf: Add selftests for bpf_perf_event_output
net/core/filter.c | 6 ++
tools/testing/selftests/bpf/test_verifier.c | 33 ++++++-
.../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
3 files changed, 132 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH] sis900: remove TxIDLE
From: Willem de Bruijn @ 2019-06-24 23:44 UTC (permalink / raw)
To: Sergej Benilov; +Cc: venza, Network Development
In-Reply-To: <20190624212102.15844-1-sergej.benilov@googlemail.com>
On Mon, Jun 24, 2019 at 6:04 PM Sergej Benilov
<sergej.benilov@googlemail.com> wrote:
>
> Before "sis900: fix TX completion" patch, TX completion was done on TxIDLE interrupt.
> TX completion also was the only thing done on TxIDLE interrupt.
> Since "sis900: fix TX completion", TX completion is done on TxDESC interrupt.
> So it is not necessary any more to set and to check for TxIDLE.
>
> Eliminate TxIDLE from sis900.
> Correct some typos, too.
>
> Signed-off-by: Sergej Benilov <sergej.benilov@googlemail.com>
LGTM
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Alexei Starovoitov @ 2019-06-24 23:38 UTC (permalink / raw)
To: Jakub Kicinski, Andrey Ignatov
Cc: Daniel Borkmann, Takshak Chahande, netdev@vger.kernel.org,
ast@kernel.org, Kernel Team, Stanislav Fomichev
In-Reply-To: <20190624154309.5ef3357b@cakuba.netronome.com>
On 6/24/19 3:43 PM, Jakub Kicinski wrote:
> On Mon, 24 Jun 2019 22:16:02 +0000, Andrey Ignatov wrote:
>> Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2019-06-24 14:51 -0700]:
>>> This is a cgroup-specific flag, right? It should be a parameter
>>> to cgroup show, not a global flag. Can we please drop this patch
>>> from the tree?
>>
>> Hey Jakub,
>>
>> I had same thought about cgroup-specific flag while reviewing the patch,
>> but then found out that all flags in bpftool are now global, no mater if
>> they're sub-command-specific or not.
>>
>> For example, --mapcompat is used only in prog-subcommand, but the option
>> is global; --bpffs is used in prog- and map-subcommands, but the option
>> is global as well, etc (there are more examples).
>
> I don't think these are equivalent. BPF_F_QUERY_EFFECTIVE is a flag
> for a syscall corresponding to a subcommand quite clearly.
>
>> I agree that limiting the scope of an option is a good idea in the long
>> term and it'd be great to rework all existing options to be available
>> only for corresponding sub-commands, but I don't see how the new `-e`
>> options is different from existing options and why it should be dropped.
>
> Agreed, TBH, but we can't change existing options, people may be using
> them. Let's drop the patch and make sure we're not making this mistake
> again :)
I don't think this patch should be penalized.
I'd rather see we fix them all.
^ permalink raw reply
* Re: [RFC PATCH 2/6] bpf: add BPF_MAP_DUMP command to access more than one entry per call
From: Brian Vazquez @ 2019-06-24 23:35 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Brian Vazquez, Alexei Starovoitov, Daniel Borkmann,
David S . Miller, Stanislav Fomichev, Willem de Bruijn,
Petar Penkov, linux-kernel, netdev, bpf
In-Reply-To: <20190624154558.65c31561@cakuba.netronome.com>
On Mon, Jun 24, 2019 at 3:46 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 21 Jun 2019 16:16:46 -0700, Brian Vazquez wrote:
> > @@ -385,6 +386,14 @@ union bpf_attr {
> > __u64 flags;
> > };
> >
> > + struct { /* struct used by BPF_MAP_DUMP command */
> > + __u32 map_fd;
>
> There is a hole here, perhaps flags don't have to be 64 bit?
The command implementation is wrapping BPF_MAP_*_ELEM commands, I
would expect this one to handle the same flags which are 64 bit.
Note that there's a hole in the anonymous structure used by the other
commands too:
struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
__u32 map_fd;
__aligned_u64 key;
union {
__aligned_u64 value;
__aligned_u64 next_key;
};
__u64 flags;
};
^ permalink raw reply
* Re: [PATCH net-next v5 2/7] etf: Add skip_sock_check
From: Patel, Vedang @ 2019-06-24 23:34 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev@vger.kernel.org, Kirsher, Jeffrey T, David Miller,
Jamal Hadi Salim, Cong Wang, Jiri Pirko,
intel-wired-lan@lists.osuosl.org, Gomes, Vinicius, l@dorileo.org,
m-karicheri2@ti.com, sergei.shtylyov@cogentembedded.com,
eric.dumazet@gmail.com, Brown, Aaron F
In-Reply-To: <20190624144013.3168dde2@cakuba.netronome.com>
> On Jun 24, 2019, at 2:40 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
>
> On Fri, 21 Jun 2019 10:28:23 -0700, Vedang Patel wrote:
>> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
>> index 8b2f993cbb77..409d1616472d 100644
>> --- a/include/uapi/linux/pkt_sched.h
>> +++ b/include/uapi/linux/pkt_sched.h
>> @@ -990,6 +990,7 @@ struct tc_etf_qopt {
>> __u32 flags;
>> #define TC_ETF_DEADLINE_MODE_ON BIT(0)
>> #define TC_ETF_OFFLOAD_ON BIT(1)
>> +#define TC_ETF_SKIP_SOCK_CHECK BIT(2)
>> };
>>
>> enum {
>
> I think build bot complained about the code not building on 32bit.
> When you respin could you include a patch to remove the uses of BIT()
> in UAPI? See: https://www.spinics.net/lists/netdev/msg579344.html
Thanks for the info Jakub. Yeah I will include it in the next version.
-Vedang
^ permalink raw reply
* Re: [PATCH] samples: bpf: make the use of xdp samples consistent
From: Song Liu @ 2019-06-24 23:25 UTC (permalink / raw)
To: Daniel T. Lee; +Cc: Daniel Borkmann, Alexei Starovoitov, Networking
In-Reply-To: <20190624112009.20048-1-danieltimlee@gmail.com>
On Mon, Jun 24, 2019 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> Currently, each xdp samples are inconsistent in the use.
> Most of the samples fetch the interface with it's name.
> (ex. xdp1, xdp2skb, xdp_redirect, xdp_sample_pkts, etc.)
>
> But only xdp_adjst_tail and xdp_tx_iptunnel fetch the interface with
> ifindex by command argument.
>
> This commit enables those two samples to fetch interface with it's name
> without changing the original index interface fetching.
> (<ifname|ifindex> fetching in the same way as xdp_sample_pkts_user.c does.)
>
> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
From patchworks, I assume you will send v2.
Please also CC bpf@vger.kernel.org in v2.
Thanks,
Song
> ---
> samples/bpf/xdp_adjust_tail_user.c | 12 ++++++++++--
> samples/bpf/xdp_tx_iptunnel_user.c | 12 ++++++++++--
> 2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/samples/bpf/xdp_adjust_tail_user.c b/samples/bpf/xdp_adjust_tail_user.c
> index 586ff751aba9..a3596b617c4c 100644
> --- a/samples/bpf/xdp_adjust_tail_user.c
> +++ b/samples/bpf/xdp_adjust_tail_user.c
> @@ -13,6 +13,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <net/if.h>
> #include <sys/resource.h>
> #include <arpa/inet.h>
> #include <netinet/ether.h>
> @@ -69,7 +70,7 @@ static void usage(const char *cmd)
> printf("Start a XDP prog which send ICMP \"packet too big\" \n"
> "messages if ingress packet is bigger then MAX_SIZE bytes\n");
> printf("Usage: %s [...]\n", cmd);
> - printf(" -i <ifindex> Interface Index\n");
> + printf(" -i <ifname|ifindex> Interface\n");
> printf(" -T <stop-after-X-seconds> Default: 0 (forever)\n");
> printf(" -S use skb-mode\n");
> printf(" -N enforce native mode\n");
> @@ -102,7 +103,9 @@ int main(int argc, char **argv)
>
> switch (opt) {
> case 'i':
> - ifindex = atoi(optarg);
> + ifindex = if_nametoindex(optarg);
> + if (!ifindex)
> + ifindex = atoi(optarg);
> break;
> case 'T':
> kill_after_s = atoi(optarg);
> @@ -136,6 +139,11 @@ int main(int argc, char **argv)
> return 1;
> }
>
> + if (!ifindex) {
> + fprintf(stderr, "Invalid ifname\n");
> + return 1;
> + }
> +
> snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> prog_load_attr.file = filename;
>
> diff --git a/samples/bpf/xdp_tx_iptunnel_user.c b/samples/bpf/xdp_tx_iptunnel_user.c
> index 394896430712..dfb68582e243 100644
> --- a/samples/bpf/xdp_tx_iptunnel_user.c
> +++ b/samples/bpf/xdp_tx_iptunnel_user.c
> @@ -9,6 +9,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <net/if.h>
> #include <sys/resource.h>
> #include <arpa/inet.h>
> #include <netinet/ether.h>
> @@ -83,7 +84,7 @@ static void usage(const char *cmd)
> "in an IPv4/v6 header and XDP_TX it out. The dst <VIP:PORT>\n"
> "is used to select packets to encapsulate\n\n");
> printf("Usage: %s [...]\n", cmd);
> - printf(" -i <ifindex> Interface Index\n");
> + printf(" -i <ifname|ifindex> Interface\n");
> printf(" -a <vip-service-address> IPv4 or IPv6\n");
> printf(" -p <vip-service-port> A port range (e.g. 433-444) is also allowed\n");
> printf(" -s <source-ip> Used in the IPTunnel header\n");
> @@ -181,7 +182,9 @@ int main(int argc, char **argv)
>
> switch (opt) {
> case 'i':
> - ifindex = atoi(optarg);
> + ifindex = if_nametoindex(optarg);
> + if (!ifindex)
> + ifindex = atoi(optarg);
> break;
> case 'a':
> vip.family = parse_ipstr(optarg, vip.daddr.v6);
> @@ -253,6 +256,11 @@ int main(int argc, char **argv)
> return 1;
> }
>
> + if (!ifindex) {
> + fprintf(stderr, "Invalid ifname\n");
> + return 1;
> + }
> +
> snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> prog_load_attr.file = filename;
>
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH v2] samples: bpf: make the use of xdp samples consistent
From: Daniel T. Lee @ 2019-06-24 23:20 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev
Currently, each xdp samples are inconsistent in the use.
Most of the samples fetch the interface with it's name.
(ex. xdp1, xdp2skb, xdp_redirect_cpu, xdp_sample_pkts, etc.)
But some of the xdp samples are fetching the interface with
ifindex by command argument.
This commit enables xdp samples to fetch interface with it's name
without changing the original index interface fetching.
(<ifname|ifindex> fetching in the same way as xdp_sample_pkts_user.c does.)
Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
Changes in v2:
- added xdp_redirect_user.c, xdp_redirect_map_user.c
samples/bpf/xdp_adjust_tail_user.c | 12 ++++++++++--
samples/bpf/xdp_redirect_map_user.c | 15 +++++++++++----
samples/bpf/xdp_redirect_user.c | 15 +++++++++++----
samples/bpf/xdp_tx_iptunnel_user.c | 12 ++++++++++--
4 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/samples/bpf/xdp_adjust_tail_user.c b/samples/bpf/xdp_adjust_tail_user.c
index 586ff751aba9..a3596b617c4c 100644
--- a/samples/bpf/xdp_adjust_tail_user.c
+++ b/samples/bpf/xdp_adjust_tail_user.c
@@ -13,6 +13,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <net/if.h>
#include <sys/resource.h>
#include <arpa/inet.h>
#include <netinet/ether.h>
@@ -69,7 +70,7 @@ static void usage(const char *cmd)
printf("Start a XDP prog which send ICMP \"packet too big\" \n"
"messages if ingress packet is bigger then MAX_SIZE bytes\n");
printf("Usage: %s [...]\n", cmd);
- printf(" -i <ifindex> Interface Index\n");
+ printf(" -i <ifname|ifindex> Interface\n");
printf(" -T <stop-after-X-seconds> Default: 0 (forever)\n");
printf(" -S use skb-mode\n");
printf(" -N enforce native mode\n");
@@ -102,7 +103,9 @@ int main(int argc, char **argv)
switch (opt) {
case 'i':
- ifindex = atoi(optarg);
+ ifindex = if_nametoindex(optarg);
+ if (!ifindex)
+ ifindex = atoi(optarg);
break;
case 'T':
kill_after_s = atoi(optarg);
@@ -136,6 +139,11 @@ int main(int argc, char **argv)
return 1;
}
+ if (!ifindex) {
+ fprintf(stderr, "Invalid ifname\n");
+ return 1;
+ }
+
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
prog_load_attr.file = filename;
diff --git a/samples/bpf/xdp_redirect_map_user.c b/samples/bpf/xdp_redirect_map_user.c
index 15bb6f67f9c3..f70ee33907fd 100644
--- a/samples/bpf/xdp_redirect_map_user.c
+++ b/samples/bpf/xdp_redirect_map_user.c
@@ -10,6 +10,7 @@
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
+#include <net/if.h>
#include <unistd.h>
#include <libgen.h>
#include <sys/resource.h>
@@ -85,7 +86,7 @@ static void poll_stats(int interval, int ifindex)
static void usage(const char *prog)
{
fprintf(stderr,
- "usage: %s [OPTS] IFINDEX_IN IFINDEX_OUT\n\n"
+ "usage: %s [OPTS] <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n\n"
"OPTS:\n"
" -S use skb-mode\n"
" -N enforce native mode\n"
@@ -127,7 +128,7 @@ int main(int argc, char **argv)
}
if (optind == argc) {
- printf("usage: %s IFINDEX_IN IFINDEX_OUT\n", argv[0]);
+ printf("usage: %s <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n", argv[0]);
return 1;
}
@@ -136,8 +137,14 @@ int main(int argc, char **argv)
return 1;
}
- ifindex_in = strtoul(argv[optind], NULL, 0);
- ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+ ifindex_in = if_nametoindex(argv[optind]);
+ if (!ifindex_in)
+ ifindex_in = strtoul(argv[optind], NULL, 0);
+
+ ifindex_out = if_nametoindex(argv[optind + 1]);
+ if (!ifindex_out)
+ ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+
printf("input: %d output: %d\n", ifindex_in, ifindex_out);
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
diff --git a/samples/bpf/xdp_redirect_user.c b/samples/bpf/xdp_redirect_user.c
index ce71be187205..39de06f3ec25 100644
--- a/samples/bpf/xdp_redirect_user.c
+++ b/samples/bpf/xdp_redirect_user.c
@@ -10,6 +10,7 @@
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
+#include <net/if.h>
#include <unistd.h>
#include <libgen.h>
#include <sys/resource.h>
@@ -85,7 +86,7 @@ static void poll_stats(int interval, int ifindex)
static void usage(const char *prog)
{
fprintf(stderr,
- "usage: %s [OPTS] IFINDEX_IN IFINDEX_OUT\n\n"
+ "usage: %s [OPTS] <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n\n"
"OPTS:\n"
" -S use skb-mode\n"
" -N enforce native mode\n"
@@ -128,7 +129,7 @@ int main(int argc, char **argv)
}
if (optind == argc) {
- printf("usage: %s IFINDEX_IN IFINDEX_OUT\n", argv[0]);
+ printf("usage: %s <IFNAME|IFINDEX>_IN <IFNAME|IFINDEX>_OUT\n", argv[0]);
return 1;
}
@@ -137,8 +138,14 @@ int main(int argc, char **argv)
return 1;
}
- ifindex_in = strtoul(argv[optind], NULL, 0);
- ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+ ifindex_in = if_nametoindex(argv[optind]);
+ if (!ifindex_in)
+ ifindex_in = strtoul(argv[optind], NULL, 0);
+
+ ifindex_out = if_nametoindex(argv[optind + 1]);
+ if (!ifindex_out)
+ ifindex_out = strtoul(argv[optind + 1], NULL, 0);
+
printf("input: %d output: %d\n", ifindex_in, ifindex_out);
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
diff --git a/samples/bpf/xdp_tx_iptunnel_user.c b/samples/bpf/xdp_tx_iptunnel_user.c
index 394896430712..dfb68582e243 100644
--- a/samples/bpf/xdp_tx_iptunnel_user.c
+++ b/samples/bpf/xdp_tx_iptunnel_user.c
@@ -9,6 +9,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <net/if.h>
#include <sys/resource.h>
#include <arpa/inet.h>
#include <netinet/ether.h>
@@ -83,7 +84,7 @@ static void usage(const char *cmd)
"in an IPv4/v6 header and XDP_TX it out. The dst <VIP:PORT>\n"
"is used to select packets to encapsulate\n\n");
printf("Usage: %s [...]\n", cmd);
- printf(" -i <ifindex> Interface Index\n");
+ printf(" -i <ifname|ifindex> Interface\n");
printf(" -a <vip-service-address> IPv4 or IPv6\n");
printf(" -p <vip-service-port> A port range (e.g. 433-444) is also allowed\n");
printf(" -s <source-ip> Used in the IPTunnel header\n");
@@ -181,7 +182,9 @@ int main(int argc, char **argv)
switch (opt) {
case 'i':
- ifindex = atoi(optarg);
+ ifindex = if_nametoindex(optarg);
+ if (!ifindex)
+ ifindex = atoi(optarg);
break;
case 'a':
vip.family = parse_ipstr(optarg, vip.daddr.v6);
@@ -253,6 +256,11 @@ int main(int argc, char **argv)
return 1;
}
+ if (!ifindex) {
+ fprintf(stderr, "Invalid ifname\n");
+ return 1;
+ }
+
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
prog_load_attr.file = filename;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] bpf: Allow bpf_skb_event_output for a few prog types
From: Song Liu @ 2019-06-24 23:12 UTC (permalink / raw)
To: allanzhang
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, Networking, bpf, open list
In-Reply-To: <CAPhsuW40c=CTdTo9YUbyj3AAL+A37TX1-Bty267bCYOaThJJ7w@mail.gmail.com>
On Mon, Jun 24, 2019 at 4:10 PM Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Mon, Jun 24, 2019 at 3:08 PM allanzhang <allanzhang@google.com> wrote:
> >
> > Software event output is only enabled by a few prog types right now (TC,
> > LWT out, XDP, sockops). Many other skb based prog types need
> > bpf_skb_event_output to produce software event.
> >
> > Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
> >
> > Test bpf code is generated from code snippet:
> >
> > struct TMP {
> > uint64_t tmp;
> > } tt;
> > tt.tmp = 5;
> > bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
> > &tt, sizeof(tt));
> > return 1;
> >
> > the bpf assembly from llvm is:
> > 0: b7 02 00 00 05 00 00 00 r2 = 5
> > 1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
> > 2: bf a4 00 00 00 00 00 00 r4 = r10
> > 3: 07 04 00 00 f8 ff ff ff r4 += -8
> > 4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
> > 6: b7 03 00 00 00 00 00 00 r3 = 0
> > 7: b7 05 00 00 08 00 00 00 r5 = 8
> > 8: 85 00 00 00 19 00 00 00 call 25
> > 9: b7 00 00 00 01 00 00 00 r0 = 1
> > 10: 95 00 00 00 00 00 00 00 exit
> >
> > Patch 1 is enabling code.
> > Patch 2 is fullly covered selftest code.
> >
> > Signed-off-by: allanzhang <allanzhang@google.com>
>
> A few logistics issues:
>
> 1. The patch should be sent as a set, as
> [PATCH bpf-next 0/2] ...
> [PATCH bpf-next 1/2] ...
> [PATCH bpf-next 2/2] ...
>
> 2. You need to specify which tree this is targeting. In this case, bpf-next.
> 3. Please use different commit log for each patch.
> 4. No need for Signed-off-by in the cover letter.
>
> Please resubmit. And generate the patches with git command similar to
> the following:
>
> git format-patch --cover-leter --subject_prefix "PATCH bpf-next v2" HEAD~2
>
And your signed-of-by should probably look like:
Signed-off-by: Allan Zhang <allanzhang@google.com>
Thanks,
Song
^ permalink raw reply
* Re: [PATCH] bpf: Allow bpf_skb_event_output for a few prog types
From: Song Liu @ 2019-06-24 23:10 UTC (permalink / raw)
To: allanzhang
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, Networking, bpf, open list
In-Reply-To: <20190624215824.118783-1-allanzhang@google.com>
On Mon, Jun 24, 2019 at 3:08 PM allanzhang <allanzhang@google.com> wrote:
>
> Software event output is only enabled by a few prog types right now (TC,
> LWT out, XDP, sockops). Many other skb based prog types need
> bpf_skb_event_output to produce software event.
>
> Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
>
> Test bpf code is generated from code snippet:
>
> struct TMP {
> uint64_t tmp;
> } tt;
> tt.tmp = 5;
> bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
> &tt, sizeof(tt));
> return 1;
>
> the bpf assembly from llvm is:
> 0: b7 02 00 00 05 00 00 00 r2 = 5
> 1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
> 2: bf a4 00 00 00 00 00 00 r4 = r10
> 3: 07 04 00 00 f8 ff ff ff r4 += -8
> 4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
> 6: b7 03 00 00 00 00 00 00 r3 = 0
> 7: b7 05 00 00 08 00 00 00 r5 = 8
> 8: 85 00 00 00 19 00 00 00 call 25
> 9: b7 00 00 00 01 00 00 00 r0 = 1
> 10: 95 00 00 00 00 00 00 00 exit
>
> Patch 1 is enabling code.
> Patch 2 is fullly covered selftest code.
>
> Signed-off-by: allanzhang <allanzhang@google.com>
A few logistics issues:
1. The patch should be sent as a set, as
[PATCH bpf-next 0/2] ...
[PATCH bpf-next 1/2] ...
[PATCH bpf-next 2/2] ...
2. You need to specify which tree this is targeting. In this case, bpf-next.
3. Please use different commit log for each patch.
4. No need for Signed-off-by in the cover letter.
Please resubmit. And generate the patches with git command similar to
the following:
git format-patch --cover-leter --subject_prefix "PATCH bpf-next v2" HEAD~2
Thanks,
Song
^ permalink raw reply
* Re: [PATCH] samples: bpf: make the use of xdp samples consistent
From: Daniel T. Lee @ 2019-06-24 22:48 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Daniel Borkmann, Alexei Starovoitov, netdev
In-Reply-To: <871rzi4zax.fsf@toke.dk>
Will do right away! :)
On Tue, Jun 25, 2019 at 3:24 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> "Daniel T. Lee" <danieltimlee@gmail.com> writes:
>
> > Currently, each xdp samples are inconsistent in the use.
> > Most of the samples fetch the interface with it's name.
> > (ex. xdp1, xdp2skb, xdp_redirect, xdp_sample_pkts, etc.)
>
> The xdp_redirect and xdp_redirect_map also only accept ifindexes, not
> interface names. Care to fix those while you're at it? :)
>
> -Toke
^ permalink raw reply
* Re: [RFC PATCH 2/6] bpf: add BPF_MAP_DUMP command to access more than one entry per call
From: Jakub Kicinski @ 2019-06-24 22:45 UTC (permalink / raw)
To: Brian Vazquez
Cc: Brian Vazquez, Alexei Starovoitov, Daniel Borkmann,
David S . Miller, Stanislav Fomichev, Willem de Bruijn,
Petar Penkov, linux-kernel, netdev, bpf
In-Reply-To: <20190621231650.32073-3-brianvv@google.com>
On Fri, 21 Jun 2019 16:16:46 -0700, Brian Vazquez wrote:
> @@ -385,6 +386,14 @@ union bpf_attr {
> __u64 flags;
> };
>
> + struct { /* struct used by BPF_MAP_DUMP command */
> + __u32 map_fd;
There is a hole here, perhaps flags don't have to be 64 bit?
> + __aligned_u64 prev_key;
> + __aligned_u64 buf;
> + __aligned_u64 buf_len; /* input/output: len of buf */
> + __u64 flags;
> + } dump;
> +
> struct { /* anonymous struct used by BPF_PROG_LOAD command */
> __u32 prog_type; /* one of enum bpf_prog_type */
> __u32 insn_cnt;
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Jakub Kicinski @ 2019-06-24 22:43 UTC (permalink / raw)
To: Andrey Ignatov
Cc: Daniel Borkmann, Takshak Chahande, netdev@vger.kernel.org,
ast@kernel.org, Kernel Team, Stanislav Fomichev
In-Reply-To: <20190624221558.GA41600@rdna-mbp.dhcp.thefacebook.com>
On Mon, 24 Jun 2019 22:16:02 +0000, Andrey Ignatov wrote:
> Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2019-06-24 14:51 -0700]:
> > This is a cgroup-specific flag, right? It should be a parameter
> > to cgroup show, not a global flag. Can we please drop this patch
> > from the tree?
>
> Hey Jakub,
>
> I had same thought about cgroup-specific flag while reviewing the patch,
> but then found out that all flags in bpftool are now global, no mater if
> they're sub-command-specific or not.
>
> For example, --mapcompat is used only in prog-subcommand, but the option
> is global; --bpffs is used in prog- and map-subcommands, but the option
> is global as well, etc (there are more examples).
I don't think these are equivalent. BPF_F_QUERY_EFFECTIVE is a flag
for a syscall corresponding to a subcommand quite clearly.
> I agree that limiting the scope of an option is a good idea in the long
> term and it'd be great to rework all existing options to be available
> only for corresponding sub-commands, but I don't see how the new `-e`
> options is different from existing options and why it should be dropped.
Agreed, TBH, but we can't change existing options, people may be using
them. Let's drop the patch and make sure we're not making this mistake
again :)
Thanks!
^ permalink raw reply
* Re: [PATCH net-next 02/18] ionic: Add hardware init and device commands
From: Shannon Nelson @ 2019-06-24 22:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev
In-Reply-To: <20190624135304.48755745@cakuba.netronome.com>
On 6/24/19 1:53 PM, Jakub Kicinski wrote:
> On Thu, 20 Jun 2019 13:24:08 -0700, Shannon Nelson wrote:
>> The ionic device has a small set of PCI registers, including a
>> device control and data space, and a large set of message
>> commands.
>>
>> Signed-off-by: Shannon Nelson <snelson@pensando.io>
>> struct ionic {
>> struct pci_dev *pdev;
>> struct device *dev;
>> + struct ionic_dev idev;
>> + struct mutex dev_cmd_lock; /* lock for dev_cmd operations */
>> + struct dentry *dentry;
>> + struct ionic_dev_bar bars[IONIC_BARS_MAX];
>> + unsigned int num_bars;
>> + struct identity ident;
>> + bool is_mgmt_nic;
> What's a management NIC?
>
>> + ionic->is_mgmt_nic =
>> + ent->device == PCI_DEVICE_ID_PENSANDO_IONIC_ETH_MGMT;
> You spent time in the docs describing how to use lspci, yet this magic
> NIC is not mentioned :)
I'll see what I can do to add some detail in the ionic.rst, and maybe
add a couple hints in driver comments.
>
>> static struct pci_driver ionic_driver = {
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
>> new file mode 100644
>> index 000000000000..e5e45e6bec9d
>> --- /dev/null
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_debugfs.c
>> @@ -0,0 +1,239 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
>> +
>> +#include <linux/netdevice.h>
>> +
>> +#include "ionic.h"
>> +#include "ionic_bus.h"
>> +#include "ionic_debugfs.h"
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +
>> +static int blob_open(struct inode *inode, struct file *filp)
>> +{
>> + filp->private_data = inode->i_private;
>> + return 0;
>> +}
>> +
>> +static ssize_t blob_read(struct file *filp, char __user *buffer,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct debugfs_blob_wrapper *blob = filp->private_data;
>> +
>> + if (*ppos >= blob->size)
>> + return 0;
>> + if (*ppos + count > blob->size)
>> + count = blob->size - *ppos;
>> +
>> + if (copy_to_user(buffer, blob->data + *ppos, count))
>> + return -EFAULT;
>> +
>> + *ppos += count;
>> +
>> + return count;
>> +}
>> +
>> +static ssize_t blob_write(struct file *filp, const char __user *buffer,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct debugfs_blob_wrapper *blob = filp->private_data;
>> +
>> + if (*ppos >= blob->size)
>> + return 0;
>> + if (*ppos + count > blob->size)
>> + count = blob->size - *ppos;
>> +
>> + if (copy_from_user(blob->data + *ppos, buffer, count))
>> + return -EFAULT;
>> +
>> + *ppos += count;
>> +
>> + return count;
>> +}
> Why would you ever have to write to a debugfs blob? Red flag.
Yes, this obviously needs to be removed. I won't go into the history of
why this is here, suffice to say I didn't get everything cleaned out.
>
>> +static const struct file_operations blob_fops = {
>> + .owner = THIS_MODULE,
>> + .open = blob_open,
>> + .read = blob_read,
>> + .write = blob_write,
>> +};
>> +
>> +struct dentry *debugfs_create_blob(const char *name, umode_t mode,
>> + struct dentry *parent,
>> + struct debugfs_blob_wrapper *blob)
>> +{
>> + return debugfs_create_file(name, mode | 0200, parent, blob,
>> + &blob_fops);
>> +}
>> +
>> +static struct dentry *ionic_dir;
>> +
>> +#define single(name) \
>> +static int name##_open(struct inode *inode, struct file *f) \
>> +{ \
>> + return single_open(f, name##_show, inode->i_private); \
>> +} \
>> + \
>> +static const struct file_operations name##_fops = { \
>> + .owner = THIS_MODULE, \
>> + .open = name##_open, \
>> + .read = seq_read, \
>> + .llseek = seq_lseek, \
>> + .release = single_release, \
>> +}
> DEFINE_SHOW_ATTRIBUTE() and friends.
>
>> +static int bars_show(struct seq_file *seq, void *v)
>> +{
>> + struct ionic *ionic = seq->private;
>> + struct ionic_dev_bar *bars = ionic->bars;
>> + unsigned int i;
>> +
>> + for (i = 0; i < IONIC_BARS_MAX; i++)
>> + if (bars[i].vaddr)
>> + seq_printf(seq, "BAR%d: len 0x%lx vaddr %pK bus_addr %pad\n",
>> + i, bars[i].len, bars[i].vaddr,
>> + &bars[i].bus_addr);
> Why? What's the value of this print beyond what's already visible from
> PCI subsystem? :S
This made debugging easier for someone
>
>> +static inline u64 encode_txq_desc_cmd(u8 opcode, u8 flags,
>> + u8 nsge, u64 addr)
>> +{
>> + u64 cmd;
>> +
>> + cmd = (opcode & IONIC_TXQ_DESC_OPCODE_MASK) << IONIC_TXQ_DESC_OPCODE_SHIFT;
> IIRC you're not a fan of the FIELD_* macros, but let me suggest them
> again :)
They don't seem to be used in the drivers from a company I used to work
for, but that doesn't necessarily mean I'm not a fan of them. My only
problem with them is that this particular file is an API description
used by other OSs as well, so I'll have to see how easily we can adapt
them into the other platforms. I'd rather not have to duplicate all the
macro magic for other OSs, or have one version of this file for Linux
and a different version for other OSs. I suspect this may be the same
concern with that other company.
Yes, I fully understand this is not a great argument for upstream code,
but it is a practical matter I'm juggling.
That said, I will keep an eye out for where these can be used in the
rest of the driver.
>
>> + cmd |= (flags & IONIC_TXQ_DESC_FLAGS_MASK) << IONIC_TXQ_DESC_FLAGS_SHIFT;
>> + cmd |= (nsge & IONIC_TXQ_DESC_NSGE_MASK) << IONIC_TXQ_DESC_NSGE_SHIFT;
>> + cmd |= (addr & IONIC_TXQ_DESC_ADDR_MASK) << IONIC_TXQ_DESC_ADDR_SHIFT;
>> +
>> + return cmd;
>> +};
>> +
>> +static inline void decode_txq_desc_cmd(u64 cmd, u8 *opcode, u8 *flags,
>> + u8 *nsge, u64 *addr)
>> +{
>> + *opcode = (cmd >> IONIC_TXQ_DESC_OPCODE_SHIFT) & IONIC_TXQ_DESC_OPCODE_MASK;
>> + *flags = (cmd >> IONIC_TXQ_DESC_FLAGS_SHIFT) & IONIC_TXQ_DESC_FLAGS_MASK;
>> + *nsge = (cmd >> IONIC_TXQ_DESC_NSGE_SHIFT) & IONIC_TXQ_DESC_NSGE_MASK;
>> + *addr = (cmd >> IONIC_TXQ_DESC_ADDR_SHIFT) & IONIC_TXQ_DESC_ADDR_MASK;
>> +};
>> +
>> +#define IONIC_TX_MAX_SG_ELEMS 8
>> +#define IONIC_RX_MAX_SG_ELEMS 8
>> +/**
>> + * struct dev_setattr_cmd - Set Device attributes on the NIC
>> + * @opcode: Opcode
>> + * @attr: Attribute type (enum dev_attr)
>> + * @state: Device state (enum dev_state)
>> + * @name: The bus info, e.g. PCI slot-device-function, 0 terminated
> Interesting, why would this be of interest to the device?
It is useful in debugging the services inside the device.
>
>> + * @features: Device features
>> + */
>> +struct dev_setattr_cmd {
>> + u8 opcode;
>> + u8 attr;
>> + __le16 rsvd;
>> + union {
>> + u8 state;
>> + char name[IONIC_IFNAMSIZ];
>> + __le64 features;
>> + u8 rsvd2[60];
>> + };
>> +};
>> +/**
>> + * struct lif_getattr_comp - LIF get attr command completion
>> + * @status: The status of the command (enum status_code)
>> + * @comp_index: The index in the descriptor ring for which this
>> + * is the completion.
>> + * @state: lif state (enum lif_state)
>> + * @name: The netdev name string, 0 terminated
>> + * @mtu: Mtu
>> + * @mac: Station mac
>> + * @features: Features (enum eth_hw_features)
>> + * @color: Color bit
>> + */
>> +struct lif_getattr_comp {
>> + u8 status;
>> + u8 rsvd;
>> + __le16 comp_index;
>> + union {
>> + u8 state;
>> + //char name[IONIC_IFNAMSIZ];
> Hi!!
Oh crap. Yes, that goes away.
Thanks for the detailed review time on a rather large file.
sln
>
>> + __le32 mtu;
>> + u8 mac[6];
>> + __le64 features;
>> + u8 rsvd2[11];
>> + };
>> + u8 color;
>> +};
^ permalink raw reply
* Re: [EXT] [PATCH] bnx2x: Prevent ptp_task to be rescheduled indefinitely
From: Guilherme Piccoli @ 2019-06-24 22:27 UTC (permalink / raw)
To: Sudarsana Reddy Kalluru
Cc: GR-everest-linux-l2, netdev@vger.kernel.org, Ariel Elior,
jay.vosburgh@canonical.com
In-Reply-To: <CAHD1Q_xJrVeZXHCpBprErkUXrxFMJ-SPSZ-w1deENcOjcT3tZA@mail.gmail.com>
V2 submitted here: https://marc.info/?l=linux-netdev&m=156141504615972
Cheers,
Guilherme
^ permalink raw reply
* [PATCH V2] bnx2x: Prevent ptp_task to be rescheduled indefinitely
From: Guilherme G. Piccoli @ 2019-06-24 22:23 UTC (permalink / raw)
To: GR-everest-linux-l2, netdev, skalluru; +Cc: aelior, gpiccoli, jay.vosburgh
Currently bnx2x ptp worker tries to read a register with timestamp
information in case of TX packet timestamping and in case it fails,
the routine reschedules itself indefinitely. This was reported as a
kworker always at 100% of CPU usage, which was narrowed down to be
bnx2x ptp_task.
By following the ioctl handler, we could narrow down the problem to
an NTP tool (chrony) requesting HW timestamping from bnx2x NIC with
RX filter zeroed; this isn't reproducible for example with ptp4l
(from linuxptp) since this tool requests a supported RX filter.
It seems NIC FW timestamp mechanism cannot work well with
RX_FILTER_NONE - driver's PTP filter init routine skips a register
write to the adapter if there's not a supported filter request.
This patch addresses the problem of bnx2x ptp thread's everlasting
reschedule by retrying the register read 10 times; between the read
attempts the thread sleeps for an increasing amount of time starting
in 50ms to give FW some time to perform the timestamping. If it still
fails after all retries, we bail out in order to prevent an unbound
resource consumption from bnx2x.
The patch also adds an ethtool statistic for accounting the skipped
TX timestamp packets and it reduces the priority of timestamping
error messages to prevent log flooding. The code was tested using
both linuxptp and chrony.
Reported-and-tested-by: Przemyslaw Hausman <przemyslaw.hausman@canonical.com>
Suggested-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
---
Sudarsana, thanks for the suggestion! I've tried to follow an identical
approach from [0], but still the ptp thread was consuming a lot of CPU
due to the good amount of reschedules.
I decided then to use the loop approach with small increasing delays,
in order to respect the time FW takes eventually to complete timestamping.
Also, I've dropped the PTP "outstanding, etc" messages to debug-level,
they're quite flooding my log. Cheers!
[0] git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=9adebac37e7d
.../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 12 +++++--
.../ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 4 ++-
.../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 36 ++++++++++++++-----
.../net/ethernet/broadcom/bnx2x/bnx2x_stats.h | 3 ++
4 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 008ad0ca89ba..6751cd04e8d8 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -3857,9 +3857,17 @@ netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
if (!(bp->flags & TX_TIMESTAMPING_EN)) {
- BNX2X_ERR("Tx timestamping was not enabled, this packet will not be timestamped\n");
+ bp->eth_stats.ptp_skip_tx_ts++;
+ netdev_err_once(bp->dev,
+ "Tx timestamping isn't enabled, this packet won't be timestamped\n");
+ DP(BNX2X_MSG_PTP,
+ "Tx timestamping isn't enabled, this packet won't be timestamped\n");
} else if (bp->ptp_tx_skb) {
- BNX2X_ERR("The device supports only a single outstanding packet to timestamp, this packet will not be timestamped\n");
+ bp->eth_stats.ptp_skip_tx_ts++;
+ netdev_err_once(bp->dev,
+ "Device supports only a single outstanding packet to timestamp, this packet won't be timestamped\n");
+ DP(BNX2X_MSG_PTP,
+ "Device supports only a single outstanding packet to timestamp, this packet won't be timestamped\n");
} else {
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
/* schedule check for Tx timestamp */
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index 51fc845de31a..4a0ba6801c9e 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -182,7 +182,9 @@ static const struct {
{ STATS_OFFSET32(driver_filtered_tx_pkt),
4, false, "driver_filtered_tx_pkt" },
{ STATS_OFFSET32(eee_tx_lpi),
- 4, true, "Tx LPI entry count"}
+ 4, true, "Tx LPI entry count"},
+ { STATS_OFFSET32(ptp_skip_tx_ts),
+ 4, false, "ptp_skipped_tx_tstamp" },
};
#define BNX2X_NUM_STATS ARRAY_SIZE(bnx2x_stats_arr)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 03ac10b1cd1e..066b24611890 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -15214,11 +15214,27 @@ static void bnx2x_ptp_task(struct work_struct *work)
u32 val_seq;
u64 timestamp, ns;
struct skb_shared_hwtstamps shhwtstamps;
+ bool bail = true;
+ int i;
- /* Read Tx timestamp registers */
- val_seq = REG_RD(bp, port ? NIG_REG_P1_TLLH_PTP_BUF_SEQID :
- NIG_REG_P0_TLLH_PTP_BUF_SEQID);
- if (val_seq & 0x10000) {
+ /* FW may take a while to complete timestamping; try a bit and if it's
+ * still not complete, may indicate an error state - bail out then.
+ */
+ for (i = 0; i <= 10; i++) {
+ /* Read Tx timestamp registers */
+ val_seq = REG_RD(bp, port ? NIG_REG_P1_TLLH_PTP_BUF_SEQID :
+ NIG_REG_P0_TLLH_PTP_BUF_SEQID);
+ if (val_seq & 0x10000) {
+ bail = false;
+ break;
+ }
+
+ if (!(i % 5)) /* Avoid log flood */
+ DP(BNX2X_MSG_PTP, "There's no valid Tx timestamp yet\n");
+ msleep(50 + 25 * i);
+ }
+
+ if (!bail) {
/* There is a valid timestamp value */
timestamp = REG_RD(bp, port ? NIG_REG_P1_TLLH_PTP_BUF_TS_MSB :
NIG_REG_P0_TLLH_PTP_BUF_TS_MSB);
@@ -15233,16 +15249,18 @@ static void bnx2x_ptp_task(struct work_struct *work)
memset(&shhwtstamps, 0, sizeof(shhwtstamps));
shhwtstamps.hwtstamp = ns_to_ktime(ns);
skb_tstamp_tx(bp->ptp_tx_skb, &shhwtstamps);
- dev_kfree_skb_any(bp->ptp_tx_skb);
- bp->ptp_tx_skb = NULL;
DP(BNX2X_MSG_PTP, "Tx timestamp, timestamp cycles = %llu, ns = %llu\n",
timestamp, ns);
} else {
- DP(BNX2X_MSG_PTP, "There is no valid Tx timestamp yet\n");
- /* Reschedule to keep checking for a valid timestamp value */
- schedule_work(&bp->ptp_task);
+ DP(BNX2X_MSG_PTP,
+ "Tx timestamp is not recorded (register read=%u)\n",
+ val_seq);
+ bp->eth_stats.ptp_skip_tx_ts++;
}
+
+ dev_kfree_skb_any(bp->ptp_tx_skb);
+ bp->ptp_tx_skb = NULL;
}
void bnx2x_set_rx_ts(struct bnx2x *bp, struct sk_buff *skb)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h
index b2644ed13d06..d55e63692cf3 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h
@@ -207,6 +207,9 @@ struct bnx2x_eth_stats {
u32 driver_filtered_tx_pkt;
/* src: Clear-on-Read register; Will not survive PMF Migration */
u32 eee_tx_lpi;
+
+ /* PTP */
+ u32 ptp_skip_tx_ts;
};
struct bnx2x_eth_q_stats {
--
2.22.0
^ permalink raw reply related
* Re: [PATCH V2] net: dsa: microchip: Use gpiod_set_value_cansleep()
From: Linus Walleij @ 2019-06-24 22:18 UTC (permalink / raw)
To: Marek Vasut
Cc: netdev, Andrew Lunn, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <20190623151257.13660-1-marex@denx.de>
On Sun, Jun 23, 2019 at 5:14 PM Marek Vasut <marex@denx.de> wrote:
> Replace gpiod_set_value() with gpiod_set_value_cansleep(), as the switch
> reset GPIO can be connected to e.g. I2C GPIO expander and it is perfectly
> fine for the kernel to sleep for a bit in ksz_switch_register().
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Tristram Ha <Tristram.Ha@microchip.com>
> Cc: Woojung Huh <Woojung.Huh@microchip.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
> V2: use _cansleep in .remove as well
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH net-next v2 4/8] net: aquantia: make all files GPL-2.0-only
From: Andrew Lunn @ 2019-06-24 22:16 UTC (permalink / raw)
To: Igor Russkikh; +Cc: David S . Miller, netdev@vger.kernel.org
In-Reply-To: <795f0f66ddf604a91de0f4a7734d0e9b282c7a3d.1561388549.git.igor.russkikh@aquantia.com>
On Mon, Jun 24, 2019 at 03:10:51PM +0000, Igor Russkikh wrote:
> It was noticed some files had -or-later, however overall driver has
> -only license. Clean this up.
>
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Andrey Ignatov @ 2019-06-24 22:16 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Daniel Borkmann, Takshak Chahande, netdev@vger.kernel.org,
ast@kernel.org, Kernel Team, Stanislav Fomichev
In-Reply-To: <20190624145111.49176d8e@cakuba.netronome.com>
Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2019-06-24 14:51 -0700]:
> On Mon, 24 Jun 2019 16:22:25 +0200, Daniel Borkmann wrote:
> > On 06/22/2019 12:33 AM, Takshak Chahande wrote:
> > > With different bpf attach_flags available to attach bpf programs specially
> > > with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective
> > > bpf-programs available to any sub-cgroups really needs to be available for
> > > easy debugging.
> > >
> > > Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached
> > > bpf-programs to a cgroup but also the inherited ones from parent cgroup.
> > >
> > > So "-e" option is introduced to use BPF_F_QUERY_EFFECTIVE query flag here to
> > > list all the effective bpf-programs available for execution at a specified
> > > cgroup.
> > >
> > > Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf:
> > > # ./test_cgroup_attach
> > >
> > > With old bpftool (without -e option):
> > >
> > > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/
> > > ID AttachType AttachFlags Name
> > > 271 egress multi pkt_cntr_1
> > > 272 egress multi pkt_cntr_2
> > >
> > > Attached new program pkt_cntr_4 in cg2 gives following:
> > >
> > > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2
> > > ID AttachType AttachFlags Name
> > > 273 egress override pkt_cntr_4
> > >
> > > And with new "-e" option it shows all effective programs for cg2:
> > >
> > > # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2
> > > ID AttachType AttachFlags Name
> > > 273 egress override pkt_cntr_4
> > > 271 egress override pkt_cntr_1
> > > 272 egress override pkt_cntr_2
> > >
> > > Signed-off-by: Takshak Chahande <ctakshak@fb.com>
> > > Acked-by: Andrey Ignatov <rdna@fb.com>
> >
> > Applied, thanks!
>
> This is a cgroup-specific flag, right? It should be a parameter
> to cgroup show, not a global flag. Can we please drop this patch
> from the tree?
Hey Jakub,
I had same thought about cgroup-specific flag while reviewing the patch,
but then found out that all flags in bpftool are now global, no mater if
they're sub-command-specific or not.
For example, --mapcompat is used only in prog-subcommand, but the option
is global; --bpffs is used in prog- and map-subcommands, but the option
is global as well, etc (there are more examples).
I agree that limiting the scope of an option is a good idea in the long
term and it'd be great to rework all existing options to be available
only for corresponding sub-commands, but I don't see how the new `-e`
options is different from existing options and why it should be dropped.
Or you were counfused by the example in the commit log? Since all
options are global they can be specific anywhere on the command line,
i.e. instead of:
# bpftool -e cgroup show /path/to/cgroup
it can be:
# bpftool cgroup show -e /path/to/cgroup
--
Andrey Ignatov
^ permalink raw reply
* Re: [PATCH net-next v2 3/8] maintainers: declare aquantia atlantic driver maintenance
From: Andrew Lunn @ 2019-06-24 22:16 UTC (permalink / raw)
To: Igor Russkikh; +Cc: David S . Miller, netdev@vger.kernel.org
In-Reply-To: <2b413d26ebc04cfca7f7dfa5422f1ea28d9cd76d.1561388549.git.igor.russkikh@aquantia.com>
On Mon, Jun 24, 2019 at 03:10:49PM +0000, Igor Russkikh wrote:
> Aquantia is resposible now for all new features and bugfixes.
> Reflect that in MAINTAINERS.
>
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v3 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Willem de Bruijn @ 2019-06-24 22:15 UTC (permalink / raw)
To: Neil Horman; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <20190624215142.GA8181@hmswarspite.think-freely.org>
> > > + if (need_wait && !packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST)) {
> > > + po->wait_on_complete = 1;
> > > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> >
> > This resets timeout on every loop. should only set above the loop once.
> >
> I explained exactly why I did that in the change log. Its because I reuse the
> timeout variable to get the return value of the wait_for_complete call.
> Otherwise I need to add additional data to the stack, which I don't want to do.
> Sock_sndtimeo is an inline function and really doesn't add any overhead to this
> path, so I see no reason not to reuse the variable.
The issue isn't the reuse. It is that timeo is reset to sk_sndtimeo
each time. Whereas wait_for_common and its variants return the
number of jiffies left in case the loop needs to sleep again later.
Reading sock_sndtimeo once and passing it to wait_.. repeatedly is a
common pattern across the stack.
> > > @@ -2728,6 +2755,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > err = net_xmit_errno(err);
> > > if (err && __packet_get_status(po, ph) ==
> > > TP_STATUS_AVAILABLE) {
> > > + /* re-init completion queue to avoid subsequent fallthrough
> > > + * on a future thread calling wait_on_complete_interruptible_timeout
> > > + */
> > > + po->wait_on_complete = 0;
> >
> > If setting where sleeping, no need for resetting if a failure happens
> > between those blocks.
> >
> > > + init_completion(&po->skb_completion);
> >
> > no need to reinit between each use?
> >
> I explained exactly why I did this in the comment above. We have to set
> wait_for_complete prior to calling transmit, so as to ensure that we call
> wait_for_completion before we exit the loop. However, in this error case, we
> exit the loop prior to calling wait_for_complete, so we need to reset the
> completion variable and the wait_for_complete flag. Otherwise we will be in a
> case where, on the next entrace to this loop we will have a completion variable
> with completion->done > 0, meaning the next wait will be a fall through case,
> which we don't want.
By moving back to the point where schedule() is called, hopefully this
complexity automatically goes away. Same as my comment to the line
immediately above.
> > > @@ -2740,6 +2772,20 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > }
> > > packet_increment_head(&po->tx_ring);
> > > len_sum += tp_len;
> > > +
> > > + if (po->wait_on_complete) {
> > > + timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
> > > + po->wait_on_complete = 0;
> >
> > I was going to argue for clearing in tpacket_destruct_skb. But then we
> > would have to separate clear on timeout instead of signal, too.
> >
> > po->wait_on_complete = 1;
> > timeo = wait_for_completion...
> > po->wait_on_complete = 0;
> >
> Also, we would have a race condition, since the destructor may be called from
> softirq context (the first cause of the bug I'm fixing here), and so if the
> packet is freed prior to us checking wait_for_complete in tpacket_snd, we will
> be in the above situation again, exiting the loop with a completion variable in
> an improper state.
Good point.
The common pattern is to clear in tpacket_destruct_skb. Then
we do need to handle the case where the wait is interrupted or
times out and reset it in those cases.
> > This is basically replacing a busy-wait with schedule() with sleeping
> > using wait_for_completion_interruptible_timeout. My main question is
> > does this really need to move control flow around and add
> > packet_next_frame? If not, especially for net, the shortest, simplest
> > change is preferable.
> >
> Its not replacing a busy wait at all, its replacing a non-blocking schedule with
> a blocking schedule (via completion queues). As for control flow, Im not sure I
> why you are bound to the existing control flow, and given that we already have
> packet_previous_frame, I didn't see anything egregious about adding
> packet_next_frame as well, but since you've seen a way to eliminate it, I'm ok
> with it.
The benefit of keeping to the existing control flow is that that is a
smaller change, so easier to verify.
I understand the benefit of moving the wait outside the loop. Before
this report, I was not even aware of that behavior on !MSG_DONTWAIT,
because it is so co-located.
But moving it elsewhere in the loop does not have the same benefit,
imho. Either way, I think we better leave any such code improvements
to net-next and focus on the minimal , least risky, patch for net.
^ permalink raw reply
* Re: [PATCH net-next v2 2/8] net: aquantia: add documentation for the atlantic driver
From: Andrew Lunn @ 2019-06-24 22:15 UTC (permalink / raw)
To: Igor Russkikh; +Cc: David S . Miller, netdev@vger.kernel.org
In-Reply-To: <438b6899b5c2d82474a0dcee543b2ae6c4ee7b1f.1561388549.git.igor.russkikh@aquantia.com>
On Mon, Jun 24, 2019 at 03:10:47PM +0000, Igor Russkikh wrote:
> Document contains configuration options description,
> details and examples of driver various settings.
>
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> ---
> .../device_drivers/aquantia/atlantic.txt | 437 ++++++++++++++++++
> 1 file changed, 437 insertions(+)
> create mode 100644 Documentation/networking/device_drivers/aquantia/atlantic.txt
>
> diff --git a/Documentation/networking/device_drivers/aquantia/atlantic.txt b/Documentation/networking/device_drivers/aquantia/atlantic.txt
> new file mode 100644
> index 000000000000..45b93f8143b4
> --- /dev/null
> +++ b/Documentation/networking/device_drivers/aquantia/atlantic.txt
> @@ -0,0 +1,437 @@
> +aQuantia AQtion Driver for the aQuantia Multi-Gigabit PCI Express Family of
> +Ethernet Adapters
> +=============================================================================
> +
> +Contents
> +========
> +
> +- Important Note
> +- Identifying Your Adapter
> +- Command Line Parameters
> +- Additional Configurations
> +- Support
Hi Igor
This TOC is now out of date. You don't have an Important Note section,
Command line parameters are near the end etc.
> +
> +Identifying Your Adapter
> +========================
> +
> +The driver in this release is compatible with AQC-100, AQC-107, AQC-108 based ethernet adapters.
> +
> +
> +SFP+ Devices (for AQC-100 based adapters)
> +----------------------------------
> +
> +This release tested with passive Direct Attach Cables (DAC) and SFP+/LC Optical Transceiver.
> +
> +Config file parameters
> +=======================
> +For some fine tuning and performance optimizations,
> +some parameters can be changed in the {source_dir}/aq_cfg.h file.
To me, these seems like the least likely option, so should come last
for those who are very desperate. Start with ethtool, then command
line parameters, and if all else fails, hack the code to change
settings.
Andrew
^ permalink raw reply
* [PATCH net-next 1/2] net: sched: refactor reinsert action
From: John Hurley @ 2019-06-24 22:13 UTC (permalink / raw)
To: netdev
Cc: davem, fw, jhs, simon.horman, jakub.kicinski, oss-drivers,
John Hurley
In-Reply-To: <1561414416-29732-1-git-send-email-john.hurley@netronome.com>
The TC_ACT_REINSERT return type was added as an in-kernel only option to
allow a packet ingress or egress redirect. This is used to avoid
unnecessary skb clones in situations where they are not required. If a TC
hook returns this code then the packet is 'reinserted' and no skb consume
is carried out as no clone took place.
This return type is only used in act_mirred. Rather than have the reinsert
called from the main datapath, call it directly in act_mirred. Instead of
returning TC_ACT_REINSERT, change the type to the new TC_ACT_CONSUMED
which tells the caller that the packet has been stolen by another process
and that no consume call is required.
Moving all redirect calls to the act_mirred code is in preparation for
tracking recursion created by act_mirred.
Signed-off-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
include/net/pkt_cls.h | 2 +-
include/net/sch_generic.h | 2 +-
net/core/dev.c | 4 +---
net/sched/act_mirred.c | 3 ++-
4 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 720f2b3..1a7596b 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -10,7 +10,7 @@
#include <net/net_namespace.h>
/* TC action not accessible from user space */
-#define TC_ACT_REINSERT (TC_ACT_VALUE_MAX + 1)
+#define TC_ACT_CONSUMED (TC_ACT_VALUE_MAX + 1)
/* Basic packet classifier frontend definitions. */
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 21f434f..855167b 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -279,7 +279,7 @@ struct tcf_result {
};
const struct tcf_proto *goto_tp;
- /* used by the TC_ACT_REINSERT action */
+ /* used in the skb_tc_reinsert function */
struct {
bool ingress;
struct gnet_stats_queue *qstats;
diff --git a/net/core/dev.c b/net/core/dev.c
index d6edd21..5852931 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4689,9 +4689,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
__skb_push(skb, skb->mac_len);
skb_do_redirect(skb);
return NULL;
- case TC_ACT_REINSERT:
- /* this does not scrub the packet, and updates stats on error */
- skb_tc_reinsert(skb, &cl_res);
+ case TC_ACT_CONSUMED:
return NULL;
default:
break;
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 58e7573d..8c1d736 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -277,7 +277,8 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
if (use_reinsert) {
res->ingress = want_ingress;
res->qstats = this_cpu_ptr(m->common.cpu_qstats);
- return TC_ACT_REINSERT;
+ skb_tc_reinsert(skb, res);
+ return TC_ACT_CONSUMED;
}
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 2/2] net: sched: protect against stack overflow in TC act_mirred
From: John Hurley @ 2019-06-24 22:13 UTC (permalink / raw)
To: netdev
Cc: davem, fw, jhs, simon.horman, jakub.kicinski, oss-drivers,
John Hurley
In-Reply-To: <1561414416-29732-1-git-send-email-john.hurley@netronome.com>
TC hooks allow the application of filters and actions to packets at both
ingress and egress of the network stack. It is possible, with poor
configuration, that this can produce loops whereby an ingress hook calls
a mirred egress action that has an egress hook that redirects back to
the first ingress etc. The TC core classifier protects against loops when
doing reclassifies but there is no protection against a packet looping
between multiple hooks and recursively calling act_mirred. This can lead
to stack overflow panics.
Add a per CPU counter to act_mirred that is incremented for each recursive
call of the action function when processing a packet. If a limit is passed
then the packet is dropped and CPU counter reset.
Note that this patch does not protect against loops in TC datapaths. Its
aim is to prevent stack overflow kernel panics that can be a consequence
of such loops.
Signed-off-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
net/sched/act_mirred.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 8c1d736..c3fce36 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -27,6 +27,9 @@
static LIST_HEAD(mirred_list);
static DEFINE_SPINLOCK(mirred_list_lock);
+#define MIRRED_RECURSION_LIMIT 4
+static DEFINE_PER_CPU(unsigned int, mirred_rec_level);
+
static bool tcf_mirred_is_act_redirect(int action)
{
return action == TCA_EGRESS_REDIR || action == TCA_INGRESS_REDIR;
@@ -210,6 +213,7 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
struct sk_buff *skb2 = skb;
bool m_mac_header_xmit;
struct net_device *dev;
+ unsigned int rec_level;
int retval, err = 0;
bool use_reinsert;
bool want_ingress;
@@ -217,6 +221,14 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
int m_eaction;
int mac_len;
+ rec_level = __this_cpu_inc_return(mirred_rec_level);
+ if (unlikely(rec_level > MIRRED_RECURSION_LIMIT)) {
+ net_warn_ratelimited("Packet exceeded mirred recursion limit on dev %s\n",
+ netdev_name(skb->dev));
+ __this_cpu_dec(mirred_rec_level);
+ return TC_ACT_SHOT;
+ }
+
tcf_lastuse_update(&m->tcf_tm);
bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
@@ -278,6 +290,7 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
res->ingress = want_ingress;
res->qstats = this_cpu_ptr(m->common.cpu_qstats);
skb_tc_reinsert(skb, res);
+ __this_cpu_dec(mirred_rec_level);
return TC_ACT_CONSUMED;
}
}
@@ -293,6 +306,7 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
if (tcf_mirred_is_act_redirect(m_eaction))
retval = TC_ACT_SHOT;
}
+ __this_cpu_dec(mirred_rec_level);
return retval;
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 0/2] Track recursive calls in TC act_mirred
From: John Hurley @ 2019-06-24 22:13 UTC (permalink / raw)
To: netdev
Cc: davem, fw, jhs, simon.horman, jakub.kicinski, oss-drivers,
John Hurley
These patches aim to prevent act_mirred causing stack overflow events from
recursively calling packet xmit or receive functions. Such events can
occur with poor TC configuration that causes packets to travel in loops
within the system.
Florian Westphal advises that a recursion crash and packets looping are
separate issues and should be treated as such. David Miller futher points
out that pcpu counters cannot track the precise skb context required to
detect loops. Hence these patches are not aimed at detecting packet loops,
rather, preventing stack flows arising from such loops.
John Hurley (2):
net: sched: refactor reinsert action
net: sched: protect against stack overflow in TC act_mirred
include/net/pkt_cls.h | 2 +-
include/net/sch_generic.h | 2 +-
net/core/dev.c | 4 +---
net/sched/act_mirred.c | 17 ++++++++++++++++-
4 files changed, 19 insertions(+), 6 deletions(-)
--
2.7.4
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox