* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Eric Dumazet @ 2018-09-18 23:56 UTC (permalink / raw)
To: Florian Fainelli, ahs3, Lendacky, Thomas, David Miller
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
isubramanian@apm.com, kchudgar@apm.com, qnguyen@apm.com
In-Reply-To: <c32598b4-3f80-f83f-9b86-1c168752d1e2@gmail.com>
On 09/18/2018 04:27 PM, Eric Dumazet wrote:
>
> I remember one of the napi_complete_done() change had to be reverted,
> for some obscure reason.
That was not exactly a revert, :
commit 129c6cda2de2a8ac44fab096152469999b727faf
Author: Eric Dumazet <edumazet@google.com>
Date: Mon Sep 18 13:03:43 2017 -0700
8139too: revisit napi_complete_done() usage
It seems we have to be more careful in napi_complete_done()
use. This patch is not a revert, as it seems we can
avoid bug that Ville reported by moving the napi_complete_done()
test in the spinlock section.
Many thanks to Ville for detective work and all tests.
Fixes: 617f01211baf ("8139too: use napi_complete_done()")
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH net-next v5 00/20] WireGuard: Secure Network Tunnel
From: Ard Biesheuvel @ 2018-09-18 18:28 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Linux Kernel Mailing List, <netdev@vger.kernel.org>,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, David S. Miller,
Greg Kroah-Hartman
In-Reply-To: <20180918161646.19105-1-Jason@zx2c4.com>
On 18 September 2018 at 09:16, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> v5 has the most comprehensive set of changes yet, and I think should
> finally address all of the relevant issues brought up on the mailing
> list. In particular, this feedback has come from:
>
> - Andy Lutomirski
> - Eric Biggers
> - Ard Biesheuvel
> - Kevin Easton
> - Andrew Lunn
> - Martin Willi
>
> Changes v4->v5:
> ---------------
> - Use fewer inlines, except when measured as necessary.
> - Reduce size of scattergather array to fit within stack on
> small systems.
> - Account for larger stack frames with KASAN.
> - The x86_64 implementations are selected according to input length.
> - Avoid using simd for small blocks on x86_64.
> - The simd_get/put API is now pass by reference, so that the user
> can lazily use the context based on whether or not it's needed.
> See the description again in the first commit for this.
> - Add cycle counts for different sizes for x86_64 commit messages.
> - Relax simd during chapoly sg loop.
> - Replace -include with #if defined(...)
> - Saner and simpler Kconfig.
> - Split into separate modules instead of one monolithic zinc.
> - The combination of these three last items means that there no
> longer are any conditionals in our Makefile.
> - Martin showed a performance regression using tcrypt in v4. This
> has been triaged and fixed, and now the Zinc code runs faster
> than the previous code.
> - While I initially wasn't going to do this for the initial
> patchset, it was just so simple to do: now there's a nosimd
> module parameter that can be used to disable simd instructions
> for debugging and testing, or on weird systems.
>
I was going to respond in the other thread but it is probably better
to move the discussion here.
My concern about the monolithic nature of each algo module is not only
about SIMD, and it has nothing to do with weird systems. It has to do
with micro-architectural differences which are more common on ARM than
on other architectures *, I suppose. But generalizing from that, it
has to do with policy which is currently owned by userland and not by
the kernel. This will also be important for choosing between the time
variant but less safe table based scalar AES and the much slower time
invariant version (which is substantially slower, especially on
decryption) once we move AES into this library.
So a command line option for the kernel is not the solution here. If
we can't have separate modules, could we at least have per-module
options that put the policy decisions back into userland?
* as an example, the SHA256 NEON code I collaborated on with Andy
Polyakov 2 years ago is significantly faster on some cores and not on
others
> -----------------------------------------------------------
>
> This patchset is available on git.kernel.org in this branch, where it may be
> pulled directly for inclusion into net-next:
>
> * https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/linux.git/log/?h=jd/wireguard
>
> -----------------------------------------------------------
>
> WireGuard is a secure network tunnel written especially for Linux, which
> has faced around three years of serious development, deployment, and
> scrutiny. It delivers excellent performance and is extremely easy to
> use and configure. It has been designed with the primary goal of being
> both easy to audit by virtue of being small and highly secure from a
> cryptography and systems security perspective. WireGuard is used by some
> massive companies pushing enormous amounts of traffic, and likely
> already today you've consumed bytes that at some point transited through
> a WireGuard tunnel. Even as an out-of-tree module, WireGuard has been
> integrated into various userspace tools, Linux distributions, mobile
> phones, and data centers. There are ports in several languages to
> several operating systems, and even commercial hardware and services
> sold integrating WireGuard. It is time, therefore, for WireGuard to be
> properly integrated into Linux.
>
> Ample information, including documentation, installation instructions,
> and project details, is available at:
>
> * https://www.wireguard.com/
> * https://www.wireguard.com/papers/wireguard.pdf
>
> As it is currently an out-of-tree module, it lives in its own git repo
> and has its own mailing list, and every commit for the module is tested
> against every stable kernel since 3.10 on a variety of architectures
> using an extensive test suite:
>
> * https://git.zx2c4.com/WireGuard
> https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/WireGuard.git/
> * https://lists.zx2c4.com/mailman/listinfo/wireguard
> * https://www.wireguard.com/build-status/
>
> The project has been broadly discussed at conferences, and was presented
> to the Netdev developers in Seoul last November, where a paper was
> released detailing some interesting aspects of the project. Dave asked
> me after the talk if I would consider sending in a v1 "sooner rather
> than later", hence this patchset. A decision is still waiting from the
> Linux Plumbers Conference, but an update on these topics may be presented
> in Vancouver in a few months. Prior presentations:
>
> * https://www.wireguard.com/presentations/
> * https://www.wireguard.com/papers/wireguard-netdev22.pdf
>
> The cryptography in the protocol itself has been formally verified by
> several independent academic teams with positive results, and I know of
> two additional efforts on their way to further corroborate those
> findings. The version 1 protocol is "complete", and so the purpose of
> this review is to assess the implementation of the protocol. However, it
> still may be of interest to know that the thing you're reviewing uses a
> protocol with various nice security properties:
>
> * https://www.wireguard.com/formal-verification/
>
> This patchset is divided into four segments. The first introduces a very
> simple helper for working with the FPU state for the purposes of amortizing
> SIMD operations. The second segment is a small collection of cryptographic
> primitives, split up into several commits by primitive and by hardware. The
> third shows usage of Zinc within the existing crypto API and as a replacement
> to the existing crypto API. The last is WireGuard itself, presented as an
> unintrusive and self-contained virtual network driver.
>
> It is intended that this entire patch series enter the kernel through
> DaveM's net-next tree. Subsequently, WireGuard patches will go through
> DaveM's net-next tree, while Zinc patches will go through Greg KH's tree.
>
> Enjoy,
> Jason
^ permalink raw reply
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Florian Fainelli @ 2018-09-19 0:03 UTC (permalink / raw)
To: Eric Dumazet, ahs3, Lendacky, Thomas, David Miller
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
isubramanian@apm.com, kchudgar@apm.com, qnguyen@apm.com
In-Reply-To: <29258606-8255-847c-b22f-8793999ce6fe@gmail.com>
On 09/18/2018 04:56 PM, Eric Dumazet wrote:
>
>
> On 09/18/2018 04:27 PM, Eric Dumazet wrote:
>>
>
>> I remember one of the napi_complete_done() change had to be reverted,
>> for some obscure reason.
>
>
>
> That was not exactly a revert, :
This is what I have so far for the drivers that both use
napi_complete_done() without checking the return value and implement a
ndo_poll_controller() callback:
https://github.com/ffainelli/linux/commits/napi-check
>
> commit 129c6cda2de2a8ac44fab096152469999b727faf
> Author: Eric Dumazet <edumazet@google.com>
> Date: Mon Sep 18 13:03:43 2017 -0700
>
> 8139too: revisit napi_complete_done() usage
>
> It seems we have to be more careful in napi_complete_done()
> use. This patch is not a revert, as it seems we can
> avoid bug that Ville reported by moving the napi_complete_done()
> test in the spinlock section.
>
> Many thanks to Ville for detective work and all tests.
>
> Fixes: 617f01211baf ("8139too: use napi_complete_done()")
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
>
>
>
>
>
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] cxgb4vf: Add ethtool private flags for changing force_link_up
From: Jakub Kicinski @ 2018-09-18 18:39 UTC (permalink / raw)
To: Arjun Vynipadath
Cc: netdev, davem, dt, nirranjan, indranil, Casey Leedom,
Ganesh Goudar
In-Reply-To: <1537276043-20666-1-git-send-email-arjun@chelsio.com>
On Tue, 18 Sep 2018 18:37:23 +0530, Arjun Vynipadath wrote:
> Forcing link up of virtual interfaces even when physical link is down
> causes packet drops and ping failures during bonding failover. Hence
> adding a ethtool private flag to toggle force_link_up whenever required.
>
> Signed-off-by: Arjun Vynipadath <arjun@chelsio.com>
> Signed-off-by: Casey Leedom <leedom@chelsio.com>
> Signed-off-by: Ganesh Goudar <ganeshgr@chelsio.com>
Could you describe how this mechanism relates to the existing
ndo_set_vf_link_state, which you seem to not make use of:
$ git grep ndo_set_vf_link_state -- drivers/net/ethernet/chelsio/
$
I understand you're configuring the setting from the VF side, but the
question, as always, is: why ;)
^ permalink raw reply
* Re: [PATCH net-next v5 09/20] zinc: Poly1305 ARM and ARM64 implementations
From: Jason A. Donenfeld @ 2018-09-19 0:17 UTC (permalink / raw)
To: Eric Biggers
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson, Andy Polyakov, Russell King - ARM Linux,
linux-arm-kernel
In-Reply-To: <20180918225552.GA74746@gmail.com>
Hi Eric,
On Wed, Sep 19, 2018 at 12:55 AM Eric Biggers <ebiggers@kernel.org> wrote:
> This will compute the wrong digest if called with simd_context=HAVE_FULL_SIMD
> and then later with simd_context=HAVE_NO_SIMD, since poly1305_blocks_neon()
> converts the accumulator from base 32 to base 26, whereas poly1305_blocks_arm()
> assumes it is still in base 32. Is that intentional? I'm sure this is a rare
> case, but my understanding is that the existing crypto API doesn't preclude
> calling successive steps in different contexts. And I'm concerned that it could
> be relevant in some cases, e.g. especially if people are importing a hash state
> that was exported earlier. Handling it by silently computing the wrong digest
> is not a great idea...
Indeed you're right; Samuel and I were just discussing that recently.
I'd rather handle this correctly even if the contexts change, so I'll
see if I can fix this up properly for that unlikely case in the next
revision.
Jason
^ permalink raw reply
* Re: [PATCH net-next v5 12/20] zinc: BLAKE2s generic C implementation and selftest
From: Jason A. Donenfeld @ 2018-09-19 0:45 UTC (permalink / raw)
To: Eric Biggers
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <20180919004132.GB74746@gmail.com>
Hey Eric,
On Wed, Sep 19, 2018 at 2:41 AM Eric Biggers <ebiggers@kernel.org> wrote:
> This buffer is 4 times too long.
Nice catch.
> Or how about something much simpler:
>
> static inline void blake2s_final(struct blake2s_state *state, u8 *out,
> const size_t outlen)
> {
> #ifdef DEBUG
> BUG_ON(!out || !outlen || outlen > BLAKE2S_OUTBYTES);
> #endif
> __blake2s_final(state);
>
> cpu_to_le32_array(state->h, ARRAY_SIZE(state->h));
> memcpy(out, state->h, outlen);
>
> memzero_explicit(state, sizeof(*state));
> }
Oh, that's excellent, thanks. Much better than prior. I'll do exactly that.
Jason
^ permalink raw reply
* [PATCH] net: phy: don't reschedule state machine when PHY is halted
From: Heiner Kallweit @ 2018-09-18 19:12 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
I think I've seen a similar or same patch before, not sure why it
didn't make it yet. When being in state PHY_HALTED we don't have to
reschedule the state machine, phy_start() will start it again.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1ee25877c..c78203b25 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1123,7 +1123,7 @@ void phy_state_machine(struct work_struct *work)
* PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
* between states from phy_mac_interrupt()
*/
- if (phy_polling_mode(phydev))
+ if (phy_polling_mode(phydev) && old_state != PHY_HALTED)
queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,
PHY_STATE_TIME * HZ);
}
--
2.19.0
^ permalink raw reply related
* Re: [PATCH net-next v5 07/20] zinc: Poly1305 generic C implementations and selftest
From: Eric Biggers @ 2018-09-19 0:50 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: linux-kernel, netdev, linux-crypto, davem, gregkh, Samuel Neves,
Andy Lutomirski, Jean-Philippe Aumasson
In-Reply-To: <20180918161646.19105-8-Jason@zx2c4.com>
On Tue, Sep 18, 2018 at 06:16:33PM +0200, Jason A. Donenfeld wrote:
> diff --git a/lib/zinc/poly1305/poly1305.c b/lib/zinc/poly1305/poly1305.c
> new file mode 100644
> index 000000000000..dbab82f33aa7
> --- /dev/null
> +++ b/lib/zinc/poly1305/poly1305.c
> @@ -0,0 +1,155 @@
> +/* SPDX-License-Identifier: MIT
> + *
> + * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + *
> + * Implementation of the Poly1305 message authenticator.
> + *
> + * Information: https://cr.yp.to/mac.html
> + */
> +
> +#include <zinc/poly1305.h>
> +
> +#include <asm/unaligned.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +
> +#ifndef HAVE_POLY1305_ARCH_IMPLEMENTATION
> +static inline bool poly1305_init_arch(void *ctx,
> + const u8 key[POLY1305_KEY_SIZE])
> +{
> + return false;
> +}
> +static inline bool poly1305_blocks_arch(void *ctx, const u8 *input,
> + const size_t len, const u32 padbit,
> + simd_context_t *simd_context)
> +{
> + return false;
> +}
> +static inline bool poly1305_emit_arch(void *ctx, u8 mac[POLY1305_MAC_SIZE],
> + const u32 nonce[4],
> + simd_context_t *simd_context)
> +{
> + return false;
> +}
> +void __init poly1305_fpu_init(void)
> +{
> +}
> +#endif
> +
> +#if defined(CONFIG_ARCH_SUPPORTS_INT128) && defined(__SIZEOF_INT128__)
> +#include "poly1305-donna64.h"
> +#else
> +#include "poly1305-donna32.h"
> +#endif
> +
> +void poly1305_init(struct poly1305_ctx *ctx, const u8 key[POLY1305_KEY_SIZE])
> +{
> + ctx->nonce[0] = get_unaligned_le32(&key[16]);
> + ctx->nonce[1] = get_unaligned_le32(&key[20]);
> + ctx->nonce[2] = get_unaligned_le32(&key[24]);
> + ctx->nonce[3] = get_unaligned_le32(&key[28]);
> +
> + if (!poly1305_init_arch(ctx->opaque, key))
> + poly1305_init_generic(ctx->opaque, key);
> +
> + ctx->num = 0;
> +}
> +EXPORT_SYMBOL(poly1305_init);
> +
> +static inline void poly1305_blocks(void *ctx, const u8 *input, const size_t len,
> + const u32 padbit,
> + simd_context_t *simd_context)
> +{
> + if (!poly1305_blocks_arch(ctx, input, len, padbit, simd_context))
> + poly1305_blocks_generic(ctx, input, len, padbit);
> +}
> +
> +static inline void poly1305_emit(void *ctx, u8 mac[POLY1305_KEY_SIZE],
> + const u32 nonce[4],
> + simd_context_t *simd_context)
> +{
> + if (!poly1305_emit_arch(ctx, mac, nonce, simd_context))
> + poly1305_emit_generic(ctx, mac, nonce);
> +}
> +
> +void poly1305_update(struct poly1305_ctx *ctx, const u8 *input, size_t len,
> + simd_context_t *simd_context)
> +{
> + const size_t num = ctx->num % POLY1305_BLOCK_SIZE;
> + size_t rem;
0 <= ctx->num < POLY1305_BLOCK_SIZE, so no need to mod by POLY1305_BLOCK_SIZE.
> +
> + if (num) {
> + rem = POLY1305_BLOCK_SIZE - num;
> + if (len < rem) {
> + memcpy(ctx->data + num, input, len);
> + ctx->num = num + len;
> + return;
> + }
> + memcpy(ctx->data + num, input, rem);
> + poly1305_blocks(ctx->opaque, ctx->data, POLY1305_BLOCK_SIZE, 1,
> + simd_context);
> + input += rem;
> + len -= rem;
> + }
> +
> + rem = len % POLY1305_BLOCK_SIZE;
> + len -= rem;
> +
> + if (len >= POLY1305_BLOCK_SIZE) {
> + poly1305_blocks(ctx->opaque, input, len, 1, simd_context);
> + input += len;
> + }
> +
> + if (rem)
> + memcpy(ctx->data, input, rem);
> +
> + ctx->num = rem;
> +}
> +EXPORT_SYMBOL(poly1305_update);
> +
> +void poly1305_final(struct poly1305_ctx *ctx, u8 mac[POLY1305_MAC_SIZE],
> + simd_context_t *simd_context)
> +{
> + size_t num = ctx->num % POLY1305_BLOCK_SIZE;
Same here.
> +++ b/lib/zinc/selftest/poly1305.h
> @@ -0,0 +1,875 @@
> +/* SPDX-License-Identifier: MIT
> + *
> + * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + */
> +
> +#ifdef DEBUG
> +struct poly1305_testvec {
> + u8 input[600];
> + u8 output[POLY1305_MAC_SIZE];
> + u8 key[POLY1305_KEY_SIZE];
> + size_t ilen;
> +};
> +
> +static const struct poly1305_testvec poly1305_testvecs[] __initconst = {
> +{ /* RFC7539 */
> + .input = { 0x43, 0x72, 0x79, 0x70, 0x74, 0x6f, 0x67, 0x72,
> + 0x61, 0x70, 0x68, 0x69, 0x63, 0x20, 0x46, 0x6f,
> + 0x72, 0x75, 0x6d, 0x20, 0x52, 0x65, 0x73, 0x65,
> + 0x61, 0x72, 0x63, 0x68, 0x20, 0x47, 0x72, 0x6f,
> + 0x75, 0x70 },
> + .ilen = 34,
> + .output = { 0xa8, 0x06, 0x1d, 0xc1, 0x30, 0x51, 0x36, 0xc6,
> + 0xc2, 0x2b, 0x8b, 0xaf, 0x0c, 0x01, 0x27, 0xa9 },
> + .key = { 0x85, 0xd6, 0xbe, 0x78, 0x57, 0x55, 0x6d, 0x33,
> + 0x7f, 0x44, 0x52, 0xfe, 0x42, 0xd5, 0x06, 0xa8,
> + 0x01, 0x03, 0x80, 0x8a, 0xfb, 0x0d, 0xb2, 0xfd,
> + 0x4a, 0xbf, 0xf6, 0xaf, 0x41, 0x49, 0xf5, 0x1b },
> +}, { /* "The Poly1305-AES message-authentication code" */
Hardcoding the 'input' array to 600 bytes forces the full amount of space to be
reserved in the kernel image for every test vector. Also, if anyone adds a
longer test vector they will need to remember to increase the value.
It should be a const pointer instead, like the test vectors in crypto/testmgr.h.
- Eric
^ permalink raw reply
* Re: [PATCH net-next v5 03/20] zinc: ChaCha20 generic C implementation and selftest
From: Eric Biggers @ 2018-09-19 1:08 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: linux-kernel, netdev, linux-crypto, davem, gregkh, Samuel Neves,
Andy Lutomirski, Jean-Philippe Aumasson
In-Reply-To: <20180918161646.19105-4-Jason@zx2c4.com>
On Tue, Sep 18, 2018 at 06:16:29PM +0200, Jason A. Donenfeld wrote:
> diff --git a/lib/zinc/chacha20/chacha20.c b/lib/zinc/chacha20/chacha20.c
> new file mode 100644
> index 000000000000..3f00e1edd4c8
> --- /dev/null
> +++ b/lib/zinc/chacha20/chacha20.c
> @@ -0,0 +1,193 @@
> +/* SPDX-License-Identifier: MIT
> + *
> + * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + *
> + * Implementation of the ChaCha20 stream cipher.
> + *
> + * Information: https://cr.yp.to/chacha.html
> + */
> +
> +#include <zinc/chacha20.h>
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <crypto/algapi.h>
> +
> +#ifndef HAVE_CHACHA20_ARCH_IMPLEMENTATION
> +void __init chacha20_fpu_init(void)
> +{
> +}
> +static inline bool chacha20_arch(u8 *out, const u8 *in, const size_t len,
> + const u32 key[8], const u32 counter[4],
> + simd_context_t *simd_context)
> +{
> + return false;
> +}
> +static inline bool hchacha20_arch(u8 *derived_key, const u8 *nonce,
> + const u8 *key, simd_context_t *simd_context)
> +{
> + return false;
> +}
> +#endif
> +
> +#define EXPAND_32_BYTE_K 0x61707865U, 0x3320646eU, 0x79622d32U, 0x6b206574U
> +
> +#define QUARTER_ROUND(x, a, b, c, d) ( \
> + x[a] += x[b], \
> + x[d] = rol32((x[d] ^ x[a]), 16), \
> + x[c] += x[d], \
> + x[b] = rol32((x[b] ^ x[c]), 12), \
> + x[a] += x[b], \
> + x[d] = rol32((x[d] ^ x[a]), 8), \
> + x[c] += x[d], \
> + x[b] = rol32((x[b] ^ x[c]), 7) \
> +)
> +
> +#define C(i, j) (i * 4 + j)
> +
> +#define DOUBLE_ROUND(x) ( \
> + /* Column Round */ \
> + QUARTER_ROUND(x, C(0, 0), C(1, 0), C(2, 0), C(3, 0)), \
> + QUARTER_ROUND(x, C(0, 1), C(1, 1), C(2, 1), C(3, 1)), \
> + QUARTER_ROUND(x, C(0, 2), C(1, 2), C(2, 2), C(3, 2)), \
> + QUARTER_ROUND(x, C(0, 3), C(1, 3), C(2, 3), C(3, 3)), \
> + /* Diagonal Round */ \
> + QUARTER_ROUND(x, C(0, 0), C(1, 1), C(2, 2), C(3, 3)), \
> + QUARTER_ROUND(x, C(0, 1), C(1, 2), C(2, 3), C(3, 0)), \
> + QUARTER_ROUND(x, C(0, 2), C(1, 3), C(2, 0), C(3, 1)), \
> + QUARTER_ROUND(x, C(0, 3), C(1, 0), C(2, 1), C(3, 2)) \
> +)
> +
> +#define TWENTY_ROUNDS(x) ( \
> + DOUBLE_ROUND(x), \
> + DOUBLE_ROUND(x), \
> + DOUBLE_ROUND(x), \
> + DOUBLE_ROUND(x), \
> + DOUBLE_ROUND(x), \
> + DOUBLE_ROUND(x), \
> + DOUBLE_ROUND(x), \
> + DOUBLE_ROUND(x), \
> + DOUBLE_ROUND(x), \
> + DOUBLE_ROUND(x) \
> +)
Does this consistently perform as well as an implementation that organizes the
operations such that the quarterrounds for all columns/diagonals are
interleaved? As-is, there are tight dependencies in QUARTER_ROUND() (as well as
in the existing chacha20_block() in lib/chacha20.c, for that matter), so we're
heavily depending on the compiler to do the needed interleaving so as to not get
potentially disastrous performance. Making it explicit could be a good idea.
> +
> +static void chacha20_block_generic(__le32 *stream, u32 *state)
> +{
> + u32 x[CHACHA20_BLOCK_WORDS];
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(x); ++i)
> + x[i] = state[i];
> +
> + TWENTY_ROUNDS(x);
> +
> + for (i = 0; i < ARRAY_SIZE(x); ++i)
> + stream[i] = cpu_to_le32(x[i] + state[i]);
> +
> + ++state[12];
> +}
> +
> +static void chacha20_generic(u8 *out, const u8 *in, u32 len, const u32 key[8],
> + const u32 counter[4])
> +{
> + __le32 buf[CHACHA20_BLOCK_WORDS];
> + u32 x[] = {
> + EXPAND_32_BYTE_K,
> + key[0], key[1], key[2], key[3],
> + key[4], key[5], key[6], key[7],
> + counter[0], counter[1], counter[2], counter[3]
> + };
> +
> + if (out != in)
> + memmove(out, in, len);
> +
> + while (len >= CHACHA20_BLOCK_SIZE) {
> + chacha20_block_generic(buf, x);
> + crypto_xor(out, (u8 *)buf, CHACHA20_BLOCK_SIZE);
> + len -= CHACHA20_BLOCK_SIZE;
> + out += CHACHA20_BLOCK_SIZE;
> + }
> + if (len) {
> + chacha20_block_generic(buf, x);
> + crypto_xor(out, (u8 *)buf, len);
> + }
> +}
If crypto_xor_cpy() is used instead of crypto_xor(), and 'in' is incremented
along with 'out', then the memmove() is not needed.
- Eric
^ permalink raw reply
* [PATCH net-next 0/2] net: phy: make phy_stop() synchronous
From: Heiner Kallweit @ 2018-09-18 19:54 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller
Cc: netdev@vger.kernel.org, Geert Uytterhoeven
There have been few not that successful attempts in the past to make
phy_stop() a synchronous call instead of just changing the state.
Patch 1 of this series addresses an issue which prevented this change.
At least for me it works fine now. Would appreciate if Geert could
re-test as well that suspend doesn't throw an error.
Heiner Kallweit (2):
net: linkwatch: add check for netdevice being present to linkwatch_do_dev
net: phy: call state machine synchronously in phy_stop
drivers/net/phy/phy.c | 2 ++
net/core/link_watch.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
--
2.19.0
^ permalink raw reply
* [PATCH net-next 1/2] net: linkwatch: add check for netdevice being present to linkwatch_do_dev
From: Heiner Kallweit @ 2018-09-18 19:55 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller
Cc: netdev@vger.kernel.org, Geert Uytterhoeven
In-Reply-To: <fc18a6b5-5022-ac2b-9e68-584b4c28bb71@gmail.com>
When bringing down the netdevice (incl. detaching it) and calling
netif_carrier_off directly or indirectly the latter triggers an
asynchronous linkwatch event.
This linkwatch event eventually may fail to access chip registers in
the ndo_get_stats/ndo_get_stats64 callback because the device isn't
accessible any longer, see call trace in [0].
To prevent this scenario don't check for IFF_UP only, but also make
sure that the netdevice is present.
[0] https://lists.openwall.net/netdev/2018/03/15/62
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
net/core/link_watch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index e38e641e9..7f51efb2b 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -155,7 +155,7 @@ static void linkwatch_do_dev(struct net_device *dev)
clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
rfc2863_policy(dev);
- if (dev->flags & IFF_UP) {
+ if (dev->flags & IFF_UP && netif_device_present(dev)) {
if (netif_carrier_ok(dev))
dev_activate(dev);
else
--
2.19.0
^ permalink raw reply related
* [PATCH net-next 2/2] net: phy: call state machine synchronously in phy_stop
From: Heiner Kallweit @ 2018-09-18 19:56 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller
Cc: netdev@vger.kernel.org, Geert Uytterhoeven
In-Reply-To: <fc18a6b5-5022-ac2b-9e68-584b4c28bb71@gmail.com>
phy_stop() may be called e.g. when suspending, therefore all needed
actions should be performed synchronously. Therefore add a synchronous
call to the state machine.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c78203b25..f5bb6a7a8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -861,6 +861,8 @@ void phy_stop(struct phy_device *phydev)
out_unlock:
mutex_unlock(&phydev->lock);
+ phy_state_machine(&phydev->state_queue.work);
+
/* Cannot call flush_scheduled_work() here as desired because
* of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
* will not reenable interrupts.
--
2.19.0
^ permalink raw reply related
* Re: [PATCH net-next v5 07/20] zinc: Poly1305 generic C implementations and selftest
From: Jason A. Donenfeld @ 2018-09-19 1:35 UTC (permalink / raw)
To: Eric Biggers
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <20180919005054.GC74746@gmail.com>
On Wed, Sep 19, 2018 at 2:50 AM Eric Biggers <ebiggers@kernel.org> wrote:
> Hardcoding the 'input' array to 600 bytes forces the full amount of space to be
> reserved in the kernel image for every test vector. Also, if anyone adds a
> longer test vector they will need to remember to increase the value.
>
> It should be a const pointer instead, like the test vectors in crypto/testmgr.h.
I know. The agony. This has been really annoying me. I originally did
it the right way, but removed it last week, when I noticed that gcc
failed to put it in the initconst section:
https://git.zx2c4.com/WireGuard/commit/?id=f4698d20f13946afc6ce99e98685ba3f9adc4474
Even changing the (u8[]){ ... } into a (const u8[]){ ... } or even
into a const string literal does not do the trick. It makes it into
the constant data section with const, but it does not make it into the
initconst section. What a bummer.
I went asking about this on the gcc mailing list, to see if there was
just some aspect of C that I had overlooked:
https://gcc.gnu.org/ml/gcc/2018-09/msg00043.html So far, it looks like
we're SOL. I could probably make some macros to do this in a .S file
but... that's pretty unacceptably gross. Or I could define lots and
lots of variables in __initconst, and then connect them all together
at the end, but that's pretty gross too. Or I could have all this data
in one variable and record offsets into it, but that's even more
atrocious.
So I think it comes down to these two non-ugly options:
- We use fixed sized buffers, waste a lot of space, and be happy that
it's cleared from memory immediately after init/insertion anyway, so
it's not actually wasting ram.
- We use const string literals / constant compound literals, save
space on disk, but not benefit from having it cleared from memory
after init/insertion.
Of these, which would you prefer? I can see the argument both ways,
but in the end opted for the first. Or perhaps you have a better third
option?
Jason
^ permalink raw reply
* Re: [PATCH] net: phy: don't reschedule state machine when PHY is halted
From: Florian Fainelli @ 2018-09-18 20:02 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <d32d4101-03cb-df57-66d2-29e9a17fc2c5@gmail.com>
On 09/18/2018 12:12 PM, Heiner Kallweit wrote:
> I think I've seen a similar or same patch before, not sure why it
> didn't make it yet. When being in state PHY_HALTED we don't have to
> reschedule the state machine, phy_start() will start it again.
Yes, this is conceptually the same patch as as this one:
https://patchwork.ozlabs.org/patch/830415/
I prefer your version, though the comment in the original patch explains
why.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/phy/phy.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 1ee25877c..c78203b25 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1123,7 +1123,7 @@ void phy_state_machine(struct work_struct *work)
> * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
> * between states from phy_mac_interrupt()
> */
> - if (phy_polling_mode(phydev))
> + if (phy_polling_mode(phydev) && old_state != PHY_HALTED)
> queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,
> PHY_STATE_TIME * HZ);
> }
>
--
Florian
^ permalink raw reply
* Re: [PATCH net-next v5 07/20] zinc: Poly1305 generic C implementations and selftest
From: Jason A. Donenfeld @ 2018-09-19 1:39 UTC (permalink / raw)
To: Eric Biggers
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <20180919005054.GC74746@gmail.com>
> > + const size_t num = ctx->num % POLY1305_BLOCK_SIZE;
> 0 <= ctx->num < POLY1305_BLOCK_SIZE, so no need to mod by POLY1305_BLOCK_SIZE.
> > + size_t num = ctx->num % POLY1305_BLOCK_SIZE;
> Same here.
I know, but I was having a hard time convincing gcc-8 of that
invariant, and it was warning me. Perhaps this is something they
fixed, though, between 8.1 and 8.2 though. I'll check back and adjust
accordingly.
^ permalink raw reply
* Re: [PATCH net-next v5 07/20] zinc: Poly1305 generic C implementations and selftest
From: Jason A. Donenfeld @ 2018-09-19 1:41 UTC (permalink / raw)
To: Eric Biggers
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <CAHmME9rab92gc68tGX384eGR71q+7JtovpxBLSYrYNHA7L5zFw@mail.gmail.com>
On Wed, Sep 19, 2018 at 3:39 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> > > + const size_t num = ctx->num % POLY1305_BLOCK_SIZE;
> > 0 <= ctx->num < POLY1305_BLOCK_SIZE, so no need to mod by POLY1305_BLOCK_SIZE.
> > > + size_t num = ctx->num % POLY1305_BLOCK_SIZE;
> > Same here.
>
> I know, but I was having a hard time convincing gcc-8 of that
> invariant, and it was warning me. Perhaps this is something they
> fixed, though, between 8.1 and 8.2 though. I'll check back and adjust
> accordingly.
This was changed here:
https://git.zx2c4.com/WireGuard/commit/?id=37f114a73ba37219b00a66f0a51219a696599745
I can't reproduce with 8.2 anymore, so perhaps I should remove it now.
Unless you'd like to avoid a warning on old compilers. Since there's
no difference in speed, probably we should avoid the 8.1 warning and
leave it be?
^ permalink raw reply
* Re: [PATCH] net: phy: don't reschedule state machine when PHY is halted
From: Heiner Kallweit @ 2018-09-18 20:17 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <faeb4977-10e9-96aa-ce70-7fc637d117cb@gmail.com>
On 18.09.2018 22:02, Florian Fainelli wrote:
> On 09/18/2018 12:12 PM, Heiner Kallweit wrote:
>> I think I've seen a similar or same patch before, not sure why it
>> didn't make it yet. When being in state PHY_HALTED we don't have to
>> reschedule the state machine, phy_start() will start it again.
>
> Yes, this is conceptually the same patch as as this one:
>
> https://patchwork.ozlabs.org/patch/830415/
>
Thanks for the link, this is what I was referring to.
> I prefer your version, though the comment in the original patch explains
> why.
>
To be sure I understand you correctly:
You're fine with the patch as is or would you prefer to add a comment
like in the original patch ?
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/phy/phy.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 1ee25877c..c78203b25 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -1123,7 +1123,7 @@ void phy_state_machine(struct work_struct *work)
>> * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
>> * between states from phy_mac_interrupt()
>> */
>> - if (phy_polling_mode(phydev))
>> + if (phy_polling_mode(phydev) && old_state != PHY_HALTED)
>> queue_delayed_work(system_power_efficient_wq, &phydev->state_queue,
>> PHY_STATE_TIME * HZ);
>> }
>>
>
>
^ permalink raw reply
* Re: [Patch net-next] ipv4: initialize ra_mutex in inet_init_net()
From: Cong Wang @ 2018-09-18 20:17 UTC (permalink / raw)
To: Kirill Tkhai; +Cc: Linux Kernel Network Developers
In-Reply-To: <be6bbcbd-0817-691a-78b8-74b9b5b0a8b0@virtuozzo.com>
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.
^ permalink raw reply
* [PATCH bpf-next] flow_dissector: fix build failure without CONFIG_NET
From: Willem de Bruijn @ 2018-09-18 20:20 UTC (permalink / raw)
To: netdev; +Cc: ast, daniel, rdunlap, Willem de Bruijn
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>
---
include/linux/bpf_types.h | 2 +-
include/linux/skbuff.h | 13 +++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 22083712dd18..c9bd6fb765b0 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -16,6 +16,7 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_LWT_SEG6LOCAL, lwt_seg6local)
BPF_PROG_TYPE(BPF_PROG_TYPE_SOCK_OPS, sock_ops)
BPF_PROG_TYPE(BPF_PROG_TYPE_SK_SKB, sk_skb)
BPF_PROG_TYPE(BPF_PROG_TYPE_SK_MSG, sk_msg)
+BPF_PROG_TYPE(BPF_PROG_TYPE_FLOW_DISSECTOR, flow_dissector)
#endif
#ifdef CONFIG_BPF_EVENTS
BPF_PROG_TYPE(BPF_PROG_TYPE_KPROBE, kprobe)
@@ -32,7 +33,6 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_LIRC_MODE2, lirc_mode2)
#ifdef CONFIG_INET
BPF_PROG_TYPE(BPF_PROG_TYPE_SK_REUSEPORT, sk_reuseport)
#endif
-BPF_PROG_TYPE(BPF_PROG_TYPE_FLOW_DISSECTOR, flow_dissector)
BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_ARRAY, percpu_array_map_ops)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ce0e863f02a2..76be85ea392a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1194,10 +1194,23 @@ void skb_flow_dissector_init(struct flow_dissector *flow_dissector,
const struct flow_dissector_key *key,
unsigned int key_count);
+#ifdef CONFIG_NET
int skb_flow_dissector_bpf_prog_attach(const union bpf_attr *attr,
struct bpf_prog *prog);
int skb_flow_dissector_bpf_prog_detach(const union bpf_attr *attr);
+#else
+static inline int skb_flow_dissector_bpf_prog_attach(const union bpf_attr *attr,
+ struct bpf_prog *prog)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int skb_flow_dissector_bpf_prog_detach(const union bpf_attr *attr)
+{
+ return -EOPNOTSUPP;
+}
+#endif
bool __skb_flow_dissect(const struct sk_buff *skb,
struct flow_dissector *flow_dissector,
--
2.19.0.397.gdd90340f6a-goog
^ permalink raw reply related
* Re: [PATCH net-next v5 20/20] net: WireGuard secure network tunnel
From: Jason A. Donenfeld @ 2018-09-19 2:04 UTC (permalink / raw)
To: Andrew Lunn
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman
In-Reply-To: <20180918233411.GB17466@lunn.ch>
Hi Andrew,
On Wed, Sep 19, 2018 at 1:34 AM Andrew Lunn <andrew@lunn.ch> wrote:
> I see this BUG_ON() is still here. It really needs to be removed. It
> does not look like you need to crash the kernel here. Can you add in a
> test of len >= 128, do a WARN and then return. I think you then leak
> some memory, but i would much prefer that to a crashed machine.
Sure, I'll change it to that.
Regards,
Jason
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Song Liu @ 2018-09-18 20:37 UTC (permalink / raw)
To: Eric Dumazet
Cc: Alexei Starovoitov, netdev, Jeff Kirsher, Alexander Duyck,
michael.chan@broadcom.com, Kernel Team
In-Reply-To: <CANn89iJTEMjYepKbr-8pmk0i03d9D+CfDFLPx+J=fqZivDJ9zQ@mail.gmail.com>
> On Sep 18, 2018, at 11:17 AM, Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 18, 2018 at 10:51 AM Alexei Starovoitov <ast@fb.com> wrote:
>>
>> On 9/18/18 6:45 AM, Eric Dumazet wrote:
>>> On Tue, Sep 18, 2018 at 1:41 AM Song Liu <songliubraving@fb.com> wrote:
>>>>
>>>> We are debugging this issue that netconsole message triggers pegged softirq
>>>> (ksoftirqd taking 100% CPU for many seconds). We found this issue in
>>>> production with both bnxt and ixgbe, on a 4.11 based kernel. This is easily
>>>> reproducible with ixgbe on 4.11, and latest net/net-next (see [1] for more
>>>> detail).
>>>>
>>>> After debugging for some time, we found that this issue is likely related
>>>> to 39e6c8208d7b ("net: solve a NAPI race"). After reverting this commit,
>>>> the steps described in [1] cannot reproduce the issue on ixgbe. Reverting
>>>> this commit also reduces the chances we hit the issue with bnxt (it still
>>>> happens with a lower rate).
>>>>
>>>> I tried to fix this issue with relaxed variant (or older version) of
>>>> napi_schedule_prep() in netpoll, just like the one on napi_watchdog().
>>>> However, my tests do not always go as expected.
>>>>
>>>> Please share your comments/suggestions on which direction shall we try
>>>> to fix this.
>>>>
>>>> Thanks in advance!
>>>> Song
>>>>
>>>>
>>>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_netdev_msg522328.html&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=i6WobKxbeG3slzHSIOxTVtYIJw7qjCE6S0spDTKL-J4&m=iSaOapj1kxjhGYLgQr0Qd8mQCzVdobmgT1L4JwFvzxs&s=lCEhrz6wQJUUaJOkxFmtOszAgkf3Jh4reX_i1GbI5RI&e=
>>>
>>> You have not traced ixgbe to understand why driver hits
>>> "clean_complete=false" all the time ?
>>
>> Eric,
>>
>> I'm looking at commit 39e6c8208d7b and wondering that it's doing
>> clear_bit(NAPI_STATE_MISSED,..);
>> for busy_poll_stop(), but not for netpoll.
>> Can that be an issue?
>>
>> and then something like below is needed:
>> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
>> index 57557a6a950c..a848be6b503c 100644
>> --- a/net/core/netpoll.c
>> +++ b/net/core/netpoll.c
>> @@ -172,6 +172,7 @@ static void poll_one_napi(struct napi_struct *napi)
>> trace_napi_poll(napi, work, 0);
>>
>> clear_bit(NAPI_STATE_NPSVC, &napi->state);
>> + clear_bit(NAPI_STATE_MISSED, &napi->state);
>> }
>
>
> NAPI_STATE_MISSED should only be cleared under strict circumstances.
>
> The clear in busy_poll_stop() is an optimization really (as explained
> in the comment)
>
> It is cleared when napi_complete_done() is eventually called, but if
> ixgbe always handle 64 RX frames in its poll function,
> napi_complete_done() will not be called. The bug is in ixgbe,
> pretending its poll function should be called forever.
Looks like a patch like the following fixes the issue for ixgbe. But I
cannot explain it yet.
Does this ring a bell?
Thanks,
Song
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 787c84fb20dd..51611f799dae 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3059,11 +3059,14 @@ static irqreturn_t ixgbe_msix_other(int irq, void *data)
static irqreturn_t ixgbe_msix_clean_rings(int irq, void *data)
{
struct ixgbe_q_vector *q_vector = data;
+ struct napi_struct *napi = &q_vector->napi;
/* EIAM disabled interrupts (on this vector) for us */
- if (q_vector->rx.ring || q_vector->tx.ring)
- napi_schedule_irqoff(&q_vector->napi);
+ if ((q_vector->rx.ring || q_vector->tx.ring) &&
+ !napi_disable_pending(napi) &&
+ !test_and_set_bit(NAPI_STATE_SCHED, &napi->state))
+ __napi_schedule_irqoff(napi);
return IRQ_HANDLED;
}
^ permalink raw reply related
* Re: [PATCH net-next v5 04/20] zinc: ChaCha20 x86_64 implementation
From: Jason A. Donenfeld @ 2018-09-19 2:14 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson, Andy Polyakov, mingo, X86 ML
In-Reply-To: <alpine.DEB.2.21.1809190017050.1468@nanos.tec.linutronix.de>
Hi Thomas,
On Wed, Sep 19, 2018 at 12:30 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> I'm a bit confused by this SOB chain. So above you write that it's from
> Andy Polakovs implementation and Samuel did the changes. But here it seems
> you are the main author. If Samuel just did some modifications then you
> want to use the Co-developed-by tag along with his SOB.
Thanks, I'll use that tag.
>
> Also I'd recommend to add a Originally-by or Based-on-code-from: Andy
> Polyakov tag. Both are not formal tags but widely in use for attributions.
Great idea.
>
> > +++ b/lib/zinc/chacha20/chacha20-x86_64-glue.h
> > @@ -0,0 +1,100 @@
> > +/* SPDX-License-Identifier: MIT
>
> Please put that into a separate one liner comment. Also this should be
> 'GPL-2.0[+] or MIT' I think.
I had that originally, but changed it to just MIT, since MIT is a
subset of GPL-2.0. And looking at tree repo, it appears this is what
others do too.
Jason
^ permalink raw reply
* Re: [bpf-next, v4 1/5] flow_dissector: implements flow dissector BPF hook
From: Willem de Bruijn @ 2018-09-18 20:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: Petar Penkov, Network Development, David Miller,
Alexei Starovoitov, Daniel Borkmann, simon.horman, ecree,
songliubraving, Tom Herbert, Petar Penkov, Willem de Bruijn
In-Reply-To: <57de8ffd-e832-27b8-dede-f9ef76306470@gmail.com>
On Tue, Sep 18, 2018 at 2:09 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 09/14/2018 07:46 AM, Petar Penkov wrote:
> > From: Petar Penkov <ppenkov@google.com>
> >
> > Adds a hook for programs of type BPF_PROG_TYPE_FLOW_DISSECTOR and
> > attach type BPF_FLOW_DISSECTOR that is executed in the flow dissector
> > path. The BPF program is per-network namespace
>
> ...
>
> >
> > + rcu_read_lock();
> > + attached = skb ? rcu_dereference(dev_net(skb->dev)->flow_dissector_prog)
> > + : NULL;
>
>
> Some skbs have a NULL skb->dev, so we are going to crash here.
Ai. Thanks, Eric.
For these cases, we can probably infer the netns from sk->sk_net. That
is assuming that all skbs have at least one of skb->sk or skb->dev
set.
^ permalink raw reply
* [PATCH net 0/2] ipv6: fix issues on accessing fib6_metrics
From: Wei Wang @ 2018-09-18 20:44 UTC (permalink / raw)
To: David Miller, netdev
Cc: Eric Dumazet, David Ahern, Cong Wang, Sabrina Dubroca, Wei Wang
From: Wei Wang <weiwan@google.com>
The latest fix on the memory leak of fib6_metrics still causes
use-after-free.
This patch series first revert the previous fix and propose a new fix
that is more inline with ipv4 logic and is tested to fix the
use-after-free issue reported.
Wei Wang (2):
Revert "ipv6: fix double refcount of fib6_metrics"
ipv6: fix memory leak on dst->_metrics
net/ipv6/route.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
--
2.19.0.397.gdd90340f6a-goog
^ permalink raw reply
* [PATCH net 1/2] Revert "ipv6: fix double refcount of fib6_metrics"
From: Wei Wang @ 2018-09-18 20:44 UTC (permalink / raw)
To: David Miller, netdev
Cc: Eric Dumazet, David Ahern, Cong Wang, Sabrina Dubroca, Wei Wang
In-Reply-To: <20180918204500.106240-1-tracywwnj@gmail.com>
From: Wei Wang <weiwan@google.com>
This reverts commit e70a3aad44cc8b24986687ffc98c4a4f6ecf25ea.
This change causes use-after-free on dst->_metrics.
The crash trace looks like this:
[ 97.763269] BUG: KASAN: use-after-free in ip6_mtu+0x116/0x140
[ 97.769038] Read of size 4 at addr ffff881781d2cf84 by task svw_NetThreadEv/8801
[ 97.777954] CPU: 76 PID: 8801 Comm: svw_NetThreadEv Not tainted 4.15.0-smp-DEV #11
[ 97.777956] Hardware name: Default string Default string/Indus_QC_02, BIOS 5.46.4 03/29/2018
[ 97.777957] Call Trace:
[ 97.777971] [<ffffffff895709db>] dump_stack+0x4d/0x72
[ 97.777985] [<ffffffff881651df>] print_address_description+0x6f/0x260
[ 97.777997] [<ffffffff88165747>] kasan_report+0x257/0x370
[ 97.778001] [<ffffffff894488e6>] ? ip6_mtu+0x116/0x140
[ 97.778004] [<ffffffff881658b9>] __asan_report_load4_noabort+0x19/0x20
[ 97.778008] [<ffffffff894488e6>] ip6_mtu+0x116/0x140
[ 97.778013] [<ffffffff892bb91e>] tcp_current_mss+0x12e/0x280
[ 97.778016] [<ffffffff892bb7f0>] ? tcp_mtu_to_mss+0x2d0/0x2d0
[ 97.778022] [<ffffffff887b45b8>] ? depot_save_stack+0x138/0x4a0
[ 97.778037] [<ffffffff87c38985>] ? __mmdrop+0x145/0x1f0
[ 97.778040] [<ffffffff881643b1>] ? save_stack+0xb1/0xd0
[ 97.778046] [<ffffffff89264c82>] tcp_send_mss+0x22/0x220
[ 97.778059] [<ffffffff89273a49>] tcp_sendmsg_locked+0x4f9/0x39f0
[ 97.778062] [<ffffffff881642b4>] ? kasan_check_write+0x14/0x20
[ 97.778066] [<ffffffff89273550>] ? tcp_sendpage+0x60/0x60
[ 97.778070] [<ffffffff881cb359>] ? rw_copy_check_uvector+0x69/0x280
[ 97.778075] [<ffffffff8873c65f>] ? import_iovec+0x9f/0x430
[ 97.778078] [<ffffffff88164be7>] ? kasan_slab_free+0x87/0xc0
[ 97.778082] [<ffffffff8873c5c0>] ? memzero_page+0x140/0x140
[ 97.778085] [<ffffffff881642b4>] ? kasan_check_write+0x14/0x20
[ 97.778088] [<ffffffff89276f6c>] tcp_sendmsg+0x2c/0x50
[ 97.778092] [<ffffffff89276f6c>] ? tcp_sendmsg+0x2c/0x50
[ 97.778098] [<ffffffff89352d43>] inet_sendmsg+0x103/0x480
[ 97.778102] [<ffffffff89352c40>] ? inet_gso_segment+0x15b0/0x15b0
[ 97.778105] [<ffffffff890294da>] sock_sendmsg+0xba/0xf0
[ 97.778108] [<ffffffff8902ab6a>] ___sys_sendmsg+0x6ca/0x8e0
[ 97.778113] [<ffffffff87dccac1>] ? hrtimer_try_to_cancel+0x71/0x3b0
[ 97.778116] [<ffffffff8902a4a0>] ? copy_msghdr_from_user+0x3d0/0x3d0
[ 97.778119] [<ffffffff881646d1>] ? memset+0x31/0x40
[ 97.778123] [<ffffffff87a0cff5>] ? schedule_hrtimeout_range_clock+0x165/0x380
[ 97.778127] [<ffffffff87a0ce90>] ? hrtimer_nanosleep_restart+0x250/0x250
[ 97.778130] [<ffffffff87dcc700>] ? __hrtimer_init+0x180/0x180
[ 97.778133] [<ffffffff87dd1f82>] ? ktime_get_ts64+0x172/0x200
[ 97.778137] [<ffffffff8822b8ec>] ? __fget_light+0x8c/0x2f0
[ 97.778141] [<ffffffff8902d5c6>] __sys_sendmsg+0xe6/0x190
[ 97.778144] [<ffffffff8902d5c6>] ? __sys_sendmsg+0xe6/0x190
[ 97.778147] [<ffffffff8902d4e0>] ? SyS_shutdown+0x20/0x20
[ 97.778152] [<ffffffff87cd4370>] ? wake_up_q+0xe0/0xe0
[ 97.778155] [<ffffffff8902d670>] ? __sys_sendmsg+0x190/0x190
[ 97.778158] [<ffffffff8902d683>] SyS_sendmsg+0x13/0x20
[ 97.778162] [<ffffffff87a1600c>] do_syscall_64+0x2ac/0x430
[ 97.778166] [<ffffffff87c17515>] ? do_page_fault+0x35/0x3d0
[ 97.778171] [<ffffffff8960131f>] ? page_fault+0x2f/0x50
[ 97.778174] [<ffffffff89600071>] entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[ 97.778177] RIP: 0033:0x7f83fa36000d
[ 97.778178] RSP: 002b:00007f83ef9229e0 EFLAGS: 00000293 ORIG_RAX: 000000000000002e
[ 97.778180] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f83fa36000d
[ 97.778182] RDX: 0000000000004000 RSI: 00007f83ef922f00 RDI: 0000000000000036
[ 97.778183] RBP: 00007f83ef923040 R08: 00007f83ef9231f8 R09: 00007f83ef923168
[ 97.778184] R10: 0000000000000000 R11: 0000000000000293 R12: 00007f83f69c5b40
[ 97.778185] R13: 000000000000001c R14: 0000000000000001 R15: 0000000000004000
[ 97.779684] Allocated by task 5919:
[ 97.783185] save_stack+0x46/0xd0
[ 97.783187] kasan_kmalloc+0xad/0xe0
[ 97.783189] kmem_cache_alloc_trace+0xdf/0x580
[ 97.783190] ip6_convert_metrics.isra.79+0x7e/0x190
[ 97.783192] ip6_route_info_create+0x60a/0x2480
[ 97.783193] ip6_route_add+0x1d/0x80
[ 97.783195] inet6_rtm_newroute+0xdd/0xf0
[ 97.783198] rtnetlink_rcv_msg+0x641/0xb10
[ 97.783200] netlink_rcv_skb+0x27b/0x3e0
[ 97.783202] rtnetlink_rcv+0x15/0x20
[ 97.783203] netlink_unicast+0x4be/0x720
[ 97.783204] netlink_sendmsg+0x7bc/0xbf0
[ 97.783205] sock_sendmsg+0xba/0xf0
[ 97.783207] ___sys_sendmsg+0x6ca/0x8e0
[ 97.783208] __sys_sendmsg+0xe6/0x190
[ 97.783209] SyS_sendmsg+0x13/0x20
[ 97.783211] do_syscall_64+0x2ac/0x430
[ 97.783213] entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[ 97.784709] Freed by task 0:
[ 97.785056] knetbase: Error: /proc/sys/net/core/txcs_enable does not exist
[ 97.794497] save_stack+0x46/0xd0
[ 97.794499] kasan_slab_free+0x71/0xc0
[ 97.794500] kfree+0x7c/0xf0
[ 97.794501] fib6_info_destroy_rcu+0x24f/0x310
[ 97.794504] rcu_process_callbacks+0x38b/0x1730
[ 97.794506] __do_softirq+0x1c8/0x5d0
Reported-by: John Sperbeck <jsperbeck@google.com>
Signed-off-by: Wei Wang <weiwan@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv6/route.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 480a79f47c52..b5d3e6b294ab 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -976,6 +976,10 @@ static void rt6_set_from(struct rt6_info *rt, struct fib6_info *from)
rt->rt6i_flags &= ~RTF_EXPIRES;
rcu_assign_pointer(rt->from, from);
dst_init_metrics(&rt->dst, from->fib6_metrics->metrics, true);
+ if (from->fib6_metrics != &dst_default_metrics) {
+ rt->dst._metrics |= DST_METRICS_REFCOUNTED;
+ refcount_inc(&from->fib6_metrics->refcnt);
+ }
}
/* Caller must already hold reference to @ort */
--
2.19.0.397.gdd90340f6a-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox