* Re: [PATCH net] ipv4: route: fix inet_rtm_getroute induced crash
From: Florian Westphal @ 2017-08-14 5:20 UTC (permalink / raw)
To: David Ahern; +Cc: Florian Westphal, netdev, Roopa Prabhu
In-Reply-To: <17031977-3e7a-9a7f-e53c-538cb030e496@gmail.com>
David Ahern <dsahern@gmail.com> wrote:
> On 8/13/17 4:52 PM, Florian Westphal wrote:
> > "ip route get $daddr iif eth0 from $saddr" causes:
> > BUG: KASAN: use-after-free in ip_route_input_rcu+0x1535/0x1b50
> > Call Trace:
> > ip_route_input_rcu+0x1535/0x1b50
> > ip_route_input_noref+0xf9/0x190
> > tcp_v4_early_demux+0x1a4/0x2b0
> > ip_rcv+0xbcb/0xc05
> > __netif_receive_skb+0x9c/0xd0
> > netif_receive_skb_internal+0x5a8/0x890
> >
> > Problem is that inet_rtm_getroute calls either ip_route_input_rcu (if an
> > iif was provided) or ip_route_output_key_hash_rcu.
> >
> > But ip_route_input_rcu, unlike ip_route_output_key_hash_rcu, already
> > associates the dst_entry with the skb. This clears the SKB_DST_NOREF
> > bit (i.e. skb_dst_drop will release/free the entry while it should not).
> >
> > Thus only set the dst if we called ip_route_output_key_hash_rcu().
> >
> > I tested this patch by running:
> > while true;do ip r get 10.0.1.2;done > /dev/null &
> > while true;do ip r get 10.0.1.2 iif eth0 from 10.0.1.1;done > /dev/null &
> > ... and saw no crash or memory leak.
> >
> > Cc: Roopa Prabhu <roopa@cumulusnetworks.com>
> > Cc: David Ahern <dsahern@gmail.com>
> > Fixes: ba52d61e0ff ("ipv4: route: restore skb_dst_set in inet_rtm_getroute")
> > Signed-off-by: Florian Westphal <fw@strlen.de>
>
> Have looked at the change in detail, but are you sure that is the
> correct Fixes?
I'm reasonably sure, yes:
if (iif) {
ip_route_input_rcu // 1 might get NOREF dst
} else {
ip_route_output_key_hash_rcu // 2 always takes dst ref
}
skb_dst_set /* 3 loses NOREF in case of 1) */
> Running these:
> while true;do ip r get 10.1.1.3;done > /dev/null &
> while true;do ip r get 10.1.1.3 iif eth0 from 192.16.1.1;done >
> /dev/null &
>
> at various commits:
> ffe95ecf3a2 - KASAN backtraces
Right, this is broken state (has both ba52d61e0ff and 3765d35ed8b9)
> 374d801522f - works fine
This is fine, it lacks 3765d35ed8b9:
both branches take a reference on dst so '3' above has no side effect.
> ba52d61e0ff - negative refcnt messages
AFAICS this is before dst gc removal, I guess (but did not
check) that KASAN vs. refcount just comes from this.
> a5e2ee5da47 - works fine
Should cause a memory leak when iif is not given (ref on dst is
taken but not released in case of 2), ba52d61e0ff cured this but
adds the problem described here).
Does that make it clearer?
^ permalink raw reply
* [PATCH net] datagram: When peeking datagrams with offset < 0 don't skip empty skbs
From: Matthew Dawson @ 2017-08-14 5:52 UTC (permalink / raw)
To: netdev; +Cc: Matthew Dawson, Macieira, Thiago, willemdebruijn.kernel
Due to commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac ("udp: remove
headers from UDP packets before queueing"), when udp packets are being
peeked the requested extra offset is always 0 as there is no need to skip
the udp header. However, when the offset is 0 and the next skb is
of length 0, it is only returned once. The behaviour can be seen with
the following python script:
#!/usr/bin/env python3
from socket import *;
f=socket(AF_INET6, SOCK_DGRAM | SOCK_NONBLOCK, 0);
g=socket(AF_INET6, SOCK_DGRAM | SOCK_NONBLOCK, 0);
f.bind(('::', 0));
addr=('::1', f.getsockname()[1]);
g.sendto(b'', addr)
g.sendto(b'b', addr)
print(f.recvfrom(10, MSG_PEEK));
print(f.recvfrom(10, MSG_PEEK));
Where the expected output should be the empty string twice.
Instead, make sk_peek_offset return negative values, and pass those values
to __skb_try_recv_datagram/__skb_try_recv_from_queue. If the passed offset
to __skb_try_recv_from_queue is negative, the checked skb is never skipped.
After the call, the offset is set to 0 if negative to ensure all further
calculations are correct.
Also simplify the if condition in __skb_try_recv_from_queue. If _off is
greater then 0, and off is greater then or equal to skb->len, then (_off ||
skb->len) must always be true assuming skb->len >= 0 is always true.
Also remove a redundant check around a call to sk_peek_offset in af_unix.c,
as it double checked if MSG_PEEK was set in the flags.
Signed-off-by: Matthew Dawson <matthew@mjdsystems.ca>
---
include/net/sock.h | 4 +---
net/core/datagram.c | 4 ++--
net/ipv4/udp.c | 4 ++++
net/ipv6/udp.c | 4 ++++
net/unix/af_unix.c | 8 +++++---
5 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 7c0632c7e870..aeeec62992ca 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -507,9 +507,7 @@ int sk_set_peek_off(struct sock *sk, int val);
static inline int sk_peek_offset(struct sock *sk, int flags)
{
if (unlikely(flags & MSG_PEEK)) {
- s32 off = READ_ONCE(sk->sk_peek_off);
- if (off >= 0)
- return off;
+ return READ_ONCE(sk->sk_peek_off);
}
return 0;
diff --git a/net/core/datagram.c b/net/core/datagram.c
index ee5647bd91b3..a91cd8aa244b 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -175,8 +175,8 @@ struct sk_buff *__skb_try_recv_from_queue(struct sock *sk,
*last = queue->prev;
skb_queue_walk(queue, skb) {
if (flags & MSG_PEEK) {
- if (_off >= skb->len && (skb->len || _off ||
- skb->peeked)) {
+ if (_off >= 0 && _off >= skb->len &&
+ (_off || skb->peeked)) {
_off -= skb->len;
continue;
}
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index a7c804f73990..a0d8d6d285f4 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1576,6 +1576,10 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock,
try_again:
peeking = off = sk_peek_offset(sk, flags);
skb = __skb_recv_udp(sk, flags, noblock, &peeked, &off, &err);
+ if (off < 0) {
+ off = 0;
+ peeking = 0;
+ }
if (!skb)
return err;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 578142b7ca3e..34d8f9f8d87d 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -364,6 +364,10 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
try_again:
peeking = off = sk_peek_offset(sk, flags);
skb = __skb_recv_udp(sk, flags, noblock, &peeked, &off, &err);
+ if (peeking < 0) {
+ off = 0;
+ peeking = 0;
+ }
if (!skb)
return err;
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 7b52a380d710..390e0627dc05 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2127,6 +2127,8 @@ static int unix_dgram_recvmsg(struct socket *sock, struct msghdr *msg,
skip = sk_peek_offset(sk, flags);
skb = __skb_try_recv_datagram(sk, flags, NULL, &peeked, &skip,
&err, &last);
+ if (skip < 0)
+ skip = 0;
if (skb)
break;
@@ -2304,10 +2306,10 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
*/
mutex_lock(&u->iolock);
- if (flags & MSG_PEEK)
- skip = sk_peek_offset(sk, flags);
- else
+ skip = sk_peek_offset(sk, flags);
+ if (skip < 0) {
skip = 0;
+ }
do {
int chunk;
--
2.13.0
^ permalink raw reply related
* Re: [patch v1 1/2] Allow Mellanox network vendor to be configured if only I2C bus is configured
From: Jiri Pirko @ 2017-08-14 7:00 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Ohad Oz, davem, netdev, saeedm, vadimp, system-sw-low-level
In-Reply-To: <20170812143715.GN24282@mtr-leonro.local>
Sat, Aug 12, 2017 at 04:37:15PM CEST, leon@kernel.org wrote:
>On Thu, Aug 10, 2017 at 05:11:51PM +0000, Ohad Oz wrote:
>> Patch allows Mellanox devices on system with no PCI, but with I2C only.
>>
>
>Did you test mlx5 device on such system? Did it work for you?
$ git grep i2c_add_driver drivers/net/ethernet/mellanox/
drivers/net/ethernet/mellanox/mlxsw/i2c.c: return i2c_add_driver(i2c_driver);
That is the only driver that would probe on i2c, non-pci system.
mlx5 just would not probe, so there is nothing to test.
>
>What is the changelog between v0 and v1 of these patches?
>
>
>> Signed-off-by: Ohad Oz <ohado@mellanox.com>
>> ---
>> drivers/net/ethernet/mellanox/Kconfig | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/Kconfig b/drivers/net/ethernet/mellanox/Kconfig
>> index 84a2007..0949741 100644
>> --- a/drivers/net/ethernet/mellanox/Kconfig
>> +++ b/drivers/net/ethernet/mellanox/Kconfig
>> @@ -5,7 +5,7 @@
>> config NET_VENDOR_MELLANOX
>> bool "Mellanox devices"
>> default y
>> - depends on PCI
>> + depends on PCI || I2C
>> ---help---
>> If you have a network (Ethernet) card belonging to this class, say Y.
>>
>> --
>> 2.8.0
>>
^ permalink raw reply
* Re: [PATCH] liquidio: fix duplicated code for different branches
From: Felix Manlunas @ 2017-08-14 7:14 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Derek Chickles, Satanand Burla, Raghu Vatsavayi, netdev,
linux-kernel
In-Reply-To: <20170813013855.GA6686@embeddedgus>
On Sat, Aug 12, 2017 at 08:38:55PM -0500, Gustavo A. R. Silva wrote:
> Refactor code in order to avoid identical code for different branches.
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> This code was tested by compilation only.
>
> drivers/net/ethernet/cavium/liquidio/lio_ethtool.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c b/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c
> index b78e296..4f65c08 100644
> --- a/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c
> +++ b/drivers/net/ethernet/cavium/liquidio/lio_ethtool.c
> @@ -578,23 +578,18 @@ static int lio_set_phys_id(struct net_device *netdev,
> break;
>
> case ETHTOOL_ID_ON:
> - if (oct->chip_id == OCTEON_CN66XX) {
> + if (oct->chip_id == OCTEON_CN66XX)
> octnet_gpio_access(netdev, VITESSE_PHY_GPIO_CFG,
> VITESSE_PHY_GPIO_HIGH);
> -
> - } else if (oct->chip_id == OCTEON_CN68XX) {
> - return -EINVAL;
> - } else {
> + else
> return -EINVAL;
> - }
> +
> break;
>
> case ETHTOOL_ID_OFF:
> if (oct->chip_id == OCTEON_CN66XX)
> octnet_gpio_access(netdev, VITESSE_PHY_GPIO_CFG,
> VITESSE_PHY_GPIO_LOW);
> - else if (oct->chip_id == OCTEON_CN68XX)
> - return -EINVAL;
> else
> return -EINVAL;
>
> --
> 2.5.0
>
Acked-by: Felix Manlunas <felix.manlunas@cavium.com>
^ permalink raw reply
* Re: [PATCH 000/102] Convert drivers to explicit reset API
From: Philipp Zabel @ 2017-08-14 7:36 UTC (permalink / raw)
To: Wolfram Sang
Cc: Linus Walleij, Dmitry Torokhov, Thomas Petazzoni, lkml,
Andrew Lunn, Prashant Gaikwad, Heiko Stuebner, Peter Chen, DRI,
Marc Dietrich, Rakesh Iyer, Peter Meerwald-Stadler, linux-clk,
Wim Van Sebroeck, Xinliang Liu, Chanwoo Choi, Alan Stern,
Jiri Slaby, Michael Turquette, Guenter
In-Reply-To: <20170812114357.v4ru75dw5hq7wemx@ninjato>
On Sat, 2017-08-12 at 13:43 +0200, Wolfram Sang wrote:
> > Thanks for the hint and the references. It seems this turned out
> > okay,
> > but I wouldn't dare to introduce such macro horror^Wmagic.
> > I'd rather have all users converted to the _exclusive/_shared
> > function
> > calls and maybe then replace the internal __reset_control_get with
> > Thomas' suggestion.
>
> I didn't follow the discussion closely. Shall I still apply the i2c
> patches?
Yes, please.
regards
Philipp
^ permalink raw reply
* Re: [PATCH net-next v2] openvswitch: enable NSH support
From: Jiri Benc @ 2017-08-14 7:51 UTC (permalink / raw)
To: Jan Scheurich
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org
In-Reply-To: <CFF8EF42F1132E4CBE2BF0AB6C21C58D72736EAE-hqolJogE5njKJFWPz4pdheaU1rCVNFv4@public.gmane.org>
On Sun, 13 Aug 2017 21:13:57 +0000, Jan Scheurich wrote:
> Jiri, I am not too familiar with conventions on the OVS netlink
> interface regarding the handling of variable length fields. What is
> the benefit of structuring the push_nsh action into
>
> OVS_ACTION_ATTR_PUSH_NSH
> +-- OVS_ACTION_ATTR_NSH_BASE_HEADER
> +-- OVS_ACTION_ATTR_NSH_MD
>
> instead of grouping the base header fields and the variable length MD
> into a single monolithic attribute OVS_ACTION_ATTR_PUSH_NSH? Is the
> main concern a potential future need to add additional parameters to
> the push_nsh datapath action? Are there examples for such structured
> actions other than OVS_ACTION_ATTR_CT where the need is obvious
> because it is very polymorphic?
This is about having the design clean. What would you do if you had two
variable length fields? Would you still put them in a single structure
and have a length field in the structure, too? That would be wrong, we
have length in the attribute header. I doubt you would do that. Which
indicates that putting variable length fields to an attribute with
anything other is wrong.
Also, look at how ugly the code would be. You'd have to subtract the
base header length from the attribute length to get the variable data
length. That's not nice at all.
Think about the netlink in the way that by default, every field should
have its own attribute. Structures are included only for performance
reasons where certain fields are always passed in a message and having
them in separate attributes would be impractical and waste of space.
Going out of your way to include the context data in the structure thus
doesn't make sense.
> BTW: The name OVS_ACTION_ATTR_NSH_BASE_HEADER is misleading because
> in the NSH draft the term base header is used for the first 32-bit
> word, whereas here it includes also the 32-bit Service Path header.
An excellent comment. This means that it's very well possible that
future NSH versions may not include SP header or may have it of a
different size. Maybe we should consider putting it into a separate
attribute, too? Not sure it is needed, though, I don't think it's
likely to happen.
Jiri
^ permalink raw reply
* Re: [PATCH net-next V2 3/3] tap: XDP support
From: Daniel Borkmann @ 2017-08-14 8:43 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, mst, netdev, linux-kernel, kubakici
In-Reply-To: <1502451678-17358-4-git-send-email-jasowang@redhat.com>
On 08/11/2017 01:41 PM, Jason Wang wrote:
> This patch tries to implement XDP for tun. The implementation was
> split into two parts:
[...]
> @@ -1402,6 +1521,22 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> skb_reset_network_header(skb);
> skb_probe_transport_header(skb, 0);
>
> + if (generic_xdp) {
> + struct bpf_prog *xdp_prog;
> + int ret;
> +
> + rcu_read_lock();
> + xdp_prog = rcu_dereference(tun->xdp_prog);
The name generic_xdp is a bit confusing in this context given this
is 'native' XDP, perhaps above if (generic_xdp) should have a comment
explaining semantics for tun and how it relates to actual generic xdp
that sits at dev->xdp_prog, and gets run from netif_rx_ni(). Or just
name the bool xdp_handle_gso with a comment that we let the generic
XDP infrastructure deal with non-linear skbs instead of having to
re-implement the do_xdp_generic() internals, plus a statement that
the actual generic XDP comes a bit later in the path. That would at
least make it more obvious to read, imho.
> + if (xdp_prog) {
> + ret = do_xdp_generic(xdp_prog, skb);
> + if (ret != XDP_PASS) {
> + rcu_read_unlock();
> + return total_len;
> + }
> + }
> + rcu_read_unlock();
> + }
> +
> rxhash = __skb_get_hash_symmetric(skb);
> #ifndef CONFIG_4KSTACKS
> tun_rx_batched(tun, tfile, skb, more);
>
^ permalink raw reply
* Re: [iproute PATCH 17/51] lib/bpf: Don't leak fp in bpf_find_mntpt()
From: Daniel Borkmann @ 2017-08-14 8:46 UTC (permalink / raw)
To: Phil Sutter, Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-18-phil@nwl.cc>
On 08/12/2017 02:04 PM, Phil Sutter wrote:
> If fopen() succeeded but len != PATH_MAX, the function leaks the open
> FILE pointer. Fix this by checking len value before calling fopen().
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* [patch net-next 0/3] mlxsw: spectrum_router: Increase VRF scale
From: Jiri Pirko @ 2017-08-14 8:54 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, mlxsw
From: Jiri Pirko <jiri@mellanox.com>
Ido says:
The purpose of this set is to increase the maximum number of supported VRF
devices on top of the Spectrum ASIC under different workloads.
This is achieved by sharing the same LPM tree across all the virtual
routers for a given L3 protocol (IPv4 / IPv6). The change is explained in
detail in the third patch. First two patches are small changes to make
review easier.
Ido Schimmel (3):
mlxsw: spectrum_router: Return void from deletion functions
mlxsw: spectrum_router: Pass argument explicitly
mlxsw: spectrum_router: Use one LPM tree for all virtual routers
.../net/ethernet/mellanox/mlxsw/spectrum_router.c | 282 +++++++++++++--------
1 file changed, 176 insertions(+), 106 deletions(-)
--
2.9.3
^ permalink raw reply
* [patch net-next 1/3] mlxsw: spectrum_router: Return void from deletion functions
From: Jiri Pirko @ 2017-08-14 8:54 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, mlxsw
In-Reply-To: <20170814085405.1452-1-jiri@resnulli.us>
From: Ido Schimmel <idosch@mellanox.com>
There is no point in returning a value from function whose return value
is never checked.
Even if the return value was checked, there wouldn't be anything to do
about it, as these functions are either called from error or deletion
paths.
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 93b6da8..220e7e7 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -505,15 +505,15 @@ static int mlxsw_sp_lpm_tree_alloc(struct mlxsw_sp *mlxsw_sp,
return mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(ralta), ralta_pl);
}
-static int mlxsw_sp_lpm_tree_free(struct mlxsw_sp *mlxsw_sp,
- struct mlxsw_sp_lpm_tree *lpm_tree)
+static void mlxsw_sp_lpm_tree_free(struct mlxsw_sp *mlxsw_sp,
+ struct mlxsw_sp_lpm_tree *lpm_tree)
{
char ralta_pl[MLXSW_REG_RALTA_LEN];
mlxsw_reg_ralta_pack(ralta_pl, false,
(enum mlxsw_reg_ralxx_protocol) lpm_tree->proto,
lpm_tree->id);
- return mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(ralta), ralta_pl);
+ mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(ralta), ralta_pl);
}
static int
@@ -569,10 +569,10 @@ mlxsw_sp_lpm_tree_create(struct mlxsw_sp *mlxsw_sp,
return ERR_PTR(err);
}
-static int mlxsw_sp_lpm_tree_destroy(struct mlxsw_sp *mlxsw_sp,
- struct mlxsw_sp_lpm_tree *lpm_tree)
+static void mlxsw_sp_lpm_tree_destroy(struct mlxsw_sp *mlxsw_sp,
+ struct mlxsw_sp_lpm_tree *lpm_tree)
{
- return mlxsw_sp_lpm_tree_free(mlxsw_sp, lpm_tree);
+ mlxsw_sp_lpm_tree_free(mlxsw_sp, lpm_tree);
}
static struct mlxsw_sp_lpm_tree *
@@ -601,12 +601,11 @@ mlxsw_sp_lpm_tree_get(struct mlxsw_sp *mlxsw_sp,
return lpm_tree;
}
-static int mlxsw_sp_lpm_tree_put(struct mlxsw_sp *mlxsw_sp,
- struct mlxsw_sp_lpm_tree *lpm_tree)
+static void mlxsw_sp_lpm_tree_put(struct mlxsw_sp *mlxsw_sp,
+ struct mlxsw_sp_lpm_tree *lpm_tree)
{
if (--lpm_tree->ref_count == 0)
- return mlxsw_sp_lpm_tree_destroy(mlxsw_sp, lpm_tree);
- return 0;
+ mlxsw_sp_lpm_tree_destroy(mlxsw_sp, lpm_tree);
}
#define MLXSW_SP_LPM_TREE_MIN 1 /* tree 0 is reserved */
--
2.9.3
^ permalink raw reply related
* [patch net-next 2/3] mlxsw: spectrum_router: Pass argument explicitly
From: Jiri Pirko @ 2017-08-14 8:54 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, mlxsw
In-Reply-To: <20170814085405.1452-1-jiri@resnulli.us>
From: Ido Schimmel <idosch@mellanox.com>
Instead of relying on the LPM tree to be assigned to the virtual router
before binding the two, lets pass it explicitly.
This will later allow us to return upon binding error instead of having
to perform a rollback of the assignment.
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 220e7e7..3c204d2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -659,13 +659,13 @@ static struct mlxsw_sp_vr *mlxsw_sp_vr_find_unused(struct mlxsw_sp *mlxsw_sp)
}
static int mlxsw_sp_vr_lpm_tree_bind(struct mlxsw_sp *mlxsw_sp,
- const struct mlxsw_sp_fib *fib)
+ const struct mlxsw_sp_fib *fib, u8 tree_id)
{
char raltb_pl[MLXSW_REG_RALTB_LEN];
mlxsw_reg_raltb_pack(raltb_pl, fib->vr->id,
(enum mlxsw_reg_ralxx_protocol) fib->proto,
- fib->lpm_tree->id);
+ tree_id);
return mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(raltb), raltb_pl);
}
@@ -777,7 +777,7 @@ mlxsw_sp_vr_lpm_tree_check(struct mlxsw_sp *mlxsw_sp, struct mlxsw_sp_fib *fib,
/* Prevent packet loss by overwriting existing binding */
fib->lpm_tree = new_tree;
- err = mlxsw_sp_vr_lpm_tree_bind(mlxsw_sp, fib);
+ err = mlxsw_sp_vr_lpm_tree_bind(mlxsw_sp, fib, new_tree->id);
if (err)
goto err_tree_bind;
mlxsw_sp_lpm_tree_put(mlxsw_sp, lpm_tree);
@@ -2631,7 +2631,7 @@ static int mlxsw_sp_fib_node_init(struct mlxsw_sp *mlxsw_sp,
if (IS_ERR(lpm_tree))
return PTR_ERR(lpm_tree);
fib->lpm_tree = lpm_tree;
- err = mlxsw_sp_vr_lpm_tree_bind(mlxsw_sp, fib);
+ err = mlxsw_sp_vr_lpm_tree_bind(mlxsw_sp, fib, lpm_tree->id);
if (err)
goto err_tree_bind;
}
--
2.9.3
^ permalink raw reply related
* [patch net-next 3/3] mlxsw: spectrum_router: Use one LPM tree for all virtual routers
From: Jiri Pirko @ 2017-08-14 8:54 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, mlxsw
In-Reply-To: <20170814085405.1452-1-jiri@resnulli.us>
From: Ido Schimmel <idosch@mellanox.com>
The number of LPM trees available for lookup is much smaller than the
number of virtual routers, which are used to implement VRFs. In
addition, an LPM tree can only be used by one protocol - either IPv4 or
IPv6.
Therefore, in order to increase the number of supported virtual routers
to the maximum we need to be able to share LPM trees across virtual
routers instead of trying to find an optimized tree for each.
Do that by allocating one LPM tree for each protocol, but make sure it
will only include prefixes that are actually used, so as to not perform
unnecessary lookups.
Since changing the structure of a bound tree isn't recommended, whenever
a new tree it required, it's first created and then bound to each
virtual router, replacing the old one.
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
.../net/ethernet/mellanox/mlxsw/spectrum_router.c | 259 +++++++++++++--------
1 file changed, 165 insertions(+), 94 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 3c204d2..3d9be36 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -321,19 +321,6 @@ struct mlxsw_sp_prefix_usage {
for_each_set_bit(prefix, (prefix_usage)->b, MLXSW_SP_PREFIX_COUNT)
static bool
-mlxsw_sp_prefix_usage_subset(struct mlxsw_sp_prefix_usage *prefix_usage1,
- struct mlxsw_sp_prefix_usage *prefix_usage2)
-{
- unsigned char prefix;
-
- mlxsw_sp_prefix_usage_for_each(prefix, prefix_usage1) {
- if (!test_bit(prefix, prefix_usage2->b))
- return false;
- }
- return true;
-}
-
-static bool
mlxsw_sp_prefix_usage_eq(struct mlxsw_sp_prefix_usage *prefix_usage1,
struct mlxsw_sp_prefix_usage *prefix_usage2)
{
@@ -589,16 +576,14 @@ mlxsw_sp_lpm_tree_get(struct mlxsw_sp *mlxsw_sp,
lpm_tree->proto == proto &&
mlxsw_sp_prefix_usage_eq(&lpm_tree->prefix_usage,
prefix_usage))
- goto inc_ref_count;
+ return lpm_tree;
}
- lpm_tree = mlxsw_sp_lpm_tree_create(mlxsw_sp, prefix_usage,
- proto);
- if (IS_ERR(lpm_tree))
- return lpm_tree;
+ return mlxsw_sp_lpm_tree_create(mlxsw_sp, prefix_usage, proto);
+}
-inc_ref_count:
+static void mlxsw_sp_lpm_tree_hold(struct mlxsw_sp_lpm_tree *lpm_tree)
+{
lpm_tree->ref_count++;
- return lpm_tree;
}
static void mlxsw_sp_lpm_tree_put(struct mlxsw_sp *mlxsw_sp,
@@ -750,46 +735,6 @@ static void mlxsw_sp_vr_destroy(struct mlxsw_sp_vr *vr)
vr->fib4 = NULL;
}
-static int
-mlxsw_sp_vr_lpm_tree_check(struct mlxsw_sp *mlxsw_sp, struct mlxsw_sp_fib *fib,
- struct mlxsw_sp_prefix_usage *req_prefix_usage)
-{
- struct mlxsw_sp_lpm_tree *lpm_tree = fib->lpm_tree;
- struct mlxsw_sp_lpm_tree *new_tree;
- int err;
-
- if (mlxsw_sp_prefix_usage_eq(req_prefix_usage, &lpm_tree->prefix_usage))
- return 0;
-
- new_tree = mlxsw_sp_lpm_tree_get(mlxsw_sp, req_prefix_usage,
- fib->proto);
- if (IS_ERR(new_tree)) {
- /* We failed to get a tree according to the required
- * prefix usage. However, the current tree might be still good
- * for us if our requirement is subset of the prefixes used
- * in the tree.
- */
- if (mlxsw_sp_prefix_usage_subset(req_prefix_usage,
- &lpm_tree->prefix_usage))
- return 0;
- return PTR_ERR(new_tree);
- }
-
- /* Prevent packet loss by overwriting existing binding */
- fib->lpm_tree = new_tree;
- err = mlxsw_sp_vr_lpm_tree_bind(mlxsw_sp, fib, new_tree->id);
- if (err)
- goto err_tree_bind;
- mlxsw_sp_lpm_tree_put(mlxsw_sp, lpm_tree);
-
- return 0;
-
-err_tree_bind:
- fib->lpm_tree = lpm_tree;
- mlxsw_sp_lpm_tree_put(mlxsw_sp, new_tree);
- return err;
-}
-
static struct mlxsw_sp_vr *mlxsw_sp_vr_get(struct mlxsw_sp *mlxsw_sp, u32 tb_id)
{
struct mlxsw_sp_vr *vr;
@@ -808,6 +753,100 @@ static void mlxsw_sp_vr_put(struct mlxsw_sp_vr *vr)
mlxsw_sp_vr_destroy(vr);
}
+static bool
+mlxsw_sp_vr_lpm_tree_should_replace(struct mlxsw_sp_vr *vr,
+ enum mlxsw_sp_l3proto proto, u8 tree_id)
+{
+ struct mlxsw_sp_fib *fib = mlxsw_sp_vr_fib(vr, proto);
+
+ if (!mlxsw_sp_vr_is_used(vr))
+ return false;
+ if (fib->lpm_tree && fib->lpm_tree->id == tree_id)
+ return true;
+ return false;
+}
+
+static int mlxsw_sp_vr_lpm_tree_replace(struct mlxsw_sp *mlxsw_sp,
+ struct mlxsw_sp_fib *fib,
+ struct mlxsw_sp_lpm_tree *new_tree)
+{
+ struct mlxsw_sp_lpm_tree *old_tree = fib->lpm_tree;
+ int err;
+
+ err = mlxsw_sp_vr_lpm_tree_bind(mlxsw_sp, fib, new_tree->id);
+ if (err)
+ return err;
+ fib->lpm_tree = new_tree;
+ mlxsw_sp_lpm_tree_hold(new_tree);
+ mlxsw_sp_lpm_tree_put(mlxsw_sp, old_tree);
+ return 0;
+}
+
+static int mlxsw_sp_vrs_lpm_tree_replace(struct mlxsw_sp *mlxsw_sp,
+ struct mlxsw_sp_fib *fib,
+ struct mlxsw_sp_lpm_tree *new_tree)
+{
+ struct mlxsw_sp_lpm_tree *old_tree = fib->lpm_tree;
+ enum mlxsw_sp_l3proto proto = fib->proto;
+ u8 old_id, new_id = new_tree->id;
+ struct mlxsw_sp_vr *vr;
+ int i, err;
+
+ if (!old_tree)
+ goto no_replace;
+ old_id = old_tree->id;
+
+ for (i = 0; i < MLXSW_CORE_RES_GET(mlxsw_sp->core, MAX_VRS); i++) {
+ vr = &mlxsw_sp->router->vrs[i];
+ if (!mlxsw_sp_vr_lpm_tree_should_replace(vr, proto, old_id))
+ continue;
+ err = mlxsw_sp_vr_lpm_tree_replace(mlxsw_sp,
+ mlxsw_sp_vr_fib(vr, proto),
+ new_tree);
+ if (err)
+ goto err_tree_replace;
+ }
+
+ return 0;
+
+err_tree_replace:
+ for (i--; i >= 0; i--) {
+ if (!mlxsw_sp_vr_lpm_tree_should_replace(vr, proto, new_id))
+ continue;
+ mlxsw_sp_vr_lpm_tree_replace(mlxsw_sp,
+ mlxsw_sp_vr_fib(vr, proto),
+ old_tree);
+ }
+ return err;
+
+no_replace:
+ err = mlxsw_sp_vr_lpm_tree_bind(mlxsw_sp, fib, new_tree->id);
+ if (err)
+ return err;
+ fib->lpm_tree = new_tree;
+ mlxsw_sp_lpm_tree_hold(new_tree);
+ return 0;
+}
+
+static void
+mlxsw_sp_vrs_prefixes(struct mlxsw_sp *mlxsw_sp,
+ enum mlxsw_sp_l3proto proto,
+ struct mlxsw_sp_prefix_usage *req_prefix_usage)
+{
+ int i;
+
+ for (i = 0; i < MLXSW_CORE_RES_GET(mlxsw_sp->core, MAX_VRS); i++) {
+ struct mlxsw_sp_vr *vr = &mlxsw_sp->router->vrs[i];
+ struct mlxsw_sp_fib *fib = mlxsw_sp_vr_fib(vr, proto);
+ unsigned char prefix;
+
+ if (!mlxsw_sp_vr_is_used(vr))
+ continue;
+ mlxsw_sp_prefix_usage_for_each(prefix, &fib->prefix_usage)
+ mlxsw_sp_prefix_usage_set(req_prefix_usage, prefix);
+ }
+}
+
static int mlxsw_sp_vrs_init(struct mlxsw_sp *mlxsw_sp)
{
struct mlxsw_sp_vr *vr;
@@ -2586,6 +2625,67 @@ mlxsw_sp_fib_node_entry_is_first(const struct mlxsw_sp_fib_node *fib_node,
struct mlxsw_sp_fib_entry, list) == fib_entry;
}
+static int mlxsw_sp_fib_lpm_tree_link(struct mlxsw_sp *mlxsw_sp,
+ struct mlxsw_sp_fib *fib,
+ struct mlxsw_sp_fib_node *fib_node)
+{
+ struct mlxsw_sp_prefix_usage req_prefix_usage = {{ 0 } };
+ struct mlxsw_sp_lpm_tree *lpm_tree;
+ int err;
+
+ /* Since the tree is shared between all virtual routers we must
+ * make sure it contains all the required prefix lengths. This
+ * can be computed by either adding the new prefix length to the
+ * existing prefix usage of a bound tree, or by aggregating the
+ * prefix lengths across all virtual routers and adding the new
+ * one as well.
+ */
+ if (fib->lpm_tree)
+ mlxsw_sp_prefix_usage_cpy(&req_prefix_usage,
+ &fib->lpm_tree->prefix_usage);
+ else
+ mlxsw_sp_vrs_prefixes(mlxsw_sp, fib->proto, &req_prefix_usage);
+ mlxsw_sp_prefix_usage_set(&req_prefix_usage, fib_node->key.prefix_len);
+
+ lpm_tree = mlxsw_sp_lpm_tree_get(mlxsw_sp, &req_prefix_usage,
+ fib->proto);
+ if (IS_ERR(lpm_tree))
+ return PTR_ERR(lpm_tree);
+
+ if (fib->lpm_tree && fib->lpm_tree->id == lpm_tree->id)
+ return 0;
+
+ err = mlxsw_sp_vrs_lpm_tree_replace(mlxsw_sp, fib, lpm_tree);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+static void mlxsw_sp_fib_lpm_tree_unlink(struct mlxsw_sp *mlxsw_sp,
+ struct mlxsw_sp_fib *fib)
+{
+ struct mlxsw_sp_prefix_usage req_prefix_usage = {{ 0 } };
+ struct mlxsw_sp_lpm_tree *lpm_tree;
+
+ /* Aggregate prefix lengths across all virtual routers to make
+ * sure we only have used prefix lengths in the LPM tree.
+ */
+ mlxsw_sp_vrs_prefixes(mlxsw_sp, fib->proto, &req_prefix_usage);
+ lpm_tree = mlxsw_sp_lpm_tree_get(mlxsw_sp, &req_prefix_usage,
+ fib->proto);
+ if (IS_ERR(lpm_tree))
+ goto err_tree_get;
+ mlxsw_sp_vrs_lpm_tree_replace(mlxsw_sp, fib, lpm_tree);
+
+err_tree_get:
+ if (!mlxsw_sp_prefix_usage_none(&fib->prefix_usage))
+ return;
+ mlxsw_sp_vr_lpm_tree_unbind(mlxsw_sp, fib);
+ mlxsw_sp_lpm_tree_put(mlxsw_sp, fib->lpm_tree);
+ fib->lpm_tree = NULL;
+}
+
static void mlxsw_sp_fib_node_prefix_inc(struct mlxsw_sp_fib_node *fib_node)
{
unsigned char prefix_len = fib_node->key.prefix_len;
@@ -2608,8 +2708,6 @@ static int mlxsw_sp_fib_node_init(struct mlxsw_sp *mlxsw_sp,
struct mlxsw_sp_fib_node *fib_node,
struct mlxsw_sp_fib *fib)
{
- struct mlxsw_sp_prefix_usage req_prefix_usage;
- struct mlxsw_sp_lpm_tree *lpm_tree;
int err;
err = mlxsw_sp_fib_node_insert(fib, fib_node);
@@ -2617,33 +2715,15 @@ static int mlxsw_sp_fib_node_init(struct mlxsw_sp *mlxsw_sp,
return err;
fib_node->fib = fib;
- mlxsw_sp_prefix_usage_cpy(&req_prefix_usage, &fib->prefix_usage);
- mlxsw_sp_prefix_usage_set(&req_prefix_usage, fib_node->key.prefix_len);
-
- if (!mlxsw_sp_prefix_usage_none(&fib->prefix_usage)) {
- err = mlxsw_sp_vr_lpm_tree_check(mlxsw_sp, fib,
- &req_prefix_usage);
- if (err)
- goto err_tree_check;
- } else {
- lpm_tree = mlxsw_sp_lpm_tree_get(mlxsw_sp, &req_prefix_usage,
- fib->proto);
- if (IS_ERR(lpm_tree))
- return PTR_ERR(lpm_tree);
- fib->lpm_tree = lpm_tree;
- err = mlxsw_sp_vr_lpm_tree_bind(mlxsw_sp, fib, lpm_tree->id);
- if (err)
- goto err_tree_bind;
- }
+ err = mlxsw_sp_fib_lpm_tree_link(mlxsw_sp, fib, fib_node);
+ if (err)
+ goto err_fib_lpm_tree_link;
mlxsw_sp_fib_node_prefix_inc(fib_node);
return 0;
-err_tree_bind:
- fib->lpm_tree = NULL;
- mlxsw_sp_lpm_tree_put(mlxsw_sp, lpm_tree);
-err_tree_check:
+err_fib_lpm_tree_link:
fib_node->fib = NULL;
mlxsw_sp_fib_node_remove(fib, fib_node);
return err;
@@ -2652,19 +2732,10 @@ static int mlxsw_sp_fib_node_init(struct mlxsw_sp *mlxsw_sp,
static void mlxsw_sp_fib_node_fini(struct mlxsw_sp *mlxsw_sp,
struct mlxsw_sp_fib_node *fib_node)
{
- struct mlxsw_sp_lpm_tree *lpm_tree = fib_node->fib->lpm_tree;
struct mlxsw_sp_fib *fib = fib_node->fib;
mlxsw_sp_fib_node_prefix_dec(fib_node);
-
- if (mlxsw_sp_prefix_usage_none(&fib->prefix_usage)) {
- mlxsw_sp_vr_lpm_tree_unbind(mlxsw_sp, fib);
- fib->lpm_tree = NULL;
- mlxsw_sp_lpm_tree_put(mlxsw_sp, lpm_tree);
- } else {
- mlxsw_sp_vr_lpm_tree_check(mlxsw_sp, fib, &fib->prefix_usage);
- }
-
+ mlxsw_sp_fib_lpm_tree_unlink(mlxsw_sp, fib);
fib_node->fib = NULL;
mlxsw_sp_fib_node_remove(fib, fib_node);
}
--
2.9.3
^ permalink raw reply related
* Something hitting my total number of connections to the server
From: Akshat Kakkar @ 2017-08-14 9:07 UTC (permalink / raw)
To: netdev
I have centos 7.3 (Kernel 3.10) running on a server with 128GB RAM and
2 x 10 Core Xeon Processor.
I have hosted a webserver on it and enabled ssh for remote maintenance.
Previously it was running on Centos 6.3.
After upgrading to CentOS 7.3, occasionally (probably when number of
hits are more on the server), I am not able to create new connections
(neither on web nor on ssh). Existing connections keeps on running
fine.
I did packet capturing using tcpdump to understand if its some
intermediate network issue.
What I found was the server is not replying for new SYN requests.
So it's clear that its not at all application issue. Also, there are
no logs in applications logs for any connections dropped, if any.
I check my firewall rules if there is some rate limiting imposed.
There is nothing in there.
I check tc, if by mistake some rate limiting is imposed. There is
nothing in there too.
I have increased noOfFiles to 1000000 and other sysctl parameters, but
the issue is still there.
Has anybody experienced the same?
How to go about? Anybody ... Please Help!!!
^ permalink raw reply
* Re: [iproute PATCH 51/51] lib/bpf: Check return value of write()
From: Daniel Borkmann @ 2017-08-14 9:17 UTC (permalink / raw)
To: Phil Sutter, Stephen Hemminger; +Cc: netdev
In-Reply-To: <20170812120510.28750-52-phil@nwl.cc>
On 08/12/2017 02:05 PM, Phil Sutter wrote:
> This is merely to silence the compiler warning. If write to stderr
> failed, assume that printing an error message will fail as well so don't
> even try.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> lib/bpf.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 1dcb261dc915f..825e071cea572 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -591,7 +591,8 @@ int bpf_trace_pipe(void)
>
> ret = read(fd, buff, sizeof(buff) - 1);
> if (ret > 0) {
> - write(2, buff, ret);
> + if (write(STDERR_FILENO, buff, ret) != ret)
> + return -1;
Quite unlikely to fail, but we should probably bark loudly
here instead of just returning -1. Perhaps assert() would
suit better.
> fflush(stderr);
> }
> }
>
^ permalink raw reply
* Re: [PATCH net] datagram: When peeking datagrams with offset < 0 don't skip empty skbs
From: Paolo Abeni @ 2017-08-14 9:27 UTC (permalink / raw)
To: Matthew Dawson, netdev; +Cc: Macieira, Thiago, willemdebruijn.kernel
In-Reply-To: <20170814055259.31078-1-matthew@mjdsystems.ca>
On Mon, 2017-08-14 at 01:52 -0400, Matthew Dawson wrote:
> Due to commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac ("udp: remove
> headers from UDP packets before queueing"), when udp packets are being
> peeked the requested extra offset is always 0 as there is no need to skip
> the udp header. However, when the offset is 0 and the next skb is
> of length 0, it is only returned once. The behaviour can be seen with
> the following python script:
>
> #!/usr/bin/env python3
> from socket import *;
> f=socket(AF_INET6, SOCK_DGRAM | SOCK_NONBLOCK, 0);
> g=socket(AF_INET6, SOCK_DGRAM | SOCK_NONBLOCK, 0);
> f.bind(('::', 0));
> addr=('::1', f.getsockname()[1]);
> g.sendto(b'', addr)
> g.sendto(b'b', addr)
> print(f.recvfrom(10, MSG_PEEK));
> print(f.recvfrom(10, MSG_PEEK));
>
> Where the expected output should be the empty string twice.
>
> Instead, make sk_peek_offset return negative values, and pass those values
> to __skb_try_recv_datagram/__skb_try_recv_from_queue. If the passed offset
> to __skb_try_recv_from_queue is negative, the checked skb is never skipped.
> After the call, the offset is set to 0 if negative to ensure all further
> calculations are correct.
>
> Also simplify the if condition in __skb_try_recv_from_queue. If _off is
> greater then 0, and off is greater then or equal to skb->len, then (_off ||
> skb->len) must always be true assuming skb->len >= 0 is always true.
>
> Also remove a redundant check around a call to sk_peek_offset in af_unix.c,
> as it double checked if MSG_PEEK was set in the flags.
>
> Signed-off-by: Matthew Dawson <matthew@mjdsystems.ca>
> ---
> include/net/sock.h | 4 +---
> net/core/datagram.c | 4 ++--
> net/ipv4/udp.c | 4 ++++
> net/ipv6/udp.c | 4 ++++
> net/unix/af_unix.c | 8 +++++---
> 5 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 7c0632c7e870..aeeec62992ca 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -507,9 +507,7 @@ int sk_set_peek_off(struct sock *sk, int val);
> static inline int sk_peek_offset(struct sock *sk, int flags)
> {
> if (unlikely(flags & MSG_PEEK)) {
> - s32 off = READ_ONCE(sk->sk_peek_off);
> - if (off >= 0)
> - return off;
> + return READ_ONCE(sk->sk_peek_off);
> }
>
> return 0;
You probably want/must also update sk_set_peek_off() to allow negative
values, elsewhere this will break as soon as the user will do
SOL_SOCKET/SO_PEEK_OFF.
I'm wondering adding an explicit SOCK_PEEK_OFF/MSG_PEEK_OFF socket flag
would help simplyifing the code: no need for negative offset; set such
flag when SOL_SOCKET/SO_PEEK_OFF with a non negative value is called
(and clear it when a negative value is used), forward such flag to
__skb_try_recv_datagram/__skb_try_recv_from_queue and use it to select
the proper peek behaviour.
Paolo
^ permalink raw reply
* RE: [net-next 11/12] igbvf: convert msleep to mdelay in atomic context
From: David Laight @ 2017-08-14 10:17 UTC (permalink / raw)
To: 'Jeff Kirsher', davem@davemloft.net
Cc: Greg Edwards, netdev@vger.kernel.org, nhorman@redhat.com,
sassmann@redhat.com, jogreene@redhat.com
In-Reply-To: <20170809214746.28139-12-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher
> Sent: 09 August 2017 22:48
> From: Greg Edwards <gedwards@ddn.com>
>
> This fixes a "scheduling while atomic" splat seen with
> CONFIG_DEBUG_ATOMIC_SLEEP enabled.
>
> Signed-off-by: Greg Edwards <gedwards@ddn.com>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> drivers/net/ethernet/intel/igbvf/vf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igbvf/vf.c b/drivers/net/ethernet/intel/igbvf/vf.c
> index 1d3aa9adcaa8..9577ccf4b26a 100644
> --- a/drivers/net/ethernet/intel/igbvf/vf.c
> +++ b/drivers/net/ethernet/intel/igbvf/vf.c
> @@ -149,7 +149,7 @@ static s32 e1000_reset_hw_vf(struct e1000_hw *hw)
> msgbuf[0] = E1000_VF_RESET;
> mbx->ops.write_posted(hw, msgbuf, 1);
>
> - msleep(10);
> + mdelay(10);
Spinning for 10ms seems somewhat sub-optimal
David
^ permalink raw reply
* Re: [PATCH net-next 2/3] Add LAN743X to Kconfig and Makefile (fwd)
From: Julia Lawall @ 2017-08-14 10:20 UTC (permalink / raw)
To: Bryan.Whitehead; +Cc: netdev, davem, UNGLinuxDriver, kbuild-all
I don't think netdev_priv can return NULL, so lines 6641 to 6646 could
just be dropped.
julia
---------- Forwarded message ----------
Date: Mon, 14 Aug 2017 18:14:12 +0800
From: kbuild test robot <fengguang.wu@intel.com>
To: kbuild@01.org
Cc: Julia Lawall <julia.lawall@lip6.fr>
Subject: Re: [PATCH net-next 2/3] Add LAN743X to Kconfig and Makefile
CC: kbuild-all@01.org
In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D264406F6049@CHN-SV-EXMX02.mchp-main.com>
TO: Bryan.Whitehead@microchip.com
CC: netdev@vger.kernel.org, davem@davemloft.net
CC: UNGLinuxDriver@microchip.com
Hi Bryan,
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Bryan-Whitehead-microchip-com/Add-LAN743X-driver/20170814-141247
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago
>> drivers/net/ethernet/microchip/lan743x.c:6642:39-45: ERROR: adapter is NULL but dereferenced.
# https://github.com/0day-ci/linux/commit/dabee19fb26fee5184b2b304b12080be0f78a3f2
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout dabee19fb26fee5184b2b304b12080be0f78a3f2
vim +6642 drivers/net/ethernet/microchip/lan743x.c
a33f5600 Bryan Whitehead 2017-08-11 6609
a33f5600 Bryan Whitehead 2017-08-11 6610 /* lan743x_pcidev_probe - Device Initialization Routine
a33f5600 Bryan Whitehead 2017-08-11 6611 * @pdev: PCI device information struct
a33f5600 Bryan Whitehead 2017-08-11 6612 * @id: entry in lan743x_pci_tbl
a33f5600 Bryan Whitehead 2017-08-11 6613 *
a33f5600 Bryan Whitehead 2017-08-11 6614 * Returns 0 on success, negative on failure
a33f5600 Bryan Whitehead 2017-08-11 6615 *
a33f5600 Bryan Whitehead 2017-08-11 6616 * initializes an adapter identified by a pci_dev structure.
a33f5600 Bryan Whitehead 2017-08-11 6617 * The OS initialization, configuring of the adapter private structure,
a33f5600 Bryan Whitehead 2017-08-11 6618 * and a hardware reset occur.
a33f5600 Bryan Whitehead 2017-08-11 6619 **/
a33f5600 Bryan Whitehead 2017-08-11 6620 static int lan743x_pcidev_probe(struct pci_dev *pdev,
a33f5600 Bryan Whitehead 2017-08-11 6621 const struct pci_device_id *id)
a33f5600 Bryan Whitehead 2017-08-11 6622 {
a33f5600 Bryan Whitehead 2017-08-11 6623 struct net_device *netdev = NULL;
a33f5600 Bryan Whitehead 2017-08-11 6624 struct lan743x_adapter *adapter = NULL;
a33f5600 Bryan Whitehead 2017-08-11 6625 int ret = -ENODEV;
a33f5600 Bryan Whitehead 2017-08-11 6626
a33f5600 Bryan Whitehead 2017-08-11 6627 NETIF_ASSERT(adapter, probe, adapter->netdev, pdev);
a33f5600 Bryan Whitehead 2017-08-11 6628
a33f5600 Bryan Whitehead 2017-08-11 6629 netdev = alloc_etherdev(sizeof(struct lan743x_adapter));
a33f5600 Bryan Whitehead 2017-08-11 6630 if (!netdev) {
a33f5600 Bryan Whitehead 2017-08-11 6631 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6632 "alloc_etherdev returned NULL");
a33f5600 Bryan Whitehead 2017-08-11 6633 ret = -ENOMEM;
a33f5600 Bryan Whitehead 2017-08-11 6634 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6635 }
a33f5600 Bryan Whitehead 2017-08-11 6636
a33f5600 Bryan Whitehead 2017-08-11 6637 strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
a33f5600 Bryan Whitehead 2017-08-11 6638 SET_NETDEV_DEV(netdev, &pdev->dev);
a33f5600 Bryan Whitehead 2017-08-11 6639 pci_set_drvdata(pdev, netdev);
a33f5600 Bryan Whitehead 2017-08-11 6640 adapter = netdev_priv(netdev);
a33f5600 Bryan Whitehead 2017-08-11 6641 if (!adapter) {
a33f5600 Bryan Whitehead 2017-08-11 @6642 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6643 "netdev_priv returned NULL");
a33f5600 Bryan Whitehead 2017-08-11 6644 ret = -ENOMEM;
a33f5600 Bryan Whitehead 2017-08-11 6645 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6646 }
a33f5600 Bryan Whitehead 2017-08-11 6647 memset(adapter, 0, sizeof(struct lan743x_adapter));
a33f5600 Bryan Whitehead 2017-08-11 6648 adapter->netdev = netdev;
a33f5600 Bryan Whitehead 2017-08-11 6649 adapter->init_flags = 0;
a33f5600 Bryan Whitehead 2017-08-11 6650 adapter->open_flags = 0;
a33f5600 Bryan Whitehead 2017-08-11 6651 adapter->msg_enable = msg_enable;
a33f5600 Bryan Whitehead 2017-08-11 6652 netdev->max_mtu = LAN743X_MAX_FRAME_SIZE;
a33f5600 Bryan Whitehead 2017-08-11 6653
a33f5600 Bryan Whitehead 2017-08-11 6654 ret = lan743x_pci_init(adapter, pdev);
a33f5600 Bryan Whitehead 2017-08-11 6655 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6656 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6657 "lan743x_pci_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6658 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6659 }
a33f5600 Bryan Whitehead 2017-08-11 6660 adapter->init_flags |= LAN743X_COMPONENT_FLAG_PCI;
a33f5600 Bryan Whitehead 2017-08-11 6661
a33f5600 Bryan Whitehead 2017-08-11 6662 ret = lan743x_csr_init(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6663 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6664 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6665 "lan743x_csr_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6666 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6667 }
a33f5600 Bryan Whitehead 2017-08-11 6668 adapter->init_flags |= LAN743X_COMPONENT_FLAG_CSR;
a33f5600 Bryan Whitehead 2017-08-11 6669
a33f5600 Bryan Whitehead 2017-08-11 6670 ret = lan743x_intr_init(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6671 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6672 NETIF_ERROR(adapter, drv, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6673 "lan743x_intr_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6674 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6675 }
a33f5600 Bryan Whitehead 2017-08-11 6676 adapter->init_flags |= LAN743X_COMPONENT_FLAG_INTR;
a33f5600 Bryan Whitehead 2017-08-11 6677
a33f5600 Bryan Whitehead 2017-08-11 6678 ret = lan743x_dp_init(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6679 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6680 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6681 "lan743x_dp_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6682 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6683 }
a33f5600 Bryan Whitehead 2017-08-11 6684 adapter->init_flags |= LAN743X_COMPONENT_FLAG_DP;
a33f5600 Bryan Whitehead 2017-08-11 6685
a33f5600 Bryan Whitehead 2017-08-11 6686 ret = lan743x_gpio_init(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6687 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6688 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6689 "lan743x_gpio_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6690 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6691 }
a33f5600 Bryan Whitehead 2017-08-11 6692 adapter->init_flags |= LAN743X_COMPONENT_FLAG_GPIO;
a33f5600 Bryan Whitehead 2017-08-11 6693
a33f5600 Bryan Whitehead 2017-08-11 6694 ret = lan743x_mac_init(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6695 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6696 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6697 "lan743x_mac_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6698 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6699 }
a33f5600 Bryan Whitehead 2017-08-11 6700 adapter->init_flags |= LAN743X_COMPONENT_FLAG_MAC;
a33f5600 Bryan Whitehead 2017-08-11 6701
a33f5600 Bryan Whitehead 2017-08-11 6702 ret = lan743x_phy_init(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6703 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6704 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6705 "lan743x_phy_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6706 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6707 }
a33f5600 Bryan Whitehead 2017-08-11 6708 adapter->init_flags |= LAN743X_COMPONENT_FLAG_PHY;
a33f5600 Bryan Whitehead 2017-08-11 6709
a33f5600 Bryan Whitehead 2017-08-11 6710 ret = lan743x_ptp_init(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6711 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6712 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6713 "lan743x_ptp_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6714 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6715 }
a33f5600 Bryan Whitehead 2017-08-11 6716 adapter->init_flags |= LAN743X_COMPONENT_FLAG_PTP;
a33f5600 Bryan Whitehead 2017-08-11 6717
a33f5600 Bryan Whitehead 2017-08-11 6718 ret = lan743x_rfe_init(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6719 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6720 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6721 "lan743x_rfe_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6722 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6723 }
a33f5600 Bryan Whitehead 2017-08-11 6724 adapter->init_flags |= LAN743X_COMPONENT_FLAG_RFE;
a33f5600 Bryan Whitehead 2017-08-11 6725
a33f5600 Bryan Whitehead 2017-08-11 6726 ret = lan743x_fct_init(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6727 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6728 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6729 "lan743x_fct_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6730 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6731 }
a33f5600 Bryan Whitehead 2017-08-11 6732 adapter->init_flags |= LAN743X_COMPONENT_FLAG_FCT;
a33f5600 Bryan Whitehead 2017-08-11 6733
a33f5600 Bryan Whitehead 2017-08-11 6734 ret = lan743x_dmac_init(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6735 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6736 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6737 "lan743x_dmac_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6738 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6739 }
a33f5600 Bryan Whitehead 2017-08-11 6740 adapter->init_flags |= LAN743X_COMPONENT_FLAG_DMAC;
a33f5600 Bryan Whitehead 2017-08-11 6741
a33f5600 Bryan Whitehead 2017-08-11 6742 ret = lan743x_rx_init(&adapter->rx[0], adapter, 0);
a33f5600 Bryan Whitehead 2017-08-11 6743 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6744 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6745 "lan743x_rx_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6746 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6747 }
a33f5600 Bryan Whitehead 2017-08-11 6748 adapter->init_flags |= LAN743X_COMPONENT_FLAG_RX(0);
a33f5600 Bryan Whitehead 2017-08-11 6749
a33f5600 Bryan Whitehead 2017-08-11 6750 ret = lan743x_tx_init(&adapter->tx[0], adapter, 0);
a33f5600 Bryan Whitehead 2017-08-11 6751 if (ret) {
a33f5600 Bryan Whitehead 2017-08-11 6752 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6753 "lan743x_tx_init failed, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6754 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6755 }
a33f5600 Bryan Whitehead 2017-08-11 6756 adapter->init_flags |= LAN743X_COMPONENT_FLAG_TX(0);
a33f5600 Bryan Whitehead 2017-08-11 6757
a33f5600 Bryan Whitehead 2017-08-11 6758 netdev->netdev_ops = &lan743x_netdev_ops;
a33f5600 Bryan Whitehead 2017-08-11 6759 netdev->ethtool_ops = &lan743x_ethtool_ops;
a33f5600 Bryan Whitehead 2017-08-11 6760 netdev->features = NETIF_F_SG | NETIF_F_TSO | NETIF_F_HW_CSUM;
a33f5600 Bryan Whitehead 2017-08-11 6761 netdev->hw_features = netdev->features;
a33f5600 Bryan Whitehead 2017-08-11 6762
a33f5600 Bryan Whitehead 2017-08-11 6763 strncpy(netdev->name, "eth%d", sizeof(netdev->name));
a33f5600 Bryan Whitehead 2017-08-11 6764 ret = register_netdev(netdev);
a33f5600 Bryan Whitehead 2017-08-11 6765 if (ret < 0) {
a33f5600 Bryan Whitehead 2017-08-11 6766 NETIF_ERROR(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6767 "failed to register net device, ret = %d", ret);
a33f5600 Bryan Whitehead 2017-08-11 6768 goto clean_up;
a33f5600 Bryan Whitehead 2017-08-11 6769 }
a33f5600 Bryan Whitehead 2017-08-11 6770 adapter->init_flags |= LAN743X_INIT_FLAG_NETDEV_REGISTERED;
a33f5600 Bryan Whitehead 2017-08-11 6771
a33f5600 Bryan Whitehead 2017-08-11 6772 NETIF_INFO(adapter, probe, adapter->netdev, "Probe succeeded");
a33f5600 Bryan Whitehead 2017-08-11 6773 ret = 0;
a33f5600 Bryan Whitehead 2017-08-11 6774
a33f5600 Bryan Whitehead 2017-08-11 6775 clean_up:
a33f5600 Bryan Whitehead 2017-08-11 6776 if (ret && adapter) {
a33f5600 Bryan Whitehead 2017-08-11 6777 NETIF_WARNING(adapter, probe, adapter->netdev,
a33f5600 Bryan Whitehead 2017-08-11 6778 "Incomplete initialization, performing clean up");
a33f5600 Bryan Whitehead 2017-08-11 6779 lan743x_device_cleanup(adapter);
a33f5600 Bryan Whitehead 2017-08-11 6780 }
a33f5600 Bryan Whitehead 2017-08-11 6781 return ret;
a33f5600 Bryan Whitehead 2017-08-11 6782 }
a33f5600 Bryan Whitehead 2017-08-11 6783
:::::: The code at line 6642 was first introduced by commit
:::::: a33f5600a21257dc396a7278c7c5ff74ac2f7844 Add LAN743X driver
:::::: TO: Bryan Whitehead <Bryan.Whitehead@microchip.com>
:::::: CC: 0day robot <fengguang.wu@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH v2] ravb: add wake-on-lan support via magic packet
From: Geert Uytterhoeven @ 2017-08-14 10:24 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Andrew Lunn, Sergei Shtylyov, netdev@vger.kernel.org,
Linux-Renesas, Ulf Hansson
In-Reply-To: <20170730195154.GE1382@bigcity.dyn.berto.se>
Hi Andrew, Niklas,
On Sun, Jul 30, 2017 at 9:51 PM, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2017-07-30 19:07:38 +0200, Andrew Lunn wrote:
>> > @@ -2041,6 +2073,11 @@ static int ravb_probe(struct platform_device *pdev)
>> >
>> > priv->chip_id = chip_id;
>> >
>> > + /* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
>> > + priv->clk = devm_clk_get(&pdev->dev, NULL);
>> > + if (IS_ERR(priv->clk))
>> > + priv->clk = NULL;
>>
>> Can you get EPROBE_DEFER returned?
>
> I don't think so, but I'm not sure :-)
>
> The clock I'm trying to get is the module clock of the ravb itself, so
> if that clock is not available (and enabled) no register writes to the
> ravb would be possible in the first place, so i guess it's safe to
> assume -EPROBE_DEFER can not happen here?
In theory, the devm_clk_get() can fail.
In practice, it cannot, as the EtherAVB device is part of a clock domain,
through its "power-domains" property in DT.
Hence if the clock (which is the module clock, provided by the SoC's system
clock controller (CPG/MSSR)) is not yet available, the PM Domain subsystem
will prevent the device from being probed.
> I'm just trying to play it safe here since the clock is only needed to
> support WoL, I though it best to not change behavior here. Try to get
> the clock, if we can great we can do WoL if not then user-space will be
> prevented from enabling WoL and nothing in the current behavior changes.
And all this explicit clock handling is done only because
device_set_wakeup_enable() does not yet prevent the PM Domain code from
powering down the device during system suspend. Once that's handled by genpd,
all clock magic can be removed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* RE: [PATCH net-next v2] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-08-14 10:35 UTC (permalink / raw)
To: Jiri Benc; +Cc: Yang, Yi Y, netdev@vger.kernel.org, dev@openvswitch.org
In-Reply-To: <20170814095117.056fdf96@griffin>
> From: Jiri Benc [mailto:jbenc@redhat.com]
> Sent: Monday, 14 August, 2017 09:51
>
> On Sun, 13 Aug 2017 21:13:57 +0000, Jan Scheurich wrote:
> > Jiri, I am not too familiar with conventions on the OVS netlink
> > interface regarding the handling of variable length fields. What is
> > the benefit of structuring the push_nsh action into
> >
> > OVS_ACTION_ATTR_PUSH_NSH
> > +-- OVS_ACTION_ATTR_NSH_BASE_HEADER
> > +-- OVS_ACTION_ATTR_NSH_MD
> >
> > instead of grouping the base header fields and the variable length MD
> > into a single monolithic attribute OVS_ACTION_ATTR_PUSH_NSH? Is the
> > main concern a potential future need to add additional parameters to
> > the push_nsh datapath action? Are there examples for such structured
> > actions other than OVS_ACTION_ATTR_CT where the need is obvious
> > because it is very polymorphic?
>
> This is about having the design clean. What would you do if you had two
> variable length fields? Would you still put them in a single structure
> and have a length field in the structure, too? That would be wrong, we
> have length in the attribute header. I doubt you would do that. Which
> indicates that putting variable length fields to an attribute with
> anything other is wrong.
>
> Also, look at how ugly the code would be. You'd have to subtract the
> base header length from the attribute length to get the variable data
> length. That's not nice at all.
>
> Think about the netlink in the way that by default, every field should
> have its own attribute. Structures are included only for performance
> reasons where certain fields are always passed in a message and having
> them in separate attributes would be impractical and waste of space.
> Going out of your way to include the context data in the structure thus
> doesn't make sense.
>
> > BTW: The name OVS_ACTION_ATTR_NSH_BASE_HEADER is misleading
> because
> > in the NSH draft the term base header is used for the first 32-bit
> > word, whereas here it includes also the 32-bit Service Path header.
>
> An excellent comment. This means that it's very well possible that
> future NSH versions may not include SP header or may have it of a
> different size. Maybe we should consider putting it into a separate
> attribute, too? Not sure it is needed, though, I don't think it's
> likely to happen.
I think the NSH RFC draft is pretty clear that the NSH header (Version 0) will always contain the Base Header, the Service Header and a (possibly empty) set of Context Headers. The length of the context headers is implied by the total NSH header Length in the Base Header. The internal structure of the context headers, implied by the "MD type" in the base header, is irrelevant for the push_nsh action as it is managed by the ofproto-dpif layer. The current NSH header format simply does not allow for a second variable length section in the header.
Is it worth to speculate on how a hypothetical future NSH version (with a different Version value in the Base header) might look like? If this should ever arise, we could introduce a new push_nsh_v2 action.
In summary, I'm still not convinced that we gain any significant generality by splitting up the OVS_ACTION_ATTR_PUSH_NSH action into nested attributes. It will only slow down the execution of the action in the datapath (or requires some optimization in the datapath to merge the attributes into a single internal action struct).
Note: For the OVS_KEY_ATTR_NSH we should split up in to OVS_KEY_ATTR_NSH_FIXED_HEADER (covering Base and Service headers) and OVS_KEY_ATTR_NSH_MD1.
Jan
^ permalink raw reply
* Re: [PATCH net-next v2] openvswitch: enable NSH support
From: Jiri Benc @ 2017-08-14 10:47 UTC (permalink / raw)
To: Jan Scheurich
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org
In-Reply-To: <CFF8EF42F1132E4CBE2BF0AB6C21C58D727373EA-hqolJogE5njKJFWPz4pdheaU1rCVNFv4@public.gmane.org>
On Mon, 14 Aug 2017 10:35:42 +0000, Jan Scheurich wrote:
> Is it worth to speculate on how a hypothetical future NSH version
> (with a different Version value in the Base header) might look like?
Absolutely. This is uAPI we're talking about and once merged, it's set
in stone. Whatever we come up with should allow future extensibility.
> If this should ever arise, we could introduce a new push_nsh_v2
> action.
Which would mean we failed with the design. We would have to maintain
two parallel APIs forever. This is not how the design should be made.
Jiri
^ permalink raw reply
* Re: [PATCH net-next v2] openvswitch: enable NSH support
From: Jan Scheurich @ 2017-08-14 11:08 UTC (permalink / raw)
To: Jiri Benc
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org
In-Reply-To: <20170814124742.7c04b72c@griffin>
> From: Jiri Benc [mailto:jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> Sent: Monday, 14 August, 2017 12:48
>
> On Mon, 14 Aug 2017 10:35:42 +0000, Jan Scheurich wrote:
> > Is it worth to speculate on how a hypothetical future NSH version
> > (with a different Version value in the Base header) might look like?
>
> Absolutely. This is uAPI we're talking about and once merged, it's set
> in stone. Whatever we come up with should allow future extensibility.
>
> > If this should ever arise, we could introduce a new push_nsh_v2
> > action.
>
> Which would mean we failed with the design. We would have to maintain
> two parallel APIs forever. This is not how the design should be made.
Well, if that is the ultimate design goal, we'll have no choice.
But if the hypothetic next NSH version looks entirely different, we will have to manage the incompatibility anyhow on the level of the nested attributes. So the only thing we will for sure manage to keep fixed might be the empty wrapper OVS_ACTION_ATTR_PUSH_NSH...
We decided earlier on to go for dedicated push/pop_<Header> actions in the datapath instead of a single pair of (very polymorphic) generic encap/decap datapath actions to make the implementation in the datapath more explicit and straightforward. Why not follow the same philosophy also when we need to support push/pop for incompatible versions of the same protocol?
Jan
^ permalink raw reply
* [PATCH net] udp: consistently apply ufo or fragmentation
From: Vasily Averin @ 2017-08-14 11:31 UTC (permalink / raw)
To: Linux Kernel Network Developers, Willem de Bruijn,
David S. Miller, Andrey Konovalov
Dear Willem,
ip*_ufo_append_data() is called now for each skb_is_gso() packet.
Before the patch it was called for UDP packets only.
Could you please clarify, is it correct?
Thank you,
Vasily Averin
^ permalink raw reply
* Re: [PATCH 3/4] net: sh_eth: constify platform_device_id
From: Sergei Shtylyov @ 2017-08-14 11:41 UTC (permalink / raw)
To: Arvind Yadav, wg, mkl, madalin.bucur, davem, grygorii.strashko
Cc: linux-kernel, netdev, linux-renesas-soc
In-Reply-To: <1502622762-11275-1-git-send-email-arvind.yadav.cs@gmail.com>
Hello!
On 08/13/2017 02:12 PM, Arvind Yadav wrote:
> platform_device_id are not supposed to change at runtime. All functions
> working with platform_device_id provided by <linux/platform_device.h>
> work with const platform_device_id. So mark the non-const structs as
> const.
>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
though it seems too late as DaveM has taken the series...
[...]
MBR, Sergei
^ permalink raw reply
* Re: [RFC PATCH 2/2] bpf: Initialise mod[] in bpf_trace_printk
From: James Hogan @ 2017-08-14 12:25 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: David Miller, ast, linux-kernel, rostedt, mingo, netdev
In-Reply-To: <598DDF88.6020809@iogearbox.net>
[-- Attachment #1: Type: text/plain, Size: 3209 bytes --]
On Fri, Aug 11, 2017 at 06:47:04PM +0200, Daniel Borkmann wrote:
> Hi James,
>
> On 08/09/2017 10:34 PM, Daniel Borkmann wrote:
> > On 08/09/2017 09:39 AM, James Hogan wrote:
> > [...]
> >> time (but please consider looking at the other patch which is certainly
> >> a more real issue).
> >
> > Sorry for the delay, started looking into that and whether we
> > have some other options, I'll get back to you on this.
>
> Could we solve this more generically (as in: originally intended) in
> the sense that we don't need to trust the gcc va_list handling; I feel
> this is relying on an implementation detail? Perhaps something like
> below poc patch?
Well it works on MIPS32 and MIPS64 with tracex5.
Tested-by: James Hogan <james.hogan@imgtec.com>
Cheers
James
>
> Thanks again,
> Daniel
>
> From 71f16544d455abb6bb82f7253c17c14d2a395e91 Mon Sep 17 00:00:00 2001
> Message-Id: <71f16544d455abb6bb82f7253c17c14d2a395e91.1502469361.git.daniel@iogearbox.net>
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Fri, 11 Aug 2017 15:56:32 +0200
> Subject: [PATCH] bpf: fix bpf_trace_printk on 32 bit
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 3738519..d4cb36f 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -204,10 +204,33 @@ static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
> fmt_cnt++;
> }
>
> - return __trace_printk(1/* fake ip will not be printed */, fmt,
> - mod[0] == 2 ? arg1 : mod[0] == 1 ? (long) arg1 : (u32) arg1,
> - mod[1] == 2 ? arg2 : mod[1] == 1 ? (long) arg2 : (u32) arg2,
> - mod[2] == 2 ? arg3 : mod[2] == 1 ? (long) arg3 : (u32) arg3);
> +#define __BPF_TP_EMIT() __BPF_ARG3_TP()
> +#define __BPF_TP(...) \
> + __trace_printk(1 /* fake ip will not be printed */, \
> + fmt, ##__VA_ARGS__)
> +
> +#define __BPF_ARG1_TP(...) \
> + ((mod[0] == 2 || (mod[0] == 1 && __BITS_PER_LONG == 64)) \
> + ? __BPF_TP(arg1, ##__VA_ARGS__) \
> + : ((mod[0] == 1 || (mod[0] == 0 && __BITS_PER_LONG == 32)) \
> + ? __BPF_TP((long)arg1, ##__VA_ARGS__) \
> + : __BPF_TP((u32)arg1, ##__VA_ARGS__)))
> +
> +#define __BPF_ARG2_TP(...) \
> + ((mod[1] == 2 || (mod[1] == 1 && __BITS_PER_LONG == 64)) \
> + ? __BPF_ARG1_TP(arg2, ##__VA_ARGS__) \
> + : ((mod[1] == 1 || (mod[1] == 0 && __BITS_PER_LONG == 32)) \
> + ? __BPF_ARG1_TP((long)arg2, ##__VA_ARGS__) \
> + : __BPF_ARG1_TP((u32)arg2, ##__VA_ARGS__)))
> +
> +#define __BPF_ARG3_TP(...) \
> + ((mod[2] == 2 || (mod[2] == 1 && __BITS_PER_LONG == 64)) \
> + ? __BPF_ARG2_TP(arg3, ##__VA_ARGS__) \
> + : ((mod[2] == 1 || (mod[2] == 0 && __BITS_PER_LONG == 32)) \
> + ? __BPF_ARG2_TP((long)arg3, ##__VA_ARGS__) \
> + : __BPF_ARG2_TP((u32)arg3, ##__VA_ARGS__)))
> +
> + return __BPF_TP_EMIT();
> }
>
> static const struct bpf_func_proto bpf_trace_printk_proto = {
> --
> 1.9.3
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* RE: [RFC PATCH 2/2] bpf: Initialise mod[] in bpf_trace_printk
From: David Laight @ 2017-08-14 12:44 UTC (permalink / raw)
To: 'Daniel Borkmann', James Hogan, David Miller
Cc: ast@kernel.org, linux-kernel@vger.kernel.org, rostedt@goodmis.org,
mingo@redhat.com, netdev@vger.kernel.org
In-Reply-To: <598DDF88.6020809@iogearbox.net>
From: Daniel Borkmann
> Sent: 11 August 2017 17:47
> On 08/09/2017 10:34 PM, Daniel Borkmann wrote:
> > On 08/09/2017 09:39 AM, James Hogan wrote:
> > [...]
> >> time (but please consider looking at the other patch which is certainly
> >> a more real issue).
> >
> > Sorry for the delay, started looking into that and whether we
> > have some other options, I'll get back to you on this.
>
> Could we solve this more generically (as in: originally intended) in
> the sense that we don't need to trust the gcc va_list handling; I feel
> this is relying on an implementation detail? Perhaps something like
> below poc patch?
That patch still has 'cond ? arg : cond1 ? (long)arg : (u32)arg' so
probably has the same warning as the original version.
The va_list handling is defined by the relevant ABI, not gcc.
It is ok on x86-64 because all 32bit values are extended to 64bits
before being passed as arguments (either in registers, or on the stack).
Nothing in the C language requires that, so other 64bit architectures
could pass 32bit values in 4 bytes of stack.
David
^ 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