* Re: [PATCH net-next v5 09/20] zinc: Poly1305 ARM and ARM64 implementations
From: Eric Biggers @ 2018-09-18 22:55 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: linux-kernel, netdev, linux-crypto, davem, gregkh, Samuel Neves,
Andy Lutomirski, Jean-Philippe Aumasson, Andy Polyakov,
Russell King, linux-arm-kernel
In-Reply-To: <20180918161646.19105-10-Jason@zx2c4.com>
On Tue, Sep 18, 2018 at 06:16:35PM +0200, Jason A. Donenfeld wrote:
> diff --git a/lib/zinc/poly1305/poly1305-arm-glue.h b/lib/zinc/poly1305/poly1305-arm-glue.h
> new file mode 100644
> index 000000000000..dd3fa5a38c62
> --- /dev/null
> +++ b/lib/zinc/poly1305/poly1305-arm-glue.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: MIT
> + *
> + * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + */
> +
> +#include <asm/hwcap.h>
> +#include <asm/neon.h>
> +
> +asmlinkage void poly1305_init_arm(void *ctx, const u8 key[16]);
> +asmlinkage void poly1305_blocks_arm(void *ctx, const u8 *inp, const size_t len,
> + const u32 padbit);
> +asmlinkage void poly1305_emit_arm(void *ctx, u8 mac[16], const u32 nonce[4]);
> +#if IS_ENABLED(CONFIG_KERNEL_MODE_NEON) && \
> + (defined(CONFIG_64BIT) || __LINUX_ARM_ARCH__ >= 7)
> +#define ARM_USE_NEON
> +asmlinkage void poly1305_blocks_neon(void *ctx, const u8 *inp, const size_t len,
> + const u32 padbit);
> +asmlinkage void poly1305_emit_neon(void *ctx, u8 mac[16], const u32 nonce[4]);
> +#endif
> +
> +static bool poly1305_use_neon __ro_after_init;
> +
> +static void __init poly1305_fpu_init(void)
> +{
> +#if defined(CONFIG_ARM64)
> + poly1305_use_neon = elf_hwcap & HWCAP_ASIMD;
> +#elif defined(CONFIG_ARM)
> + poly1305_use_neon = elf_hwcap & HWCAP_NEON;
> +#endif
> +}
> +
> +static inline bool poly1305_init_arch(void *ctx,
> + const u8 key[POLY1305_KEY_SIZE])
> +{
> + poly1305_init_arm(ctx, key);
> + return true;
> +}
> +
> +static inline bool poly1305_blocks_arch(void *ctx, const u8 *inp,
> + const size_t len, const u32 padbit,
> + simd_context_t *simd_context)
> +{
> +#if defined(ARM_USE_NEON)
> + if (poly1305_use_neon && simd_use(simd_context)) {
> + poly1305_blocks_neon(ctx, inp, len, padbit);
> + return true;
> + }
> +#endif
> + poly1305_blocks_arm(ctx, inp, len, padbit);
> + return true;
> +}
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...
- Eric
^ permalink raw reply
* Re: [PATCH bpf-next 2/2] xsk: fix bug when trying to use both copy and zero-copy on one queue id
From: Y Song @ 2018-09-18 17:22 UTC (permalink / raw)
To: magnus.karlsson
Cc: Björn Töpel, Alexei Starovoitov, Daniel Borkmann,
netdev
In-Reply-To: <1537265538-5882-3-git-send-email-magnus.karlsson@intel.com>
On Tue, Sep 18, 2018 at 3:13 AM Magnus Karlsson
<magnus.karlsson@intel.com> wrote:
>
> Previously, the xsk code did not record which umem was bound to a
> specific queue id. This was not required if all drivers were zero-copy
> enabled as this had to be recorded in the driver anyway. So if a user
> tried to bind two umems to the same queue, the driver would say
> no. But if copy-mode was first enabled and then zero-copy mode (or the
> reverse order), we mistakenly enabled both of them on the same umem
> leading to buggy behavior. The main culprit for this is that we did
> not store the association of umem to queue id in the copy case and
> only relied on the driver reporting this. As this relation was not
> stored in the driver for copy mode (it does not rely on the AF_XDP
> NDOs), this obviously could not work.
>
> This patch fixes the problem by always recording the umem to queue id
> relationship in the netdev_queue and netdev_rx_queue structs. This way
> we always know what kind of umem has been bound to a queue id and can
> act appropriately at bind time.
>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
> net/xdp/xdp_umem.c | 87 +++++++++++++++++++++++++++++++++++++++++++-----------
> net/xdp/xdp_umem.h | 2 +-
> 2 files changed, 71 insertions(+), 18 deletions(-)
>
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index b3b632c..12300b5 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -42,6 +42,41 @@ void xdp_del_sk_umem(struct xdp_umem *umem, struct xdp_sock *xs)
> }
> }
>
> +/* The umem is stored both in the _rx struct and the _tx struct as we do
> + * not know if the device has more tx queues than rx, or the opposite.
> + * This might also change during run time.
> + */
> +static void xdp_reg_umem_at_qid(struct net_device *dev, struct xdp_umem *umem,
> + u16 queue_id)
> +{
> + if (queue_id < dev->real_num_rx_queues)
> + dev->_rx[queue_id].umem = umem;
> + if (queue_id < dev->real_num_tx_queues)
> + dev->_tx[queue_id].umem = umem;
> +}
> +
> +static struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev,
> + u16 queue_id)
> +{
> + if (queue_id < dev->real_num_rx_queues)
> + return dev->_rx[queue_id].umem;
> + if (queue_id < dev->real_num_tx_queues)
> + return dev->_tx[queue_id].umem;
> +
> + return NULL;
> +}
> +
> +static void xdp_clear_umem_at_qid(struct net_device *dev, u16 queue_id)
> +{
> + /* Zero out the entry independent on how many queues are configured
> + * at this point in time, as it might be used in the future.
> + */
> + if (queue_id < dev->num_rx_queues)
> + dev->_rx[queue_id].umem = NULL;
> + if (queue_id < dev->num_tx_queues)
> + dev->_tx[queue_id].umem = NULL;
> +}
> +
I am sure whether the following scenario can happen or not.
Could you clarify?
1. suppose initially we have num_rx_queues = num_tx_queues = 10
xdp_reg_umem_at_qid() set umem1 to queue_id = 8
2. num_tx_queues is changed to 5
3. xdp_clear_umem_at_qid() is called for queue_id = 8,
and dev->_rx[8].umum = 0.
4. xdp_reg_umem_at_qid() is called gain to set for queue_id = 8
dev->_rx[8].umem = umem2
5. num_tx_queues is changed to 10
Now dev->_rx[8].umem != dev->_tx[8].umem, is this possible and is it
a problem?
> int xdp_umem_query(struct net_device *dev, u16 queue_id)
> {
> struct netdev_bpf bpf;
> @@ -58,11 +93,11 @@ int xdp_umem_query(struct net_device *dev, u16 queue_id)
> }
>
> int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> - u32 queue_id, u16 flags)
> + u16 queue_id, u16 flags)
> {
> bool force_zc, force_copy;
> struct netdev_bpf bpf;
> - int err;
> + int err = 0;
>
> force_zc = flags & XDP_ZEROCOPY;
> force_copy = flags & XDP_COPY;
> @@ -70,18 +105,28 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> if (force_zc && force_copy)
> return -EINVAL;
>
> + rtnl_lock();
> + if (xdp_get_umem_from_qid(dev, queue_id)) {
> + err = -EBUSY;
> + goto rtnl_unlock;
> + }
> +
> + xdp_reg_umem_at_qid(dev, umem, queue_id);
> + umem->dev = dev;
> + umem->queue_id = queue_id;
> if (force_copy)
> - return 0;
> + /* For copy-mode, we are done. */
> + goto rtnl_unlock;
>
> - if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_async_xmit)
> - return force_zc ? -EOPNOTSUPP : 0; /* fail or fallback */
> + if (!dev->netdev_ops->ndo_bpf ||
> + !dev->netdev_ops->ndo_xsk_async_xmit) {
> + err = -EOPNOTSUPP;
> + goto err_unreg_umem;
> + }
>
> - rtnl_lock();
> err = xdp_umem_query(dev, queue_id);
> - if (err) {
> - err = err < 0 ? -EOPNOTSUPP : -EBUSY;
> - goto err_rtnl_unlock;
> - }
> + if (err)
> + goto err_unreg_umem;
>
> bpf.command = XDP_SETUP_XSK_UMEM;
> bpf.xsk.umem = umem;
> @@ -89,18 +134,20 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
>
> err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> if (err)
> - goto err_rtnl_unlock;
> + goto err_unreg_umem;
> rtnl_unlock();
>
> dev_hold(dev);
> - umem->dev = dev;
> - umem->queue_id = queue_id;
> umem->zc = true;
> return 0;
>
> -err_rtnl_unlock:
> +err_unreg_umem:
> + xdp_clear_umem_at_qid(dev, queue_id);
You did not clear umem->dev and umem->queue_id,is a problem here?
For example in xdp_umem_clear_dev(), umem->dev is checked.
> + if (!force_zc)
> + err = 0; /* fallback to copy mode */
> +rtnl_unlock:
> rtnl_unlock();
> - return force_zc ? err : 0; /* fail or fallback */
> + return err;
> }
>
> static void xdp_umem_clear_dev(struct xdp_umem *umem)
> @@ -108,7 +155,7 @@ static void xdp_umem_clear_dev(struct xdp_umem *umem)
> struct netdev_bpf bpf;
> int err;
>
> - if (umem->dev) {
> + if (umem->zc) {
> bpf.command = XDP_SETUP_XSK_UMEM;
> bpf.xsk.umem = NULL;
> bpf.xsk.queue_id = umem->queue_id;
> @@ -121,7 +168,13 @@ static void xdp_umem_clear_dev(struct xdp_umem *umem)
> WARN(1, "failed to disable umem!\n");
>
> dev_put(umem->dev);
> - umem->dev = NULL;
> + umem->zc = false;
> + }
> +
> + if (umem->dev) {
> + rtnl_lock();
> + xdp_clear_umem_at_qid(umem->dev, umem->queue_id);
> + rtnl_unlock();
Previously, umem->dev is reset to NULL. Now, it is left as is. I
assume it is not possible
that this function xdp_umem_clear_dev() is called again for this umem, right?
> }
> }
>
> diff --git a/net/xdp/xdp_umem.h b/net/xdp/xdp_umem.h
> index c8be1ad..2760322 100644
> --- a/net/xdp/xdp_umem.h
> +++ b/net/xdp/xdp_umem.h
> @@ -9,7 +9,7 @@
> #include <net/xdp_sock.h>
>
> int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> - u32 queue_id, u16 flags);
> + u16 queue_id, u16 flags);
> bool xdp_umem_validate_queues(struct xdp_umem *umem);
> void xdp_get_umem(struct xdp_umem *umem);
> void xdp_put_umem(struct xdp_umem *umem);
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH bpf] tools: bpf: fix license for a compat header file
From: Y Song @ 2018-09-18 17:24 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, oss-drivers
In-Reply-To: <20180918171359.18750-1-jakub.kicinski@netronome.com>
On Tue, Sep 18, 2018 at 10:15 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> libc_compat.h is used by libbpf so make sure it's licensed under
> LGPL or BSD license. The license change should be OK, I'm the only
> author of the file.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Lendacky, Thomas @ 2018-09-18 23:09 UTC (permalink / raw)
To: David Miller, ahs3@redhat.com
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
isubramanian@apm.com, kchudgar@apm.com, qnguyen@apm.com
In-Reply-To: <20180917.193531.1424852853827678152.davem@davemloft.net>
On 09/17/2018 09:35 PM, David Miller wrote:
> From: Al Stone <ahs3@redhat.com>
> Date: Mon, 17 Sep 2018 17:35:33 -0600
>
>> @@ -866,8 +866,11 @@ static int xgene_enet_napi(struct napi_struct *napi, const int budget)
>> processed = xgene_enet_process_ring(ring, budget);
>>
>> if (processed != budget) {
>> + struct irq_desc *desc = irq_to_desc(ring->irq);
>> +
>> napi_complete_done(napi, processed);
The problem could be that the driver isn't checking the
napi_complete_done() return code. It was changed to return a bool and
the check should be more like:
if ((processed != budget) && napi_complete_done(napi, processed)) {
If it returns false, then the driver will get called for polling again
after having issued enable_irq() and it well then issue the enable_irq()
a second (or more) time without having the matching diable_irq().
Thanks,
Tom
>> - enable_irq(ring->irq);
>> + if (desc && desc->depth > 0)
>> + enable_irq(ring->irq);
>
> I really can't accept a patch that grovels into IRQ layer internals
> to work around a driver's IRQ enable/disable usage problem.
>
> Sorry.
>
^ permalink raw reply
* Re: [RFC 2/5] netlink: set extack error message in nla_validate()
From: Johannes Berg @ 2018-09-18 17:36 UTC (permalink / raw)
To: David Ahern, netdev
In-Reply-To: <f21246a8-6c0a-e99d-96fb-f6e549f78563@gmail.com>
On Tue, 2018-09-18 at 10:18 -0700, David Ahern wrote:
> On 9/18/18 6:12 AM, Johannes Berg wrote:
> > diff --git a/lib/nlattr.c b/lib/nlattr.c
> > index 120ad569e13d..efbd6c1aff29 100644
> > --- a/lib/nlattr.c
> > +++ b/lib/nlattr.c
> > @@ -181,9 +181,13 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
> > int rem;
> >
> > nla_for_each_attr(nla, head, len, rem) {
> > - int err = validate_nla(nla, maxtype, policy, NULL);
> > + static const char _msg[] = "Attribute failed policy validation";
> > + const char *msg = _msg;
> > + int err = validate_nla(nla, maxtype, policy, &msg);
> >
> > if (err < 0) {
> > + if (extack)
> > + extack->_msg = msg;
> > NL_SET_BAD_ATTR(extack, nla);
> > return err;
> > }
> >
>
> I take it this set is on top of another set - the NLA_REJECT?
Yeah, these two:
https://patchwork.ozlabs.org/project/netdev/list/?series=65982
johannes
^ permalink raw reply
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Al Stone @ 2018-09-18 23:15 UTC (permalink / raw)
To: 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: <a5adee73-dde7-d00f-e7ef-1f81dc7c813a@amd.com>
On 09/18/2018 05:09 PM, Lendacky, Thomas wrote:
>
>
> On 09/17/2018 09:35 PM, David Miller wrote:
>> From: Al Stone <ahs3@redhat.com>
>> Date: Mon, 17 Sep 2018 17:35:33 -0600
>>
>>> @@ -866,8 +866,11 @@ static int xgene_enet_napi(struct napi_struct *napi, const int budget)
>>> processed = xgene_enet_process_ring(ring, budget);
>>>
>>> if (processed != budget) {
>>> + struct irq_desc *desc = irq_to_desc(ring->irq);
>>> +
>>> napi_complete_done(napi, processed);
>
> The problem could be that the driver isn't checking the
> napi_complete_done() return code. It was changed to return a bool and
> the check should be more like:
>
> if ((processed != budget) && napi_complete_done(napi, processed)) {
>
> If it returns false, then the driver will get called for polling again
> after having issued enable_irq() and it well then issue the enable_irq()
> a second (or more) time without having the matching diable_irq().
>
> Thanks,
> Tom
Aha, that might be. My apologies -- I play in ACPI but seldom in the network
drivers, so was not fully aware of that change. I can give that a try.
Thanks for the pointer.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
^ permalink raw reply
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Florian Fainelli @ 2018-09-18 23:21 UTC (permalink / raw)
To: 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: <4929e93d-508a-4670-9450-66e4fc85be7e@redhat.com>
On 09/18/2018 04:15 PM, Al Stone wrote:
> On 09/18/2018 05:09 PM, Lendacky, Thomas wrote:
>>
>>
>> On 09/17/2018 09:35 PM, David Miller wrote:
>>> From: Al Stone <ahs3@redhat.com>
>>> Date: Mon, 17 Sep 2018 17:35:33 -0600
>>>
>>>> @@ -866,8 +866,11 @@ static int xgene_enet_napi(struct napi_struct *napi, const int budget)
>>>> processed = xgene_enet_process_ring(ring, budget);
>>>>
>>>> if (processed != budget) {
>>>> + struct irq_desc *desc = irq_to_desc(ring->irq);
>>>> +
>>>> napi_complete_done(napi, processed);
>>
>> The problem could be that the driver isn't checking the
>> napi_complete_done() return code. It was changed to return a bool and
>> the check should be more like:
>>
>> if ((processed != budget) && napi_complete_done(napi, processed)) {
>>
>> If it returns false, then the driver will get called for polling again
>> after having issued enable_irq() and it well then issue the enable_irq()
>> a second (or more) time without having the matching diable_irq().
>>
>> Thanks,
>> Tom
>
> Aha, that might be. My apologies -- I play in ACPI but seldom in the network
> drivers, so was not fully aware of that change. I can give that a try.
>
> Thanks for the pointer.
FWIW this is being discussed in this thread as well:
https://www.spinics.net/lists/netdev/msg523760.html
there should be an update to drivers that have a ndo_poll_controller()
and don't check napi_complete_done(), though I am not clear who is doing
that yet.
--
Florian
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Alexei Starovoitov @ 2018-09-18 17:51 UTC (permalink / raw)
To: Eric Dumazet, Song Liu
Cc: netdev, Jeff Kirsher, Alexander Duyck, michael.chan@broadcom.com,
Kernel Team
In-Reply-To: <CANn89iLAPvPn2C9J+sSHsQ5EeF0hq47iMiiAfwoQkdZurQiyzA@mail.gmail.com>
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);
}
^ permalink raw reply related
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Eric Dumazet @ 2018-09-18 23:25 UTC (permalink / raw)
To: 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: <4929e93d-508a-4670-9450-66e4fc85be7e@redhat.com>
On 09/18/2018 04:15 PM, Al Stone wrote:
> On 09/18/2018 05:09 PM, Lendacky, Thomas wrote:
>>
>>
>> On 09/17/2018 09:35 PM, David Miller wrote:
>>> From: Al Stone <ahs3@redhat.com>
>>> Date: Mon, 17 Sep 2018 17:35:33 -0600
>>>
>>>> @@ -866,8 +866,11 @@ static int xgene_enet_napi(struct napi_struct *napi, const int budget)
>>>> processed = xgene_enet_process_ring(ring, budget);
>>>>
>>>> if (processed != budget) {
>>>> + struct irq_desc *desc = irq_to_desc(ring->irq);
>>>> +
>>>> napi_complete_done(napi, processed);
>>
>> The problem could be that the driver isn't checking the
>> napi_complete_done() return code. It was changed to return a bool and
>> the check should be more like:
>>
>> if ((processed != budget) && napi_complete_done(napi, processed)) {
>>
>> If it returns false, then the driver will get called for polling again
>> after having issued enable_irq() and it well then issue the enable_irq()
>> a second (or more) time without having the matching diable_irq().
>>
>> Thanks,
>> Tom
>
> Aha, that might be. My apologies -- I play in ACPI but seldom in the network
> drivers, so was not fully aware of that change. I can give that a try.
>
> Thanks for the pointer.
>
Yes, please look at commit d7aba644ffdebf756e51e26a2229055211838e89
("amd-xgbe: Enable IRQs only if napi_complete_done() is true")
^ permalink raw reply
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Eric Dumazet @ 2018-09-18 23:27 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: <cdb8a00c-a3bf-72e0-38b2-3c4b2427270a@gmail.com>
On 09/18/2018 04:21 PM, Florian Fainelli wrote:
> On 09/18/2018 04:15 PM, Al Stone wrote:
>> On 09/18/2018 05:09 PM, Lendacky, Thomas wrote:
>>>
>>>
>>> On 09/17/2018 09:35 PM, David Miller wrote:
>>>> From: Al Stone <ahs3@redhat.com>
>>>> Date: Mon, 17 Sep 2018 17:35:33 -0600
>>>>
>>>>> @@ -866,8 +866,11 @@ static int xgene_enet_napi(struct napi_struct *napi, const int budget)
>>>>> processed = xgene_enet_process_ring(ring, budget);
>>>>>
>>>>> if (processed != budget) {
>>>>> + struct irq_desc *desc = irq_to_desc(ring->irq);
>>>>> +
>>>>> napi_complete_done(napi, processed);
>>>
>>> The problem could be that the driver isn't checking the
>>> napi_complete_done() return code. It was changed to return a bool and
>>> the check should be more like:
>>>
>>> if ((processed != budget) && napi_complete_done(napi, processed)) {
>>>
>>> If it returns false, then the driver will get called for polling again
>>> after having issued enable_irq() and it well then issue the enable_irq()
>>> a second (or more) time without having the matching diable_irq().
>>>
>>> Thanks,
>>> Tom
>>
>> Aha, that might be. My apologies -- I play in ACPI but seldom in the network
>> drivers, so was not fully aware of that change. I can give that a try.
>>
>> Thanks for the pointer.
>
> FWIW this is being discussed in this thread as well:
>
> https://www.spinics.net/lists/netdev/msg523760.html
>
> there should be an update to drivers that have a ndo_poll_controller()
> and don't check napi_complete_done(), though I am not clear who is doing
> that yet.
That might be tricky.
I remember one of the napi_complete_done() change had to be reverted,
for some obscure reason.
So clearly, doing a mass update (without being able to test the driver on real hardware) might be risky.
^ permalink raw reply
* Re: [PATCH net-next v5 20/20] net: WireGuard secure network tunnel
From: Andrew Lunn @ 2018-09-18 23:34 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: linux-kernel, netdev, linux-crypto, davem, gregkh
In-Reply-To: <20180918161646.19105-21-Jason@zx2c4.com>
> +#define push_rcu(stack, p, len) ({ \
> + if (rcu_access_pointer(p)) { \
> + BUG_ON(len >= 128); \
> + stack[len++] = rcu_dereference_raw(p); \
> + } \
> + true; \
> + })
> +static void root_free_rcu(struct rcu_head *rcu)
> +{
> + struct allowedips_node *node, *stack[128] = {
> + container_of(rcu, struct allowedips_node, rcu) };
> + unsigned int len = 1;
> +
> + while (len > 0 && (node = stack[--len]) &&
> + push_rcu(stack, node->bit[0], len) &&
> + push_rcu(stack, node->bit[1], len))
> + kfree(node);
> +}
Hi Jason
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.
Andrew
^ permalink raw reply
* Re: [bpf-next, v4 1/5] flow_dissector: implements flow dissector BPF hook
From: Eric Dumazet @ 2018-09-18 18:07 UTC (permalink / raw)
To: Petar Penkov, netdev
Cc: davem, ast, daniel, simon.horman, ecree, songliubraving, tom,
Petar Penkov, Willem de Bruijn
In-Reply-To: <20180914144622.16436-2-peterpenkov96@gmail.com>
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.
(AF_UNIX generates skbs)
> + if (attached) {
> + /* Note that even though the const qualifier is discarded
> + * throughout the execution of the BPF program, all changes(the
> + * control block) are reverted after the BPF program returns.
> + * Therefore, __skb_flow_dissect does not alter the skb.
> + */
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Eric Dumazet @ 2018-09-18 18:17 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: songliubraving, netdev, Jeff Kirsher, Alexander Duyck,
michael.chan, kernel-team
In-Reply-To: <a946c4d2-5e29-b1d4-3af6-8657caeebc11@fb.com>
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.
^ permalink raw reply
* 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
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