* 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
* Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion
From: Johannes Berg @ 2018-09-19 19:09 UTC (permalink / raw)
To: Marcelo Ricardo Leitner, David Ahern; +Cc: linux-wireless, netdev
In-Reply-To: <20180919190851.GM4590@localhost.localdomain>
On Wed, 2018-09-19 at 16:08 -0300, Marcelo Ricardo Leitner wrote:
>
> > 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.
I think it only avoids the part where we have to worry about "have I
already set this" - which is David's point AFAICT.
I'll reply over to your other email (as I already started writing a
reply there)
johannes
^ permalink raw reply
* [PATCH] mpls: allow routes on ip6gre devices
From: Saif Hhasan @ 2018-09-19 17:15 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller, Calvin Owens, Saif Hasan
From: Saif Hasan <has@fb.com>
Summary:
This appears to be necessary and sufficient change to enable `MPLS` on
`ip6gre` tunnels (RFC4023).
This diff allows IP6GRE devices to be recognized by MPLS kernel module
and hence user can configure interface to accept packets with mpls
headers as well setup mpls routes on them.
Test Plan:
Test plan consists of multiple containers connected via GRE-V6 tunnel.
Then carrying out testing steps as below.
- Carry out necessary sysctl settings on all containers
```
sysctl -w net.mpls.platform_labels=65536
sysctl -w net.mpls.ip_ttl_propagate=1
sysctl -w net.mpls.conf.lo.input=1
```
- Establish IP6GRE tunnels
```
ip -6 tunnel add name if_1_2_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::2 key 1
ip link set dev if_1_2_1 up
sysctl -w net.mpls.conf.if_1_2_1.input=1
ip -4 addr add 169.254.0.2/31 dev if_1_2_1 scope link
ip -6 tunnel add name if_1_3_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::3 key 1
ip link set dev if_1_3_1 up
sysctl -w net.mpls.conf.if_1_3_1.input=1
ip -4 addr add 169.254.0.4/31 dev if_1_3_1 scope link
```
- Install MPLS encap rules on node-1 towards node-2
```
ip route add 192.168.0.11/32 nexthop encap mpls 32/64 \
via inet 169.254.0.3 dev if_1_2_1
```
- Install MPLS forwarding rules on node-2 and node-3
```
// node2
ip -f mpls route add 32 via inet 169.254.0.7 dev if_2_4_1
// node3
ip -f mpls route add 64 via inet 169.254.0.12 dev if_4_3_1
```
- Ping 192.168.0.11 (node4) from 192.168.0.1 (node1) (where routing
towards 192.168.0.1 is via IP route directly towards node1 from node4)
```
ping 192.168.0.11
```
- tcpdump on interface to capture ping packets wrapped within MPLS
header which inturn wrapped within IP6GRE header
```
16:43:41.121073 IP6
2401:db00:21:6048:feed::1 > 2401:db00:21:6048:feed::2:
DSTOPT GREv0, key=0x1, length 100:
MPLS (label 32, exp 0, ttl 255) (label 64, exp 0, [S], ttl 255)
IP 192.168.0.1 > 192.168.0.11:
ICMP echo request, id 1208, seq 45, length 64
0x0000: 6000 2cdb 006c 3c3f 2401 db00 0021 6048 `.,..l<?$....!`H
0x0010: feed 0000 0000 0001 2401 db00 0021 6048 ........$....!`H
0x0020: feed 0000 0000 0002 2f00 0401 0401 0100 ......../.......
0x0030: 2000 8847 0000 0001 0002 00ff 0004 01ff ...G............
0x0040: 4500 0054 3280 4000 ff01 c7cb c0a8 0001 E..T2.@.........
0x0050: c0a8 000b 0800 a8d7 04b8 002d 2d3c a05b ...........--<.[
0x0060: 0000 0000 bcd8 0100 0000 0000 1011 1213 ................
0x0070: 1415 1617 1819 1a1b 1c1d 1e1f 2021 2223 .............!"#
0x0080: 2425 2627 2829 2a2b 2c2d 2e2f 3031 3233 $%&'()*+,-./0123
0x0090: 3435 3637 4567
```
Signed-off-by: Saif Hasan <has@fb.com>
---
net/mpls/af_mpls.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7a4de6d618b1..aeb5bf2f7595 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1533,10 +1533,11 @@ static int mpls_dev_notify(struct notifier_block *this, unsigned long event,
unsigned int flags;
if (event == NETDEV_REGISTER) {
- /* For now just support Ethernet, IPGRE, SIT and IPIP devices */
+ /* For now just support Ethernet, IPGRE, IP6GRE, SIT and IPIP devices */
if (dev->type == ARPHRD_ETHER ||
dev->type == ARPHRD_LOOPBACK ||
dev->type == ARPHRD_IPGRE ||
+ dev->type == ARPHRD_IP6GRE ||
dev->type == ARPHRD_SIT ||
dev->type == ARPHRD_TUNNEL) {
mdev = mpls_add_dev(dev);
^ permalink raw reply related
* [PATCH] mpls: allow routes on ip6gre devices
From: Saif Hasan @ 2018-09-19 17:20 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller, Calvin Owens, Saif Hasan
Summary:
This appears to be necessary and sufficient change to enable `MPLS` on
`ip6gre` tunnels (RFC4023).
This diff allows IP6GRE devices to be recognized by MPLS kernel module
and hence user can configure interface to accept packets with mpls
headers as well setup mpls routes on them.
Test Plan:
Test plan consists of multiple containers connected via GRE-V6 tunnel.
Then carrying out testing steps as below.
- Carry out necessary sysctl settings on all containers
```
sysctl -w net.mpls.platform_labels=65536
sysctl -w net.mpls.ip_ttl_propagate=1
sysctl -w net.mpls.conf.lo.input=1
```
- Establish IP6GRE tunnels
```
ip -6 tunnel add name if_1_2_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::2 key 1
ip link set dev if_1_2_1 up
sysctl -w net.mpls.conf.if_1_2_1.input=1
ip -4 addr add 169.254.0.2/31 dev if_1_2_1 scope link
ip -6 tunnel add name if_1_3_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::3 key 1
ip link set dev if_1_3_1 up
sysctl -w net.mpls.conf.if_1_3_1.input=1
ip -4 addr add 169.254.0.4/31 dev if_1_3_1 scope link
```
- Install MPLS encap rules on node-1 towards node-2
```
ip route add 192.168.0.11/32 nexthop encap mpls 32/64 \
via inet 169.254.0.3 dev if_1_2_1
```
- Install MPLS forwarding rules on node-2 and node-3
```
// node2
ip -f mpls route add 32 via inet 169.254.0.7 dev if_2_4_1
// node3
ip -f mpls route add 64 via inet 169.254.0.12 dev if_4_3_1
```
- Ping 192.168.0.11 (node4) from 192.168.0.1 (node1) (where routing
towards 192.168.0.1 is via IP route directly towards node1 from node4)
```
ping 192.168.0.11
```
- tcpdump on interface to capture ping packets wrapped within MPLS
header which inturn wrapped within IP6GRE header
```
16:43:41.121073 IP6
2401:db00:21:6048:feed::1 > 2401:db00:21:6048:feed::2:
DSTOPT GREv0, key=0x1, length 100:
MPLS (label 32, exp 0, ttl 255) (label 64, exp 0, [S], ttl 255)
IP 192.168.0.1 > 192.168.0.11:
ICMP echo request, id 1208, seq 45, length 64
0x0000: 6000 2cdb 006c 3c3f 2401 db00 0021 6048 `.,..l<?$....!`H
0x0010: feed 0000 0000 0001 2401 db00 0021 6048 ........$....!`H
0x0020: feed 0000 0000 0002 2f00 0401 0401 0100 ......../.......
0x0030: 2000 8847 0000 0001 0002 00ff 0004 01ff ...G............
0x0040: 4500 0054 3280 4000 ff01 c7cb c0a8 0001 E..T2.@.........
0x0050: c0a8 000b 0800 a8d7 04b8 002d 2d3c a05b ...........--<.[
0x0060: 0000 0000 bcd8 0100 0000 0000 1011 1213 ................
0x0070: 1415 1617 1819 1a1b 1c1d 1e1f 2021 2223 .............!"#
0x0080: 2425 2627 2829 2a2b 2c2d 2e2f 3031 3233 $%&'()*+,-./0123
0x0090: 3435 3637 4567
```
Signed-off-by: Saif Hasan <has@fb.com>
---
net/mpls/af_mpls.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7a4de6d618b1..aeb5bf2f7595 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1533,10 +1533,11 @@ static int mpls_dev_notify(struct notifier_block *this, unsigned long event,
unsigned int flags;
if (event == NETDEV_REGISTER) {
- /* For now just support Ethernet, IPGRE, SIT and IPIP devices */
+ /* For now just support Ethernet, IPGRE, IP6GRE, SIT and IPIP devices */
if (dev->type == ARPHRD_ETHER ||
dev->type == ARPHRD_LOOPBACK ||
dev->type == ARPHRD_IPGRE ||
+ dev->type == ARPHRD_IP6GRE ||
dev->type == ARPHRD_SIT ||
dev->type == ARPHRD_TUNNEL) {
mdev = mpls_add_dev(dev);
^ permalink raw reply related
* [PATCH] mpls: allow routes on ip6gre devices
From: Saif Hasan @ 2018-09-19 19:17 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller, Calvin Owens, Saif Hasan, Saif Hasan
Summary:
This appears to be necessary and sufficient change to enable `MPLS` on
`ip6gre` tunnels (RFC4023).
This diff allows IP6GRE devices to be recognized by MPLS kernel module
and hence user can configure interface to accept packets with mpls
headers as well setup mpls routes on them.
Test Plan:
Test plan consists of multiple containers connected via GRE-V6 tunnel.
Then carrying out testing steps as below.
- Carry out necessary sysctl settings on all containers
```
sysctl -w net.mpls.platform_labels=65536
sysctl -w net.mpls.ip_ttl_propagate=1
sysctl -w net.mpls.conf.lo.input=1
```
- Establish IP6GRE tunnels
```
ip -6 tunnel add name if_1_2_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::2 key 1
ip link set dev if_1_2_1 up
sysctl -w net.mpls.conf.if_1_2_1.input=1
ip -4 addr add 169.254.0.2/31 dev if_1_2_1 scope link
ip -6 tunnel add name if_1_3_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::3 key 1
ip link set dev if_1_3_1 up
sysctl -w net.mpls.conf.if_1_3_1.input=1
ip -4 addr add 169.254.0.4/31 dev if_1_3_1 scope link
```
- Install MPLS encap rules on node-1 towards node-2
```
ip route add 192.168.0.11/32 nexthop encap mpls 32/64 \
via inet 169.254.0.3 dev if_1_2_1
```
- Install MPLS forwarding rules on node-2 and node-3
```
// node2
ip -f mpls route add 32 via inet 169.254.0.7 dev if_2_4_1
// node3
ip -f mpls route add 64 via inet 169.254.0.12 dev if_4_3_1
```
- Ping 192.168.0.11 (node4) from 192.168.0.1 (node1) (where routing
towards 192.168.0.1 is via IP route directly towards node1 from node4)
```
ping 192.168.0.11
```
- tcpdump on interface to capture ping packets wrapped within MPLS
header which inturn wrapped within IP6GRE header
```
16:43:41.121073 IP6
2401:db00:21:6048:feed::1 > 2401:db00:21:6048:feed::2:
DSTOPT GREv0, key=0x1, length 100:
MPLS (label 32, exp 0, ttl 255) (label 64, exp 0, [S], ttl 255)
IP 192.168.0.1 > 192.168.0.11:
ICMP echo request, id 1208, seq 45, length 64
0x0000: 6000 2cdb 006c 3c3f 2401 db00 0021 6048 `.,..l<?$....!`H
0x0010: feed 0000 0000 0001 2401 db00 0021 6048 ........$....!`H
0x0020: feed 0000 0000 0002 2f00 0401 0401 0100 ......../.......
0x0030: 2000 8847 0000 0001 0002 00ff 0004 01ff ...G............
0x0040: 4500 0054 3280 4000 ff01 c7cb c0a8 0001 E..T2.@.........
0x0050: c0a8 000b 0800 a8d7 04b8 002d 2d3c a05b ...........--<.[
0x0060: 0000 0000 bcd8 0100 0000 0000 1011 1213 ................
0x0070: 1415 1617 1819 1a1b 1c1d 1e1f 2021 2223 .............!"#
0x0080: 2425 2627 2829 2a2b 2c2d 2e2f 3031 3233 $%&'()*+,-./0123
0x0090: 3435 3637 4567
```
Signed-off-by: Saif Hasan <has@fb.com>
---
net/mpls/af_mpls.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7a4de6d618b1..aeb5bf2f7595 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1533,10 +1533,11 @@ static int mpls_dev_notify(struct notifier_block *this, unsigned long event,
unsigned int flags;
if (event == NETDEV_REGISTER) {
- /* For now just support Ethernet, IPGRE, SIT and IPIP devices */
+ /* For now just support Ethernet, IPGRE, IP6GRE, SIT and IPIP devices */
if (dev->type == ARPHRD_ETHER ||
dev->type == ARPHRD_LOOPBACK ||
dev->type == ARPHRD_IPGRE ||
+ dev->type == ARPHRD_IP6GRE ||
dev->type == ARPHRD_SIT ||
dev->type == ARPHRD_TUNNEL) {
mdev = mpls_add_dev(dev);
^ permalink raw reply related
* Re: [RFC 4/5] netlink: prepare validate extack setting for recursion
From: Johannes Berg @ 2018-09-19 19:19 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: netdev
In-Reply-To: <20180919184652.GL4590@localhost.localdomain>
On Wed, 2018-09-19 at 15:46 -0300, Marcelo Ricardo Leitner wrote:
> > 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.
True.
I'm not really sure I'd go as far as calling it an issue though.
IMHO, we really get into this situation if the code is badly structured
to start with. Taking the above code, if you write it as
err = nla_parse(...);
if (err)
return err;
/* do something */
NLA_SET_ERR_MSG(extack, "warning: deprecated command");
return 0;
instead, then you have no such problems.
Well, perhaps you do, if "/* do something */" might *also* set the
message, but basically you have to statically decide which one wins by
ordering the code correctly.
I'm still not convinced, btw, that we actually have any instance in the
kernel today of the issue we've been discussing - namely that some code
does something like the original quoted code at the top of the email.
> > 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.
I'm not really sure what for though :-)
FWIW, if you do think that there's a need for distinguishing this, then
I'd argue that perhaps the right way to address this would be to extend
this all the way to userspace and have two separate attributes for
errors and warnings in the extended ACK message?
> > 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.
Yes, arguably, if you really had an "error" then you'd probably have
never gotten to the parsing. It's possible - although sort of stupid -
to write this code too though:
NL_SET_ERR_MSG(extack, "error: unsupported command");
err = nla_parse(...);
return err ? : -EOPNOTSUPP;
Which one should win in that case?
Again, in my opinion we've got enough flexibility if we let
nla_parse/nla_validate behave as today (mostly, nla_validate doesn't set
a message and I'd like to change that, it does set the error attribute
pointer/offset) and let the calling code sort it out.
In many cases nla_parse() will be called by generic code (e.g.
genetlink) and you'd never get into this situation. Once you're past
that, you can do whatever you like.
> While for the situation you are describing here, it will set a generic
> error message in case the inner code didn't do it.
Yes, but that's not a change in semantics as far as the caller of
nla_parse/nla_validate is concerned - it'll continue to unconditionally
set a message if an error occurred, only the internal behaviour as to
which message seems more relevant is at issue, and the whole recursion
thing and avoiding an outer one overwriting an inner one is IMHO more
useful because that's the more specific error.
> 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'm still not really sure what the use case for a warning is, so not
sure I can really comment on this.
> 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.
Yes, I admit that's the same, just not part of pure policy checking, but
open-coded (in a way).
> Currently it (tc_new_tfilter) doesn't set any default error message,
> so this issue is/was not noticed.
True.
Except that I'd still say that tc_new_tfilter() can very well assume
that nothing has set a message before it ran, it's invoked directly by
the core netlink code after all.
So IMHO we don't have an issue here because there aren't arbitrary
callers of this and it can't know what the state is; it does in fact
know very well what the state is when it's called.
With nla_validate/nla_parse that have far more callers we can't be as
certain, although again - I doubt we actually have places today that
would run into this situation (given how all this stuff is still fairly
new).
> > 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.
Ok, I guess that's fair. I don't think this actually changes the
"trickiness" of extack handling though; it's purely an internal detail
of nla_validate/nla_parse due to me wanting to add recursion; from an
external POV nothing changed.
johannes
^ permalink raw reply
* [PATCH v2 0/5] netlink: nested policy validation
From: Johannes Berg @ 2018-09-19 19:49 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: David Ahern
Ok, I should've tried the idea that David came up with first - it does
in fact make the code quite a bit simpler, and indeed removes the need
for the previously introduced "**error_msg" argument that I hadn't
really liked anyway.
So, changes here are:
* move setting the bad attr pointer/message into validate_nla()
* remove the recursion patch since that's no longer needed
* simply skip the generic bad attr pointer/message setting in
case of nested nla_validate() failing since that could fail
only due to validate_nla() failing inside, which already sets
the extack information
johannes
^ permalink raw reply
* [PATCH v2 3/5] netlink: move extack setting into validate_nla()
From: Johannes Berg @ 2018-09-19 19:49 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: David Ahern, Johannes Berg
In-Reply-To: <20180919194905.16462-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes.berg@intel.com>
This unifies the code between nla_parse() which sets the bad
attribute pointer and an error message, and nla_validate()
which only sets the bad attribute pointer.
It also cleans up the code for NLA_REJECT and paves the way
for nested policy validation, as it will allow us to easily
skip setting the "generic" message without any extra args
like the **error_msg now, just passing the extack through is
now enough.
While at it, remove the unnecessary label in nla_parse().
Suggested-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
lib/nlattr.c | 63 ++++++++++++++++++++++++++++++------------------------------
1 file changed, 32 insertions(+), 31 deletions(-)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index e2e5b38394d5..ff6d48d7c19a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -69,10 +69,11 @@ 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)
{
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
+ int err = -ERANGE;
if (type <= 0 || type > maxtype)
return 0;
@@ -90,24 +91,28 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
switch (pt->type) {
case NLA_EXACT_LEN:
if (attrlen != pt->len)
- return -ERANGE;
+ goto out_err;
break;
case NLA_REJECT:
- if (pt->validation_data && error_msg)
- *error_msg = pt->validation_data;
+ NL_SET_BAD_ATTR(extack, nla);
+ if (extack && pt->validation_data)
+ extack->_msg = pt->validation_data;
return -EINVAL;
case NLA_FLAG:
if (attrlen > 0)
- return -ERANGE;
+ goto out_err;
break;
case NLA_BITFIELD32:
if (attrlen != sizeof(struct nla_bitfield32))
- return -ERANGE;
+ goto out_err;
- return validate_nla_bitfield32(nla, pt->validation_data);
+ err = validate_nla_bitfield32(nla, pt->validation_data);
+ if (err)
+ goto out_err;
+ break;
case NLA_NUL_STRING:
if (pt->len)
@@ -115,13 +120,15 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
else
minlen = attrlen;
- if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL)
- return -EINVAL;
+ if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL) {
+ err = -EINVAL;
+ goto out_err;
+ }
/* fall through */
case NLA_STRING:
if (attrlen < 1)
- return -ERANGE;
+ goto out_err;
if (pt->len) {
char *buf = nla_data(nla);
@@ -130,13 +137,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
attrlen--;
if (attrlen > pt->len)
- return -ERANGE;
+ goto out_err;
}
break;
case NLA_BINARY:
if (pt->len && attrlen > pt->len)
- return -ERANGE;
+ goto out_err;
break;
case NLA_NESTED:
@@ -152,10 +159,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
minlen = nla_attr_minlen[pt->type];
if (attrlen < minlen)
- return -ERANGE;
+ goto out_err;
}
return 0;
+out_err:
+ NL_SET_ERR_MSG_ATTR(extack, nla, "Attribute failed policy validation");
+ return err;
}
/**
@@ -180,12 +190,10 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
int rem;
nla_for_each_attr(nla, head, len, rem) {
- int err = validate_nla(nla, maxtype, policy, NULL);
+ int err = validate_nla(nla, maxtype, policy, extack);
- if (err < 0) {
- NL_SET_BAD_ATTR(extack, nla);
+ if (err < 0)
return err;
- }
}
return 0;
@@ -241,7 +249,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
struct netlink_ext_ack *extack)
{
const struct nlattr *nla;
- int rem, err;
+ int rem;
memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
@@ -249,17 +257,12 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
u16 type = nla_type(nla);
if (type > 0 && type <= maxtype) {
- static const char _msg[] = "Attribute failed policy validation";
- const char *msg = _msg;
-
if (policy) {
- err = validate_nla(nla, maxtype, policy, &msg);
- if (err < 0) {
- NL_SET_BAD_ATTR(extack, nla);
- if (extack)
- extack->_msg = msg;
- goto errout;
- }
+ int err = validate_nla(nla, maxtype, policy,
+ extack);
+
+ if (err < 0)
+ return err;
}
tb[type] = (struct nlattr *)nla;
@@ -270,9 +273,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n",
rem, current->comm);
- err = 0;
-errout:
- return err;
+ return 0;
}
EXPORT_SYMBOL(nla_parse);
--
2.14.4
^ permalink raw reply related
* [PATCH v2 5/5] netlink: add nested array policy validation
From: Johannes Berg @ 2018-09-19 19:49 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: David Ahern, Johannes Berg
In-Reply-To: <20180919194905.16462-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes.berg@intel.com>
Sometimes nested netlink attributes are just used as arrays, with
the nla_type() of each not being used; we have this in nl80211 and
e.g. NFTA_SET_ELEM_LIST_ELEMENTS.
Add the ability to validate this type of message directly in the
policy, by adding the type NLA_NESTED_ARRAY which does exactly
this: require a first level of nesting but ignore the attribute
type, and then inside each require a second level of nested and
validate those attributes against a given policy (if present).
Note that some nested array types actually require that all of
the entries have the same index, this is possible to express in
a nested policy already, apart from the validation that only the
one allowed type is used.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
include/net/netlink.h | 12 +++++++++++-
lib/nlattr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 91907852da1c..3698ca8ff92c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -172,6 +172,7 @@ enum {
NLA_FLAG,
NLA_MSECS,
NLA_NESTED,
+ NLA_NESTED_ARRAY,
NLA_NUL_STRING,
NLA_BINARY,
NLA_S8,
@@ -200,7 +201,8 @@ enum {
* NLA_NUL_STRING Maximum length of string (excluding NUL)
* NLA_FLAG Unused
* NLA_BINARY Maximum length of attribute payload
- * NLA_NESTED Length verification is done by checking len of
+ * NLA_NESTED,
+ * NLA_NESTED_ARRAY Length verification is done by checking len of
* nested header (or empty); len field is used if
* validation_data is also used, for the max attr
* number in the nested policy.
@@ -230,6 +232,12 @@ enum {
* `len' to the max attribute number.
* Note that nla_parse() will validate, but of course not
* parse, the nested sub-policies.
+ * NLA_NESTED_ARRAY Points to a nested policy to validate, must also set
+ * `len' to the max attribute number. The difference to
+ * NLA_NESTED is the structure - NLA_NESTED has the
+ * nested attributes directly inside, while an array has
+ * the nested attributes at another level down and the
+ * attributes directly in the nesting don't matter.
* All other Unused
*
* Example:
@@ -255,6 +263,8 @@ struct nla_policy {
#define NLA_POLICY_NESTED(maxattr, policy) \
{ .type = NLA_NESTED, .validation_data = policy, .len = maxattr }
+#define NLA_POLICY_NESTED_ARRAY(maxattr, policy) \
+ { .type = NLA_NESTED_ARRAY, .validation_data = policy, .len = maxattr }
/**
* struct nl_info - netlink source information
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 4cbfc64f6d10..9d8e1dc71680 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -67,6 +67,34 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
return 0;
}
+static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ const struct nlattr *entry;
+ int rem;
+
+ nla_for_each_attr(entry, head, len, rem) {
+ int ret;
+
+ if (nla_len(entry) == 0)
+ continue;
+
+ if (nla_len(entry) < NLA_HDRLEN) {
+ NL_SET_ERR_MSG_ATTR(extack, entry,
+ "Array element too short");
+ return -ERANGE;
+ }
+
+ ret = nla_validate(nla_data(entry), nla_len(entry),
+ maxtype, policy, extack);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy,
struct netlink_ext_ack *extack)
@@ -166,6 +194,29 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
}
}
break;
+ case NLA_NESTED_ARRAY:
+ /* a nested array attribute 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)
+ goto out_err;
+ if (pt->validation_data) {
+ int err;
+
+ err = nla_validate_array(nla_data(nla), nla_len(nla),
+ pt->len, pt->validation_data,
+ extack);
+ if (err < 0) {
+ /*
+ * return directly to preserve the inner
+ * error message/attribute pointer
+ */
+ return err;
+ }
+ }
+ break;
default:
if (pt->len)
minlen = pt->len;
--
2.14.4
^ permalink raw reply related
* [PATCH v2 4/5] netlink: allow NLA_NESTED to specify nested policy to validate
From: Johannes Berg @ 2018-09-19 19:49 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: David Ahern, Johannes Berg
In-Reply-To: <20180919194905.16462-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes.berg@intel.com>
Now that we have a validation_data pointer, and the len field in
the policy is unused for NLA_NESTED, we can allow using them both
to have nested validation. This can be nice in code, although we
still have to use nla_parse_nested() or similar which would also
take a policy; however, it also serves as documentation in the
policy without requiring a look at the code.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
include/net/netlink.h | 13 +++++++++++--
lib/nlattr.c | 14 ++++++++++++++
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0d698215d4d9..91907852da1c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -200,8 +200,10 @@ enum {
* NLA_NUL_STRING Maximum length of string (excluding NUL)
* NLA_FLAG Unused
* NLA_BINARY Maximum length of attribute payload
- * NLA_NESTED Don't use `len' field -- length verification is
- * done by checking len of nested header (or empty)
+ * NLA_NESTED Length verification is done by checking len of
+ * nested header (or empty); len field is used if
+ * validation_data is also used, for the max attr
+ * number in the nested policy.
* NLA_U8, NLA_U16,
* NLA_U32, NLA_U64,
* NLA_S8, NLA_S16,
@@ -224,6 +226,10 @@ enum {
* NLA_REJECT This attribute is always rejected and validation data
* may point to a string to report as the error instead
* of the generic one in extended ACK.
+ * NLA_NESTED Points to a nested policy to validate, must also set
+ * `len' to the max attribute number.
+ * Note that nla_parse() will validate, but of course not
+ * parse, the nested sub-policies.
* All other Unused
*
* Example:
@@ -247,6 +253,9 @@ struct nla_policy {
#define NLA_POLICY_ETH_ADDR NLA_POLICY_EXACT_LEN(ETH_ALEN)
#define NLA_POLICY_ETH_ADDR_COMPAT NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN)
+#define NLA_POLICY_NESTED(maxattr, policy) \
+ { .type = NLA_NESTED, .validation_data = policy, .len = maxattr }
+
/**
* struct nl_info - netlink source information
* @nlh: Netlink message header of original request
diff --git a/lib/nlattr.c b/lib/nlattr.c
index ff6d48d7c19a..4cbfc64f6d10 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -152,6 +152,20 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
*/
if (attrlen == 0)
break;
+ if (attrlen < NLA_HDRLEN)
+ goto out_err;
+ if (pt->validation_data) {
+ err = nla_validate(nla_data(nla), nla_len(nla), pt->len,
+ pt->validation_data, extack);
+ if (err < 0) {
+ /*
+ * return directly to preserve the inner
+ * error message/attribute pointer
+ */
+ return err;
+ }
+ }
+ break;
default:
if (pt->len)
minlen = pt->len;
--
2.14.4
^ permalink raw reply related
* Re: [PATCH v2 3/5] netlink: move extack setting into validate_nla()
From: Johannes Berg @ 2018-09-19 19:52 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: David Ahern
In-Reply-To: <20180919194905.16462-4-johannes@sipsolutions.net>
On Wed, 2018-09-19 at 21:49 +0200, Johannes Berg wrote:
>
> case NLA_REJECT:
> - if (pt->validation_data && error_msg)
> - *error_msg = pt->validation_data;
> + NL_SET_BAD_ATTR(extack, nla);
> + if (extack && pt->validation_data)
> + extack->_msg = pt->validation_data;
> return -EINVAL;
Damn. This of course needs to happen only if pt->validation_data is set,
otherwise it needs to "goto out_err" to set the default message.
I'll respin another day with that fixed, and any other comments that
come in until then addressed.
Sorry for the noise :-(
johannes
^ permalink raw reply
* (unknown),
From: Saif Hasan @ 2018-09-19 19:57 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Saif Hasan, Calvin Owens
>From e4f144286efe0f298c11efe58e17b1ab91c7ee3f Mon Sep 17 00:00:00 2001
From: Saif Hasan <has@fb.com>
Date: Mon, 17 Sep 2018 16:28:54 -0700
Subject: [PATCH] mpls: allow routes on ip6gre devices
Summary:
This appears to be necessary and sufficient change to enable `MPLS` on
`ip6gre` tunnels (RFC4023).
This diff allows IP6GRE devices to be recognized by MPLS kernel module
and hence user can configure interface to accept packets with mpls
headers as well setup mpls routes on them.
Test Plan:
Test plan consists of multiple containers connected via GRE-V6 tunnel.
Then carrying out testing steps as below.
- Carry out necessary sysctl settings on all containers
```
sysctl -w net.mpls.platform_labels=65536
sysctl -w net.mpls.ip_ttl_propagate=1
sysctl -w net.mpls.conf.lo.input=1
```
- Establish IP6GRE tunnels
```
ip -6 tunnel add name if_1_2_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::2 key 1
ip link set dev if_1_2_1 up
sysctl -w net.mpls.conf.if_1_2_1.input=1
ip -4 addr add 169.254.0.2/31 dev if_1_2_1 scope link
ip -6 tunnel add name if_1_3_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::3 key 1
ip link set dev if_1_3_1 up
sysctl -w net.mpls.conf.if_1_3_1.input=1
ip -4 addr add 169.254.0.4/31 dev if_1_3_1 scope link
```
- Install MPLS encap rules on node-1 towards node-2
```
ip route add 192.168.0.11/32 nexthop encap mpls 32/64 \
via inet 169.254.0.3 dev if_1_2_1
```
- Install MPLS forwarding rules on node-2 and node-3
```
// node2
ip -f mpls route add 32 via inet 169.254.0.7 dev if_2_4_1
// node3
ip -f mpls route add 64 via inet 169.254.0.12 dev if_4_3_1
```
- Ping 192.168.0.11 (node4) from 192.168.0.1 (node1) (where routing
towards 192.168.0.1 is via IP route directly towards node1 from node4)
```
ping 192.168.0.11
```
- tcpdump on interface to capture ping packets wrapped within MPLS
header which inturn wrapped within IP6GRE header
```
16:43:41.121073 IP6
2401:db00:21:6048:feed::1 > 2401:db00:21:6048:feed::2:
DSTOPT GREv0, key=0x1, length 100:
MPLS (label 32, exp 0, ttl 255) (label 64, exp 0, [S], ttl 255)
IP 192.168.0.1 > 192.168.0.11:
ICMP echo request, id 1208, seq 45, length 64
0x0000: 6000 2cdb 006c 3c3f 2401 db00 0021 6048 `.,..l<?$....!`H
0x0010: feed 0000 0000 0001 2401 db00 0021 6048 ........$....!`H
0x0020: feed 0000 0000 0002 2f00 0401 0401 0100 ......../.......
0x0030: 2000 8847 0000 0001 0002 00ff 0004 01ff ...G............
0x0040: 4500 0054 3280 4000 ff01 c7cb c0a8 0001 E..T2.@.........
0x0050: c0a8 000b 0800 a8d7 04b8 002d 2d3c a05b ...........--<.[
0x0060: 0000 0000 bcd8 0100 0000 0000 1011 1213 ................
0x0070: 1415 1617 1819 1a1b 1c1d 1e1f 2021 2223 .............!"#
0x0080: 2425 2627 2829 2a2b 2c2d 2e2f 3031 3233 $%&'()*+,-./0123
0x0090: 3435 3637 4567
```
Signed-off-by: Saif Hasan <has@fb.com>
---
net/mpls/af_mpls.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7a4de6d618b1..aeb5bf2f7595 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1533,10 +1533,11 @@ static int mpls_dev_notify(struct
notifier_block *this, unsigned long event,
unsigned int flags;
if (event == NETDEV_REGISTER) {
- /* For now just support Ethernet, IPGRE, SIT and IPIP devices */
+ /* For now just support Ethernet, IPGRE, IP6GRE, SIT and IPIP devices */
if (dev->type == ARPHRD_ETHER ||
dev->type == ARPHRD_LOOPBACK ||
dev->type == ARPHRD_IPGRE ||
+ dev->type == ARPHRD_IP6GRE ||
dev->type == ARPHRD_SIT ||
dev->type == ARPHRD_TUNNEL) {
mdev = mpls_add_dev(dev);
^ permalink raw reply related
* [PATCH net] mpls: allow routes on ip6gre devices
From: Saif Hasan @ 2018-09-19 19:59 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Calvin Owens, Saif Hasan
>From e4f144286efe0f298c11efe58e17b1ab91c7ee3f Mon Sep 17 00:00:00 2001
From: Saif Hasan <has@fb.com>
Date: Mon, 17 Sep 2018 16:28:54 -0700
Subject: [PATCH] mpls: allow routes on ip6gre devices
Summary:
This appears to be necessary and sufficient change to enable `MPLS` on
`ip6gre` tunnels (RFC4023).
This diff allows IP6GRE devices to be recognized by MPLS kernel module
and hence user can configure interface to accept packets with mpls
headers as well setup mpls routes on them.
Test Plan:
Test plan consists of multiple containers connected via GRE-V6 tunnel.
Then carrying out testing steps as below.
- Carry out necessary sysctl settings on all containers
```
sysctl -w net.mpls.platform_labels=65536
sysctl -w net.mpls.ip_ttl_propagate=1
sysctl -w net.mpls.conf.lo.input=1
```
- Establish IP6GRE tunnels
```
ip -6 tunnel add name if_1_2_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::2 key 1
ip link set dev if_1_2_1 up
sysctl -w net.mpls.conf.if_1_2_1.input=1
ip -4 addr add 169.254.0.2/31 dev if_1_2_1 scope link
ip -6 tunnel add name if_1_3_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::3 key 1
ip link set dev if_1_3_1 up
sysctl -w net.mpls.conf.if_1_3_1.input=1
ip -4 addr add 169.254.0.4/31 dev if_1_3_1 scope link
```
- Install MPLS encap rules on node-1 towards node-2
```
ip route add 192.168.0.11/32 nexthop encap mpls 32/64 \
via inet 169.254.0.3 dev if_1_2_1
```
- Install MPLS forwarding rules on node-2 and node-3
```
// node2
ip -f mpls route add 32 via inet 169.254.0.7 dev if_2_4_1
// node3
ip -f mpls route add 64 via inet 169.254.0.12 dev if_4_3_1
```
- Ping 192.168.0.11 (node4) from 192.168.0.1 (node1) (where routing
towards 192.168.0.1 is via IP route directly towards node1 from node4)
```
ping 192.168.0.11
```
- tcpdump on interface to capture ping packets wrapped within MPLS
header which inturn wrapped within IP6GRE header
```
16:43:41.121073 IP6
2401:db00:21:6048:feed::1 > 2401:db00:21:6048:feed::2:
DSTOPT GREv0, key=0x1, length 100:
MPLS (label 32, exp 0, ttl 255) (label 64, exp 0, [S], ttl 255)
IP 192.168.0.1 > 192.168.0.11:
ICMP echo request, id 1208, seq 45, length 64
0x0000: 6000 2cdb 006c 3c3f 2401 db00 0021 6048 `.,..l<?$....!`H
0x0010: feed 0000 0000 0001 2401 db00 0021 6048 ........$....!`H
0x0020: feed 0000 0000 0002 2f00 0401 0401 0100 ......../.......
0x0030: 2000 8847 0000 0001 0002 00ff 0004 01ff ...G............
0x0040: 4500 0054 3280 4000 ff01 c7cb c0a8 0001 E..T2.@.........
0x0050: c0a8 000b 0800 a8d7 04b8 002d 2d3c a05b ...........--<.[
0x0060: 0000 0000 bcd8 0100 0000 0000 1011 1213 ................
0x0070: 1415 1617 1819 1a1b 1c1d 1e1f 2021 2223 .............!"#
0x0080: 2425 2627 2829 2a2b 2c2d 2e2f 3031 3233 $%&'()*+,-./0123
0x0090: 3435 3637 4567
```
Signed-off-by: Saif Hasan <has@fb.com>
---
net/mpls/af_mpls.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7a4de6d618b1..aeb5bf2f7595 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1533,10 +1533,11 @@ static int mpls_dev_notify(struct
notifier_block *this, unsigned long event,
unsigned int flags;
if (event == NETDEV_REGISTER) {
- /* For now just support Ethernet, IPGRE, SIT and IPIP devices */
+ /* For now just support Ethernet, IPGRE, IP6GRE, SIT and IPIP devices */
if (dev->type == ARPHRD_ETHER ||
dev->type == ARPHRD_LOOPBACK ||
dev->type == ARPHRD_IPGRE ||
+ dev->type == ARPHRD_IP6GRE ||
dev->type == ARPHRD_SIT ||
dev->type == ARPHRD_TUNNEL) {
mdev = mpls_add_dev(dev);
^ permalink raw reply related
* [PATCH net-next 1/2] r8169: simplify RTL8169 PHY initialization
From: Heiner Kallweit @ 2018-09-19 20:00 UTC (permalink / raw)
To: David Miller, Realtek linux nic maintainers; +Cc: netdev@vger.kernel.org
PCI_LATENCY_TIMER is ignored on PCIe, therefore we have to do this
for the PCI chips (version <= 06) only. Also we can move setting
PCI_CACHE_LINE_SIZE.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 7f4647c58..a27bf5807 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4046,16 +4046,13 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
rtl_hw_phy_config(dev);
if (tp->mac_version <= RTL_GIGA_MAC_VER_06) {
+ pci_write_config_byte(tp->pci_dev, PCI_LATENCY_TIMER, 0x40);
+ pci_write_config_byte(tp->pci_dev, PCI_CACHE_LINE_SIZE, 0x08);
netif_dbg(tp, drv, dev,
"Set MAC Reg C+CR Offset 0x82h = 0x01h\n");
RTL_W8(tp, 0x82, 0x01);
}
- pci_write_config_byte(tp->pci_dev, PCI_LATENCY_TIMER, 0x40);
-
- if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
- pci_write_config_byte(tp->pci_dev, PCI_CACHE_LINE_SIZE, 0x08);
-
if (tp->mac_version == RTL_GIGA_MAC_VER_02) {
netif_dbg(tp, drv, dev,
"Set MAC Reg C+CR Offset 0x82h = 0x01h\n");
--
2.19.0
^ permalink raw reply related
* [PATCH net-next 2/2] r8169: remove duplicated RTL8169s PHY initialization steps
From: Heiner Kallweit @ 2018-09-19 20:02 UTC (permalink / raw)
To: David Miller, Realtek linux nic maintainers; +Cc: netdev@vger.kernel.org
In-Reply-To: <35548e5c-2421-04b6-b6d7-335c1b7fb62d@gmail.com>
Setting register 0x82 to value 01 is done a few lines before for all
chip versions <= 06 anyway. And setting PHY register 0x0b to value 00
is done at the end of rtl8169s_hw_phy_config() already. So we can
remove this.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index a27bf5807..e2713867c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4053,15 +4053,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
RTL_W8(tp, 0x82, 0x01);
}
- if (tp->mac_version == RTL_GIGA_MAC_VER_02) {
- netif_dbg(tp, drv, dev,
- "Set MAC Reg C+CR Offset 0x82h = 0x01h\n");
- RTL_W8(tp, 0x82, 0x01);
- netif_dbg(tp, drv, dev,
- "Set PHY Reg 0x0bh = 0x00h\n");
- rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
- }
-
/* We may have called phy_speed_down before */
phy_speed_up(dev->phydev);
--
2.19.0
^ permalink raw reply related
* [PATCH net] xfrm: validate template mode
From: Sean Tranchetti @ 2018-09-19 19:54 UTC (permalink / raw)
To: netdev, steffen.klassert; +Cc: Sean Tranchetti
XFRM mode parameters passed as part of the user templates
in the IP_XFRM_POLICY are never properly validated. Passing
values other than valid XFRM modes can cause stack-out-of-bounds
reads to occur later in the XFRM processing:
[ 140.535608] ================================================================
[ 140.543058] BUG: KASAN: stack-out-of-bounds in xfrm_state_find+0x17e4/0x1cc4
[ 140.550306] Read of size 4 at addr ffffffc0238a7a58 by task repro/5148
[ 140.557369]
[ 140.558927] Call trace:
[ 140.558936] dump_backtrace+0x0/0x388
[ 140.558940] show_stack+0x24/0x30
[ 140.558946] __dump_stack+0x24/0x2c
[ 140.558949] dump_stack+0x8c/0xd0
[ 140.558956] print_address_description+0x74/0x234
[ 140.558960] kasan_report+0x240/0x264
[ 140.558963] __asan_report_load4_noabort+0x2c/0x38
[ 140.558967] xfrm_state_find+0x17e4/0x1cc4
[ 140.558971] xfrm_resolve_and_create_bundle+0x40c/0x1fb8
[ 140.558975] xfrm_lookup+0x238/0x1444
[ 140.558977] xfrm_lookup_route+0x48/0x11c
[ 140.558984] ip_route_output_flow+0x88/0xc4
[ 140.558991] raw_sendmsg+0xa74/0x266c
[ 140.558996] inet_sendmsg+0x258/0x3b0
[ 140.559002] sock_sendmsg+0xbc/0xec
[ 140.559005] SyS_sendto+0x3a8/0x5a8
[ 140.559008] el0_svc_naked+0x34/0x38
[ 140.559009]
[ 140.592245] page dumped because: kasan: bad access detected
[ 140.597981] page_owner info is not active (free page?)
[ 140.603267]
[ 140.653503] ================================================================
Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
---
net/xfrm/xfrm_user.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 4791aa8..ab2f280 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1480,6 +1480,9 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
(ut[i].family != prev_family))
return -EINVAL;
+ if (ut[i].mode >= XFRM_MODE_MAX)
+ return -EINVAL;
+
prev_family = ut[i].family;
switch (ut[i].family) {
--
1.9.1
^ permalink raw reply related
* [PATCH] net: toshiba: remove a redundant local variable 'index_specified'
From: zhong jiang @ 2018-09-20 1:56 UTC (permalink / raw)
To: davem; +Cc: benh, paulus, mpe, netdev, linux-kernel
The local variable 'index_specified' is never used after being assigned.
hence it should be redundant adn can be removed.
Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
drivers/net/ethernet/toshiba/ps3_gelic_wireless.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
index 302079e..00ab417 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
@@ -1094,7 +1094,7 @@ static int gelic_wl_get_encode(struct net_device *netdev,
struct gelic_wl_info *wl = port_wl(netdev_priv(netdev));
struct iw_point *enc = &data->encoding;
unsigned long irqflag;
- unsigned int key_index, index_specified;
+ unsigned int key_index;
int ret = 0;
pr_debug("%s: <-\n", __func__);
@@ -1105,13 +1105,10 @@ static int gelic_wl_get_encode(struct net_device *netdev,
return -EINVAL;
spin_lock_irqsave(&wl->lock, irqflag);
- if (key_index) {
- index_specified = 1;
+ if (key_index)
key_index--;
- } else {
- index_specified = 0;
+ else
key_index = wl->current_key;
- }
if (wl->group_cipher_method == GELIC_WL_CIPHER_WEP) {
switch (wl->auth_method) {
--
1.7.12.4
^ permalink raw reply related
* [PATCH net-next] e1000: remove set but not used variable 'txb2b'
From: YueHaibing @ 2018-09-20 2:04 UTC (permalink / raw)
To: Jeff Kirsher, David S. Miller
Cc: YueHaibing, intel-wired-lan, netdev, kernel-janitors,
linux-kernel
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/net/ethernet/intel/e1000/e1000_main.c: In function 'e1000_watchdog':
drivers/net/ethernet/intel/e1000/e1000_main.c:2436:9: warning:
variable 'txb2b' set but not used [-Wunused-but-set-variable]
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 7e0f1f9..43b6d3c 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -2433,7 +2433,6 @@ static void e1000_watchdog(struct work_struct *work)
if (link) {
if (!netif_carrier_ok(netdev)) {
u32 ctrl;
- bool txb2b = true;
/* update snapshot of PHY registers on LSC */
e1000_get_speed_and_duplex(hw,
&adapter->link_speed,
@@ -2455,11 +2454,9 @@ static void e1000_watchdog(struct work_struct *work)
adapter->tx_timeout_factor = 1;
switch (adapter->link_speed) {
case SPEED_10:
- txb2b = false;
adapter->tx_timeout_factor = 16;
break;
case SPEED_100:
- txb2b = false;
/* maybe add some timeout factor ? */
break;
}
^ permalink raw reply related
* Re: Bridge connectivity interruptions while devices join or leave the bridge
From: Johannes Wienke @ 2018-09-19 20:57 UTC (permalink / raw)
To: Stephen Hemminger, Ido Schimmel; +Cc: netdev
In-Reply-To: <20180919100358.0ea95c20@xeon-e3>
On 19.09.18 19:03, Stephen Hemminger wrote:
> 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.
Yes sure. It was just unexpected to meet this and we ha no clue that
this could be a reasonable default. I just wonder what the motivation
for hat is and of course some more obvious documentation would have
helped a lot in debugging/avoid this.
Cheers,
Johannes
--
Johannes Wienke, Researcher at CoR-Lab / CITEC, Bielefeld University
Address: Inspiration 1, D-33619 Bielefeld, Germany (Room 1.307)
Phone: +49 521 106-67277
^ permalink raw reply
* Re: [PATCH mlx5-next 03/25] net/mlx5: Set uid as part of RQ commands
From: Jason Gunthorpe @ 2018-09-19 21:10 UTC (permalink / raw)
To: Saeed Mahameed
Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Yishai Hadas, Saeed Mahameed, Linux Netdev List
In-Reply-To: <CALzJLG-YOtkm0HBWBrgPOSYy1bcxvW+D2YorjVHCLMQZRpZUbA@mail.gmail.com>
On Wed, Sep 19, 2018 at 11:40:45AM -0700, Saeed Mahameed wrote:
> 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 :) ..
{0} will throw a 'missing-field-initializers' compiler warning,
however it recognizes '= {}' as an idom meaning 'zero everything' and
does not throw a warning.
Jason
^ permalink raw reply
* Re: [RFC 4/5] netlink: prepare validate extack setting for recursion
From: Marcelo Ricardo Leitner @ 2018-09-19 21:10 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev
In-Reply-To: <1537384771.10305.68.camel@sipsolutions.net>
On Wed, Sep 19, 2018 at 09:19:31PM +0200, Johannes Berg wrote:
> On Wed, 2018-09-19 at 15:46 -0300, Marcelo Ricardo Leitner wrote:
>
> > > 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.
>
> True.
>
> I'm not really sure I'd go as far as calling it an issue though.
Ok, s/issue/matter in question/.
...
> > > 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.
>
> I'm not really sure what for though :-)
Hehe :-)
>
> FWIW, if you do think that there's a need for distinguishing this, then
> I'd argue that perhaps the right way to address this would be to extend
> this all the way to userspace and have two separate attributes for
> errors and warnings in the extended ACK message?
Likely, yes. And perhaps even support multiple messages. That way we
could, for example, parse all attributes and return a list of the all
the offending ones, instead of returning one at a time. Net benefit?
Not clear.. over engineering? Possibly.
I agree with you that in general we should not need this.
> > While for the situation you are describing here, it will set a generic
> > error message in case the inner code didn't do it.
>
> Yes, but that's not a change in semantics as far as the caller of
> nla_parse/nla_validate is concerned - it'll continue to unconditionally
> set a message if an error occurred, only the internal behaviour as to
> which message seems more relevant is at issue, and the whole recursion
> thing and avoiding an outer one overwriting an inner one is IMHO more
> useful because that's the more specific error.
That's fine. My point here was only to clarify the use case, which
would be used in other places too (like in the example below).
>
> > 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'm still not really sure what the use case for a warning is, so not
> sure I can really comment on this.
Good point. From iproute POV, a warning is a non-fatal message. From
an user perspective, that probably translates are nothing because in
the end the command actually worked. :-)
Seriously, I do think it's more of a hint for developers than anyone
else.
>
> > 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.
>
> Yes, I admit that's the same, just not part of pure policy checking, but
> open-coded (in a way).
>
> > Currently it (tc_new_tfilter) doesn't set any default error message,
> > so this issue is/was not noticed.
>
> True.
>
> Except that I'd still say that tc_new_tfilter() can very well assume
> that nothing has set a message before it ran, it's invoked directly by
> the core netlink code after all.
>
> So IMHO we don't have an issue here because there aren't arbitrary
> callers of this and it can't know what the state is; it does in fact
> know very well what the state is when it's called.
Good point.
>
> With nla_validate/nla_parse that have far more callers we can't be as
> certain, although again - I doubt we actually have places today that
> would run into this situation (given how all this stuff is still fairly
> new).
>
> > > 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.
>
> Ok, I guess that's fair. I don't think this actually changes the
> "trickiness" of extack handling though; it's purely an internal detail
> of nla_validate/nla_parse due to me wanting to add recursion; from an
> external POV nothing changed.
>
> johannes
>
Thanks,
Marcelo
^ permalink raw reply
* Re: [Patch net-next] ipv4: initialize ra_mutex in inet_init_net()
From: Cong Wang @ 2018-09-19 21:28 UTC (permalink / raw)
To: Kirill Tkhai; +Cc: Linux Kernel Network Developers
In-Reply-To: <b22a5e5f-7a63-04fd-79ed-9b40e09db3af@virtuozzo.com>
On Wed, Sep 19, 2018 at 1:25 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> On 18.09.2018 23:17, Cong Wang wrote:
> > On Mon, Sep 17, 2018 at 12:25 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> >> In inet_init() the order of registration is:
> >>
> >> ip_mr_init();
> >> init_inet_pernet_ops();
> >>
> >> This means, ipmr_net_ops pernet operations are before af_inet_ops
> >> in pernet_list. So, there is a theoretical probability, sometimes
> >> in the future, we will have a problem during a fail of net initialization.
> >>
> >> Say,
> >>
> >> setup_net():
> >> ipmr_net_ops->init() returns 0
> >> xxx->init() returns error
> >> and then we do:
> >> ipmr_net_ops->exit(),
> >>
> >> which could touch ra_mutex (theoretically).
> >
> > How could ra_mutex be touched in this scenario?
> >
> > ra_mutex is only used in ip_ra_control() which is called
> > only by {get,set}sockopt(). I don't see anything related
> > to netns exit() path here.
>
> Currently, it is not touched. But it's an ordinary practice,
> someone closes sockets in pernet ->exit methods. For example,
> we close percpu icmp sockets in icmp_sk_exit(), which are
> also of RAW type, and there is also called ip_ra_control()
> for them. Yes, they differ by their protocol; icmp sockets
> are of IPPROTO_ICMP protocol, while ip_ra_control() acts
> on IPPROTO_RAW sockets, but it's not good anyway. This does
> not look reliable for the future. In case of someone changes
> something here, we may do not notice this for the long time,
> while some users will meet bugs on their places.
First of all, we only consider current code base. Even if you
really planned to changed this in the future, it would be still your
responsibility to take care of it. Why? Simple, FIFO. My patch
comes ahead of any future changes here, obviously.
Secondly, it is certainly not hard to notice. I am pretty sure
you would get a warning/crash if this bug triggered.
>
> Problems on error paths is not easy to detect on testing,
> while user may meet them. We had issue of same type with
> uninitialized xfrm_policy_lock. It was introduced in 2013,
> while the problem was found only in 2017:
Been there, done that, I've fixed multiple IPv6 init code
error path. This is not where we disagree, by the way.
>
> introduced by 283bc9f35bbb
> fixed by c282222a45cb
>
> (Last week I met it on RH7 kernel, which still has no a fix.
> But this talk is not about distribution kernels, just about
> the way).
>
> I just want to say if someone makes some creativity on top
> of this code, it will be to more friendly from us to him/her
> to not force this person to think about such not obvious details,
> but just to implement nice architecture right now.
You keep saying nice architecture, how did you architect
ipv4.ra_mutex into net/core/net_namespace.c? It is apparently
not nice.
Good luck with your future.
I am tired, let's just drop it. I have no interest to fix your mess.
Thanks!
^ permalink raw reply
* [PATCH net-next] nfp: provide a better warning when ring allocation fails
From: Jakub Kicinski @ 2018-09-19 21:42 UTC (permalink / raw)
To: davem; +Cc: netdev, oss-drivers, Jakub Kicinski
NFP supports fairly enormous ring sizes (up to 256k descriptors).
In commit 466271703867 ("nfp: use kvcalloc() to allocate SW buffer
descriptor arrays") we have started using kvcalloc() functions to
make sure the allocation of software state arrays doesn't hit
the MAX_ORDER limit. Unfortunately, we can't use virtual mappings
for the DMA region holding HW descriptors. In case this allocation
fails instead of the generic (and fairly scary) warning/splat in
the logs print a helpful message explaining what happened and
suggesting how to fix it.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com>
---
.../net/ethernet/netronome/nfp/nfp_net_common.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 1aac55d66404..6dab45c1f21e 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -2180,9 +2180,13 @@ nfp_net_tx_ring_alloc(struct nfp_net_dp *dp, struct nfp_net_tx_ring *tx_ring)
tx_ring->size = array_size(tx_ring->cnt, sizeof(*tx_ring->txds));
tx_ring->txds = dma_zalloc_coherent(dp->dev, tx_ring->size,
- &tx_ring->dma, GFP_KERNEL);
- if (!tx_ring->txds)
+ &tx_ring->dma,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!tx_ring->txds) {
+ netdev_warn(dp->netdev, "failed to allocate TX descriptor ring memory, requested descriptor count: %d, consider lowering descriptor count\n",
+ tx_ring->cnt);
goto err_alloc;
+ }
tx_ring->txbufs = kvcalloc(tx_ring->cnt, sizeof(*tx_ring->txbufs),
GFP_KERNEL);
@@ -2334,9 +2338,13 @@ nfp_net_rx_ring_alloc(struct nfp_net_dp *dp, struct nfp_net_rx_ring *rx_ring)
rx_ring->cnt = dp->rxd_cnt;
rx_ring->size = array_size(rx_ring->cnt, sizeof(*rx_ring->rxds));
rx_ring->rxds = dma_zalloc_coherent(dp->dev, rx_ring->size,
- &rx_ring->dma, GFP_KERNEL);
- if (!rx_ring->rxds)
+ &rx_ring->dma,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!rx_ring->rxds) {
+ netdev_warn(dp->netdev, "failed to allocate RX descriptor ring memory, requested descriptor count: %d, consider lowering descriptor count\n",
+ rx_ring->cnt);
goto err_alloc;
+ }
rx_ring->rxbufs = kvcalloc(rx_ring->cnt, sizeof(*rx_ring->rxbufs),
GFP_KERNEL);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next v2 01/10] net: core: netlink: add helper refcount dec and lock function
From: Cong Wang @ 2018-09-19 21:46 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller, Stephen Hemminger, Kirill Tkhai, Nicolas Dichtel,
Greg KH, mark.rutland, Leon Romanovsky, Paul E. McKenney,
Florian Westphal, David Ahern, christian, lucien xin,
Jakub Kicinski, Jiri Benc
In-Reply-To: <1537168660-24032-2-git-send-email-vladbu@mellanox.com>
On Mon, Sep 17, 2018 at 12:19 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -130,6 +130,12 @@ int rtnl_is_locked(void)
> }
> EXPORT_SYMBOL(rtnl_is_locked);
>
> +bool refcount_dec_and_rtnl_lock(refcount_t *r)
> +{
> + return refcount_dec_and_mutex_lock(r, &rtnl_mutex);
> +}
> +EXPORT_SYMBOL(refcount_dec_and_rtnl_lock);
I think you probably want to #include <linux/refcount.h>
explicitly.
^ permalink raw reply
* Re: [PATCH bpf-next] flow_dissector: fix build failure without CONFIG_NET
From: Daniel Borkmann @ 2018-09-19 21:48 UTC (permalink / raw)
To: Willem de Bruijn, netdev; +Cc: ast, rdunlap, Willem de Bruijn
In-Reply-To: <20180918202018.204099-1-willemdebruijn.kernel@gmail.com>
On 09/18/2018 10:20 PM, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> If boolean CONFIG_BPF_SYSCALL is enabled, kernel/bpf/syscall.c will
> call flow_dissector functions from net/core/flow_dissector.c.
>
> This causes this build failure if CONFIG_NET is disabled:
>
> kernel/bpf/syscall.o: In function `__x64_sys_bpf':
> syscall.c:(.text+0x3278): undefined reference to
> `skb_flow_dissector_bpf_prog_attach'
> syscall.c:(.text+0x3310): undefined reference to
> `skb_flow_dissector_bpf_prog_detach'
> kernel/bpf/syscall.o:(.rodata+0x3f0): undefined reference to
> `flow_dissector_prog_ops'
> kernel/bpf/verifier.o:(.rodata+0x250): undefined reference to
> `flow_dissector_verifier_ops'
>
> Analogous to other optional BPF program types in syscall.c, add stubs
> if the relevant functions are not compiled and move the BPF_PROG_TYPE
> definition in the #ifdef CONFIG_NET block.
>
> Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
Applied to bpf-next, thanks Willem!
^ 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