* Re: [PATCH 3/7] netlink: set extack error message in nla_validate()
From: Johannes Berg @ 2018-09-19 16:31 UTC (permalink / raw)
To: David Ahern, linux-wireless, netdev
In-Reply-To: <3e7aadff-5228-660a-537a-9b54c11fa5cc@gmail.com>
On Wed, 2018-09-19 at 09:20 -0700, David Ahern wrote:
> > nla_for_each_attr(nla, head, len, rem) {
> > - int err = validate_nla(nla, maxtype, policy, NULL);
> > + static const char _msg[] = "Attribute failed policy validation";
> > + const char *msg = _msg;
> > + int err = validate_nla(nla, maxtype, policy, &msg);
> >
> > if (err < 0) {
> > + if (extack)
> > + extack->_msg = msg;
> > NL_SET_BAD_ATTR(extack, nla);
>
> msg, NL_SET_BAD_ATTR and extack handling all can be done in validate_nla
> removing the need for the same message ("Attribute failed policy
> validation") to be declared twice and simplifying the extack setting.
Yeah, perhaps I should take another look at that. I didn't want to do
that originally as validate_nla() has so many exit points, but perhaps
it's better to put a goto label there.
FWIW, in the next patch I'm getting rid of the duplication again - I
wanted to have the patch that has an effect (this one, setting the
message) more clearly separated.
johannes
^ permalink raw reply
* Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion
From: Johannes Berg @ 2018-09-19 16:36 UTC (permalink / raw)
To: David Ahern, linux-wireless, netdev
In-Reply-To: <18ea5293-5a12-7aca-7373-12d1ab3a0821@gmail.com>
On Wed, 2018-09-19 at 09:28 -0700, David Ahern wrote:
> On 9/19/18 5:08 AM, Johannes Berg wrote:
> > diff --git a/lib/nlattr.c b/lib/nlattr.c
> > index 966cd3dcf31b..2b015e43b725 100644
> > --- a/lib/nlattr.c
> > +++ b/lib/nlattr.c
> > @@ -69,7 +69,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
> >
> > static int validate_nla(const struct nlattr *nla, int maxtype,
> > const struct nla_policy *policy,
> > - const char **error_msg)
> > + struct netlink_ext_ack *extack, bool *extack_set)
>
> extack_set arg is not needed if you handle the "Attribute failed policy
> validation" message and NL_SET_BAD_ATTR here as well.
I'm not sure that's true, but perhaps you have a better idea than me?
My thought would be to introduce an "error" label in validate_nla(),
that sets up the extack data.
Then we could skip over that if we have a separate message to report,
making the NLA_REJECT case easier.
However, if we do nested validation, I'm not sure it really is that much
easier? We still need to figure out if the nested validation was setting
the message (and bad attr), rather than it having been set before we
even get into this function.
So let's say we have
case NLA_NESTED:
/* a nested attributes is allowed to be empty; if its not,
* it must have a size of at least NLA_HDRLEN.
*/
if (attrlen == 0)
break;
if (attrlen < NLA_HDRLEN)
return -ERANGE;
if (pt->validation_data) {
int err;
err = nla_validate_parse(nla_data(nla), nla_len(nla),
pt->len, pt->validation_data,
extack, extack_set, NULL);
if (err < 0)
return err;
}
break;
right now after all the patches.
The "return -ERANGE;" would become "{ err = -ERANGE; goto error; }", but
I'm not really sure we can cleanly handle the other case?
Hmm. Maybe it works if we ensure that nla_validate_parse() has no other
return points that can fail outside of validate_nla(), or we set up the
extack data there as well, so that once we have a nested
nla_validate_parse() we know that it's been set.
Actually, we need to do that anyway so that we can move the setting into
validate_nla(), and then it should work.
Mechanics aside - I'll take a look later tonight or tomorrow - do you
think the goal/external interface of this makes sense?
johannes
^ permalink raw reply
* Re: [PATCH 1/1] macsec: reflect the master interface state
From: Radu Rendec @ 2018-09-19 16:44 UTC (permalink / raw)
To: sd; +Cc: netdev, davem, ptalbert
In-Reply-To: <20180919152425.GA3046@bistromath.localdomain>
Hello,
On Wed, Sep 19, 2018 at 11:24 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2018-09-18, 20:16:12 -0400, Radu Rendec wrote:
> > This patch makes macsec interfaces reflect the state of the underlying
> > interface: if the master interface changes state to up/down, the macsec
> > interface changes its state accordingly.
>
> Yes, that's a bit unfortunate. I was wondering if we should just allow
> setting the device up, and let link state handle the rest.
Yes, that could work. It would also be consistent with the idea of
propagating only the link state.
> It's missing small parts of link state handling that I have been
> testing, mainly when creating a new device.
Can you please be more specific? I've just looked at macsec_newlink()
and I didn't notice anything related to link state handling.
> > - list_for_each_entry_safe(m, n, &rxd->secys, secys) {
> > + list_for_each_entry_safe(m, n, &rxd->secys, secys)
> > macsec_common_dellink(m->secy.netdev, &head);
> > - }
>
> Please don't mix coding style changes with bug fixes.
Noted.
> > + case NETDEV_DOWN: {
> > + struct net_device *dev, *tmp;
> > + LIST_HEAD(close_list);
> > +
> > + list_for_each_entry(m, &rxd->secys, secys) {
> > + dev = m->secy.netdev;
> > +
> > + if (dev->flags & IFF_UP)
> > + list_add(&dev->close_list, &close_list);
> > + }
> > +
> > + dev_close_many(&close_list, false);
> > +
> > + list_for_each_entry_safe(dev, tmp, &close_list, close_list) {
> > + netif_stacked_transfer_operstate(real_dev, dev);
> > + list_del_init(&dev->close_list);
> > + }
> > + list_del(&close_list);
> > + break;
>
> My version of the patch only does netif_stacked_transfer_operstate(),
> and skips setting the device administratively down (ie, same as
> NETDEV_CHANGE).
That makes sense. But, since you mentioned you had your own patch, does
it make sense for me to continue working on mine?
> > }
> > + case NETDEV_UP:
> > + list_for_each_entry(m, &rxd->secys, secys) {
> > + struct net_device *dev = m->secy.netdev;
> > + int flags = dev_get_flags(dev);
> > +
> > + if (!(flags & IFF_UP)) {
> > + dev_change_flags(dev, flags | IFF_UP);
> > + netif_stacked_transfer_operstate(real_dev, dev);
> > + }
> > + }
> > + break;
>
> I don't like that this completely ignores whether the device was done
> independently of the lower link. Maybe the administrator actually
> wants the macsec device down, regardless of state changes on the lower
> device.
I thought about that too and I don't like it either. Perhaps the vlan
approach of having a "loose binding" flag is worth considering. Then the
admin can decice for themselves.
However, I guess the problem disappears if only the link state is
propagated. The only caveat to that is to not propagate an "up" link
state while the macsec interface is administratively down, but
"remember" to propagate it later if the macsec interface is
administratively set to "up".
Thanks for the feedback!
Radu Rendec
^ permalink raw reply
* Re: Bridge connectivity interruptions while devices join or leave the bridge
From: Ido Schimmel @ 2018-09-19 16:45 UTC (permalink / raw)
To: Johannes Wienke; +Cc: netdev
In-Reply-To: <cc7b7c8d-16bb-18a5-4b3a-91690acf47dc@techfak.uni-bielefeld.de>
On Wed, Sep 19, 2018 at 01:00:15PM +0200, Johannes Wienke wrote:
> This behavior of inheriting the mac address is really unexpected to us.
> Is it documented somewhere?
Not that I'm aware, but it's a well established behavior.
^ permalink raw reply
* Re: array bounds warning in xfrm_output_resume
From: David Ahern @ 2018-09-19 16:57 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev@vger.kernel.org
In-Reply-To: <f116bb15-7fed-3b2e-4b20-a7a2c4317964@gmail.com>
On 6/18/18 11:10 AM, David Ahern wrote:
> Florian:
>
> I am seeing this warning:
>
> $ make O=kbuild/perf -j 24 -s
> In file included from /home/dsa/kernel-3.git/include/linux/kernel.h:10:0,
> from /home/dsa/kernel-3.git/include/linux/list.h:9,
> from /home/dsa/kernel-3.git/include/linux/module.h:9,
> from /home/dsa/kernel-3.git/net/xfrm/xfrm_output.c:13:
> /home/dsa/kernel-3.git/net/xfrm/xfrm_output.c: In function
> ‘xfrm_output_resume’:
> /home/dsa/kernel-3.git/include/linux/compiler.h:252:20: warning: array
> subscript is above array bounds [-Warray-bounds]
> __read_once_size(&(x), __u.__c, sizeof(x)); \
> ^~~~
> /home/dsa/kernel-3.git/include/linux/compiler.h:258:22: note: in
> expansion of macro ‘__READ_ONCE’
> #define READ_ONCE(x) __READ_ONCE(x, 1)
> ^~~~~~~~~~~
> /home/dsa/kernel-3.git/include/linux/rcupdate.h:350:48: note: in
> expansion of macro ‘READ_ONCE’
> typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); \
> ^~~~~~~~~
> /home/dsa/kernel-3.git/include/linux/rcupdate.h:487:2: note: in
> expansion of macro ‘__rcu_dereference_check’
> __rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu)
> ^~~~~~~~~~~~~~~~~~~~~~~
> /home/dsa/kernel-3.git/include/linux/rcupdate.h:545:28: note: in
> expansion of macro ‘rcu_dereference_check’
> #define rcu_dereference(p) rcu_dereference_check(p, 0)
> ^~~~~~~~~~~~~~~~~~~~~
> /home/dsa/kernel-3.git/include/linux/netfilter.h:218:15: note: in
> expansion of macro ‘rcu_dereference’
> hook_head = rcu_dereference(net->nf.hooks_arp[hook]);
> ^~~~~~~~~~~~~~~
>
> Line in question is the nf_hook in xfrm_output_resume.
> NF_INET_POST_ROUTING = 4 which is greater than NF_ARP_NUMHOOKS = 3
>
> I believe ef57170bbfdd6 is the commit that introduced the warning
>
Hi Florian:
I am still seeing this. I realize the compiler is not understanding the
conditions properly, but it is a distraction every time I do a kernel
build on Debian Stretch having to filter the above noise from whether I
introduce another warning.
^ permalink raw reply
* [PATCH net] bnxt_en: don't try to offload VLAN 'modify' action
From: Davide Caratti @ 2018-09-19 17:01 UTC (permalink / raw)
To: Michael Chan, David S. Miller, netdev; +Cc: Sathya Perla
bnxt offload code currently supports only 'push' and 'pop' operation: let
.ndo_setup_tc() return -EOPNOTSUPP if VLAN 'modify' action is configured.
Fixes: 2ae7408fedfe ("bnxt_en: bnxt: add TC flower filter offload support")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 092c817f8f11..e1594c9df4c6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -75,17 +75,23 @@ static int bnxt_tc_parse_redir(struct bnxt *bp,
return 0;
}
-static void bnxt_tc_parse_vlan(struct bnxt *bp,
- struct bnxt_tc_actions *actions,
- const struct tc_action *tc_act)
+static int bnxt_tc_parse_vlan(struct bnxt *bp,
+ struct bnxt_tc_actions *actions,
+ const struct tc_action *tc_act)
{
- if (tcf_vlan_action(tc_act) == TCA_VLAN_ACT_POP) {
+ switch (tcf_vlan_action(tc_act)) {
+ case TCA_VLAN_ACT_POP:
actions->flags |= BNXT_TC_ACTION_FLAG_POP_VLAN;
- } else if (tcf_vlan_action(tc_act) == TCA_VLAN_ACT_PUSH) {
+ break;
+ case TCA_VLAN_ACT_PUSH:
actions->flags |= BNXT_TC_ACTION_FLAG_PUSH_VLAN;
actions->push_vlan_tci = htons(tcf_vlan_push_vid(tc_act));
actions->push_vlan_tpid = tcf_vlan_push_proto(tc_act);
+ break;
+ default:
+ return -EOPNOTSUPP;
}
+ return 0;
}
static int bnxt_tc_parse_tunnel_set(struct bnxt *bp,
@@ -134,7 +140,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
/* Push/pop VLAN */
if (is_tcf_vlan(tc_act)) {
- bnxt_tc_parse_vlan(bp, actions, tc_act);
+ rc = bnxt_tc_parse_vlan(bp, actions, tc_act);
+ if (rc)
+ return rc;
continue;
}
--
2.17.1
^ permalink raw reply related
* Re: Bridge connectivity interruptions while devices join or leave the bridge
From: Stephen Hemminger @ 2018-09-19 17:03 UTC (permalink / raw)
To: Ido Schimmel; +Cc: Johannes Wienke, netdev
In-Reply-To: <20180919164508.GA4975@splinter>
On Wed, 19 Sep 2018 19:45:08 +0300
Ido Schimmel <idosch@idosch.org> wrote:
> On Wed, Sep 19, 2018 at 01:00:15PM +0200, Johannes Wienke wrote:
> > This behavior of inheriting the mac address is really unexpected to us.
> > Is it documented somewhere?
>
> Not that I'm aware, but it's a well established behavior.
Not documented, has always been that way. It seems to be part of 802 standard maybe?
Anyway, if you set a MAC address of the bridge device it makes it sticky;
i.e it won't change if ports of bridge change.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: Allow the l3mdev to be a loopback
From: David Ahern @ 2018-09-19 17:19 UTC (permalink / raw)
To: Mike Manning, netdev; +Cc: Robert Shearman
In-Reply-To: <20180919125653.15913-1-mmanning@vyatta.att-mail.com>
On 9/19/18 5:56 AM, Mike Manning wrote:
> From: Robert Shearman <rshearma@vyatta.att-mail.com>
>
> There is no way currently for an IPv6 client connect using a loopback
> address in a VRF, whereas for IPv4 the loopback address can be added:
>
> $ sudo ip addr add dev vrfred 127.0.0.1/8
> $ sudo ip -6 addr add ::1/128 dev vrfred
> RTNETLINK answers: Cannot assign requested address
>
> So allow ::1 to be configured on an L3 master device. In order for
> this to be usable ip_route_output_flags needs to not consider ::1 to
> be a link scope address (since oif == l3mdev and so it would be
> dropped), and ipv6_rcv needs to consider the l3mdev to be a loopback
> device so that it doesn't drop the packets.
>
> Signed-off-by: Robert Shearman <rshearma@vyatta.att-mail.com>
> Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>
> ---
> net/ipv6/addrconf.c | 1 +
> net/ipv6/ip6_input.c | 3 ++-
> net/ipv6/route.c | 3 ++-
> 3 files changed, 5 insertions(+), 2 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
Been on my to-do list for a while. Thanks for the patch. This resolves,
for example, a harmless error message from the 'host' command from
bind9-host-9.10.3 which probes for dscp support via the loopback
address. e.g.,
$ host www.google.com
../../../../lib/isc/unix/net.c:581: sendmsg() failed: Network is unreachable
^ permalink raw reply
* Re: [PATCH net-next v5 00/20] WireGuard: Secure Network Tunnel
From: Ard Biesheuvel @ 2018-09-19 17:21 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Linux Kernel Mailing List, <netdev@vger.kernel.org>,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, David S. Miller,
Greg Kroah-Hartman
In-Reply-To: <20180918210120.GA29812@zx2c4.com>
On 18 September 2018 at 14:01, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Hi Ard,
>
> On Tue, Sep 18, 2018 at 11:28:50AM -0700, Ard Biesheuvel wrote:
>> On 18 September 2018 at 09:16, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> > - While I initially wasn't going to do this for the initial
>> > patchset, it was just so simple to do: now there's a nosimd
>> > module parameter that can be used to disable simd instructions
>> > for debugging and testing, or on weird systems.
>> >
>>
>> I was going to respond in the other thread but it is probably better
>> to move the discussion here.
>>
>> My concern about the monolithic nature of each algo module is not only
>> about SIMD, and it has nothing to do with weird systems. It has to do
>> with micro-architectural differences which are more common on ARM than
>> on other architectures *, I suppose. But generalizing from that, it
>> has to do with policy which is currently owned by userland and not by
>> the kernel. This will also be important for choosing between the time
>> variant but less safe table based scalar AES and the much slower time
>> invariant version (which is substantially slower, especially on
>> decryption) once we move AES into this library.
>>
>> So a command line option for the kernel is not the solution here. If
>> we can't have separate modules, could we at least have per-module
>> options that put the policy decisions back into userland?
>>
>> * as an example, the SHA256 NEON code I collaborated on with Andy
>> Polyakov 2 years ago is significantly faster on some cores and not on
>> others
>
> Interesting concern. There are micro-architectural quirks on x86 too
> that the current code actually already considers. Notably, we use an
> AVX-512VL path for Skylake-X but an AVX-512F path for Knights Landing
> and Coffee Lake and others, due to thermal throttling when touching the
> zmm registers on Skylake-X. So, in the code, we have it automatically
> select the right thing based on the micro-architecture.
>
> Is the same thing not possible with ARM? Do you not have access to this
> information already, such that the module can just always do the right
> thing and not require any user intervention?
>
That depends on what the right thing is. 'Fastest' does not
necessarily mean 'optimal', and I guess the thermal throttling on
Skylake-X may still result in the most power efficient implementation,
which may be the preferred one in some contexts.
The point is that this is a policy decision, and those belong in
userland not in the kernel.
> If so, that would be ideal. If not (and I'm curious to learn why not
> exactly), then indeed we could add some runtime nobs in /sys/module/
> {algo}/parameters/{nob}, or the like. This would be super easy to do,
> should we ever encounter a situation where we're unable to auto-detect
> the correct thing.
>
> Regards,
> Jason
^ permalink raw reply
* Re: [PATCH mlx5-next 02/25] net/mlx5: Set uid as part of QP commands
From: Jason Gunthorpe @ 2018-09-19 17:27 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180917110418.18937-3-leon@kernel.org>
On Mon, Sep 17, 2018 at 02:03:55PM +0300, Leon Romanovsky wrote:
> From: Yishai Hadas <yishaih@mellanox.com>
>
> Set uid as part of QP commands so that the firmware can manage the
> QP object in a secured way.
>
> That will enable using a QP that was created by verbs application to
> be used by the DEVX flow in case the uid is equal.
>
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/net/ethernet/mellanox/mlx5/core/qp.c | 45 +++++++++++++++++-----------
> include/linux/mlx5/mlx5_ifc.h | 22 +++++++-------
> include/linux/mlx5/qp.h | 1 +
> 3 files changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/qp.c b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> index 4ca07bfb6b14..04f72a1cdbcc 100644
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> @@ -240,6 +240,7 @@ int mlx5_core_create_qp(struct mlx5_core_dev *dev,
> if (err)
> return err;
>
> + qp->uid = MLX5_GET(create_qp_in, in, uid);
> qp->qpn = MLX5_GET(create_qp_out, out, qpn);
> mlx5_core_dbg(dev, "qpn = 0x%x\n", qp->qpn);
>
> @@ -261,6 +262,7 @@ int mlx5_core_create_qp(struct mlx5_core_dev *dev,
> memset(dout, 0, sizeof(dout));
> MLX5_SET(destroy_qp_in, din, opcode, MLX5_CMD_OP_DESTROY_QP);
> MLX5_SET(destroy_qp_in, din, qpn, qp->qpn);
> + MLX5_SET(destroy_qp_in, din, uid, qp->uid);
> mlx5_cmd_exec(dev, din, sizeof(din), dout, sizeof(dout));
> return err;
> }
> @@ -320,6 +322,7 @@ int mlx5_core_destroy_qp(struct mlx5_core_dev *dev,
>
> MLX5_SET(destroy_qp_in, in, opcode, MLX5_CMD_OP_DESTROY_QP);
> MLX5_SET(destroy_qp_in, in, qpn, qp->qpn);
> + MLX5_SET(destroy_qp_in, in, uid, qp->uid);
> err = mlx5_cmd_exec(dev, in, sizeof(in), out, sizeof(out));
> if (err)
> return err;
> @@ -373,7 +376,7 @@ static void mbox_free(struct mbox_info *mbox)
>
> static int modify_qp_mbox_alloc(struct mlx5_core_dev *dev, u16 opcode, int qpn,
> u32 opt_param_mask, void *qpc,
> - struct mbox_info *mbox)
> + struct mbox_info *mbox, u16 uid)
> {
> mbox->out = NULL;
> mbox->in = NULL;
> @@ -381,26 +384,32 @@ static int modify_qp_mbox_alloc(struct mlx5_core_dev *dev, u16 opcode, int qpn,
> #define MBOX_ALLOC(mbox, typ) \
> mbox_alloc(mbox, MLX5_ST_SZ_BYTES(typ##_in), MLX5_ST_SZ_BYTES(typ##_out))
>
> -#define MOD_QP_IN_SET(typ, in, _opcode, _qpn) \
> - MLX5_SET(typ##_in, in, opcode, _opcode); \
> - MLX5_SET(typ##_in, in, qpn, _qpn)
> -
> -#define MOD_QP_IN_SET_QPC(typ, in, _opcode, _qpn, _opt_p, _qpc) \
> - MOD_QP_IN_SET(typ, in, _opcode, _qpn); \
> - MLX5_SET(typ##_in, in, opt_param_mask, _opt_p); \
> - memcpy(MLX5_ADDR_OF(typ##_in, in, qpc), _qpc, MLX5_ST_SZ_BYTES(qpc))
> +#define MOD_QP_IN_SET(typ, in, _opcode, _qpn, _uid) \
> + do { \
> + MLX5_SET(typ##_in, in, opcode, _opcode); \
> + MLX5_SET(typ##_in, in, qpn, _qpn); \
> + MLX5_SET(typ##_in, in, uid, _uid); \
> + } while (0)
> +
> +#define MOD_QP_IN_SET_QPC(typ, in, _opcode, _qpn, _opt_p, _qpc, _uid) \
> + do { \
> + MOD_QP_IN_SET(typ, in, _opcode, _qpn, _uid); \
> + MLX5_SET(typ##_in, in, opt_param_mask, _opt_p); \
> + memcpy(MLX5_ADDR_OF(typ##_in, in, qpc), \
> + _qpc, MLX5_ST_SZ_BYTES(qpc)); \
> + } while (0)
These should get touched by clang-format to follow the usual
formatting for macros..
Jason
^ permalink raw reply
* Re: [PATCH mlx5-next 03/25] net/mlx5: Set uid as part of RQ commands
From: Jason Gunthorpe @ 2018-09-19 17:28 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180917110418.18937-4-leon@kernel.org>
On Mon, Sep 17, 2018 at 02:03:56PM +0300, Leon Romanovsky wrote:
> From: Yishai Hadas <yishaih@mellanox.com>
>
> Set uid as part of RQ commands so that the firmware can manage the
> RQ object in a secured way.
>
> That will enable using an RQ that was created by verbs application
> to be used by the DEVX flow in case the uid is equal.
>
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/net/ethernet/mellanox/mlx5/core/qp.c | 16 ++++++++++++++--
> include/linux/mlx5/mlx5_ifc.h | 6 +++---
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/qp.c b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> index 04f72a1cdbcc..0ca68ef54d93 100644
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> @@ -540,6 +540,17 @@ int mlx5_core_xrcd_dealloc(struct mlx5_core_dev *dev, u32 xrcdn)
> }
> EXPORT_SYMBOL_GPL(mlx5_core_xrcd_dealloc);
>
> +static void destroy_rq_tracked(struct mlx5_core_dev *dev, u32 rqn, u16 uid)
> +{
> + u32 in[MLX5_ST_SZ_DW(destroy_rq_in)] = {0};
> + u32 out[MLX5_ST_SZ_DW(destroy_rq_out)] = {0};
= {} is the preferred version of this, right?
{0} explicitly initializes the first element to zero and only works if
the first element happens to be something integral..
Jason
^ permalink raw reply
* [PATCH net-next 01/12] net: hns3: Add default irq affinity
From: Salil Mehta @ 2018-09-19 17:29 UTC (permalink / raw)
To: davem
Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil, netdev,
linux-kernel, linuxarm
In-Reply-To: <20180919172958.12992-1-salil.mehta@huawei.com>
From: Peng Li <lipeng321@huawei.com>
All irq will float to cpu0 if do not set irq affinity.
This patch adds default irq affinity in hns3 driver, users can
also change the irq affinity in OS.
Signed-off-by: Peng Li <lipeng321@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 49 +++++++++++++++++++++++++
drivers/net/ethernet/hisilicon/hns3/hns3_enet.h | 2 +
2 files changed, 51 insertions(+)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 75e8ee9..052b7fb 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -66,6 +66,23 @@ static irqreturn_t hns3_irq_handle(int irq, void *vector)
return IRQ_HANDLED;
}
+/* This callback function is used to set affinity changes to the irq affinity
+ * masks when the irq_set_affinity_notifier function is used.
+ */
+static void hns3_nic_irq_affinity_notify(struct irq_affinity_notify *notify,
+ const cpumask_t *mask)
+{
+ struct hns3_enet_tqp_vector *tqp_vectors =
+ container_of(notify, struct hns3_enet_tqp_vector,
+ affinity_notify);
+
+ tqp_vectors->affinity_mask = *mask;
+}
+
+static void hns3_nic_irq_affinity_release(struct kref *ref)
+{
+}
+
static void hns3_nic_uninit_irq(struct hns3_nic_priv *priv)
{
struct hns3_enet_tqp_vector *tqp_vectors;
@@ -77,6 +94,10 @@ static void hns3_nic_uninit_irq(struct hns3_nic_priv *priv)
if (tqp_vectors->irq_init_flag != HNS3_VECTOR_INITED)
continue;
+ /* clear the affinity notifier and affinity mask */
+ irq_set_affinity_notifier(tqp_vectors->vector_irq, NULL);
+ irq_set_affinity_hint(tqp_vectors->vector_irq, NULL);
+
/* release the irq resource */
free_irq(tqp_vectors->vector_irq, tqp_vectors);
tqp_vectors->irq_init_flag = HNS3_VECTOR_NOT_INITED;
@@ -127,6 +148,15 @@ static int hns3_nic_init_irq(struct hns3_nic_priv *priv)
return ret;
}
+ tqp_vectors->affinity_notify.notify =
+ hns3_nic_irq_affinity_notify;
+ tqp_vectors->affinity_notify.release =
+ hns3_nic_irq_affinity_release;
+ irq_set_affinity_notifier(tqp_vectors->vector_irq,
+ &tqp_vectors->affinity_notify);
+ irq_set_affinity_hint(tqp_vectors->vector_irq,
+ &tqp_vectors->affinity_mask);
+
tqp_vectors->irq_init_flag = HNS3_VECTOR_INITED;
}
@@ -2640,6 +2670,23 @@ static void hns3_add_ring_to_group(struct hns3_enet_ring_group *group,
group->count++;
}
+static void hns3_nic_set_cpumask(struct hns3_nic_priv *priv)
+{
+ struct pci_dev *pdev = priv->ae_handle->pdev;
+ struct hns3_enet_tqp_vector *tqp_vector;
+ int num_vectors = priv->vector_num;
+ int numa_node;
+ int vector_i;
+
+ numa_node = dev_to_node(&pdev->dev);
+
+ for (vector_i = 0; vector_i < num_vectors; vector_i++) {
+ tqp_vector = &priv->tqp_vector[vector_i];
+ cpumask_set_cpu(cpumask_local_spread(vector_i, numa_node),
+ &tqp_vector->affinity_mask);
+ }
+}
+
static int hns3_nic_init_vector_data(struct hns3_nic_priv *priv)
{
struct hnae3_ring_chain_node vector_ring_chain;
@@ -2648,6 +2695,8 @@ static int hns3_nic_init_vector_data(struct hns3_nic_priv *priv)
int ret = 0;
u16 i;
+ hns3_nic_set_cpumask(priv);
+
for (i = 0; i < priv->vector_num; i++) {
tqp_vector = &priv->tqp_vector[i];
hns3_vector_gl_rl_init_hw(tqp_vector, priv);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
index cb450d7..31fa21f 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
@@ -491,7 +491,9 @@ struct hns3_enet_tqp_vector {
struct hns3_enet_ring_group rx_group;
struct hns3_enet_ring_group tx_group;
+ cpumask_t affinity_mask;
u16 num_tqps; /* total number of tqps in TQP vector */
+ struct irq_affinity_notify affinity_notify;
char name[HNAE3_INT_NAME_LEN];
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 11/12] net: hns3: Fix client initialize state issue when roce client initialize failed
From: Salil Mehta @ 2018-09-19 17:29 UTC (permalink / raw)
To: davem
Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil, netdev,
linux-kernel, linuxarm, Jian Shen
In-Reply-To: <20180919172958.12992-1-salil.mehta@huawei.com>
From: Jian Shen <shenjian15@huawei.com>
When roce is loaded before nic, the roce client will not be initialized
until nic client is initialized, but roce init flag is set before it.
Furthermore, in this case of nic initialized success and roce failed,
the nic init flag is not set, and roce init flag is not cleared.
This patch fixes it by set init flag only after the client is initialized
successfully.
Fixes: e2cb1dec9779 ("net: hns3: Add HNS3 VF HCL(Hardware Compatibility Layer) Support")
Fixes: 46a3df9f9718 ("net: hns3: Add HNS3 Acceleration Engine & Compatibility Layer Support")
Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Peng Li <lipeng321@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
drivers/net/ethernet/hisilicon/hns3/hnae3.c | 12 +++++-------
drivers/net/ethernet/hisilicon/hns3/hnae3.h | 3 +++
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 9 +++++++++
drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 9 +++++++++
4 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.c b/drivers/net/ethernet/hisilicon/hns3/hnae3.c
index fff5be8..781e5de 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.c
@@ -29,8 +29,8 @@ static bool hnae3_client_match(enum hnae3_client_type client_type,
return false;
}
-static void hnae3_set_client_init_flag(struct hnae3_client *client,
- struct hnae3_ae_dev *ae_dev, int inited)
+void hnae3_set_client_init_flag(struct hnae3_client *client,
+ struct hnae3_ae_dev *ae_dev, int inited)
{
switch (client->type) {
case HNAE3_CLIENT_KNIC:
@@ -46,6 +46,7 @@ static void hnae3_set_client_init_flag(struct hnae3_client *client,
break;
}
}
+EXPORT_SYMBOL(hnae3_set_client_init_flag);
static int hnae3_get_client_init_flag(struct hnae3_client *client,
struct hnae3_ae_dev *ae_dev)
@@ -86,14 +87,11 @@ static int hnae3_match_n_instantiate(struct hnae3_client *client,
/* now, (un-)instantiate client by calling lower layer */
if (is_reg) {
ret = ae_dev->ops->init_client_instance(client, ae_dev);
- if (ret) {
+ if (ret)
dev_err(&ae_dev->pdev->dev,
"fail to instantiate client, ret = %d\n", ret);
- return ret;
- }
- hnae3_set_client_init_flag(client, ae_dev, 1);
- return 0;
+ return ret;
}
if (hnae3_get_client_init_flag(client, ae_dev)) {
diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index c48c187..17db631 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -523,4 +523,7 @@ void hnae3_register_ae_algo(struct hnae3_ae_algo *ae_algo);
void hnae3_unregister_client(struct hnae3_client *client);
int hnae3_register_client(struct hnae3_client *client);
+
+void hnae3_set_client_init_flag(struct hnae3_client *client,
+ struct hnae3_ae_dev *ae_dev, int inited);
#endif
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 141da18..cf365d4 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -5171,6 +5171,8 @@ static int hclge_init_client_instance(struct hnae3_client *client,
goto clear_nic;
}
+ hnae3_set_client_init_flag(client, ae_dev, 1);
+
if (hdev->roce_client &&
hnae3_dev_roce_supported(hdev)) {
struct hnae3_client *rc = hdev->roce_client;
@@ -5182,6 +5184,9 @@ static int hclge_init_client_instance(struct hnae3_client *client,
ret = rc->ops->init_instance(&vport->roce);
if (ret)
goto clear_roce;
+
+ hnae3_set_client_init_flag(hdev->roce_client,
+ ae_dev, 1);
}
break;
@@ -5193,6 +5198,8 @@ static int hclge_init_client_instance(struct hnae3_client *client,
if (ret)
goto clear_nic;
+ hnae3_set_client_init_flag(client, ae_dev, 1);
+
break;
case HNAE3_CLIENT_ROCE:
if (hnae3_dev_roce_supported(hdev)) {
@@ -5208,6 +5215,8 @@ static int hclge_init_client_instance(struct hnae3_client *client,
ret = client->ops->init_instance(&vport->roce);
if (ret)
goto clear_roce;
+
+ hnae3_set_client_init_flag(client, ae_dev, 1);
}
}
}
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
index 88cff5f..166a2cf 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
@@ -1626,6 +1626,8 @@ static int hclgevf_init_client_instance(struct hnae3_client *client,
if (ret)
goto clear_nic;
+ hnae3_set_client_init_flag(client, ae_dev, 1);
+
if (hdev->roce_client && hnae3_dev_roce_supported(hdev)) {
struct hnae3_client *rc = hdev->roce_client;
@@ -1635,6 +1637,9 @@ static int hclgevf_init_client_instance(struct hnae3_client *client,
ret = rc->ops->init_instance(&hdev->roce);
if (ret)
goto clear_roce;
+
+ hnae3_set_client_init_flag(hdev->roce_client, ae_dev,
+ 1);
}
break;
case HNAE3_CLIENT_UNIC:
@@ -1644,6 +1649,8 @@ static int hclgevf_init_client_instance(struct hnae3_client *client,
ret = client->ops->init_instance(&hdev->nic);
if (ret)
goto clear_nic;
+
+ hnae3_set_client_init_flag(client, ae_dev, 1);
break;
case HNAE3_CLIENT_ROCE:
if (hnae3_dev_roce_supported(hdev)) {
@@ -1660,6 +1667,8 @@ static int hclgevf_init_client_instance(struct hnae3_client *client,
if (ret)
goto clear_roce;
}
+
+ hnae3_set_client_init_flag(client, ae_dev, 1);
}
return 0;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 10/12] net: hns3: Clear client pointer when initialize client failed or unintialize finished
From: Salil Mehta @ 2018-09-19 17:29 UTC (permalink / raw)
To: davem
Cc: salil.mehta, yisen.zhuang, lipeng321, mehta.salil, netdev,
linux-kernel, linuxarm, Jian Shen
In-Reply-To: <20180919172958.12992-1-salil.mehta@huawei.com>
From: Jian Shen <shenjian15@huawei.com>
If initialize client failed or finish uninitializing client, we should
clear the client pointer. It may cause unexpected result when use
uninitialized client. Meanwhile, we also should check whether client
exist when uninitialize it.
Fixes: 46a3df9f9718 ("net: hns3: Add HNS3 Acceleration Engine & Compatibility Layer Support")
Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Peng Li <lipeng321@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
.../ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 25 ++++++++++------
.../ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 33 ++++++++++++++++------
2 files changed, 41 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 8a50688..141da18 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -5162,13 +5162,13 @@ static int hclge_init_client_instance(struct hnae3_client *client,
vport->nic.client = client;
ret = client->ops->init_instance(&vport->nic);
if (ret)
- return ret;
+ goto clear_nic;
ret = hclge_init_instance_hw(hdev);
if (ret) {
client->ops->uninit_instance(&vport->nic,
0);
- return ret;
+ goto clear_nic;
}
if (hdev->roce_client &&
@@ -5177,11 +5177,11 @@ static int hclge_init_client_instance(struct hnae3_client *client,
ret = hclge_init_roce_base_info(vport);
if (ret)
- return ret;
+ goto clear_roce;
ret = rc->ops->init_instance(&vport->roce);
if (ret)
- return ret;
+ goto clear_roce;
}
break;
@@ -5191,7 +5191,7 @@ static int hclge_init_client_instance(struct hnae3_client *client,
ret = client->ops->init_instance(&vport->nic);
if (ret)
- return ret;
+ goto clear_nic;
break;
case HNAE3_CLIENT_ROCE:
@@ -5203,16 +5203,25 @@ static int hclge_init_client_instance(struct hnae3_client *client,
if (hdev->roce_client && hdev->nic_client) {
ret = hclge_init_roce_base_info(vport);
if (ret)
- return ret;
+ goto clear_roce;
ret = client->ops->init_instance(&vport->roce);
if (ret)
- return ret;
+ goto clear_roce;
}
}
}
return 0;
+
+clear_nic:
+ hdev->nic_client = NULL;
+ vport->nic.client = NULL;
+ return ret;
+clear_roce:
+ hdev->roce_client = NULL;
+ vport->roce.client = NULL;
+ return ret;
}
static void hclge_uninit_client_instance(struct hnae3_client *client,
@@ -5232,7 +5241,7 @@ static void hclge_uninit_client_instance(struct hnae3_client *client,
}
if (client->type == HNAE3_CLIENT_ROCE)
return;
- if (client->ops->uninit_instance) {
+ if (hdev->nic_client && client->ops->uninit_instance) {
hclge_uninit_instance_hw(hdev);
client->ops->uninit_instance(&vport->nic, 0);
hdev->nic_client = NULL;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
index 320043e..88cff5f 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
@@ -1624,17 +1624,17 @@ static int hclgevf_init_client_instance(struct hnae3_client *client,
ret = client->ops->init_instance(&hdev->nic);
if (ret)
- return ret;
+ goto clear_nic;
if (hdev->roce_client && hnae3_dev_roce_supported(hdev)) {
struct hnae3_client *rc = hdev->roce_client;
ret = hclgevf_init_roce_base_info(hdev);
if (ret)
- return ret;
+ goto clear_roce;
ret = rc->ops->init_instance(&hdev->roce);
if (ret)
- return ret;
+ goto clear_roce;
}
break;
case HNAE3_CLIENT_UNIC:
@@ -1643,7 +1643,7 @@ static int hclgevf_init_client_instance(struct hnae3_client *client,
ret = client->ops->init_instance(&hdev->nic);
if (ret)
- return ret;
+ goto clear_nic;
break;
case HNAE3_CLIENT_ROCE:
if (hnae3_dev_roce_supported(hdev)) {
@@ -1654,15 +1654,24 @@ static int hclgevf_init_client_instance(struct hnae3_client *client,
if (hdev->roce_client && hdev->nic_client) {
ret = hclgevf_init_roce_base_info(hdev);
if (ret)
- return ret;
+ goto clear_roce;
ret = client->ops->init_instance(&hdev->roce);
if (ret)
- return ret;
+ goto clear_roce;
}
}
return 0;
+
+clear_nic:
+ hdev->nic_client = NULL;
+ hdev->nic.client = NULL;
+ return ret;
+clear_roce:
+ hdev->roce_client = NULL;
+ hdev->roce.client = NULL;
+ return ret;
}
static void hclgevf_uninit_client_instance(struct hnae3_client *client,
@@ -1671,13 +1680,19 @@ static void hclgevf_uninit_client_instance(struct hnae3_client *client,
struct hclgevf_dev *hdev = ae_dev->priv;
/* un-init roce, if it exists */
- if (hdev->roce_client)
+ if (hdev->roce_client) {
hdev->roce_client->ops->uninit_instance(&hdev->roce, 0);
+ hdev->roce_client = NULL;
+ hdev->roce.client = NULL;
+ }
/* un-init nic/unic, if this was not called by roce client */
- if ((client->ops->uninit_instance) &&
- (client->type != HNAE3_CLIENT_ROCE))
+ if (client->ops->uninit_instance && hdev->nic_client &&
+ client->type != HNAE3_CLIENT_ROCE) {
client->ops->uninit_instance(&hdev->nic, 0);
+ hdev->nic_client = NULL;
+ hdev->nic.client = NULL;
+ }
}
static int hclgevf_pci_init(struct hclgevf_dev *hdev)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH mlx5-next 07/25] net/mlx5: Update mlx5_ifc with DEVX UID bits
From: Jason Gunthorpe @ 2018-09-19 17:31 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180917110418.18937-8-leon@kernel.org>
On Mon, Sep 17, 2018 at 02:04:00PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Add DEVX information to WQ, SRQ, CQ, TRI, TIS, QP,
> RQ, XRCD, PD, MKEY and MCG.
>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
> include/linux/mlx5/mlx5_ifc.h | 67 +++++++++++++++++++++++++++----------------
> 1 file changed, 43 insertions(+), 24 deletions(-)
It is weird that we sometimes have these IFC bundle updates and
sometimes the IFC is inlined in the patch..
Why not just do one big IFC only patch with everything the series
needs?
Jason
^ permalink raw reply
* Re: array bounds warning in xfrm_output_resume
From: Florian Westphal @ 2018-09-19 17:45 UTC (permalink / raw)
To: David Ahern; +Cc: Florian Westphal, netdev@vger.kernel.org
In-Reply-To: <8808f8a3-ea1c-f91b-8288-cc4952086f04@gmail.com>
David Ahern <dsahern@gmail.com> wrote:
> > I believe ef57170bbfdd6 is the commit that introduced the warning
> >
>
> Hi Florian:
>
> I am still seeing this. I realize the compiler is not understanding the
> conditions properly, but it is a distraction every time I do a kernel
> build on Debian Stretch having to filter the above noise from whether I
> introduce another warning.
Sorry, I forgot about this. I'll look into it tomorrow.
^ permalink raw reply
* Re: [PATCH] netfilter: nf_tables: add SECMARK support
From: Casey Schaufler @ 2018-09-19 23:36 UTC (permalink / raw)
To: Christian Göttsche, pablo, kadlec, fw, davem,
netfilter-devel, coreteam, netdev, linux-kernel, paul, sds,
eparis, jmorris, serge, selinux, linux-security-module
In-Reply-To: <20180919231402.4482-1-cgzones@googlemail.com>
On 9/19/2018 4:14 PM, Christian Göttsche wrote:
> Add the ability to set the security context of packets within the nf_tables framework.
> Add a nft_object for holding security contexts in the kernel and manipulating packets on the wire.
> The contexts are kept as strings and are evaluated to security identifiers at runtime (packet arrival),
> so that the nft_objects do not need to be refreshed after security changes.
> The maximum security context length is set to 256.
>
> Based on v4.18.6
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
I've only had a cursory look at your patch, but how is it
different from what's in xt_SECMARK.c ?
> ---
> include/net/netfilter/nf_tables_core.h | 4 +
> include/uapi/linux/netfilter/nf_tables.h | 18 ++++-
> net/netfilter/nf_tables_core.c | 28 ++++++-
> net/netfilter/nft_meta.c | 95 ++++++++++++++++++++++++
> 4 files changed, 140 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/netfilter/nf_tables_core.h b/include/net/netfilter/nf_tables_core.h
> index a0513450..0d1f3b96 100644
> --- a/include/net/netfilter/nf_tables_core.h
> +++ b/include/net/netfilter/nf_tables_core.h
> @@ -16,6 +16,10 @@ extern struct nft_expr_type nft_meta_type;
> extern struct nft_expr_type nft_rt_type;
> extern struct nft_expr_type nft_exthdr_type;
>
> +#ifdef CONFIG_NETWORK_SECMARK
> +extern struct nft_object_type nft_secmark_obj_type;
> +#endif
> +
> int nf_tables_core_module_init(void);
> void nf_tables_core_module_exit(void);
>
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index 89438e68..f1527962 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -1169,6 +1169,21 @@ enum nft_quota_attributes {
> };
> #define NFTA_QUOTA_MAX (__NFTA_QUOTA_MAX - 1)
>
> +/**
> + * enum nft_secmark_attributes - nf_tables secmark object netlink attributes
> + *
> + * @NFTA_SECMARK_CTX: security context (NLA_STRING)
> + */
> +enum nft_secmark_attributes {
> + NFTA_SECMARK_UNSPEC,
> + NFTA_SECMARK_CTX,
> + __NFTA_SECMARK_MAX,
> +};
> +#define NFTA_SECMARK_MAX (__NFTA_SECMARK_MAX - 1)
> +
> +/* Max security context length */
> +#define NFT_SECMARK_CTX_MAXLEN 256
> +
> /**
> * enum nft_reject_types - nf_tables reject expression reject types
> *
> @@ -1398,7 +1413,8 @@ enum nft_ct_helper_attributes {
> #define NFT_OBJECT_CT_HELPER 3
> #define NFT_OBJECT_LIMIT 4
> #define NFT_OBJECT_CONNLIMIT 5
> -#define __NFT_OBJECT_MAX 6
> +#define NFT_OBJECT_SECMARK 6
> +#define __NFT_OBJECT_MAX 7
> #define NFT_OBJECT_MAX (__NFT_OBJECT_MAX - 1)
>
> /**
> diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
> index 8de912ca..d59ebba0 100644
> --- a/net/netfilter/nf_tables_core.c
> +++ b/net/netfilter/nf_tables_core.c
> @@ -235,12 +235,24 @@ static struct nft_expr_type *nft_basic_types[] = {
> &nft_exthdr_type,
> };
>
> +static struct nft_object_type *nft_basic_objects[] = {
> +#ifdef CONFIG_NETWORK_SECMARK
> + &nft_secmark_obj_type,
> +#endif
> +};
> +
> int __init nf_tables_core_module_init(void)
> {
> - int err, i;
> + int err, i, j = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(nft_basic_objects); i++) {
> + err = nft_register_obj(nft_basic_objects[i]);
> + if (err)
> + goto err;
> + }
>
> - for (i = 0; i < ARRAY_SIZE(nft_basic_types); i++) {
> - err = nft_register_expr(nft_basic_types[i]);
> + for (j = 0; j < ARRAY_SIZE(nft_basic_types); j++) {
> + err = nft_register_expr(nft_basic_types[j]);
> if (err)
> goto err;
> }
> @@ -248,8 +260,12 @@ int __init nf_tables_core_module_init(void)
> return 0;
>
> err:
> + while (j-- > 0)
> + nft_unregister_expr(nft_basic_types[j]);
> +
> while (i-- > 0)
> - nft_unregister_expr(nft_basic_types[i]);
> + nft_unregister_obj(nft_basic_objects[i]);
> +
> return err;
> }
>
> @@ -260,4 +276,8 @@ void nf_tables_core_module_exit(void)
> i = ARRAY_SIZE(nft_basic_types);
> while (i-- > 0)
> nft_unregister_expr(nft_basic_types[i]);
> +
> + i = ARRAY_SIZE(nft_basic_objects);
> + while (i-- > 0)
> + nft_unregister_obj(nft_basic_objects[i]);
> }
> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index 1105a23b..26b79a3c 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -540,3 +540,98 @@ struct nft_expr_type nft_meta_type __read_mostly = {
> .maxattr = NFTA_META_MAX,
> .owner = THIS_MODULE,
> };
> +
> +#ifdef CONFIG_NETWORK_SECMARK
> +
> +struct nft_secmark {
> + char ctx[NFT_SECMARK_CTX_MAXLEN];
> + int len;
> +};
> +
> +static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
> + [NFTA_SECMARK_CTX] = { .type = NLA_STRING, .len = NFT_SECMARK_CTX_MAXLEN },
> +};
> +
> +
> +static void nft_secmark_obj_eval(struct nft_object *obj, struct nft_regs *regs, const struct nft_pktinfo *pkt)
> +{
> + const struct nft_secmark *priv = nft_obj_data(obj);
> + struct sk_buff *skb = pkt->skb;
> + int err;
> + u32 secid = 0;
> +
> + /* skip if packet has already a secmark */
> + if (skb->secmark)
> + return;
> +
> + err = security_secctx_to_secid(priv->ctx, priv->len, &secid);
> + if (err) {
> + if (err == -EINVAL)
> + pr_notice_ratelimited("invalid security context \'%s\'\n", priv->ctx);
> + else
> + pr_notice_ratelimited("unable to convert security context \'%s\': %d\n", priv->ctx, -err);
> + return;
> + }
> +
> + if (!secid) {
> + pr_notice_ratelimited("unable to map security context \'%s\'\n", priv->ctx);
> + return;
> + }
> +
> + err = security_secmark_relabel_packet(secid);
> + if (err) {
> + pr_notice_ratelimited("unable to obtain relabeling permission: %d\n", -err);
> + return;
> + }
> +
> + skb->secmark = secid;
> +}
> +
> +
> +static int nft_secmark_obj_init(const struct nft_ctx *ctx, const struct nlattr * const tb[], struct nft_object *obj)
> +{
> + struct nft_secmark *priv = nft_obj_data(obj);
> +
> + if (tb[NFTA_SECMARK_CTX] == NULL)
> + return -EINVAL;
> +
> + nla_strlcpy(priv->ctx, tb[NFTA_SECMARK_CTX], NFT_SECMARK_CTX_MAXLEN);
> + priv->len = strlen(priv->ctx);
> +
> + security_secmark_refcount_inc();
> +
> + return 0;
> +}
> +
> +static int nft_secmark_obj_dump(struct sk_buff *skb, struct nft_object *obj, bool reset)
> +{
> + const struct nft_secmark *priv = nft_obj_data(obj);
> +
> + if (nla_put_string(skb, NFTA_SECMARK_CTX, priv->ctx))
> + return -1;
> +
> + return 0;
> +}
> +
> +static void nft_secmark_obj_destroy(const struct nft_ctx *ctx, struct nft_object *obj)
> +{
> + security_secmark_refcount_dec();
> +}
> +
> +static const struct nft_object_ops nft_secmark_obj_ops = {
> + .type = &nft_secmark_obj_type,
> + .size = sizeof(struct nft_secmark),
> + .init = nft_secmark_obj_init,
> + .eval = nft_secmark_obj_eval,
> + .dump = nft_secmark_obj_dump,
> + .destroy = nft_secmark_obj_destroy,
> +};
> +struct nft_object_type nft_secmark_obj_type __read_mostly = {
> + .type = NFT_OBJECT_SECMARK,
> + .ops = &nft_secmark_obj_ops,
> + .maxattr = NFTA_SECMARK_MAX,
> + .policy = nft_secmark_policy,
> + .owner = THIS_MODULE,
> +};
> +
> +#endif /* CONFIG_NETWORK_SECMARK */
^ permalink raw reply
* Re: [PATCH v8 0/4] gpiolib: speed up GPIO array processing
From: Linus Walleij @ 2018-09-19 18:08 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Jonathan Corbet, Miguel Ojeda Sandonis, Peter Korsgaard,
Peter Rosin, Ulf Hansson, Andrew Lunn, Florian Fainelli,
David S. Miller, Dominik Brodowski, Greg KH, kishon,
Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald, Jiri Slaby, Willy Tarreau,
Geert Uytterhoeven, Sebastien Bourdelin
In-Reply-To: <CACRpkdbAG1N0v-BrXruf5088L0x3XwSHbFTWOFxjHQWx0LKz5g@mail.gmail.com>
On Thu, Sep 13, 2018 at 2:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 5, 2018 at 11:49 PM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
>
> > The goal is to boost performance of get/set array functions while
> > processing GPIO arrays which represent pins of a signle chip in
> > hardware order. If resulting performance is close to PIO, GPIO API
> > can be used for data I/O without much loss of speed.
>
> I applied the v8 to an immutable branch and pushed to kernelorg
> so the build servers can churn it a bit, and if it works fine
> then we can merge this into the devel branch and also set up
> that as something other subsystems can pull in if they need it.
>
> I'm really excited to merge this!
The branch built with no problems, and now I merged this into
devel. If that also builds fine, I will let it hit linux-next so we can
stabilize it for v4.20.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH rdma-next 00/24] Extend DEVX functionality
From: Jason Gunthorpe @ 2018-09-19 18:17 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180917110418.18937-1-leon@kernel.org>
On Mon, Sep 17, 2018 at 02:03:53PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> From Yishai,
>
> This series comes to enable the DEVX functionality in some wider scope,
> specifically,
> - It enables using kernel objects that were created by the verbs
> API in the DEVX flow.
> - It enables white list commands without DEVX user context.
> - It enables the IB link layer under CAP_NET_RAW capabilities.
> - It exposes the PRM handles for RAW QP (i.e. TIRN, TISN, RQN, SQN)
> to be used later on directly by the DEVX interface.
>
> In General,
> Each object that is created/destroyed/modified via verbs will be stamped
> with a UID based on its user context. This is already done for DEVX objects
> commands.
>
> This will enable the firmware to enforce the usage of kernel objects
> from the DEVX flow by validating that the same UID is used and the resources are
> really related to the same user.
>
> For example in case a CQ was created with verbs it will be stamped with
> UID and once will be pointed by a DEVX create QP command the firmware will
> validate that the input CQN really belongs to the UID which issues the create QP
> command.
>
> As of the above, all the PRM objects (except of the public ones which
> are managed by the kernel e.g. FLOW, etc.) will have a UID upon their
> create/modify/destroy commands. The detection of UMEM / physical
> addressed in the relevant commands will be done by firmware according to a 'umem
> valid bit' as the UID may be used in both cases.
>
> The series also enables white list commands which don't require a
> specific DEVX context, instead of this a device UID is used so that
> the firmware will mask un-privileged functionality. The IB link layer
> is also enabled once CAP_NET_RAW permission exists.
>
> To enable using the RAW QP underlay objects (e.g. TIRN, RQN, etc.) later
> on by DEVX commands the UHW output for this case was extended to return this
> data when a DEVX context is used.
>
> Thanks
>
> Leon Romanovsky (1):
> net/mlx5: Update mlx5_ifc with DEVX UID bits
>
> Yishai Hadas (24):
> net/mlx5: Set uid as part of CQ commands
> net/mlx5: Set uid as part of QP commands
> net/mlx5: Set uid as part of RQ commands
> net/mlx5: Set uid as part of SQ commands
> net/mlx5: Set uid as part of SRQ commands
> net/mlx5: Set uid as part of DCT commands
> IB/mlx5: Set uid as part of CQ creation
> IB/mlx5: Set uid as part of QP creation
> IB/mlx5: Set uid as part of RQ commands
> IB/mlx5: Set uid as part of SQ commands
> IB/mlx5: Set uid as part of TIR commands
> IB/mlx5: Set uid as part of TIS commands
> IB/mlx5: Set uid as part of RQT commands
> IB/mlx5: Set uid as part of PD commands
> IB/mlx5: Set uid as part of TD commands
> IB/mlx5: Set uid as part of SRQ commands
> IB/mlx5: Set uid as part of DCT commands
> IB/mlx5: Set uid as part of XRCD commands
> IB/mlx5: Set uid as part of MCG commands
This is really too many patches.. They are small and not too hard to
review, but it is well beyond the guideline.
And I'm not totally happy with the extensive use of ucontext in the IB
portions, it is problematic looking into the future, and uboject is
really not supposed to be used in the drivers.
The driver needs to store the uid in the PD (copied from the ucontext
that created it) and use that in all the dependent places, not use
pd->uobject->ucontext->devx_uid or some other convoluted way to get
to it.
The ucontext variable should only be used when creating the PD, CQ and
devx objects.
This detail becomes quite important, for instance, if we get to the
'shared pd' that has been talked about at conference. In this case
when the 'receiver' of the 'shared pd' creates a child object, like a
MR, the MR must be stamped with the devx_uid of the PD (ie the
originating context's devx_uid), not the dev_uid of its local ufile!
If we do that, then the series can be split, so long as pd->devx_uid ==
0 until the entire series is applied. uid tagging is an all-or-nothing
thing, as partial tagging will break verbs. So breaking it up also
makes it more bi-section safe.
Something like these patches:
> net/mlx5: Update mlx5_ifc with DEVX UID bits
> net/mlx5: Set uid as part of CQ commands
> net/mlx5: Set uid as part of QP commands
> net/mlx5: Set uid as part of RQ commands
> net/mlx5: Set uid as part of SQ commands
> net/mlx5: Set uid as part of SRQ commands
> net/mlx5: Set uid as part of DCT commands
> IB/mlx5: Set uid as part of PD commands
> IB/mlx5: Set uid as part of QP creation
> IB/mlx5: Set uid as part of RQ commands
> IB/mlx5: Set uid as part of SQ commands
> IB/mlx5: Set uid as part of SRQ commands
> IB/mlx5: Set uid as part of DCT commands
(13 patches)
Followed by the rest of the IB uid patches
Followed by a patch to make pd->uid != 0 along with these:
> IB/mlx5: Set uid as part of CQ creation
> IB/mlx5: Set valid umem bit on DEVX
> IB/mlx5: Expose RAW QP device handles to user space
> IB/mlx5: Manage device uid for DEVX white list commands
> IB/mlx5: Enable DEVX white list commands
> IB/mlx5: Enable DEVX on IB
Jason
^ permalink raw reply
* Re: kernels > v4.12 oops/crash with ipsec-traffic: bisected to b838d5e1c5b6e57b10ec8af2268824041e3ea911: ipv4: mark DST_NOGC and remove the operation of dst_free()
From: Tobias Hommel @ 2018-09-19 18:38 UTC (permalink / raw)
To: Steffen Klassert
Cc: Wolfgang Walter, Kristian Evensen, Network Development, weiwan,
edumazet
In-Reply-To: <20180912151823.z2wk7hnex4zxly3e@arbeitstier>
> After running for about 24 hours, I now encountered another panic. This time it
> is caused by an out of memory situation. Although the trace shows action in the
> filesystem code I'm posting it here because I cannot isolate the error and
> maybe it is caused by our NULL pointer bug or by the new fix.
> I do not have a serial console attached, so I could only attach a screenshot of
> the panic to this mail.
>
> I am running v4.19-rc3 from git with the above mentioned patch applied.
> After 19 hours everything still looked fine, XfrmFwdHdrError value was at ~950.
> Overall memory usage shown by htop was at 1.2G/15.6G.
> I had htop running via ssh so I was able to see at least some status post
> mortem. Uptime: 23:50:57
> Overall memory usage was at 10.2G/15.6G and user processes were just
> using the usual amount of memory, so it looks like the kernel was eating up at
> least 9G of RAM.
>
> Maybe this information is not very helpful for debugging, but it is at least a
> warning that something might still be wrong.
>
> I'll try to gather some more information and keep you updated.
Running stable under load for more than 5 days now, I was not able to reproduce
that OOM situation. I leave it at that, the fix for the initial bug is fine for
me.
^ permalink raw reply
* Re: [PATCH mlx5-next 03/25] net/mlx5: Set uid as part of RQ commands
From: Saeed Mahameed @ 2018-09-19 18:40 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Yishai Hadas, Saeed Mahameed, Linux Netdev List
In-Reply-To: <20180919172855.GN11367@ziepe.ca>
On Wed, Sep 19, 2018 at 10:28 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Mon, Sep 17, 2018 at 02:03:56PM +0300, Leon Romanovsky wrote:
> > From: Yishai Hadas <yishaih@mellanox.com>
> >
> > Set uid as part of RQ commands so that the firmware can manage the
> > RQ object in a secured way.
> >
> > That will enable using an RQ that was created by verbs application
> > to be used by the DEVX flow in case the uid is equal.
> >
> > Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > drivers/net/ethernet/mellanox/mlx5/core/qp.c | 16 ++++++++++++++--
> > include/linux/mlx5/mlx5_ifc.h | 6 +++---
> > 2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/qp.c b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> > index 04f72a1cdbcc..0ca68ef54d93 100644
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> > @@ -540,6 +540,17 @@ int mlx5_core_xrcd_dealloc(struct mlx5_core_dev *dev, u32 xrcdn)
> > }
> > EXPORT_SYMBOL_GPL(mlx5_core_xrcd_dealloc);
> >
> > +static void destroy_rq_tracked(struct mlx5_core_dev *dev, u32 rqn, u16 uid)
> > +{
> > + u32 in[MLX5_ST_SZ_DW(destroy_rq_in)] = {0};
> > + u32 out[MLX5_ST_SZ_DW(destroy_rq_out)] = {0};
>
> = {} is the preferred version of this, right?
>
> {0} explicitly initializes the first element to zero and only works if
> the first element happens to be something integral..
>
Both are perfectly ok in our scenarios.
I remember one of the syntaxes yielded a statistic checker warning, i
don't remember which syntax and what static checker :) ..
> Jason
^ permalink raw reply
* Re: [RFC bpf-next 2/4] bpf: return EOPNOTSUPP when map lookup isn't supported
From: Mauricio Vasquez @ 2018-09-19 18:40 UTC (permalink / raw)
To: Alexei Starovoitov, Prashant Bhole
Cc: Alexei Starovoitov, Daniel Borkmann, Jakub Kicinski,
Quentin Monnet, David S . Miller, netdev
In-Reply-To: <20180919151422.tysdcku2hxaq37xw@ast-mbp>
On 09/19/2018 10:14 AM, Alexei Starovoitov wrote:
> On Wed, Sep 19, 2018 at 04:51:41PM +0900, Prashant Bhole wrote:
>> Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
>> map types:
>> - BPF_MAP_TYPE_PROG_ARRAY
>> - BPF_MAP_TYPE_STACK_TRACE
>> - BPF_MAP_TYPE_XSKMAP
>> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
>>
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>> ---
>> kernel/bpf/arraymap.c | 2 +-
>> kernel/bpf/sockmap.c | 2 +-
>> kernel/bpf/stackmap.c | 2 +-
>> kernel/bpf/xskmap.c | 2 +-
>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>> index dded84cbe814..24583da9ffd1 100644
>> --- a/kernel/bpf/arraymap.c
>> +++ b/kernel/bpf/arraymap.c
>> @@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
>>
>> static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
>> {
>> - return NULL;
>> + return ERR_PTR(-EOPNOTSUPP);
>> }
> conceptually the set looks good to me.
> Please add a test to test_verifier.c to make sure
> that these lookup helpers cannot be called from BPF program.
> Otherwise this diff may cause crashes.
>
>
I think we can remove all these stub functions that return EOPNOTSUPP or
EINVAL and return the error in syscall.c if ops->map_[update, delete,
lookup, ...] is null.
This will require to extend (and test) the verifier to guarantee that
those helpers are not called in wrong maps, for example
map_delete_element in array like maps.
Would it make sense?
^ permalink raw reply
* Re: [RFC 4/5] netlink: prepare validate extack setting for recursion
From: Marcelo Ricardo Leitner @ 2018-09-19 18:46 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev
In-Reply-To: <1537349117.10305.25.camel@sipsolutions.net>
On Wed, Sep 19, 2018 at 11:25:17AM +0200, Johannes Berg wrote:
> On Wed, 2018-09-19 at 00:37 -0300, Marcelo Ricardo Leitner wrote:
>
> > Did you consider indicating the message level, and only overwrite the
> > message that is already in there if the new message level is higher
> > than the current one?
>
> Hmm, no, I guess I didn't - I'm not even sure I understand what you're
> saying.
>
> This code in itself generates no "warning" messages; that was just a
> construct we discussed in the NLA_REJECT thread, e.g. if you say (like I
> just also wrote in my reply to Jiri):
>
> NL_SET_ERR_MSG(extack, "warning: deprecated command");
> err = nla_parse(..., extack);
> if (err)
> return err;
> /* do something */
> return 0;
>
> Here you could consider the message there a warning that's transported
> out even if we return 0, but if we return with a failure from
> nla_parse() (or nla_validate instead if you wish), then that failure
> message "wins".
Agree. This is the core issue here, IMHO. Once out of the context that
set the message, we have no way of knowing if we can nor should
overwrite the message that is already in there.
>
> > This way the first to set an Error message will have it, and Warning
> > messages would be overwritten by such if needed. But it also would
> > cause the first warning to be held, and not the last one, as it does
> > today. We want the first error, but the last warning otherwise.
> >
> > It would not be possible to overwrite if new_msglvl >= cur_msglvl
> > because then it would trigger the initial issue again, so some extra
> > logic would be needed to solve this.
>
> That sounds way more complex than what I'm doing now?
Yes but hopefully it would a clearer way of how it is/should be handled.
>
> Note, like I said above, this isn't *generic* in any way. This code here
> will only ever set error messages that should "win".
>
> I suppose we could - technically - make that generic, in that we could
> have both
>
> NLA_SET_WARN_MSG(extack, "...");
> NLA_SET_ERR_MSG(extack, "...");
I like this.
>
> and keep track of warning vs. error; however, just like my first version
> of the NLA_REJECT patch, that would break existing code.
Hm, I may have missed something but I think the discussion in there
was for a different context. For an extack msg to be set by
when validate_nla() call returns on nla_parse(), the previous message
had to be a "warning" because otherwise the parsing wouldn't be even
attempted. So in that case, we are safe to simply overwrite it.
While for the situation you are describing here, it will set a generic
error message in case the inner code didn't do it.
Using the semantics of NLA_SET_WARN_MSG and ERR, then WARN would
replace other WARNs but not ERRs, and ERR would replace other WARNs
too but not other ERRs. All we need to do handle this is a bit in
extack saying if the message is considered a warning or not, or an
error/fatal message or not.
>
> I also don't think that we actually *need* this complexity in general.
> It should almost always be possible (and realistically, pretty easy) to
> structure your code in a way that warning messages only go out if no
> error message overwrites them. The only reason we were ever even
> discussing this was that in NLA_REJECT I missed the fact that somebody
> could've set a message before and thus would keep it rather than
> overwrite it, which was a change in behaviour.
Okay but we have split parsing of netlink messages and this could be
useful in there too:
In cls_api.c, tc_new_tfilter() calls nmmsg_parse() and do some
processing, and then handle it to a classifier. cls_flower, for
example, will then do another parsing. If, for whatever reason, flower
failed and did not set an extack msg, tc_new_tfilter() could set a
default error message, but at this moment we can't tell if the msg
already set was just a warning from the first parsing (or any other
code before engaging flower) (which we can overwrite) or if it a
message that flower set (which we should not overwrite).
Hopefully my description clear.. 8-)
I think this is the same situation as with the nested parsing you're
proposing.
Currently it (tc_new_tfilter) doesn't set any default error message,
so this issue is/was not noticed.
>
> Now, with this patch, all I'm doing is changing the internal behaviour
> of nla_parse/nla_validate - externally, it still overwrites any existing
> message if an error occurs, but internally it keeps the inner-most
> error.
>
> Why is this? Consider this:
>
> static const struct nla_policy inner_policy[] = {
> [INNER_FLAG] = { .type = NLA_REJECT,
> .validation_data = "must not set this flag" }
> };
>
> static const struct nla_policy outer_policy[] = {
> [OUTER_NESTING] = { .type = NLA_NESTED, .len = INNER_MAX,
> .validation_data = inner_policy,
> };
>
> Now if you invoke nla_parse/nla_validate with a message like this
>
> [ OUTER_NESTING => [ INNER_FLAG, ... ], ... ]
>
> you'd get "must not set this flag" with the error offset pointing to
> that; if I didn't do this construction here with inner messages winning,
> you'd get "Attribute failed policy validation" with the error offset
> pointing to the "OUTER_NESTING" attribute, that's pretty useless.
Yes, agree.
>
> From an external API POV though, nla_validate/nla_parse will continue to
> unconditionally overwrite any existing "warning" messages with errors,
> if such occurred. They just won't overwrite their own messages when
> returning from a nested policy validation.
Yes, that's fine. I'm not objecting to the behavior. It just feels
that extack handling is getting tricky by the day and we could seize
the moment to improve it while we can.
Marcelo
^ permalink raw reply
* [PATCH] net-next: mscc: remove unused ocelot_dev_gmii.h
From: Corentin Labbe @ 2018-09-19 19:06 UTC (permalink / raw)
To: alexandre.belloni, davem; +Cc: linux-kernel, netdev, Corentin Labbe
The header ocelot_dev_gmii.h is unused since the inclusion of the driver.
It is unused, lets just remove it.
Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
drivers/net/ethernet/mscc/ocelot_dev_gmii.h | 154 ----------------------------
1 file changed, 154 deletions(-)
delete mode 100644 drivers/net/ethernet/mscc/ocelot_dev_gmii.h
diff --git a/drivers/net/ethernet/mscc/ocelot_dev_gmii.h b/drivers/net/ethernet/mscc/ocelot_dev_gmii.h
deleted file mode 100644
index 6aa40ea..0000000
--- a/drivers/net/ethernet/mscc/ocelot_dev_gmii.h
+++ /dev/null
@@ -1,154 +0,0 @@
-/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
-/*
- * Microsemi Ocelot Switch driver
- *
- * Copyright (c) 2017 Microsemi Corporation
- */
-
-#ifndef _MSCC_OCELOT_DEV_GMII_H_
-#define _MSCC_OCELOT_DEV_GMII_H_
-
-#define DEV_GMII_PORT_MODE_CLOCK_CFG 0x0
-
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_MAC_TX_RST BIT(5)
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_MAC_RX_RST BIT(4)
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_PORT_RST BIT(3)
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_PHY_RST BIT(2)
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_LINK_SPEED(x) ((x) & GENMASK(1, 0))
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_LINK_SPEED_M GENMASK(1, 0)
-
-#define DEV_GMII_PORT_MODE_PORT_MISC 0x4
-
-#define DEV_GMII_PORT_MODE_PORT_MISC_MPLS_RX_ENA BIT(5)
-#define DEV_GMII_PORT_MODE_PORT_MISC_FWD_ERROR_ENA BIT(4)
-#define DEV_GMII_PORT_MODE_PORT_MISC_FWD_PAUSE_ENA BIT(3)
-#define DEV_GMII_PORT_MODE_PORT_MISC_FWD_CTRL_ENA BIT(2)
-#define DEV_GMII_PORT_MODE_PORT_MISC_GMII_LOOP_ENA BIT(1)
-#define DEV_GMII_PORT_MODE_PORT_MISC_DEV_LOOP_ENA BIT(0)
-
-#define DEV_GMII_PORT_MODE_EVENTS 0x8
-
-#define DEV_GMII_PORT_MODE_EEE_CFG 0xc
-
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_ENA BIT(22)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_AGE(x) (((x) << 15) & GENMASK(21, 15))
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_AGE_M GENMASK(21, 15)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_AGE_X(x) (((x) & GENMASK(21, 15)) >> 15)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_WAKEUP(x) (((x) << 8) & GENMASK(14, 8))
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_WAKEUP_M GENMASK(14, 8)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_WAKEUP_X(x) (((x) & GENMASK(14, 8)) >> 8)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_HOLDOFF(x) (((x) << 1) & GENMASK(7, 1))
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_HOLDOFF_M GENMASK(7, 1)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_HOLDOFF_X(x) (((x) & GENMASK(7, 1)) >> 1)
-#define DEV_GMII_PORT_MODE_EEE_CFG_PORT_LPI BIT(0)
-
-#define DEV_GMII_PORT_MODE_RX_PATH_DELAY 0x10
-
-#define DEV_GMII_PORT_MODE_TX_PATH_DELAY 0x14
-
-#define DEV_GMII_PORT_MODE_PTP_PREDICT_CFG 0x18
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_ENA_CFG 0x1c
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_ENA_CFG_RX_ENA BIT(4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_ENA_CFG_TX_ENA BIT(0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_MODE_CFG 0x20
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_MODE_CFG_FC_WORD_SYNC_ENA BIT(8)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_MODE_CFG_GIGA_MODE_ENA BIT(4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_MODE_CFG_FDX_ENA BIT(0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_MAXLEN_CFG 0x24
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG 0x28
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_TAG_ID(x) (((x) << 16) & GENMASK(31, 16))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_TAG_ID_M GENMASK(31, 16)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_TAG_ID_X(x) (((x) & GENMASK(31, 16)) >> 16)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_PB_ENA BIT(1)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_VLAN_AWR_ENA BIT(0)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_VLAN_LEN_AWR_ENA BIT(2)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_ADV_CHK_CFG 0x2c
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_ADV_CHK_CFG_LEN_DROP_ENA BIT(0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG 0x30
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RESTORE_OLD_IPG_CHECK BIT(17)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_REDUCED_TX_IFG BIT(16)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_TX_IFG(x) (((x) << 8) & GENMASK(12, 8))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_TX_IFG_M GENMASK(12, 8)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_TX_IFG_X(x) (((x) & GENMASK(12, 8)) >> 8)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RX_IFG2(x) (((x) << 4) & GENMASK(7, 4))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RX_IFG2_M GENMASK(7, 4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RX_IFG2_X(x) (((x) & GENMASK(7, 4)) >> 4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RX_IFG1(x) ((x) & GENMASK(3, 0))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RX_IFG1_M GENMASK(3, 0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG 0x34
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_BYPASS_COL_SYNC BIT(26)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_OB_ENA BIT(25)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_WEXC_DIS BIT(24)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_SEED(x) (((x) << 16) & GENMASK(23, 16))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_SEED_M GENMASK(23, 16)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_SEED_X(x) (((x) & GENMASK(23, 16)) >> 16)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_SEED_LOAD BIT(12)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_RETRY_AFTER_EXC_COL_ENA BIT(8)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_LATE_COL_POS(x) ((x) & GENMASK(6, 0))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_LATE_COL_POS_M GENMASK(6, 0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_DBG_CFG 0x38
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_DBG_CFG_TBI_MODE BIT(4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_DBG_CFG_IFG_CRS_EXT_CHK_ENA BIT(0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_FC_MAC_LOW_CFG 0x3c
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_FC_MAC_HIGH_CFG 0x40
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY 0x44
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_RX_IPG_SHRINK_STICKY BIT(9)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_RX_PREAM_SHRINK_STICKY BIT(8)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_RX_CARRIER_EXT_STICKY BIT(7)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_RX_CARRIER_EXT_ERR_STICKY BIT(6)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_RX_JUNK_STICKY BIT(5)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_TX_RETRANSMIT_STICKY BIT(4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_TX_JAM_STICKY BIT(3)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_TX_FIFO_OFLW_STICKY BIT(2)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_TX_FRM_LEN_OVR_STICKY BIT(1)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_TX_ABORT_STICKY BIT(0)
-
-#define DEV_GMII_MM_CONFIG_ENABLE_CONFIG 0x48
-
-#define DEV_GMII_MM_CONFIG_ENABLE_CONFIG_MM_RX_ENA BIT(0)
-#define DEV_GMII_MM_CONFIG_ENABLE_CONFIG_MM_TX_ENA BIT(4)
-#define DEV_GMII_MM_CONFIG_ENABLE_CONFIG_KEEP_S_AFTER_D BIT(8)
-
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG 0x4c
-
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_PRM_VERIFY_DIS BIT(0)
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_PRM_VERIFY_TIME(x) (((x) << 4) & GENMASK(11, 4))
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_PRM_VERIFY_TIME_M GENMASK(11, 4)
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_PRM_VERIFY_TIME_X(x) (((x) & GENMASK(11, 4)) >> 4)
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_VERIF_TIMER_UNITS(x) (((x) << 12) & GENMASK(13, 12))
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_VERIF_TIMER_UNITS_M GENMASK(13, 12)
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_VERIF_TIMER_UNITS_X(x) (((x) & GENMASK(13, 12)) >> 12)
-
-#define DEV_GMII_MM_STATISTICS_MM_STATUS 0x50
-
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_PRMPT_ACTIVE_STATUS BIT(0)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_PRMPT_ACTIVE_STICKY BIT(4)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_PRMPT_VERIFY_STATE(x) (((x) << 8) & GENMASK(10, 8))
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_PRMPT_VERIFY_STATE_M GENMASK(10, 8)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_PRMPT_VERIFY_STATE_X(x) (((x) & GENMASK(10, 8)) >> 8)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_UNEXP_RX_PFRM_STICKY BIT(12)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_UNEXP_TX_PFRM_STICKY BIT(16)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_MM_RX_FRAME_STATUS BIT(20)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_MM_TX_FRAME_STATUS BIT(24)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_MM_TX_PRMPT_STATUS BIT(28)
-
-#endif
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion
From: Marcelo Ricardo Leitner @ 2018-09-19 19:08 UTC (permalink / raw)
To: David Ahern; +Cc: Johannes Berg, linux-wireless, netdev
In-Reply-To: <50773483-5732-8874-c5bf-99fa09d7e94a@gmail.com>
On Wed, Sep 19, 2018 at 09:44:37AM -0700, David Ahern wrote:
> On 9/19/18 9:36 AM, Johannes Berg wrote:
> > On Wed, 2018-09-19 at 09:28 -0700, David Ahern wrote:
> >> On 9/19/18 5:08 AM, Johannes Berg wrote:
> >>> diff --git a/lib/nlattr.c b/lib/nlattr.c
> >>> index 966cd3dcf31b..2b015e43b725 100644
> >>> --- a/lib/nlattr.c
> >>> +++ b/lib/nlattr.c
> >>> @@ -69,7 +69,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
> >>>
> >>> static int validate_nla(const struct nlattr *nla, int maxtype,
> >>> const struct nla_policy *policy,
> >>> - const char **error_msg)
> >>> + struct netlink_ext_ack *extack, bool *extack_set)
> >>
> >> extack_set arg is not needed if you handle the "Attribute failed policy
> >> validation" message and NL_SET_BAD_ATTR here as well.
> >
> > I'm not sure that's true, but perhaps you have a better idea than me?
> >
> > My thought would be to introduce an "error" label in validate_nla(),
> > that sets up the extack data.
> >
> > Then we could skip over that if we have a separate message to report,
> > making the NLA_REJECT case easier.
> >
> > However, if we do nested validation, I'm not sure it really is that much
> > easier? We still need to figure out if the nested validation was setting
> > the message (and bad attr), rather than it having been set before we
> > even get into this function.
> >
> > So let's say we have
> >
> > case NLA_NESTED:
> > /* a nested attributes is allowed to be empty; if its not,
> > * it must have a size of at least NLA_HDRLEN.
> > */
> > if (attrlen == 0)
> > break;
> > if (attrlen < NLA_HDRLEN)
> > return -ERANGE;
> > if (pt->validation_data) {
> > int err;
> >
> > err = nla_validate_parse(nla_data(nla), nla_len(nla),
> > pt->len, pt->validation_data,
> > extack, extack_set, NULL);
> > if (err < 0)
> > return err;
> > }
> > break;
> >
> > right now after all the patches.
> >
> > The "return -ERANGE;" would become "{ err = -ERANGE; goto error; }", but
> > I'm not really sure we can cleanly handle the other case?
> >
> > Hmm. Maybe it works if we ensure that nla_validate_parse() has no other
> > return points that can fail outside of validate_nla(), or we set up the
> > extack data there as well, so that once we have a nested
> > nla_validate_parse() we know that it's been set.
> >
> > Actually, we need to do that anyway so that we can move the setting into
> > validate_nla(), and then it should work.
> >
> > Mechanics aside - I'll take a look later tonight or tomorrow - do you
> > think the goal/external interface of this makes sense?
>
> If it fails and returns (nested and all) on the first failure it should
> be fine. I was thinking something like this (whitespace damaged on paste):
This will avoid the situation that we were discussing in the older
thread, btw.
Marcelo
^ 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