* RE: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
From: Parav Pandit @ 2019-09-25 17:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: netdev@vger.kernel.org, Leon Romanovsky, Eli Cohen, Doug Ledford,
Jason Gunthorpe, linux-rdma@vger.kernel.org,
kernel-janitors@vger.kernel.org
In-Reply-To: <20190924091823.GM20699@kadam>
Hi Dan,
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Tuesday, September 24, 2019 4:21 AM
> To: Parav Pandit <parav@mellanox.com>
> Cc: netdev@vger.kernel.org; Leon Romanovsky <leon@kernel.org>; Eli Cohen
> <eli@mellanox.com>; Doug Ledford <dledford@redhat.com>; Jason Gunthorpe
> <jgg@ziepe.ca>; linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo()
>
> On Thu, Apr 25, 2019 at 06:15:13AM +0000, Parav Pandit wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dan Carpenter <dan.carpenter@oracle.com>
> > > Sent: Wednesday, April 24, 2019 9:08 AM
> > > To: Parav Pandit <parav@mellanox.com>; netdev@vger.kernel.org
> > > Cc: Leon Romanovsky <leon@kernel.org>; Eli Cohen <eli@mellanox.com>;
> > > Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> > > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org
> > > Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from
> > > do_setvfinfo()
> > >
> > > I think I'm just going to ask netdev for an opinion on this. It
> > > could be that we're just reading the code wrong...
> > >
> > > I'm getting a lot of Smatch warning about buffer underflows. The
> > > problem is that Smatch marks everything from nla_data() as unknown
> > > and untrusted user data. In do_setvfinfo() we get the "->vf" values
> > > from nla_data(). It starts as u32, but all the function pointers in
> > > net_device_ops use it as a signed integer. Most of the functions
> > > return -EINVAL if "vf" is negative but there are at least 48 which
> > > potentially use negative values as an offset into an array.
> > >
> > > To me making "vf" a u32 throughout seems like a good idea but it's
> > > an extensive patch and I'm not really able to test it at all.
> >
> > I will be try to get you patch early next week for core and in mlx5,
> > tested on mlx5 VFs, that possibly you can carry forward?
>
> Whatever happened with this?
>
I had internal few patches that Leon and Saeed reviewed, but it needs more rework at core and driver level.
I haven't had chance to finish it.
> regards,
> dan carpenter
^ permalink raw reply
* Re: [PATCH net v3] net/sched: cbs: Fix not adding cbs instance to list
From: Vinicius Costa Gomes @ 2019-09-25 17:31 UTC (permalink / raw)
To: Guedes, Andre
Cc: Linux Kernel Network Developers, jhs@mojatatu.com,
xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net
In-Reply-To: <99755D97-F59A-4E68-87AE-6CE88EDE66A3@intel.com>
Hi Andre,
"Guedes, Andre" <andre.guedes@intel.com> writes:
> Hi Vinicius,
>
>> On Sep 23, 2019, at 10:04 PM, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>
>> The problem happens because that when offloading is enabled, the cbs
>> instance is not added to the list.
>>
>> Also, the current code doesn't handle correctly the case when offload
>> is disabled without removing the qdisc: if the link speed changes the
>> credit calculations will be wrong. When we create the cbs instance
>> with offloading enabled, it's not added to the notification list, when
>> later we disable offloading, it's not in the list, so link speed
>> changes will not affect it.
>>
>> The solution for both issues is the same, add the cbs instance being
>> created unconditionally to the global list, even if the link state
>> notification isn't useful "right now".
>
> I believe we could fix both issues described above and still don’t
> notify the qdisc about link state if we handled the list
> insertion/removal in cbs_change() instead.
>
> Reading the cbs code more carefully, it seems it would be beneficial
> to refactor the offload handling. For example, we currently init the
> qdisc_watchdog even if it’s not useful when offload is enabled. Now,
> we’re going to notify the qdisc even if it’s not useful too.
I like your idea, but even after reading your email and the code a
couple of times, I couldn't come up with anything quickly that wouldn't
complicate things (i.e. add more code), I would need to experiment a
bit. (btw, qdisc_watchdog_init() is just initializing some fields in a
struct, and the notification part should be quite rare in practice).
So my suggestion is to keep this patch as is, as it solves a real crash
that a colleague faced. Later, we can try and simplify things even more.
Cheers,
--
Vinicius
P.S.: I think I am still a bit traumatized but getting the init() and
destroy() right were the hardest parts when we were trying to uptream
this. That's why I am hesitant about adding more code to those flows.
^ permalink raw reply
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
From: Florian Fainelli @ 2019-09-25 17:31 UTC (permalink / raw)
To: Jose Abreu, David Miller
Cc: thierry.reding@gmail.com, peppe.cavallaro@st.com,
alexandre.torgue@st.com, jonathanh@nvidia.com, bbiswas@nvidia.com,
netdev@vger.kernel.org, linux-tegra@vger.kernel.org
In-Reply-To: <BN8PR12MB32667F9FDDB2161E9B63C1AFD3870@BN8PR12MB3266.namprd12.prod.outlook.com>
On 9/25/19 4:46 AM, Jose Abreu wrote:
> From: Jose Abreu <joabreu@synopsys.com>
> Date: Sep/25/2019, 12:41:04 (UTC+00:00)
>
>> From: David Miller <davem@davemloft.net>
>> Date: Sep/25/2019, 12:33:53 (UTC+00:00)
>>
>>> From: Jose Abreu <Jose.Abreu@synopsys.com>
>>> Date: Wed, 25 Sep 2019 10:44:53 +0000
>>>
>>>> From: David Miller <davem@davemloft.net>
>>>> Date: Sep/24/2019, 20:45:08 (UTC+00:00)
>>>>
>>>>> From: Thierry Reding <thierry.reding@gmail.com>
>>>>> Date: Fri, 20 Sep 2019 19:00:34 +0200
>>>>>
>>>>> Also, you're now writing to the high 32-bits unconditionally, even when
>>>>> it will always be zero because of 32-bit addressing. That looks like
>>>>> a step backwards to me.
>>>>
>>>> Don't agree. As per previous discussions and as per my IP knowledge, if
>>>> EAME is not enabled / not supported the register can still be written.
>>>> This is not fast path and will not impact any remaining operation. Can
>>>> you please explain what exactly is the concern about this ?
>>>>
>>>> Anyway, this is an important feature for performance so I hope Thierry
>>>> re-submits this once -next opens and addressing the review comments.
>>>
>>> Perhaps I misunderstand the context, isn't this code writing the
>>> descriptors for every packet?
>>
>> No, its just setting up the base address for the descriptors which is
>> done in open(). The one that's in the fast path is the tail address,
>> which is always the lower 32 bits.
>
> Oops, sorry. Indeed it's done in refill operation in function
> dwmac4_set_addr() for rx/tx which is fast path so you do have a point
> that I was not seeing. Thanks for bringing this up!
>
> Now, the point would be:
> a) Is it faster to have an condition check in dwmac4_set_addr(), or
> b) Always write to descs the upper 32 bits. Which always exists in the
> IP and is a standard write to memory.
The way I would approach it (as done in bcmgenet.c) is that if the
platform both has CONFIG_PHYS_ADDR_T_64BIT=y and supports > 32-bits
addresses, then you write the upper 32-bits otherwise, you do not. Given
you indicate that the registers are safe to write regardless, then maybe
just the check on CONFIG_PHYS_ADDR_T_64BIT is enough for your case. The
rationale in my case is that register writes to on-chip descriptors are
fairly expensive (~200ns per operation) and get in the hot-path.
The CONFIG_PHYS_ADDR_T_64BIT check addresses both native 64-bit
platforms (e.g.: ARM64) and those that do support LPAE (ARM LPAE for
instance).
--
Florian
^ permalink raw reply
* [PATCH] nfp: flower: prevent memory leak in nfp_flower_spawn_phy_reprs
From: Navid Emamdoost @ 2019-09-25 18:24 UTC (permalink / raw)
Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Jakub Kicinski,
David S. Miller, John Hurley, Simon Horman,
Pieter Jansen van Vuuren, Fred Lotter, oss-drivers, netdev,
linux-kernel
In nfp_flower_spawn_phy_reprs, in the for loop over eth_tbl if any of
intermediate allocations or initializations fail memory is leaked.
requiered releases are added.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
drivers/net/ethernet/netronome/nfp/flower/main.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c b/drivers/net/ethernet/netronome/nfp/flower/main.c
index 7a20447cca19..91a47899220f 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
@@ -515,6 +515,7 @@ nfp_flower_spawn_phy_reprs(struct nfp_app *app, struct nfp_flower_priv *priv)
repr_priv = kzalloc(sizeof(*repr_priv), GFP_KERNEL);
if (!repr_priv) {
err = -ENOMEM;
+ nfp_repr_free(repr);
goto err_reprs_clean;
}
@@ -525,11 +526,13 @@ nfp_flower_spawn_phy_reprs(struct nfp_app *app, struct nfp_flower_priv *priv)
port = nfp_port_alloc(app, NFP_PORT_PHYS_PORT, repr);
if (IS_ERR(port)) {
err = PTR_ERR(port);
+ kfree(repr_priv);
nfp_repr_free(repr);
goto err_reprs_clean;
}
err = nfp_port_init_phy_port(app->pf, app, port, i);
if (err) {
+ kfree(repr_priv);
nfp_port_free(port);
nfp_repr_free(repr);
goto err_reprs_clean;
@@ -542,6 +545,7 @@ nfp_flower_spawn_phy_reprs(struct nfp_app *app, struct nfp_flower_priv *priv)
err = nfp_repr_init(app, repr,
cmsg_port_id, port, priv->nn->dp.netdev);
if (err) {
+ kfree(repr_priv);
nfp_port_free(port);
nfp_repr_free(repr);
goto err_reprs_clean;
--
2.17.1
^ permalink raw reply related
* [PATCH bpf] libbpf: fix false uninitialized variable warning
From: Andrii Nakryiko @ 2019-09-25 18:30 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Some compilers emit warning for potential uninitialized next_id usage.
The code is correct, but control flow is too complicated for some
compilers to figure this out. Re-initialize next_id to satisfy
compiler.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/btf_dump.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 715967762312..84b0661db7f3 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1140,60 +1140,61 @@ static void btf_dump_emit_type_chain(struct btf_dump *d,
case BTF_KIND_ARRAY: {
const struct btf_array *a = btf_array(t);
const struct btf_type *next_t;
__u32 next_id;
bool multidim;
/*
* GCC has a bug
* (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=8354)
* which causes it to emit extra const/volatile
* modifiers for an array, if array's element type has
* const/volatile modifiers. Clang doesn't do that.
* In general, it doesn't seem very meaningful to have
* a const/volatile modifier for array, so we are
* going to silently skip them here.
*/
while (decls->cnt) {
next_id = decls->ids[decls->cnt - 1];
next_t = btf__type_by_id(d->btf, next_id);
if (btf_is_mod(next_t))
decls->cnt--;
else
break;
}
if (decls->cnt == 0) {
btf_dump_emit_name(d, fname, last_was_ptr);
btf_dump_printf(d, "[%u]", a->nelems);
return;
}
+ next_id = decls->ids[decls->cnt - 1];
next_t = btf__type_by_id(d->btf, next_id);
multidim = btf_is_array(next_t);
/* we need space if we have named non-pointer */
if (fname[0] && !last_was_ptr)
btf_dump_printf(d, " ");
/* no parentheses for multi-dimensional array */
if (!multidim)
btf_dump_printf(d, "(");
btf_dump_emit_type_chain(d, decls, fname, lvl);
if (!multidim)
btf_dump_printf(d, ")");
btf_dump_printf(d, "[%u]", a->nelems);
return;
}
case BTF_KIND_FUNC_PROTO: {
const struct btf_param *p = btf_params(t);
__u16 vlen = btf_vlen(t);
int i;
btf_dump_emit_mods(d, decls);
if (decls->cnt) {
btf_dump_printf(d, " (");
btf_dump_emit_type_chain(d, decls, fname, lvl);
btf_dump_printf(d, ")");
} else {
btf_dump_emit_name(d, fname, last_was_ptr);
}
btf_dump_printf(d, "(");
/*
* Clang for BPF target generates func_proto with no
--
2.17.1
^ permalink raw reply related
* [PATCH] net: flow_offload: fix memory leak in nfp_abm_u32_knode_replace
From: Navid Emamdoost @ 2019-09-25 18:34 UTC (permalink / raw)
Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Jakub Kicinski,
David S. Miller, Pablo Neira Ayuso, Colin Ian King, oss-drivers,
netdev, linux-kernel
In nfp_abm_u32_knode_replace if the allocation for match fails it should
go to the error handling instead of returning.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
drivers/net/ethernet/netronome/nfp/abm/cls.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/abm/cls.c b/drivers/net/ethernet/netronome/nfp/abm/cls.c
index 23ebddfb9532..32eaab99d96c 100644
--- a/drivers/net/ethernet/netronome/nfp/abm/cls.c
+++ b/drivers/net/ethernet/netronome/nfp/abm/cls.c
@@ -174,7 +174,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
struct nfp_abm_u32_match *match = NULL, *iter;
unsigned int tos_off;
u8 mask, val;
- int err;
+ int err, ret = -EOPNOTSUPP;
if (!nfp_abm_u32_check_knode(alink->abm, knode, proto, extack))
goto err_delete;
@@ -204,8 +204,11 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
if (!match) {
match = kzalloc(sizeof(*match), GFP_KERNEL);
- if (!match)
- return -ENOMEM;
+ if (!match) {
+ ret = -ENOMEM;
+ goto err_delete;
+ }
+
list_add(&match->list, &alink->dscp_map);
}
match->handle = knode->handle;
@@ -221,7 +224,7 @@ nfp_abm_u32_knode_replace(struct nfp_abm_link *alink,
err_delete:
nfp_abm_u32_knode_delete(alink, knode);
- return -EOPNOTSUPP;
+ return ret;
}
static int nfp_abm_setup_tc_block_cb(enum tc_setup_type type,
--
2.17.1
^ permalink raw reply related
* [PATCH bpf] selftests/bpf: delete unused variables in test_sysctl
From: Andrii Nakryiko @ 2019-09-25 18:36 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Remove no longer used variables and avoid compiler warnings.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/test_sysctl.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
index 4f8ec1f10a80..a320e3844b17 100644
--- a/tools/testing/selftests/bpf/test_sysctl.c
+++ b/tools/testing/selftests/bpf/test_sysctl.c
@@ -1385,7 +1385,6 @@ static int fixup_sysctl_value(const char *buf, size_t buf_len,
uint8_t raw[sizeof(uint64_t)];
uint64_t num;
} value = {};
- uint8_t c, i;
if (buf_len > sizeof(value)) {
log_err("Value is too big (%zd) to use in fixup", buf_len);
--
2.17.1
^ permalink raw reply related
* [PATCH bpf] selftests/bpf: adjust strobemeta loop to satisfy latest clang
From: Andrii Nakryiko @ 2019-09-25 18:52 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
Some recent changes in latest Clang started causing the following
warning when unrolling strobemeta test case main loop:
progs/strobemeta.h:416:2: warning: loop not unrolled: the optimizer was
unable to perform the requested transformation; the transformation might
be disabled or specified as part of an unsupported transformation
ordering [-Wpass-failed=transform-warning]
This patch simplifies loop's exit condition to depend only on constant
max iteration number (STROBE_MAX_MAP_ENTRIES), while moving early
termination logic inside the loop body. The changes are equivalent from
program logic standpoint, but fixes the warning. It also appears to
improve generated BPF code, as it fixes previously failing non-unrolled
strobemeta test cases.
Cc: Alexei Starovoitov <ast@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/progs/strobemeta.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/progs/strobemeta.h b/tools/testing/selftests/bpf/progs/strobemeta.h
index 8a399bdfd920..067eb625d01c 100644
--- a/tools/testing/selftests/bpf/progs/strobemeta.h
+++ b/tools/testing/selftests/bpf/progs/strobemeta.h
@@ -413,7 +413,10 @@ static __always_inline void *read_map_var(struct strobemeta_cfg *cfg,
#else
#pragma unroll
#endif
- for (int i = 0; i < STROBE_MAX_MAP_ENTRIES && i < map.cnt; ++i) {
+ for (int i = 0; i < STROBE_MAX_MAP_ENTRIES; ++i) {
+ if (i >= map.cnt)
+ break;
+
descr->key_lens[i] = 0;
len = bpf_probe_read_str(payload, STROBE_MAX_STR_LEN,
map.entries[i].key);
--
2.17.1
^ permalink raw reply related
* [PATCH] nfp: flower: fix memory leak in nfp_flower_spawn_vnic_reprs
From: Navid Emamdoost @ 2019-09-25 19:05 UTC (permalink / raw)
Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Jakub Kicinski,
David S. Miller, John Hurley, Simon Horman,
Pieter Jansen van Vuuren, Fred Lotter, oss-drivers, netdev,
linux-kernel
In nfp_flower_spawn_vnic_reprs in the loop if initialization or the
allocations fail memory is leaked. Appropriate releases are added.
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
drivers/net/ethernet/netronome/nfp/flower/main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c b/drivers/net/ethernet/netronome/nfp/flower/main.c
index 7a20447cca19..c20677059cb2 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/main.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
@@ -400,6 +400,7 @@ nfp_flower_spawn_vnic_reprs(struct nfp_app *app,
repr_priv = kzalloc(sizeof(*repr_priv), GFP_KERNEL);
if (!repr_priv) {
err = -ENOMEM;
+ nfp_repr_free(repr);
goto err_reprs_clean;
}
@@ -413,6 +414,7 @@ nfp_flower_spawn_vnic_reprs(struct nfp_app *app,
port = nfp_port_alloc(app, port_type, repr);
if (IS_ERR(port)) {
err = PTR_ERR(port);
+ kfree(repr_priv);
nfp_repr_free(repr);
goto err_reprs_clean;
}
@@ -433,6 +435,7 @@ nfp_flower_spawn_vnic_reprs(struct nfp_app *app,
err = nfp_repr_init(app, repr,
port_id, port, priv->nn->dp.netdev);
if (err) {
+ kfree(repr_priv);
nfp_port_free(port);
nfp_repr_free(repr);
goto err_reprs_clean;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf v2] selftests/bpf: test_progs: fix client/server race in tcp_rtt
From: Daniel Borkmann @ 2019-09-25 20:19 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: netdev, bpf, davem, ast
In-Reply-To: <20190923184112.196358-1-sdf@google.com>
On Mon, Sep 23, 2019 at 11:41:12AM -0700, Stanislav Fomichev wrote:
> This is the same problem I found earlier in test_sockopt_inherit:
> there is a race between server thread doing accept() and client
> thread doing connect(). Let's explicitly synchronize them via
> pthread conditional variable.
>
> v2:
> * don't exit from server_thread without signaling condvar,
> fixes possible issue where main() would wait forever (Andrii Nakryiko)
>
> Fixes: b55873984dab ("selftests/bpf: test BPF_SOCK_OPS_RTT_CB")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH net] bpf/xskmap: Return ERR_PTR for failure case instead of NULL.
From: Daniel Borkmann @ 2019-09-25 20:19 UTC (permalink / raw)
To: Björn Töpel
Cc: Jonathan Lemon, netdev, Alexei Starovoitov, kernel-team, bpf,
Karlsson, Magnus, syzkaller-bugs, syzbot+491c1b7565ba9069ecae
In-Reply-To: <5f85df65-0f2e-3533-9734-147b0734e254@intel.com>
On Tue, Sep 24, 2019 at 09:30:44PM +0200, Björn Töpel wrote:
> On 2019-09-24 18:25, Jonathan Lemon wrote:
> > When kzalloc() failed, NULL was returned to the caller, which
> > tested the pointer with IS_ERR(), which didn't match, so the
> > pointer was used later, resulting in a NULL dereference.
> >
> > Return ERR_PTR(-ENOMEM) instead of NULL.
> >
> > Reported-by: syzbot+491c1b7565ba9069ecae@syzkaller.appspotmail.com
> > Fixes: 0402acd683c6 ("xsk: remove AF_XDP socket from map when the socket is released")
> > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>
> Thanks Jonathan! You beat me to it! :-P
>
> Acked-by: Björn Töpel <bjorn.topel@intel.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH bpf] libbpf: fix false uninitialized variable warning
From: Daniel Borkmann @ 2019-09-25 20:27 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, andrii.nakryiko, kernel-team
In-Reply-To: <20190925183038.2755521-1-andriin@fb.com>
On Wed, Sep 25, 2019 at 11:30:38AM -0700, Andrii Nakryiko wrote:
> Some compilers emit warning for potential uninitialized next_id usage.
> The code is correct, but control flow is too complicated for some
> compilers to figure this out. Re-initialize next_id to satisfy
> compiler.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Applied, thanks!
^ permalink raw reply
* [PATCH 0/5] Netfilter fixes for net
From: Pablo Neira Ayuso @ 2019-09-25 20:29 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
Hi,
The following patchset contains Netfilter fixes for net:
1) Add NFT_CHAIN_POLICY_UNSET to replace hardcoded -1 to
specify that the chain policy is unset. The chain policy
field is actually defined as an 8-bit unsigned integer.
2) Remove always true condition reported by smatch in
chain policy check.
3) Fix element lookup on dynamic sets, from Florian Westphal.
4) Use __u8 in ebtables uapi header, from Masahiro Yamada.
5) Bogus EBUSY when removing flowtable after chain flush,
from Laura Garcia Liebana.
You can pull these changes from:
git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git
Thanks.
----------------------------------------------------------------
The following changes since commit 864668bfc374dfbf4851ec828b9049e08f9057b1:
selftests: Add test cases for `ip nexthop flush proto XX` (2019-09-19 18:35:55 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git HEAD
for you to fetch changes up to 9b05b6e11d5e93a3a517cadc12b9836e0470c255:
netfilter: nf_tables: bogus EBUSY when deleting flowtable after flush (2019-09-25 11:01:19 +0200)
----------------------------------------------------------------
Florian Westphal (1):
netfilter: nf_tables: allow lookups in dynamic sets
Laura Garcia Liebana (1):
netfilter: nf_tables: bogus EBUSY when deleting flowtable after flush
Masahiro Yamada (1):
netfilter: ebtables: use __u8 instead of uint8_t in uapi header
Pablo Neira Ayuso (2):
netfilter: nf_tables: add NFT_CHAIN_POLICY_UNSET and use it
netfilter: nf_tables_offload: fix always true policy is unset check
include/net/netfilter/nf_tables.h | 6 ++++++
include/uapi/linux/netfilter_bridge/ebtables.h | 6 +++---
net/netfilter/nf_tables_api.c | 25 ++++++++++++++++++++++---
net/netfilter/nf_tables_offload.c | 2 +-
net/netfilter/nft_flow_offload.c | 19 +++++++++++++++++++
net/netfilter/nft_lookup.c | 3 ---
usr/include/Makefile | 1 -
7 files changed, 51 insertions(+), 11 deletions(-)
^ permalink raw reply
* [PATCH 5/5] netfilter: nf_tables: bogus EBUSY when deleting flowtable after flush
From: Pablo Neira Ayuso @ 2019-09-25 20:30 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <20190925203003.20112-1-pablo@netfilter.org>
From: Laura Garcia Liebana <nevola@gmail.com>
The deletion of a flowtable after a flush in the same transaction
results in EBUSY. This patch adds an activation and deactivation of
flowtables in order to update the _use_ counter.
Signed-off-by: Laura Garcia Liebana <nevola@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tables.h | 4 ++++
net/netfilter/nf_tables_api.c | 16 ++++++++++++++++
net/netfilter/nft_flow_offload.c | 19 +++++++++++++++++++
3 files changed, 39 insertions(+)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index a26d64056fc8..001d294edf57 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1183,6 +1183,10 @@ struct nft_flowtable *nft_flowtable_lookup(const struct nft_table *table,
const struct nlattr *nla,
u8 genmask);
+void nf_tables_deactivate_flowtable(const struct nft_ctx *ctx,
+ struct nft_flowtable *flowtable,
+ enum nft_trans_phase phase);
+
void nft_register_flowtable_type(struct nf_flowtable_type *type);
void nft_unregister_flowtable_type(struct nf_flowtable_type *type);
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 6dc46f9b5f7b..d481f9baca2f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5598,6 +5598,22 @@ struct nft_flowtable *nft_flowtable_lookup(const struct nft_table *table,
}
EXPORT_SYMBOL_GPL(nft_flowtable_lookup);
+void nf_tables_deactivate_flowtable(const struct nft_ctx *ctx,
+ struct nft_flowtable *flowtable,
+ enum nft_trans_phase phase)
+{
+ switch (phase) {
+ case NFT_TRANS_PREPARE:
+ case NFT_TRANS_ABORT:
+ case NFT_TRANS_RELEASE:
+ flowtable->use--;
+ /* fall through */
+ default:
+ return;
+ }
+}
+EXPORT_SYMBOL_GPL(nf_tables_deactivate_flowtable);
+
static struct nft_flowtable *
nft_flowtable_lookup_byhandle(const struct nft_table *table,
const struct nlattr *nla, u8 genmask)
diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index 22cf236eb5d5..f29bbc74c4bf 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -177,6 +177,23 @@ static int nft_flow_offload_init(const struct nft_ctx *ctx,
return nf_ct_netns_get(ctx->net, ctx->family);
}
+static void nft_flow_offload_deactivate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr,
+ enum nft_trans_phase phase)
+{
+ struct nft_flow_offload *priv = nft_expr_priv(expr);
+
+ nf_tables_deactivate_flowtable(ctx, priv->flowtable, phase);
+}
+
+static void nft_flow_offload_activate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_flow_offload *priv = nft_expr_priv(expr);
+
+ priv->flowtable->use++;
+}
+
static void nft_flow_offload_destroy(const struct nft_ctx *ctx,
const struct nft_expr *expr)
{
@@ -205,6 +222,8 @@ static const struct nft_expr_ops nft_flow_offload_ops = {
.size = NFT_EXPR_SIZE(sizeof(struct nft_flow_offload)),
.eval = nft_flow_offload_eval,
.init = nft_flow_offload_init,
+ .activate = nft_flow_offload_activate,
+ .deactivate = nft_flow_offload_deactivate,
.destroy = nft_flow_offload_destroy,
.validate = nft_flow_offload_validate,
.dump = nft_flow_offload_dump,
--
2.11.0
^ permalink raw reply related
* [PATCH 3/5] netfilter: nf_tables: allow lookups in dynamic sets
From: Pablo Neira Ayuso @ 2019-09-25 20:30 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <20190925203003.20112-1-pablo@netfilter.org>
From: Florian Westphal <fw@strlen.de>
This un-breaks lookups in sets that have the 'dynamic' flag set.
Given this active example configuration:
table filter {
set set1 {
type ipv4_addr
size 64
flags dynamic,timeout
timeout 1m
}
chain input {
type filter hook input priority 0; policy accept;
}
}
... this works:
nft add rule ip filter input add @set1 { ip saddr }
-> whenever rule is triggered, the source ip address is inserted
into the set (if it did not exist).
This won't work:
nft add rule ip filter input ip saddr @set1 counter
Error: Could not process rule: Operation not supported
In other words, we can add entries to the set, but then can't make
matching decision based on that set.
That is just wrong -- all set backends support lookups (else they would
not be very useful).
The failure comes from an explicit rejection in nft_lookup.c.
Looking at the history, it seems like NFT_SET_EVAL used to mean
'set contains expressions' (aka. "is a meter"), for instance something like
nft add rule ip filter input meter example { ip saddr limit rate 10/second }
or
nft add rule ip filter input meter example { ip saddr counter }
The actual meaning of NFT_SET_EVAL however, is
'set can be updated from the packet path'.
'meters' and packet-path insertions into sets, such as
'add @set { ip saddr }' use exactly the same kernel code (nft_dynset.c)
and thus require a set backend that provides the ->update() function.
The only set that provides this also is the only one that has the
NFT_SET_EVAL feature flag.
Removing the wrong check makes the above example work.
While at it, also fix the flag check during set instantiation to
allow supported combinations only.
Fixes: 8aeff920dcc9b3f ("netfilter: nf_tables: add stateful object reference to set elements")
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_tables_api.c | 7 +++++--
net/netfilter/nft_lookup.c | 3 ---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 4a5d6ef2b706..6dc46f9b5f7b 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3562,8 +3562,11 @@ static int nf_tables_newset(struct net *net, struct sock *nlsk,
NFT_SET_OBJECT))
return -EINVAL;
/* Only one of these operations is supported */
- if ((flags & (NFT_SET_MAP | NFT_SET_EVAL | NFT_SET_OBJECT)) ==
- (NFT_SET_MAP | NFT_SET_EVAL | NFT_SET_OBJECT))
+ if ((flags & (NFT_SET_MAP | NFT_SET_OBJECT)) ==
+ (NFT_SET_MAP | NFT_SET_OBJECT))
+ return -EOPNOTSUPP;
+ if ((flags & (NFT_SET_EVAL | NFT_SET_OBJECT)) ==
+ (NFT_SET_EVAL | NFT_SET_OBJECT))
return -EOPNOTSUPP;
}
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index c0560bf3c31b..660bad688e2b 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -73,9 +73,6 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
if (IS_ERR(set))
return PTR_ERR(set);
- if (set->flags & NFT_SET_EVAL)
- return -EOPNOTSUPP;
-
priv->sreg = nft_parse_register(tb[NFTA_LOOKUP_SREG]);
err = nft_validate_register_load(priv->sreg, set->klen);
if (err < 0)
--
2.11.0
^ permalink raw reply related
* [PATCH 2/5] netfilter: nf_tables_offload: fix always true policy is unset check
From: Pablo Neira Ayuso @ 2019-09-25 20:30 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <20190925203003.20112-1-pablo@netfilter.org>
New smatch warnings:
net/netfilter/nf_tables_offload.c:316 nft_flow_offload_chain() warn: always true condition '(policy != -1) => (0-255 != (-1))'
Reported-by: kbuild test robot <lkp@intel.com>
Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_tables_offload.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index 21bb772cb4b7..e546f759b7a7 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -313,7 +313,7 @@ static int nft_flow_offload_chain(struct nft_chain *chain,
policy = ppolicy ? *ppolicy : basechain->policy;
/* Only default policy to accept is supported for now. */
- if (cmd == FLOW_BLOCK_BIND && policy != -1 && policy != NF_ACCEPT)
+ if (cmd == FLOW_BLOCK_BIND && policy == NF_DROP)
return -EOPNOTSUPP;
if (dev->netdev_ops->ndo_setup_tc)
--
2.11.0
^ permalink raw reply related
* [PATCH 4/5] netfilter: ebtables: use __u8 instead of uint8_t in uapi header
From: Pablo Neira Ayuso @ 2019-09-25 20:30 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <20190925203003.20112-1-pablo@netfilter.org>
From: Masahiro Yamada <yamada.masahiro@socionext.com>
When CONFIG_UAPI_HEADER_TEST=y, exported headers are compile-tested to
make sure they can be included from user-space.
Currently, linux/netfilter_bridge/ebtables.h is excluded from the test
coverage. To make it join the compile-test, we need to fix the build
errors attached below.
For a case like this, we decided to use __u{8,16,32,64} variable types
in this discussion:
https://lkml.org/lkml/2019/6/5/18
Build log:
CC usr/include/linux/netfilter_bridge/ebtables.h.s
In file included from <command-line>:32:0:
./usr/include/linux/netfilter_bridge/ebtables.h:126:4: error: unknown type name ‘uint8_t’
uint8_t revision;
^~~~~~~
./usr/include/linux/netfilter_bridge/ebtables.h:139:4: error: unknown type name ‘uint8_t’
uint8_t revision;
^~~~~~~
./usr/include/linux/netfilter_bridge/ebtables.h:152:4: error: unknown type name ‘uint8_t’
uint8_t revision;
^~~~~~~
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/uapi/linux/netfilter_bridge/ebtables.h | 6 +++---
usr/include/Makefile | 1 -
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/netfilter_bridge/ebtables.h b/include/uapi/linux/netfilter_bridge/ebtables.h
index 3b86c14ea49d..8076c940ffeb 100644
--- a/include/uapi/linux/netfilter_bridge/ebtables.h
+++ b/include/uapi/linux/netfilter_bridge/ebtables.h
@@ -123,7 +123,7 @@ struct ebt_entry_match {
union {
struct {
char name[EBT_EXTENSION_MAXNAMELEN];
- uint8_t revision;
+ __u8 revision;
};
struct xt_match *match;
} u;
@@ -136,7 +136,7 @@ struct ebt_entry_watcher {
union {
struct {
char name[EBT_EXTENSION_MAXNAMELEN];
- uint8_t revision;
+ __u8 revision;
};
struct xt_target *watcher;
} u;
@@ -149,7 +149,7 @@ struct ebt_entry_target {
union {
struct {
char name[EBT_EXTENSION_MAXNAMELEN];
- uint8_t revision;
+ __u8 revision;
};
struct xt_target *target;
} u;
diff --git a/usr/include/Makefile b/usr/include/Makefile
index 1fb6abe29b2f..379cc5abc162 100644
--- a/usr/include/Makefile
+++ b/usr/include/Makefile
@@ -38,7 +38,6 @@ header-test- += linux/ivtv.h
header-test- += linux/jffs2.h
header-test- += linux/kexec.h
header-test- += linux/matroxfb.h
-header-test- += linux/netfilter_bridge/ebtables.h
header-test- += linux/netfilter_ipv4/ipt_LOG.h
header-test- += linux/netfilter_ipv6/ip6t_LOG.h
header-test- += linux/nfc.h
--
2.11.0
^ permalink raw reply related
* Re: [PATCH bpf] selftests/bpf: delete unused variables in test_sysctl
From: Daniel Borkmann @ 2019-09-25 20:30 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, andrii.nakryiko, kernel-team
In-Reply-To: <20190925183614.2775293-1-andriin@fb.com>
On Wed, Sep 25, 2019 at 11:36:14AM -0700, Andrii Nakryiko wrote:
> Remove no longer used variables and avoid compiler warnings.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Applied, thanks!
^ permalink raw reply
* [PATCH 1/5] netfilter: nf_tables: add NFT_CHAIN_POLICY_UNSET and use it
From: Pablo Neira Ayuso @ 2019-09-25 20:29 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <20190925203003.20112-1-pablo@netfilter.org>
Default policy is defined as a unsigned 8-bit field, do not use a
negative value to leave it unset, use this new NFT_CHAIN_POLICY_UNSET
instead.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tables.h | 2 ++
net/netfilter/nf_tables_api.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 2655e03dbe1b..a26d64056fc8 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -889,6 +889,8 @@ enum nft_chain_flags {
NFT_CHAIN_HW_OFFLOAD = 0x2,
};
+#define NFT_CHAIN_POLICY_UNSET U8_MAX
+
/**
* struct nft_chain - nf_tables chain
*
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index e4a68dc42694..4a5d6ef2b706 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1715,7 +1715,7 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
goto err2;
}
- nft_trans_chain_policy(trans) = -1;
+ nft_trans_chain_policy(trans) = NFT_CHAIN_POLICY_UNSET;
if (nft_is_base_chain(chain))
nft_trans_chain_policy(trans) = policy;
--
2.11.0
^ permalink raw reply related
* [PATCH] brcmsmac: remove duplicated if condition
From: Denis Efremov @ 2019-09-25 20:31 UTC (permalink / raw)
To: Kalle Valo
Cc: Denis Efremov, Wright Feng, Chi-Hsien Lin, Hante Meuleman,
Franky Lin, Arend van Spriel, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, netdev, linux-kernel
The nested 'li_mimo == &locale_bn' check is excessive and always
true. Thus it can be safely removed.
Signed-off-by: Denis Efremov <efremov@linux.com>
---
.../net/wireless/broadcom/brcm80211/brcmsmac/channel.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
index db783e94f929..5a6d9c86552a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
@@ -496,13 +496,11 @@ brcms_c_channel_reg_limits(struct brcms_cm_info *wlc_cm, u16 chanspec,
* table and override CDD later
*/
if (li_mimo == &locale_bn) {
- if (li_mimo == &locale_bn) {
- maxpwr20 = QDB(16);
- maxpwr40 = 0;
+ maxpwr20 = QDB(16);
+ maxpwr40 = 0;
- if (chan >= 3 && chan <= 11)
- maxpwr40 = QDB(16);
- }
+ if (chan >= 3 && chan <= 11)
+ maxpwr40 = QDB(16);
for (i = 0; i < BRCMS_NUM_RATES_MCS_1_STREAM; i++) {
txpwr->mcs_20_siso[i] = (u8) maxpwr20;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH bpf] selftests/bpf: adjust strobemeta loop to satisfy latest clang
From: Daniel Borkmann @ 2019-09-25 20:33 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, andrii.nakryiko, kernel-team
In-Reply-To: <20190925185205.2857838-1-andriin@fb.com>
On Wed, Sep 25, 2019 at 11:52:05AM -0700, Andrii Nakryiko wrote:
> Some recent changes in latest Clang started causing the following
> warning when unrolling strobemeta test case main loop:
>
> progs/strobemeta.h:416:2: warning: loop not unrolled: the optimizer was
> unable to perform the requested transformation; the transformation might
> be disabled or specified as part of an unsupported transformation
> ordering [-Wpass-failed=transform-warning]
>
> This patch simplifies loop's exit condition to depend only on constant
> max iteration number (STROBE_MAX_MAP_ENTRIES), while moving early
> termination logic inside the loop body. The changes are equivalent from
> program logic standpoint, but fixes the warning. It also appears to
> improve generated BPF code, as it fixes previously failing non-unrolled
> strobemeta test cases.
>
> Cc: Alexei Starovoitov <ast@fb.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Sounds like a clang regression? Was that from an official release?
Applied.
^ permalink raw reply
* Re: [PATCH bpf-next v11 4/4] tools/testing/selftests/bpf: Add self-tests for new helper.
From: Carlos Antonio Neira Bustos @ 2019-09-25 20:33 UTC (permalink / raw)
To: Yonghong Song
Cc: netdev@vger.kernel.org, ebiederm@xmission.com, brouer@redhat.com,
bpf@vger.kernel.org
In-Reply-To: <7562dae7-fd18-e334-8167-8c67234b3c3f@fb.com>
On Wed, Sep 25, 2019 at 04:07:09PM +0000, Yonghong Song wrote:
>
>
> On 9/24/19 8:20 AM, Carlos Neira wrote:
> > Self tests added for new helper
> >
> > Signed-off-by: Carlos Neira <cneirabustos@gmail.com>
> > ---
> > tools/testing/selftests/bpf/Makefile | 2 +-
> > tools/testing/selftests/bpf/bpf_helpers.h | 3 +
> > .../selftests/bpf/progs/test_pidns_kern.c | 71 ++++++++
> > tools/testing/selftests/bpf/test_pidns.c | 152 ++++++++++++++++++
> > 4 files changed, 227 insertions(+), 1 deletion(-)
> > create mode 100644 tools/testing/selftests/bpf/progs/test_pidns_kern.c
> > create mode 100644 tools/testing/selftests/bpf/test_pidns.c
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 7f3196af1ae4..d86b28aa8f44 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -28,7 +28,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
> > test_sock test_btf test_sockmap get_cgroup_id_user test_socket_cookie \
> > test_cgroup_storage test_select_reuseport test_section_names \
> > test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
> > - test_btf_dump test_cgroup_attach xdping
> > + test_btf_dump test_cgroup_attach xdping test_pidns
>
> Could you fold test_pidns into test_progs?
>
> >
> > BPF_OBJ_FILES = $(patsubst %.c,%.o, $(notdir $(wildcard progs/*.c)))
> > TEST_GEN_FILES = $(BPF_OBJ_FILES)
> > diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> > index 6c4930bc6e2e..03d0e15ae29f 100644
> > --- a/tools/testing/selftests/bpf/bpf_helpers.h
> > +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> > @@ -231,6 +231,9 @@ static int (*bpf_send_signal)(unsigned sig) = (void *)BPF_FUNC_send_signal;
> > static long long (*bpf_tcp_gen_syncookie)(struct bpf_sock *sk, void *ip,
> > int ip_len, void *tcp, int tcp_len) =
> > (void *) BPF_FUNC_tcp_gen_syncookie;
> > +static int (*bpf_get_ns_current_pid_tgid)(__u32 dev, __u64 inum) =
> > + (void *) BPF_FUNC_get_ns_current_pid_tgid;
> > +
> >
> > /* llvm builtin functions that eBPF C program may use to
> > * emit BPF_LD_ABS and BPF_LD_IND instructions
> > diff --git a/tools/testing/selftests/bpf/progs/test_pidns_kern.c b/tools/testing/selftests/bpf/progs/test_pidns_kern.c
> > new file mode 100644
> > index 000000000000..96cb707db3ee
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_pidns_kern.c
> > @@ -0,0 +1,71 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2018 Carlos Neira cneirabustos@gmail.com
>
> 2019
>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of version 2 of the GNU General Public
> > + * License as published by the Free Software Foundation.
>
> You do not need the above statement, which is covered by
> "SPDX-License-Identifier: GPL-2.0".
>
> > + */
> > +
> > +#include <linux/bpf.h>
> > +#include "bpf_helpers.h"
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_ARRAY);
> > + __uint(max_entries, 1);
> > + __type(key, __u32);
> > + __type(value, __u64);
> > +} ns_inum_map SEC(".maps");
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_ARRAY);
> > + __uint(max_entries, 1);
> > + __type(key, __u32);
> > + __type(value, __u32);
> > +} ns_dev_map SEC(".maps");
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_ARRAY);
> > + __uint(max_entries, 1);
> > + __type(key, __u32);
> > + __type(value, __u32);
> > +} pidmap SEC(".maps");
>
> Can we make the value __u64 to include
> both pid and tid to compare both?
>
> > +
> > +struct {
> > + __uint(type, BPF_MAP_TYPE_ARRAY);
> > + __uint(max_entries, 1);
> > + __type(key, __u32);
> > + __type(value, __u64);
> > +} ns_pid_map SEC(".maps");
> > +
> >
>
> The above four one-element maps are perfectly examples
> to use static global variables which are supported
> by the kernel.
>
> You can take a look at the patch
> https://patchwork.ozlabs.org/patch/1081014/
> which shows how user space can modify the map
> values. In the future, we could have a better
> interface to read/update those static variable
> values.
>
> > +
> > +SEC("tracepoint/syscalls/sys_enter_nanosleep")
> > +int trace(void *ctx)
> > +{
> > + __u32 key = 0, *expected_pid, *dev;
>
> expected_pid => __u64 *?
>
> > + __u64 *val, *inum, nspid;
> > + __u32 pid;
> > +
> > + dev = bpf_map_lookup_elem(&ns_dev_map, &key);
> > + if (!dev)
> > + return 0;
> > +
> > + inum = bpf_map_lookup_elem(&ns_inum_map, &key);
> > + if (!inum)
> > + return 0;
> > +
> > + nspid = bpf_get_ns_current_pid_tgid(*dev, *inum);
> > + expected_pid = bpf_map_lookup_elem(&pidmap, &key);
> > +
> > + if (!expected_pid || *expected_pid != nspid)
> > + return 0;
> > +
> > + val = bpf_map_lookup_elem(&ns_pid_map, &key);
> > + if (val)
> > + *val = nspid;
> > +
> > + return 0;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
> > +__u32 _version SEC("version") = 1;
> > diff --git a/tools/testing/selftests/bpf/test_pidns.c b/tools/testing/selftests/bpf/test_pidns.c
> > new file mode 100644
> > index 000000000000..088f8025f2bf
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_pidns.c
> > @@ -0,0 +1,152 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2018 Carlos Neira cneirabustos@gmail.com
>
> 2019
>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of version 2 of the GNU General Public
> > + * License as published by the Free Software Foundation.
>
> ditto. you do not need the above statement.
>
> > + */
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <syscall.h>
> > +#include <unistd.h>
> > +#include <linux/perf_event.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/time.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf.h>
> > +#include <bpf/libbpf.h>
> > +
> > +#include "cgroup_helpers.h"
> > +#include "bpf_rlimit.h"
> > +
> > +#define CHECK(condition, tag, format...) ({ \
> > + int __ret = !!(condition); \
> > + if (__ret) { \
> > + printf("%s:FAIL:%s ", __func__, tag); \
> > + printf(format); \
> > + } else { \
> > + printf("%s:PASS:%s\n", __func__, tag); \
> > + } \
> > + __ret; \
> > +})
> > +
> > +static int bpf_find_map(const char *test, struct bpf_object *obj,
> > + const char *name)
> > +{
> > + struct bpf_map *map;
> > +
> > + map = bpf_object__find_map_by_name(obj, name);
> > + if (!map)
> > + return -1;
> > + return bpf_map__fd(map);
>
> The 'test' argument is not used here.
> Also, we have bpf_object__find_map_fd_by_name() API, you can
> use it and you do not need this function.
>
> > +}
> > +
> > +
> > +int main(int argc, char **argv)
> > +{
> > + int pidmap_fd, ns_inum_map_fd, ns_dev_map_fd, ns_pid_map_fd;
> > + const char *probe_name = "syscalls/sys_enter_nanosleep";
> > + const char *file = "test_pidns_kern.o";
> > + int err, bytes, efd, prog_fd, pmu_fd;
> > + struct perf_event_attr attr = {};
> > + struct bpf_object *obj;
> > + __u32 nspid = 0;
> > + __u32 key = 0, pid;
> > + int exit_code = 1;
>
> to make it reverse Christmas tree style?
>
> > + struct stat st;
> > + char buf[256];
> > +
> > + err = bpf_prog_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
> > + if (CHECK(err, "bpf_prog_load", "err %d errno %d\n", err, errno))
> > + goto cleanup_cgroup_env;
> > +
> > + ns_dev_map_fd = bpf_find_map(__func__, obj, "ns_dev_map");
> > + if (CHECK(ns_dev_map_fd < 0, "bpf_find_map", "err %d errno %d\n",
> > + ns_dev_map_fd, errno))
> > + goto close_prog;
> > +
> > + ns_inum_map_fd = bpf_find_map(__func__, obj, "ns_inum_map");
> > + if (CHECK(ns_inum_map_fd < 0, "bpf_find_map", "err %d errno %d\n",
> > + ns_inum_map_fd, errno))
> > + goto close_prog;
> > +
> > + ns_pid_map_fd = bpf_find_map(__func__, obj, "ns_pid_map");
> > + if (CHECK(ns_pid_map_fd < 0, "bpf_find_map", "err %d errno %d\n",
> > + ns_pid_map_fd, errno))
> > + goto close_prog;
> > +
> > + pidmap_fd = bpf_find_map(__func__, obj, "pidmap");
> > + if (CHECK(pidmap_fd < 0, "bpf_find_map", "err %d errno %d\n",
> > + pidmap_fd, errno))
> > + goto close_prog;
> > +
> > + pid = getpid();
> > + bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
>
> maybe
> id = (__u64) getpid() << 32 | gettid();
> bpf_map_update_elem(pidmap_fd, &key, &id, 0);
>
> In your original above kernel code, you actually compare user space pid
> to kernel tid, which is okay for this program as it is the main thread
> and tid == pid, but the mechanism is wrong.
>
> > +
> > + if (stat("/proc/self/ns/pid", &st))
> > + goto close_prog;
> > +
> > + bpf_map_update_elem(ns_inum_map_fd, &key, &st.st_ino, 0);
> > + bpf_map_update_elem(ns_dev_map_fd, &key, &st.st_dev, 0);
> > +
> > +
> > + snprintf(buf, sizeof(buf),
> > + "/sys/kernel/debug/tracing/events/%s/id", probe_name);
> > + efd = open(buf, O_RDONLY, 0);
> > + if (CHECK(efd < 0, "open", "err %d errno %d\n", efd, errno))
> > + goto close_prog;
> > + bytes = read(efd, buf, sizeof(buf));
> > + close(efd);
> > + if (CHECK(bytes <= 0 || bytes >= sizeof(buf), "read",
> > + "bytes %d errno %d\n", bytes, errno))
> > + goto close_prog;
> > +
> > + attr.config = strtol(buf, NULL, 0);
> > + attr.type = PERF_TYPE_TRACEPOINT;
> > + attr.sample_type = PERF_SAMPLE_RAW;
> > + attr.sample_period = 1;
> > + attr.wakeup_events = 1;
> > +
> > + pmu_fd = syscall(__NR_perf_event_open, &attr, getpid(), -1, -1, 0);
> > + if (CHECK(pmu_fd < 0, "perf_event_open", "err %d errno %d\n", pmu_fd,
> > + errno))
> > + goto close_prog;
> > +
> > + err = ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
> > + if (CHECK(err, "perf_event_ioc_enable", "err %d errno %d\n", err,
> > + errno))
> > + goto close_pmu;
> > +
> > + err = ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd);
> > + if (CHECK(err, "perf_event_ioc_set_bpf", "err %d errno %d\n", err,
> > + errno))
> > + goto close_pmu;
>
> The whole thing above related to tracepoint can be simplified. Please
> see prog_tests/stacktrace_map.c.
>
> > +
> > + /* trigger some syscalls */
> > + sleep(1);
> > +
> > + err = bpf_map_lookup_elem(ns_pid_map_fd, &key, &nspid);
> > + if (CHECK(err, "bpf_map_lookup_elem", "err %d errno %d\n", err, errno))
> > + goto close_pmu;
> > +
> > + if (CHECK(nspid != pid, "compare nspid vs pid",
> > + "kern nspid %u user pid %u", nspid, pid))
> > + goto close_pmu;
> > +
> > + exit_code = 0;
> > + printf("%s:PASS\n", argv[0]);
> > +
> > +close_pmu:
> > + close(pmu_fd);
> > +close_prog:
> > + bpf_object__close(obj);
> > +cleanup_cgroup_env:
> > + return exit_code;
> > +}
> >
Thanks Yonghong,
I'll correct these issues on v12.
Bests
^ permalink raw reply
* Re: [PATCH bpf] selftests/bpf: adjust strobemeta loop to satisfy latest clang
From: Andrii Nakryiko @ 2019-09-25 20:35 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Kernel Team
In-Reply-To: <20190925203301.GE9500@pc-63.home>
On Wed, Sep 25, 2019 at 1:33 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On Wed, Sep 25, 2019 at 11:52:05AM -0700, Andrii Nakryiko wrote:
> > Some recent changes in latest Clang started causing the following
> > warning when unrolling strobemeta test case main loop:
> >
> > progs/strobemeta.h:416:2: warning: loop not unrolled: the optimizer was
> > unable to perform the requested transformation; the transformation might
> > be disabled or specified as part of an unsupported transformation
> > ordering [-Wpass-failed=transform-warning]
> >
> > This patch simplifies loop's exit condition to depend only on constant
> > max iteration number (STROBE_MAX_MAP_ENTRIES), while moving early
> > termination logic inside the loop body. The changes are equivalent from
> > program logic standpoint, but fixes the warning. It also appears to
> > improve generated BPF code, as it fixes previously failing non-unrolled
> > strobemeta test cases.
> >
> > Cc: Alexei Starovoitov <ast@fb.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Sounds like a clang regression? Was that from an official release?
It does, but I didn't dig deep enough to figure out what exactly
caused this. The version I used was latest Clang 10 built from
sources. Might be worth-while to investigate this further to prevent
some other unexpected breakages for user programs.
>
> Applied.
Thanks!
^ permalink raw reply
* [PATCH bpf] libbpf: teach btf_dumper to emit stand-alone anonymous enum definitions
From: Andrii Nakryiko @ 2019-09-25 20:37 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
BTF-to-C converter previously skipped anonymous enums in an assumption
that those are embedded in struct's field definitions. This is not
always the case and a lot of kernel constants are defined as part of
anonymous enums. This change fixes the logic by eagerly marking all
types as either referenced by any other type or not. This is enough to
distinguish two classes of anonymous enums and emit previously omitted
enum definitions.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/btf_dump.c | 93 +++++++++++++++++++++++++++++++++++++---
1 file changed, 87 insertions(+), 6 deletions(-)
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 84b0661db7f3..ede55fec3618 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -48,6 +48,8 @@ struct btf_dump_type_aux_state {
__u8 fwd_emitted: 1;
/* whether unique non-duplicate name was already assigned */
__u8 name_resolved: 1;
+ /* whether type is referenced from any other type */
+ __u8 referenced: 1;
};
struct btf_dump {
@@ -173,6 +175,7 @@ void btf_dump__free(struct btf_dump *d)
free(d);
}
+static int btf_dump_mark_referenced(struct btf_dump *d);
static int btf_dump_order_type(struct btf_dump *d, __u32 id, bool through_ptr);
static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id);
@@ -213,6 +216,11 @@ int btf_dump__dump_type(struct btf_dump *d, __u32 id)
/* VOID is special */
d->type_states[0].order_state = ORDERED;
d->type_states[0].emit_state = EMITTED;
+
+ /* eagerly determine referenced types for anon enums */
+ err = btf_dump_mark_referenced(d);
+ if (err)
+ return err;
}
d->emit_queue_cnt = 0;
@@ -226,6 +234,79 @@ int btf_dump__dump_type(struct btf_dump *d, __u32 id)
return 0;
}
+/*
+ * Mark all types that are referenced from any other type. This is used to
+ * determine top-level anonymous enums that need to be emitted as an
+ * independent type declarations.
+ * Anonymous enums come in two flavors: either embedded in a struct's field
+ * definition, in which case they have to be declared inline as part of field
+ * type declaration; or as a top-level anonymous enum, typically used for
+ * declaring global constants. It's impossible to distinguish between two
+ * without knowning whether given enum type was referenced from other type:
+ * top-level anonymous enum won't be referenced by anything, while embedded
+ * one will.
+ */
+static int btf_dump_mark_referenced(struct btf_dump *d)
+{
+ int i, j, n = btf__get_nr_types(d->btf);
+ const struct btf_type *t;
+ __u16 vlen;
+
+ for (i = 1; i <= n; i++) {
+ t = btf__type_by_id(d->btf, i);
+ vlen = btf_vlen(t);
+
+ switch (btf_kind(t)) {
+ case BTF_KIND_INT:
+ case BTF_KIND_ENUM:
+ case BTF_KIND_FWD:
+ break;
+
+ case BTF_KIND_VOLATILE:
+ case BTF_KIND_CONST:
+ case BTF_KIND_RESTRICT:
+ case BTF_KIND_PTR:
+ case BTF_KIND_TYPEDEF:
+ case BTF_KIND_FUNC:
+ case BTF_KIND_VAR:
+ d->type_states[t->type].referenced = 1;
+ break;
+
+ case BTF_KIND_ARRAY: {
+ const struct btf_array *a = btf_array(t);
+
+ d->type_states[a->index_type].referenced = 1;
+ d->type_states[a->type].referenced = 1;
+ break;
+ }
+ case BTF_KIND_STRUCT:
+ case BTF_KIND_UNION: {
+ const struct btf_member *m = btf_members(t);
+
+ for (j = 0; j < vlen; j++, m++)
+ d->type_states[m->type].referenced = 1;
+ break;
+ }
+ case BTF_KIND_FUNC_PROTO: {
+ const struct btf_param *p = btf_params(t);
+
+ for (j = 0; j < vlen; j++, p++)
+ d->type_states[p->type].referenced = 1;
+ break;
+ }
+ case BTF_KIND_DATASEC: {
+ const struct btf_var_secinfo *v = btf_var_secinfos(t);
+
+ for (j = 0; j < vlen; j++, v++)
+ d->type_states[v->type].referenced = 1;
+ break;
+ }
+ default:
+ return -EINVAL;
+ }
+ }
+ return 0;
+}
static int btf_dump_add_emit_queue_id(struct btf_dump *d, __u32 id)
{
__u32 *new_queue;
@@ -395,7 +476,12 @@ static int btf_dump_order_type(struct btf_dump *d, __u32 id, bool through_ptr)
}
case BTF_KIND_ENUM:
case BTF_KIND_FWD:
- if (t->name_off != 0) {
+ /*
+ * non-anonymous or non-referenced enums are top-level
+ * declarations and should be emitted. Same logic can be
+ * applied to FWDs, it won't hurt anyways.
+ */
+ if (t->name_off != 0 || !tstate->referenced) {
err = btf_dump_add_emit_queue_id(d, id);
if (err)
return err;
@@ -536,11 +622,6 @@ static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id)
t = btf__type_by_id(d->btf, id);
kind = btf_kind(t);
- if (top_level_def && t->name_off == 0) {
- pr_warning("unexpected nameless definition, id:[%u]\n", id);
- return;
- }
-
if (tstate->emit_state == EMITTING) {
if (tstate->fwd_emitted)
return;
--
2.17.1
^ permalink raw reply related
* [PATCH] iwlwifi: dvm: excessive if in rs_bt_update_lq()
From: Denis Efremov @ 2019-09-25 20:49 UTC (permalink / raw)
To: Kalle Valo
Cc: Denis Efremov, Johannes Berg, Emmanuel Grumbach, Luca Coelho,
Intel Linux Wireless, David S. Miller, linux-wireless, netdev,
linux-kernel
There is no need to check 'priv->bt_ant_couple_ok' twice in
rs_bt_update_lq(). The second check is always true. Thus, the
expression can be simplified.
Signed-off-by: Denis Efremov <efremov@linux.com>
---
drivers/net/wireless/intel/iwlwifi/dvm/rs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/rs.c b/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
index 74229fcb63a9..226165db7dfd 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/rs.c
@@ -851,7 +851,7 @@ static void rs_bt_update_lq(struct iwl_priv *priv, struct iwl_rxon_context *ctx,
* Is there a need to switch between
* full concurrency and 3-wire?
*/
- if (priv->bt_ci_compliance && priv->bt_ant_couple_ok)
+ if (priv->bt_ci_compliance)
full_concurrent = true;
else
full_concurrent = false;
--
2.21.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox