* Re: [PATCH net-next 0/2] net: sched: act_ctinfo: fixes
From: David Miller @ 2019-06-17 21:01 UTC (permalink / raw)
To: ldir; +Cc: netdev
In-Reply-To: <20190617100327.24796-1-ldir@darbyshire-bryant.me.uk>
From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Date: Mon, 17 Jun 2019 11:03:25 +0100
> This is first attempt at sending a small series. Order is important
> because one bug (policy validation) prevents us from encountering the
> more important 'OOPS' generating bug in action creation. Fix the OOPS
> first.
>
> Confession time: Until very recently, development of this module has
> been done on 'net-next' tree to 'clean compile' level with run-time
> testing on backports to 4.14 & 4.19 kernels under openwrt. It turns out
> that sched: action: based code has been under more active change than I
> realised.
>
> During the back & forward porting during development & testing, the
> critical ACT_P_CREATED return code got missed despite being in the 4.14
> & 4.19 backports. I have now gone through the init functions, using
> act_csum as reference with a fine toothed comb and am happy they do the
> same things.
>
> This issue hadn't been caught till now due to another issue caused by
> new strict nla_parse_nested function failing parsing validation before
> action creation.
>
> Thanks to Marcelo Leitner <marcelo.leitner@gmail.com> for flagging
> extack deficiency (fixed in 733f0766c3de sched: act_ctinfo: use extack
> error reporting) which led to b424e432e770 ("netlink: add validation of
> NLA_F_NESTED flag") and 8cb081746c03 ("netlink: make validation more
> configurable for future strictness”) which led to the policy validation
> fix, which then led to the action creation fix both contained in this
> series.
>
> If I ever get to a developer conference please feel free to
> tar/feather/apply cone of shame.
:-) In kernel networking development we prefer brown paper bags over
cones of shame, just FYI :) :) :)
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH] bpf: optimize constant blinding
From: Jiong Wang @ 2019-06-17 21:01 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jiong Wang, Edward Cree, Alexei Starovoitov, Andrii Nakryiko,
Naveen N. Rao, Daniel Borkmann, bpf, Network Development,
Michael Ellerman, Jakub Kicinski
In-Reply-To: <CAADnVQLt7=9XQRccerLqHO8cdXTLy-uNBt-JOjbx=bAFouLf3Q@mail.gmail.com>
Alexei Starovoitov writes:
> On Mon, Jun 17, 2019 at 1:40 PM Jiong Wang <jiong.wang@netronome.com> wrote:
>>
>> After digest Alexei and Andrii's reply, I still don't see the need to turn
>> branch target into list, and I am not sure whether pool based list sound
>> good? it saves size, resize pool doesn't invalid allocated node (the offset
>> doesn't change) but requires one extra addition to calculate the pointer.
>
> I don't think it worth to do a pool to accelerate kmalloc.
Got it.
> I doubt it will be faster either.
Will benchmark.
Regards,
Jiong
^ permalink raw reply
* Re: [PATCH] net/mlx5e: reduce stack usage in mlx5_eswitch_termtbl_create
From: David Miller @ 2019-06-17 21:02 UTC (permalink / raw)
To: arnd
Cc: saeedm, leon, ozsh, paulb, elibr, markb, ogerlitz, maorg, netdev,
linux-rdma, linux-kernel
In-Reply-To: <20190617110855.2085326-1-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 17 Jun 2019 13:08:22 +0200
> Putting an empty 'mlx5_flow_spec' structure on the stack is a bit
> wasteful and causes a warning on 32-bit architectures when building
> with clang -fsanitize-coverage:
>
> drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c: In function 'mlx5_eswitch_termtbl_create':
> drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c:90:1: error: the frame size of 1032 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> Since the structure is never written to, we can statically allocate
> it to avoid the stack usage. To be on the safe side, mark all
> subsequent function arguments that we pass it into as 'const'
> as well.
>
> Fixes: 10caabdaad5a ("net/mlx5e: Use termination table for VLAN push actions")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Saeed, once Arnd fixes the reverse christmas tree issue, I assume you will take
this in via your tree?
Thanks.
^ permalink raw reply
* Re: [PATCH][next] net: hns3: fix dereference of ae_dev before it is null checked
From: David Miller @ 2019-06-17 21:05 UTC (permalink / raw)
To: colin.king
Cc: yisen.zhuang, salil.mehta, netdev, kernel-janitors, linux-kernel
In-Reply-To: <20190617114214.25276-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Mon, 17 Jun 2019 12:42:14 +0100
> From: Colin Ian King <colin.king@canonical.com>
>
> Pointer ae_dev is null checked however, prior to that it is dereferenced
> when assigned pointer ops. Fix this by assigning pointer ops after ae_dev
> has been null checked.
>
> Addresses-Coverity: ("Dereference before null check")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied.
^ permalink raw reply
* Re: [patch net-next] net: sched: cls_matchall: allow to delete filter
From: David Miller @ 2019-06-17 21:06 UTC (permalink / raw)
To: jiri; +Cc: netdev, mlxsw, eli, jhs, xiyou.wangcong
In-Reply-To: <20190617160232.7599-1-jiri@resnulli.us>
From: Jiri Pirko <jiri@resnulli.us>
Date: Mon, 17 Jun 2019 18:02:32 +0200
> From: Jiri Pirko <jiri@mellanox.com>
>
> Currently user is unable to delete the filter. See following example:
> $ tc filter add dev ens16np1 ingress pref 1 handle 1 matchall action drop
> $ tc filter show dev ens16np1 ingress
> filter protocol all pref 1 matchall chain 0
> filter protocol all pref 1 matchall chain 0 handle 0x1
> in_hw
> action order 1: gact action drop
> random type none pass val 0
> index 1 ref 1 bind 1
>
> $ tc filter del dev ens16np1 ingress pref 1 handle 1 matchall action drop
> RTNETLINK answers: Operation not supported
>
> Implement tcf_proto_ops->delete() op and allow user to delete the filter.
>
> Reported-by: Eli Cohen <eli@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH bpf-next 8/8] selftests/bpf: switch tests to BTF-defined map definitions
From: Stanislav Fomichev @ 2019-06-17 21:07 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
In-Reply-To: <CAEf4BzbV-W1KsuN3AuPas_3dG7MVwZO6RsqohS2uvnEf49M67w@mail.gmail.com>
On 06/17, Andrii Nakryiko wrote:
> On Fri, Jun 14, 2019 at 5:01 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 06/14, Andrii Nakryiko wrote:
> > > On Fri, Jun 14, 2019 at 4:23 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > > >
> > > > On 06/10, Andrii Nakryiko wrote:
> > > > > Switch test map definition to new BTF-defined format.
> > > > Reiterating my concerns on non-RFC version:
> > > >
> > > > Pretty please, let's not convert everything at once. Let's start
> > > > with stuff that explicitly depends on BTF (spinlocks?).
> > >
> > > How about this approach. I can split last commit into two. One
> > > converting all the stuff that needs BTF (spinlocks, etc). Another part
> > > - everything else. If it's so important for your use case, you'll be
> > > able to just back out my last commit. Or we just don't land last
> > > commit.
> > I can always rollback or do not backport internally; the issue is that
> > it would be much harder to backport any future fixes/extensions to
> > those tests. So splitting in two and not landing the last one is
> > preferable ;-)
>
> So I just posted v2 and I split all the test conversions into three parts:
> 1. tests that already rely on BTF
> 2. tests w/ custom key/value types
> 3. all the reset
>
> I think we should definitely apply #1. I think #2 would be nice. And
> we can probably hold off on #3. I'll let Alexei or Daniel decide, but
> it shouldn't be hard for them to do that.
Awesome, thanks!
> > > > One good argument (aside from the one that we'd like to be able to
> > > > run tests internally without BTF for a while): libbpf doesn't
> > > > have any tests as far as I'm aware. If we don't have 'legacy' maps in the
> > > > selftests, libbpf may bit rot.
> > >
> > > I left few legacy maps exactly for that reason. See progs/test_btf_*.c.
> > Damn it, you've destroyed my only good argument.
>
> Heh :)
>
> >
> > > > (Andrii, feel free to ignore, since we've already discussed that)
> > > >
> > > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > > ---
> > >
> > >
> > > <snip>
^ permalink raw reply
* Re: [PATCH] bpf: optimize constant blinding
From: Edward Cree @ 2019-06-17 21:16 UTC (permalink / raw)
To: Jiong Wang, Alexei Starovoitov, Andrii Nakryiko
Cc: Alexei Starovoitov, Naveen N. Rao, Daniel Borkmann, bpf,
Network Development, Michael Ellerman, Jakub Kicinski
In-Reply-To: <877e9kgd39.fsf@netronome.com>
On 17/06/2019 21:40, Jiong Wang wrote:
> Now if we don't split patch when patch an insn inside patch, instead, if we
> replace the patched insn using what you suggested, then the logic looks to
> me becomes even more complex, something like
>
> for (idx = 0; idx < insn_cnt; idx++) {
> if (insns[idx] is not BPF_LIST_INSN) {
> do_insn(...)
> }
> else if (insns[idx] is BPF_LIST_INSN) {
> list = pool_base + insn.imm;
> while (list) {
> insn = list_head->insn;
> if (insn is BF_LIST_INSN) {
> sub_list = ...
> while ()
> do_insn()
> continue;
> }
> do_insn(...)
> list = pool_base + list->next;
> }
> }
> }
Why can't do_insn() just go like:
if (insn is BPF_LIST_INSN)
for (idx = 0; idx < LIST_COUNT(insn); idx++)
do_insn(pool_base + LIST_START(insn) + idx);
else
rest of processing
?
Alternatively, iterate with something more sophisticated than 'idx++'
(standard recursion-to-loop transformation).
You shouldn't ever need a for() tower statically in the code...
> So, I am thinking what Alexei and Andrii suggested make sense, just use
> single data structure (singly linked list) to represent everything, so the
> insn traversal etc could be simple
But then you have to also store orig_insn_idx with each insn, so you can
calculate the new jump offsets when you linearise. Having an array of
patched_orig_insns gives you that for free.
^ permalink raw reply
* Re: [PATCH v2 bpf-next 00/11] BTF-defined BPF map definitions
From: Daniel Borkmann @ 2019-06-17 21:17 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: andrii.nakryiko, ast, netdev, bpf, kernel-team, jakub.kicinski,
joe
In-Reply-To: <20190617192700.2313445-1-andriin@fb.com>
On 06/17/2019 09:26 PM, Andrii Nakryiko wrote:
> This patch set implements initial version (as discussed at LSF/MM2019
> conference) of a new way to specify BPF maps, relying on BTF type information,
> which allows for easy extensibility, preserving forward and backward
> compatibility. See details and examples in description for patch #6.
>
> [0] contains an outline of follow up extensions to be added after this basic
> set of features lands. They are useful by itself, but also allows to bring
> libbpf to feature-parity with iproute2 BPF loader. That should open a path
> forward for BPF loaders unification.
>
> Patch #1 centralizes commonly used min/max macro in libbpf_internal.h.
> Patch #2 extracts .BTF and .BTF.ext loading loging from elf_collect().
> Patch #3 simplifies elf_collect() error-handling logic.
> Patch #4 refactors map initialization logic into user-provided maps and global
> data maps, in preparation to adding another way (BTF-defined maps).
> Patch #5 adds support for map definitions in multiple ELF sections and
> deprecates bpf_object__find_map_by_offset() API which doesn't appear to be
> used anymore and makes assumption that all map definitions reside in single
> ELF section.
> Patch #6 splits BTF intialization from sanitization/loading into kernel to
> preserve original BTF at the time of map initialization.
> Patch #7 adds support for BTF-defined maps.
> Patch #8 adds new test for BTF-defined map definition.
> Patches #9-11 convert test BPF map definitions to use BTF way.
>
> [0] https://lore.kernel.org/bpf/CAEf4BzbfdG2ub7gCi0OYqBrUoChVHWsmOntWAkJt47=FE+km+A@mail.gmail.com/
Quoting above in here for some clarifications on the approach. Basically for
iproute2, we would add libbpf library support on top of the current loader,
this means existing object files keep working as-is, and users would have to
decide whether they want to go with the new format or stick to the old one;
incentive for the new format would be to get all the other libbpf features
from upstream automatically. Though it means that once they switch there is
no object file compatibility with older iproute2 versions anymore. For the
case of Cilium, the container image ships with its own iproute2 version as
we don't want to rely on distros that they keep iproute2<->kernel release in
sync (some really don't). Switch should be fine in our case. For people
upgrading, the 'external' behavior (e.g. bpf fs interaction etc) would need
to stay the same to not run into any service disruption when switching versions.
>1. Pinning. This one is simple:
> - add pinning attribute, that will either be "no pinning", "global
>pinning", "object-scope pinning".
> - by default pinning root will be "/sys/fs/bpf", but one will be
>able to override this per-object using extra options (so that
>"/sys/fs/bpf/tc" can be specified).
I would just drop the object-scope pinning. We avoided using it and I'm not
aware if anyone else make use. It also has the ugly side-effect that this
relies on AF_ALG which e.g. on some cloud provider shipped kernels is disabled.
The pinning attribute should be part of the standard set of map attributes for
libbpf though as it's generally useful for networking applications.
>2. Map-in-map declaration:
>
>As outlined at LSF/MM, we can extend value type to be another map
>definition, specifying a prototype for inner map:
>
>struct {
> int type;
> int max_entries;
> struct outer_key *key;
> struct { /* this is definition of inner map */
> int type;
> int max_entries;
> struct inner_key *key;
> struct inner_value *value;
> } value;
>} my_hash_of_arrays BPF_MAP = {
> .type = BPF_MAP_TYPE_HASH_OF_MAPS,
> .max_entries = 1024,
> .value = {
> .type = BPF_MAP_TYPE_ARRAY,
> .max_entries = 64,
> },
>};
>
>This would declare a hash_of_maps, where inner maps are arrays of 64
>elements each. Notice, that struct defining inner map can be declared
>outside and shared with other maps:
>
>struct inner_map_t {
> int type;
> int max_entries;
> struct inner_key *key;
> struct inner_value *value;
>};
>
>struct {
> int type;
> int max_entries;
> struct outer_key *key;
> struct inner_map_t value;
>} my_hash_of_arrays BPF_MAP = {
> .type = BPF_MAP_TYPE_HASH_OF_MAPS,
> .max_entries = 1024,
> .value = {
> .type = BPF_MAP_TYPE_ARRAY,
> .max_entries = 64,
> },
>};
This one feels a bit weird to me. My expectation would have been something
around the following to make this work:
struct my_inner_map {
int type;
int max_entries;
int *key;
struct my_value *value;
} btf_inner SEC(".maps") = {
.type = BPF_MAP_TYPE_ARRAY,
.max_entries = 16,
};
And:
struct {
int type;
int max_entries;
int *key;
struct my_inner_map *value;
} btf_outer SEC(".maps") = {
.type = BPF_MAP_TYPE_ARRAY,
.max_entries = 16,
.value = &btf_inner,
};
And the loader should figure this out and combine everything in the background.
Otherwise above 'struct inner_map_t value' would be mixing convention of using
pointer vs non-pointer which may be even more confusing.
>3. Initialization of prog array. Iproute2 supports a convention-driven
>initialization of BPF_MAP_TYPE_PROG_ARRAY using special section names
>(wrapped into __section_tail(ID, IDX)):
>
>struct bpf_elf_map SEC("maps") POLICY_CALL_MAP = {
> .type = BPF_MAP_TYPE_PROG_ARRAY,
> .id = MAP_ID,
> .size_key = sizeof(__u32),
> .size_value = sizeof(__u32),
> .max_elem = 16,
>};
>
>__section_tail(MAP_ID, MAP_IDX) int handle_policy(struct __sk_buff *skb)
>{
> ...
>}
>
>For each such program, iproute2 will put its FD (for later
>tail-calling) into a corresponding MAP with id == MAP_ID at index
>MAP_IDX.
>
>Here's how I see this supported in BTF-defined maps case.
>
>typedef int (* skbuff_tailcall_fn)(struct __sk_buff *);
>
>struct {
> int type;
> int max_entries;
> int *key;
> skbuff_tailcall_fb value[];
>} POLICY_CALL_MAP SEC(".maps") = {
> .type = BPF_MAP_TYPE_PROG_ARRAY,
> .max_entries = 16,
> .value = {
> &handle_policy,
> NULL,
> &handle_some_other_policy,
> },
>};
>
>libbpf loader will greate BPF_MAP_TYPE_PROG_ARRAY map with 16 elements
>and will initialize first and third entries with FDs of handle_policy
>and handle_some_other_policy programs. As an added nice bonus,
>compiler should also warn on signature mismatch. ;)
Seems okay, I guess the explicit initialization could lead people to think
that /after/ loading completed the NULL entries really won't have anything in
that tail call slot. In our case, we share some of the tail call maps for different
programs and each a different __section_tail(, idx), but in the end it's just a
matter of initialization by index for the above. iproute2 today fetches the map
from bpf fs if present, and only updates slots with __section_tail() present in
the object file. Invocation would again be via index I presume (tail_call(skb,
&policy_map, skb->mark), for example). For the __section_tail(MAP_ID, MAP_IDX),
we do dynamically generate the MAP_IDX define in some cases, but that MAP_IDX
would then simply be used in above POLICY_CALL_MAP instead; seems fine.
>4. We can extend this idea into ARRAY_OF_MAPS initialization. This is
>currently implemented in iproute2 using .id, .inner_id, and .inner_idx
>fields.
>
>struct inner_map_t {
> int type;
> int max_entries;
> struct inner_key *key;
> struct inner_value *value;
>};
>
>struct inner_map_t map1 = {...};
>struct inner_map_t map2 = {...};
>
>struct {
> int type;
> int max_entries;
> struct outer_key *key;
> struct inner_map_t value[];
>} my_hash_of_arrays BPF_MAP = {
> .type = BPF_MAP_TYPE_ARRAY_OF_MAPS,
> .max_entries = 2,
> .value = {
> &map1,
> &map2,
> },
>};
Yeah, agree.
> v1->v2:
> - more BTF-sanity checks in parsing map definitions (Song);
> - removed confusing usage of "attribute", switched to "field;
> - split off elf_collect() refactor from btf loading refactor (Song);
> - split selftests conversion into 3 patches (Stanislav):
> 1. test already relying on BTF;
> 2. tests w/ custom types as key/value (so benefiting from BTF);
> 3. all the rest tests (integers as key/value, special maps w/o BTF support).
> - smaller code improvements (Song);
>
> rfc->v1:
> - error out on unknown field by default (Stanislav, Jakub, Lorenz);
>
> Andrii Nakryiko (11):
> libbpf: add common min/max macro to libbpf_internal.h
> libbpf: extract BTF loading logic
> libbpf: streamline ELF parsing error-handling
> libbpf: refactor map initialization
> libbpf: identify maps by section index in addition to offset
> libbpf: split initialization and loading of BTF
> libbpf: allow specifying map definitions using BTF
> selftests/bpf: add test for BTF-defined maps
> selftests/bpf: switch BPF_ANNOTATE_KV_PAIR tests to BTF-defined maps
> selftests/bpf: convert tests w/ custom values to BTF-defined maps
> selftests/bpf: convert remaining selftests to BTF-defined maps
>
> tools/lib/bpf/bpf.c | 7 +-
> tools/lib/bpf/bpf_prog_linfo.c | 5 +-
> tools/lib/bpf/btf.c | 3 -
> tools/lib/bpf/btf.h | 1 +
> tools/lib/bpf/btf_dump.c | 3 -
> tools/lib/bpf/libbpf.c | 781 +++++++++++++-----
> tools/lib/bpf/libbpf_internal.h | 7 +
> tools/testing/selftests/bpf/progs/bpf_flow.c | 18 +-
> .../selftests/bpf/progs/get_cgroup_id_kern.c | 18 +-
> .../testing/selftests/bpf/progs/netcnt_prog.c | 22 +-
> .../selftests/bpf/progs/sample_map_ret0.c | 18 +-
> .../selftests/bpf/progs/socket_cookie_prog.c | 11 +-
> .../bpf/progs/sockmap_verdict_prog.c | 36 +-
> .../selftests/bpf/progs/test_btf_newkv.c | 73 ++
> .../bpf/progs/test_get_stack_rawtp.c | 27 +-
> .../selftests/bpf/progs/test_global_data.c | 27 +-
> tools/testing/selftests/bpf/progs/test_l4lb.c | 45 +-
> .../selftests/bpf/progs/test_l4lb_noinline.c | 45 +-
> .../selftests/bpf/progs/test_map_in_map.c | 20 +-
> .../selftests/bpf/progs/test_map_lock.c | 22 +-
> .../testing/selftests/bpf/progs/test_obj_id.c | 9 +-
> .../bpf/progs/test_select_reuseport_kern.c | 45 +-
> .../bpf/progs/test_send_signal_kern.c | 22 +-
> .../bpf/progs/test_skb_cgroup_id_kern.c | 9 +-
> .../bpf/progs/test_sock_fields_kern.c | 60 +-
> .../selftests/bpf/progs/test_spin_lock.c | 33 +-
> .../bpf/progs/test_stacktrace_build_id.c | 44 +-
> .../selftests/bpf/progs/test_stacktrace_map.c | 40 +-
> .../testing/selftests/bpf/progs/test_tc_edt.c | 9 +-
> .../bpf/progs/test_tcp_check_syncookie_kern.c | 9 +-
> .../selftests/bpf/progs/test_tcp_estats.c | 9 +-
> .../selftests/bpf/progs/test_tcpbpf_kern.c | 18 +-
> .../selftests/bpf/progs/test_tcpnotify_kern.c | 18 +-
> tools/testing/selftests/bpf/progs/test_xdp.c | 18 +-
> .../selftests/bpf/progs/test_xdp_noinline.c | 60 +-
> tools/testing/selftests/bpf/test_btf.c | 10 +-
> .../selftests/bpf/test_queue_stack_map.h | 20 +-
> .../testing/selftests/bpf/test_sockmap_kern.h | 72 +-
> 38 files changed, 1199 insertions(+), 495 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/progs/test_btf_newkv.c
>
^ permalink raw reply
* Re: [PATCH net-next v2 1/3] net: sched: add mpls manipulation actions to TC
From: Cong Wang @ 2019-06-17 21:18 UTC (permalink / raw)
To: John Hurley
Cc: Linux Kernel Network Developers, David Miller, Jiri Pirko,
Davide Caratti, Simon Horman, Jakub Kicinski, oss-drivers
In-Reply-To: <CAK+XE=mXVW84MXE5bDYyGhK5XrC_q3ECiaj5=WsXFV0FXBk+eA@mail.gmail.com>
On Fri, Jun 14, 2019 at 3:56 PM John Hurley <john.hurley@netronome.com> wrote:
>
> On Fri, Jun 14, 2019 at 5:59 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Thu, Jun 13, 2019 at 10:44 AM John Hurley <john.hurley@netronome.com> wrote:
> > > +static inline void tcf_mpls_set_eth_type(struct sk_buff *skb, __be16 ethertype)
> > > +{
> > > + struct ethhdr *hdr = eth_hdr(skb);
> > > +
> > > + skb_postpull_rcsum(skb, &hdr->h_proto, ETH_TLEN);
> > > + hdr->h_proto = ethertype;
> > > + skb_postpush_rcsum(skb, &hdr->h_proto, ETH_TLEN);
> >
> > So you just want to adjust the checksum with the new ->h_proto
> > value. please use a right csum API, rather than skb_post*_rcsum().
> >
>
> Hi Cong,
> Yes, I'm trying to maintain the checksum value if checksum complete
> has been set.
> The function above pulls the old eth type out of the checksum value
> (if it is checksum complete), updates the eth type, and pushes the new
> eth type into the checksum.
> This passes my tests on the checksum.
> I couldn't see an appropriate function to do this other than
> recalculating the whole thing.
> Maybe I missed something?
I never say it is wrong, I mean to say using a csum API is more
clear. Please look into checksum API's, there are many options
for different scenarios, there must be one serves your purpose.
Thanks.
^ permalink raw reply
* Re: [PATCH bpf] bpf: fix the check that forwarding is enabled in bpf_ipv6_fib_lookup
From: Daniel Borkmann @ 2019-06-17 21:20 UTC (permalink / raw)
To: Anton Protopopov, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
In-Reply-To: <20190615225348.2539-1-a.s.protopopov@gmail.com>
On 06/16/2019 12:53 AM, Anton Protopopov wrote:
> The bpf_ipv6_fib_lookup function should return BPF_FIB_LKUP_RET_FWD_DISABLED
> when forwarding is disabled for the input device. However instead of checking
> if forwarding is enabled on the input device, it checked the global
> net->ipv6.devconf_all->forwarding flag. Change it to behave as expected.
>
> Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH] net/mlx5e: reduce stack usage in mlx5_eswitch_termtbl_create
From: Saeed Mahameed @ 2019-06-17 21:30 UTC (permalink / raw)
To: davem@davemloft.net, arnd@arndb.de
Cc: linux-rdma@vger.kernel.org, Or Gerlitz, Oz Shlomo, Paul Blakey,
Mark Bloch, Maor Gottlieb, linux-kernel@vger.kernel.org,
Eli Britstein, netdev@vger.kernel.org, leon@kernel.org
In-Reply-To: <20190617.140230.537297372523260104.davem@davemloft.net>
On Mon, 2019-06-17 at 14:02 -0700, David Miller wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Mon, 17 Jun 2019 13:08:22 +0200
>
> > Putting an empty 'mlx5_flow_spec' structure on the stack is a bit
> > wasteful and causes a warning on 32-bit architectures when building
> > with clang -fsanitize-coverage:
> >
> > drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c:
> > In function 'mlx5_eswitch_termtbl_create':
> > drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c:
> > 90:1: error: the frame size of 1032 bytes is larger than 1024 bytes
> > [-Werror=frame-larger-than=]
> >
> > Since the structure is never written to, we can statically allocate
> > it to avoid the stack usage. To be on the safe side, mark all
> > subsequent function arguments that we pass it into as 'const'
> > as well.
> >
> > Fixes: 10caabdaad5a ("net/mlx5e: Use termination table for VLAN
> > push actions")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Saeed, once Arnd fixes the reverse christmas tree issue, I assume you
> will take
> this in via your tree?
Yes, this is a core change that might conflict with some ongoing stuff.
Thank you Dave !
Saeed.
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: mediatek: Add MT7621 TRGMII mode support
From: René van Dorst @ 2019-06-17 21:33 UTC (permalink / raw)
To: Andrew Lunn
Cc: Sean Wang, Florian Fainelli, David S . Miller, Matthias Brugger,
Vivien Didelot, netdev, john, linux-mediatek, linux-mips
In-Reply-To: <20190617140223.GC25211@lunn.ch>
Quoting Andrew Lunn <andrew@lunn.ch>:
> On Sun, Jun 16, 2019 at 08:20:08PM +0200, René van Dorst wrote:
>> Like many other mediatek SOCs, the MT7621 SOC and the internal
>> MT7530 switch both
>> supports TRGMII mode. MT7621 TRGMII speed is 1200MBit.
>
> Hi René
>
Hi Andrew,
> Is TRGMII used only between the SoC and the Switch? Or does external
> ports of the switch also support 1200Mbit/s? If external ports support
> this, what does ethtool show for Speed?
Only the first GMAC of the SOC and port 6 of the switch supports this mode.
The switch can be internal in the SOC but also a separate chip.
PHYLINK and ethertool reports the link as 1Gbit.
The link is fixed-link with speed = 1000.
dmesg output with unposted PHYLINK patches:
[ 5.236763] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
[ 5.249813] mt7530 mdio-bus:1f: phylink_mac_config:
mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1
an=1
[ 6.389435] mtk_soc_eth 1e100000.ethernet eth0: phylink_mac_config:
mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1
an=1
# ethtool eth0
Settings for eth0:
Supported ports: [ MII ]
Supported link modes: 1000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: No
Supported FEC modes: Not reported
Advertised link modes: 1000baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: No
Advertised FEC modes: Not reported
Speed: 1000Mb/s
Duplex: Full
Port: MII
PHYAD: 0
Transceiver: internal
Auto-negotiation: on
Current message level: 0x000000ff (255)
drv probe link timer ifdown ifup rx_err tx_err
Link detected: yes
I already have report from a MT7623 user that this patch gives issues.
I send v2 of the patch if I fixed that issue.
Also I think it is better to add a XTAL frequency check.
The PLL values are only valid with a 40MHz crystal.
Any other comments for v2?
Greats,
René
>
> Thanks
> Andrew
^ permalink raw reply
* Re: [PATCH v2 bpf-next 09/11] selftests/bpf: switch BPF_ANNOTATE_KV_PAIR tests to BTF-defined maps
From: Song Liu @ 2019-06-17 21:41 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, daniel@iogearbox.net,
netdev@vger.kernel.org, bpf@vger.kernel.org, Kernel Team
In-Reply-To: <20190617192700.2313445-10-andriin@fb.com>
> On Jun 17, 2019, at 12:26 PM, Andrii Nakryiko <andriin@fb.com> wrote:
>
> Switch tests that already rely on BTF to BTF-defined map definitions.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
For 09 to 11:
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: mediatek: Add MT7621 TRGMII mode support
From: Andrew Lunn @ 2019-06-17 21:44 UTC (permalink / raw)
To: René van Dorst
Cc: Sean Wang, Florian Fainelli, David S . Miller, Matthias Brugger,
Vivien Didelot, netdev, john, linux-mediatek, linux-mips
In-Reply-To: <20190617213312.Horde.fcb9-g80Zzfd-IMC8EQy50h@www.vdorst.com>
On Mon, Jun 17, 2019 at 09:33:12PM +0000, René van Dorst wrote:
> Quoting Andrew Lunn <andrew@lunn.ch>:
>
> >On Sun, Jun 16, 2019 at 08:20:08PM +0200, René van Dorst wrote:
> >>Like many other mediatek SOCs, the MT7621 SOC and the internal MT7530
> >>switch both
> >>supports TRGMII mode. MT7621 TRGMII speed is 1200MBit.
> >
> >Hi René
> >
>
> Hi Andrew,
>
> >Is TRGMII used only between the SoC and the Switch? Or does external
> >ports of the switch also support 1200Mbit/s? If external ports support
> >this, what does ethtool show for Speed?
>
> Only the first GMAC of the SOC and port 6 of the switch supports this mode.
> The switch can be internal in the SOC but also a separate chip.
>
> PHYLINK and ethertool reports the link as 1Gbit.
> The link is fixed-link with speed = 1000.
>
> dmesg output with unposted PHYLINK patches:
> [ 5.236763] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
> [ 5.249813] mt7530 mdio-bus:1f: phylink_mac_config:
> mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1 an=1
> [ 6.389435] mtk_soc_eth 1e100000.ethernet eth0: phylink_mac_config:
> mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1 an=1
With PHYLINK, you can probably set the fixed link to the true 1.2Gbps.
> # ethtool eth0
> Settings for eth0:
> Supported ports: [ MII ]
> Supported link modes: 1000baseT/Full
> Supported pause frame use: No
> Supports auto-negotiation: No
> Supported FEC modes: Not reported
> Advertised link modes: 1000baseT/Full
> Advertised pause frame use: No
> Advertised auto-negotiation: No
> Advertised FEC modes: Not reported
> Speed: 1000Mb/s
We could consider adding 1200BaseT/Full?
Andrew
^ permalink raw reply
* Re: [PATCH v3] net: ipv4: move tcp_fastopen server side code to SipHash library
From: Eric Dumazet @ 2019-06-17 22:09 UTC (permalink / raw)
To: David Miller
Cc: Eric Dumazet, Ard Biesheuvel, netdev, linux-crypto, Herbert Xu,
ebiggers, Alexey Kuznetsov, Hideaki YOSHIFUJI, Jason Baron,
Christoph Paasch, David Laight, Yuchung Cheng
In-Reply-To: <20190617.135712.1886619234506210997.davem@davemloft.net>
On Mon, Jun 17, 2019 at 1:57 PM David Miller <davem@davemloft.net> wrote:
>
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 17 Jun 2019 10:00:28 -0700
>
> > All our fastopen packetdrill tests pass (after I changed all the cookie values in them)
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> I'm going to apply this to net-next, I want it to sit there for a
> while.
Yes, the patch only applies cleanly on net-next anyway :)
Thanks.
^ permalink raw reply
* Re: [PATCH net-next v2 1/3] net: sched: add mpls manipulation actions to TC
From: John Hurley @ 2019-06-17 22:09 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David Miller, Jiri Pirko,
Davide Caratti, Simon Horman, Jakub Kicinski, oss-drivers
In-Reply-To: <CAM_iQpW+bnMG+43cyMJuLYCqEKM4jk5LbxGtsHFOFY=Ha7nZfA@mail.gmail.com>
On Mon, Jun 17, 2019 at 10:18 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Fri, Jun 14, 2019 at 3:56 PM John Hurley <john.hurley@netronome.com> wrote:
> >
> > On Fri, Jun 14, 2019 at 5:59 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Thu, Jun 13, 2019 at 10:44 AM John Hurley <john.hurley@netronome.com> wrote:
> > > > +static inline void tcf_mpls_set_eth_type(struct sk_buff *skb, __be16 ethertype)
> > > > +{
> > > > + struct ethhdr *hdr = eth_hdr(skb);
> > > > +
> > > > + skb_postpull_rcsum(skb, &hdr->h_proto, ETH_TLEN);
> > > > + hdr->h_proto = ethertype;
> > > > + skb_postpush_rcsum(skb, &hdr->h_proto, ETH_TLEN);
> > >
> > > So you just want to adjust the checksum with the new ->h_proto
> > > value. please use a right csum API, rather than skb_post*_rcsum().
> > >
> >
> > Hi Cong,
> > Yes, I'm trying to maintain the checksum value if checksum complete
> > has been set.
> > The function above pulls the old eth type out of the checksum value
> > (if it is checksum complete), updates the eth type, and pushes the new
> > eth type into the checksum.
> > This passes my tests on the checksum.
> > I couldn't see an appropriate function to do this other than
> > recalculating the whole thing.
> > Maybe I missed something?
>
> I never say it is wrong, I mean to say using a csum API is more
> clear. Please look into checksum API's, there are many options
> for different scenarios, there must be one serves your purpose.
>
Ok, I'll have another look here and see if I can get something more appropriate.
Thanks
> Thanks.
^ permalink raw reply
* Re: [RFC net-next 1/2] net: sched: refactor reinsert action
From: John Hurley @ 2019-06-17 22:11 UTC (permalink / raw)
To: Edward Cree
Cc: Linux Netdev List, David Miller, Florian Westphal,
Jamal Hadi Salim, Simon Horman, Jakub Kicinski, oss-drivers
In-Reply-To: <dbd77b82-5951-8512-bc9d-e47abd400be3@solarflare.com>
On Mon, Jun 17, 2019 at 7:44 PM Edward Cree <ecree@solarflare.com> wrote:
>
> On 14/06/2019 15:33, John Hurley wrote:
> > 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.
> Possibly a dumb question, but why does this need a new CONSUMED rather
> than, say, taking an additional ref and returning TC_ACT_STOLEN?
>
> Apart from that, the series lgtm.
>
Thanks for comments, Ed.
The CONSUMED was to replace the REINSERT function but yes, this is
probably not required now.
I can respin
> -Ed
^ permalink raw reply
* Re: [PATCH v2 bpf-next 00/11] BTF-defined BPF map definitions
From: Daniel Borkmann @ 2019-06-17 22:17 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: andrii.nakryiko, ast, netdev, bpf, kernel-team, jakub.kicinski,
joe
In-Reply-To: <30a2c470-5057-bd96-1889-e77fd5536960@iogearbox.net>
On 06/17/2019 11:17 PM, Daniel Borkmann wrote:
> On 06/17/2019 09:26 PM, Andrii Nakryiko wrote:
>> This patch set implements initial version (as discussed at LSF/MM2019
>> conference) of a new way to specify BPF maps, relying on BTF type information,
>> which allows for easy extensibility, preserving forward and backward
>> compatibility. See details and examples in description for patch #6.
>>
>> [0] contains an outline of follow up extensions to be added after this basic
>> set of features lands. They are useful by itself, but also allows to bring
>> libbpf to feature-parity with iproute2 BPF loader. That should open a path
>> forward for BPF loaders unification.
>>
>> Patch #1 centralizes commonly used min/max macro in libbpf_internal.h.
>> Patch #2 extracts .BTF and .BTF.ext loading loging from elf_collect().
>> Patch #3 simplifies elf_collect() error-handling logic.
>> Patch #4 refactors map initialization logic into user-provided maps and global
>> data maps, in preparation to adding another way (BTF-defined maps).
>> Patch #5 adds support for map definitions in multiple ELF sections and
>> deprecates bpf_object__find_map_by_offset() API which doesn't appear to be
>> used anymore and makes assumption that all map definitions reside in single
>> ELF section.
>> Patch #6 splits BTF intialization from sanitization/loading into kernel to
>> preserve original BTF at the time of map initialization.
>> Patch #7 adds support for BTF-defined maps.
>> Patch #8 adds new test for BTF-defined map definition.
>> Patches #9-11 convert test BPF map definitions to use BTF way.
LGTM as a base, applied 1-10 as per Stanislav's concern, added Song's Ack to
patch 10, and fixed up typos in patch 2 while at it.
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: mediatek: Add MT7621 TRGMII mode support
From: Florian Fainelli @ 2019-06-17 22:19 UTC (permalink / raw)
To: René van Dorst, Andrew Lunn
Cc: Sean Wang, David S . Miller, Matthias Brugger, Vivien Didelot,
netdev, john, linux-mediatek, linux-mips
In-Reply-To: <20190617213312.Horde.fcb9-g80Zzfd-IMC8EQy50h@www.vdorst.com>
On 6/17/19 2:33 PM, René van Dorst wrote:
> Quoting Andrew Lunn <andrew@lunn.ch>:
>
>> On Sun, Jun 16, 2019 at 08:20:08PM +0200, René van Dorst wrote:
>>> Like many other mediatek SOCs, the MT7621 SOC and the internal MT7530
>>> switch both
>>> supports TRGMII mode. MT7621 TRGMII speed is 1200MBit.
>>
>> Hi René
>>
>
> Hi Andrew,
>
>> Is TRGMII used only between the SoC and the Switch? Or does external
>> ports of the switch also support 1200Mbit/s? If external ports support
>> this, what does ethtool show for Speed?
>
> Only the first GMAC of the SOC and port 6 of the switch supports this mode.
> The switch can be internal in the SOC but also a separate chip.
>
> PHYLINK and ethertool reports the link as 1Gbit.
> The link is fixed-link with speed = 1000.
>
> dmesg output with unposted PHYLINK patches:
> [ 5.236763] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
> [ 5.249813] mt7530 mdio-bus:1f: phylink_mac_config:
> mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1 an=1
> [ 6.389435] mtk_soc_eth 1e100000.ethernet eth0: phylink_mac_config:
> mode=fixed/trgmii/1Gbps/Full adv=00,00000000,00000220 pause=12 link=1 an=1
>
> # ethtool eth0
> Settings for eth0:
> Supported ports: [ MII ]
> Supported link modes: 1000baseT/Full
> Supported pause frame use: No
> Supports auto-negotiation: No
> Supported FEC modes: Not reported
> Advertised link modes: 1000baseT/Full
> Advertised pause frame use: No
> Advertised auto-negotiation: No
> Advertised FEC modes: Not reported
> Speed: 1000Mb/s
> Duplex: Full
> Port: MII
> PHYAD: 0
> Transceiver: internal
> Auto-negotiation: on
> Current message level: 0x000000ff (255)
> drv probe link timer ifdown ifup rx_err
> tx_err
> Link detected: yes
>
>
>
> I already have report from a MT7623 user that this patch gives issues.
>
> I send v2 of the patch if I fixed that issue.
>
> Also I think it is better to add a XTAL frequency check.
> The PLL values are only valid with a 40MHz crystal.
>
> Any other comments for v2?
Looks good to me otherwise.
--
Florian
^ permalink raw reply
* [PATCH v6 2/8] linux/net.h: use unique identifier for each struct _ddebug
From: Rasmus Villemoes @ 2019-06-17 22:20 UTC (permalink / raw)
To: Ingo Molnar, Andrew Morton
Cc: Jason Baron, Nathan Chancellor, Nick Desaulniers, linux-kernel,
Rasmus Villemoes, netdev
In-Reply-To: <20190617222034.10799-1-linux@rasmusvillemoes.dk>
Changes on x86-64 later in this series require that all struct _ddebug
descriptors in a translation unit uses distinct identifiers. Realize
that for net_dbg_ratelimited by generating such an identifier via
__UNIQUE_ID and pass that to an extra level of macros.
No functional change.
Cc: netdev@vger.kernel.org
Acked-by: Jason Baron <jbaron@akamai.com>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
include/linux/net.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/net.h b/include/linux/net.h
index f7d672cf25b5..a080ede95b47 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -264,7 +264,7 @@ do { \
#define net_info_ratelimited(fmt, ...) \
net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
#if defined(CONFIG_DYNAMIC_DEBUG)
-#define net_dbg_ratelimited(fmt, ...) \
+#define _net_dbg_ratelimited(descriptor, fmt, ...) \
do { \
DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
if (DYNAMIC_DEBUG_BRANCH(descriptor) && \
@@ -272,6 +272,8 @@ do { \
__dynamic_pr_debug(&descriptor, pr_fmt(fmt), \
##__VA_ARGS__); \
} while (0)
+#define net_dbg_ratelimited(fmt, ...) \
+ _net_dbg_ratelimited(__UNIQUE_ID(ddebug), fmt, ##__VA_ARGS__)
#elif defined(DEBUG)
#define net_dbg_ratelimited(fmt, ...) \
net_ratelimited_function(pr_debug, fmt, ##__VA_ARGS__)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 bpf-next 09/11] selftests/bpf: switch BPF_ANNOTATE_KV_PAIR tests to BTF-defined maps
From: Daniel Borkmann @ 2019-06-17 22:20 UTC (permalink / raw)
To: Song Liu, Andrii Nakryiko
Cc: Andrii Nakryiko, Alexei Starovoitov, netdev@vger.kernel.org,
bpf@vger.kernel.org, Kernel Team
In-Reply-To: <4DCC6EC0-9B6A-49D9-8664-ADC557C5DD36@fb.com>
On 06/17/2019 11:41 PM, Song Liu wrote:
>> On Jun 17, 2019, at 12:26 PM, Andrii Nakryiko <andriin@fb.com> wrote:
>>
>> Switch tests that already rely on BTF to BTF-defined map definitions.
>>
>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> For 09 to 11:
>
> Acked-by: Song Liu <songliubraving@fb.com>
I've added it to patch 10 by hand given the manual labor for dropping 11 anyway.
Please keep in mind that patchwork doesn't understand/propagate 'for 09 to 11',
and explicitly ack in future, thanks.
^ permalink raw reply
* Re: [PATCH iproute2 v2 0/3] refactor the cmd_exec()
From: Matteo Croce @ 2019-06-17 22:20 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, Stephen Hemminger, David Ahern
In-Reply-To: <CAGnkfhwH-4+Ov+QRBZaQaHnnbTczpavuV8_yJBg=GOHSLD0pQw@mail.gmail.com>
On Sat, Jun 15, 2019 at 4:06 PM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Fri, Jun 14, 2019 at 4:36 PM David Ahern <dsahern@gmail.com> wrote:
> >
> > On 6/11/19 10:10 AM, Matteo Croce wrote:
> > > Refactor the netns and ipvrf code so less steps are needed to exec commands
> > > in a netns or a VRF context.
> > > Also remove some code which became dead. bloat-o-meter output:
> > >
> >
> > This breaks the vrf reset after namespace switch
> >
> >
> > # ip vrf ls
> > Name Table
> > -----------------------
> > red 1001
> >
> > Set shell into vrf red context:
> > # ip vrf exec red bash
> >
> > Add new namespace and do netns exec:
> > # ip netns add foo
> > # ./ip netns exec foo bash
> >
> > Check the vrf id:
> > # ip vrf id
> > red
> >
> > With the current command:
> > # ip netns exec foo bash
> > # ip vrf id
> > <nothing - no vrf bind>
>
> Hi David,
>
> if the vrf needs to be reset after a netns change, why don't we do in
> netns_switch()?
> This way all code paths will be covered.
>
> Cheers,
> --
> Matteo Croce
> per aspera ad upstream
Hi David,
I looked for every occourrence of netns_switch():
bridge/bridge.c: if (netns_switch(argv[1]))
include/namespace.h:int netns_switch(char *netns);
ip/ip.c: if (netns_switch(argv[1]))
ip/ipnetns.c: if (netns_switch(argv[0]))
lib/namespace.c:int netns_switch(char *name)
lib/utils.c: if (netns_switch(nsname))
misc/ss.c: if (netns_switch(optarg))
tc/tc.c: if (netns_switch(argv[1]))
the ones in {ss,tc,bridge,ip}.c are obviously handling the '-n netns'
command line option.
my question here is: should the VRF association be reset in all these cases?
If we need to reset, we should really call vrf_reset() from netns_switch().
If don't, I can add the missing vrf_reset() in ipnetns.c but I'm
curious to know what can happen to change netns and keep VRF
associations belonging to another netns.
Regards,
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* Re: [PATCH iproute2 v2 0/3] refactor the cmd_exec()
From: David Ahern @ 2019-06-17 22:24 UTC (permalink / raw)
To: Matteo Croce; +Cc: netdev, Stephen Hemminger, David Ahern
In-Reply-To: <CAGnkfhz940SbWpKau0j13rzqH7Zw7BKTStuCcSeg9EQBLeaEOg@mail.gmail.com>
On 6/17/19 4:20 PM, Matteo Croce wrote:
>
> I looked for every occourrence of netns_switch():
>
> bridge/bridge.c: if (netns_switch(argv[1]))
> include/namespace.h:int netns_switch(char *netns);
> ip/ip.c: if (netns_switch(argv[1]))
> ip/ipnetns.c: if (netns_switch(argv[0]))
> lib/namespace.c:int netns_switch(char *name)
> lib/utils.c: if (netns_switch(nsname))
> misc/ss.c: if (netns_switch(optarg))
> tc/tc.c: if (netns_switch(argv[1]))
>
> the ones in {ss,tc,bridge,ip}.c are obviously handling the '-n netns'
> command line option.
> my question here is: should the VRF association be reset in all these cases?
> If we need to reset, we should really call vrf_reset() from netns_switch().
> If don't, I can add the missing vrf_reset() in ipnetns.c but I'm
> curious to know what can happen to change netns and keep VRF
> associations belonging to another netns.
>
The VRF reset is needed on a netns exec - ie., running a command in a
network namespace.
Any netns_switch that only involves sending a netlink command in a
different namespace does not need the reset.
^ permalink raw reply
* Re: [PATCH] netfilter: nft_paylaod: add base type NFT_PAYLOAD_LL_HEADER_NO_TAG
From: Pablo Neira Ayuso @ 2019-06-17 22:30 UTC (permalink / raw)
To: Florian Westphal; +Cc: wenxu, netfilter-devel, netdev
In-Reply-To: <20190610094433.3wjmpfiph7iwguan@breakpoint.cc>
Hi Florian,
On Mon, Jun 10, 2019 at 11:44:33AM +0200, Florian Westphal wrote:
> wenxu@ucloud.cn <wenxu@ucloud.cn> wrote:
> > From: wenxu <wenxu@ucloud.cn>
> >
> > nft add rule bridge firewall rule-100-ingress ip protocol icmp drop
>
> nft --debug=netlink add rule bridge firewall rule-100-ingress ip protocol icmp drop
> bridge firewall rule-100-ingress
> [ payload load 2b @ link header + 12 => reg 1 ]
> [ cmp eq reg 1 0x00000008 ]
> [ payload load 1b @ network header + 9 => reg 1 ]
> [ cmp eq reg 1 0x00000001 ]
> [ immediate reg 0 drop ]
>
> so problem is that nft inserts a dependency on the ethernet protocol
> type (0x800).
>
> But when vlan is involved, that will fail to compare.
>
> It would also fail for qinq etc.
>
> Because of vlan tag offload, the rule about will probably already work
> just fine when nft userspace is patched to insert the dependency based
> on 'meta protocol'. Can you see if this patch works?
>
> Subject: Change bridge l3 dependency to meta protocol
>
> This examines skb->protocol instead of ethernet header type, which
> might be different when vlan is involved.
>
> nft payload expression will re-insert the vlan tag so ether type
> will not be ETH_P_IP.
>
> ---
> src/meta.c | 6 +++++-
> src/payload.c | 20 ++++++++++++++++++++
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/src/meta.c b/src/meta.c
> index 583e790ff47d..1e8964eb48c4 100644
> --- a/src/meta.c
> +++ b/src/meta.c
> @@ -539,7 +539,11 @@ static void meta_expr_pctx_update(struct proto_ctx *ctx,
> proto_ctx_update(ctx, PROTO_BASE_TRANSPORT_HDR, &expr->location, desc);
> break;
> case NFT_META_PROTOCOL:
> - if (h->base < PROTO_BASE_NETWORK_HDR && ctx->family != NFPROTO_NETDEV)
> + if (h->base != PROTO_BASE_LL_HDR)
> + return;
> +
> + if (ctx->family != NFPROTO_NETDEV &&
> + ctx->family != NFPROTO_BRIDGE)
> return;
>
> desc = proto_find_upper(h->desc, ntohs(mpz_get_uint16(right->value)));
> diff --git a/src/payload.c b/src/payload.c
> index 6a8118ece890..c99bb2f69977 100644
> --- a/src/payload.c
> +++ b/src/payload.c
> @@ -18,6 +18,7 @@
> #include <net/if_arp.h>
> #include <arpa/inet.h>
> #include <linux/netfilter.h>
> +#include <linux/if_ether.h>
>
> #include <rule.h>
> #include <expression.h>
> @@ -307,6 +308,19 @@ payload_gen_special_dependency(struct eval_ctx *ctx, const struct expr *expr)
> return NULL;
> }
>
> +static const struct proto_desc proto_metaeth = {
> + .name = "ethmeta",
> + .base = PROTO_BASE_LL_HDR,
> + .protocols = {
> + PROTO_LINK(__constant_htons(ETH_P_IP), &proto_ip),
> + PROTO_LINK(__constant_htons(ETH_P_ARP), &proto_arp),
> + PROTO_LINK(__constant_htons(ETH_P_IPV6), &proto_ip6),
> + },
> + .templates = {
> + [0] = PROTO_META_TEMPLATE("protocol", ðertype_type, NFT_META_PROTOCOL, 16),
> + },
> +};
> +
> /**
> * payload_gen_dependency - generate match expression on payload dependency
> *
> @@ -369,6 +383,12 @@ int payload_gen_dependency(struct eval_ctx *ctx, const struct expr *expr,
> "no %s protocol specified",
> proto_base_names[expr->payload.base - 1]);
>
> + if (ctx->pctx.family == NFPROTO_BRIDGE && desc == &proto_eth) {
> + if (expr->payload.desc == &proto_ip ||
> + expr->payload.desc == &proto_ip6)
> + desc = &proto_metaeth;
> + }i
Is this sufficient to restrict the matching? Is this still buggy from
ingress?
I wonder if an explicit NFT_PAYLOAD_CHECK_VLAN flag would be useful in
the kernel, if so we could rename NFTA_PAYLOAD_CSUM_FLAGS to
NFTA_PAYLOAD_FLAGS and place it there. Just an idea.
Thanks!
^ permalink raw reply
* Re: [PATCH net v2 1/2] net: netem: fix backlog accounting for corrupted GSO frames
From: Cong Wang @ 2019-06-17 22:32 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, Stephen Hemminger, Jamal Hadi Salim, Jiri Pirko,
netem, Linux Kernel Network Developers, oss-drivers, Eric Dumazet,
posk, Neil Horman
In-Reply-To: <20190617181111.5025-2-jakub.kicinski@netronome.com>
On Mon, Jun 17, 2019 at 11:11 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> When GSO frame has to be corrupted netem uses skb_gso_segment()
> to produce the list of frames, and re-enqueues the segments one
> by one. The backlog length has to be adjusted to account for
> new frames.
>
> The current calculation is incorrect, leading to wrong backlog
> lengths in the parent qdisc (both bytes and packets), and
> incorrect packet backlog count in netem itself.
>
> Parent backlog goes negative, netem's packet backlog counts
> all non-first segments twice (thus remaining non-zero even
> after qdisc is emptied).
>
> Move the variables used to count the adjustment into local
> scope to make 100% sure they aren't used at any stage in
> backports.
>
> Fixes: 6071bd1aa13e ("netem: Segment GSO packets on enqueue")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
Looks good!
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
^ 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