Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net] tcp/dccp: fix lockdep issue when SYN is backlogged
From: Eric Dumazet @ 2018-10-02 18:25 UTC (permalink / raw)
  To: David Miller, edumazet; +Cc: netdev, eric.dumazet
In-Reply-To: <20181001.154308.1092275550794942572.davem@davemloft.net>



On 10/01/2018 03:43 PM, David Miller wrote:
> From: Eric Dumazet <edumazet@google.com>
> Date: Mon,  1 Oct 2018 15:02:26 -0700

>> This patch extends what I did in commit 449809a66c1d ("tcp/dccp:
>> block BH for SYN processing") by adding an extra rcu_read_{lock|unlock}
>> pair in the paths that might be taken when processing SYN from
>> socket backlog (thus possibly in process context)
>>
>> Fixes: 06f877d613be ("tcp/dccp: fix other lockdep splats accessing ireq_opt")
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Reported-by: syzbot <syzkaller@googlegroups.com>
> 
> Applied and queued up for -stable, thanks Eric.
> 

This needs a followup, timers do not imply rcu_read_lock() :/

I will send a patch, sorry for not having caught this earlier.

^ permalink raw reply

* Re: [PATCH v2 1/5] ixgbe: added Rx/Tx ring disable/enable functions
From: William Tu @ 2018-10-02 18:25 UTC (permalink / raw)
  To: Björn Töpel
  Cc: jeffrey.t.kirsher, intel-wired-lan, Björn Töpel,
	Karlsson, Magnus, Magnus Karlsson, Alexei Starovoitov,
	Daniel Borkmann, Linux Kernel Network Developers,
	Jesper Dangaard Brouer, Test, Jakub Kicinski
In-Reply-To: <20181002080034.11754-2-bjorn.topel@gmail.com>

On Tue, Oct 2, 2018 at 1:01 AM Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> From: Björn Töpel <bjorn.topel@intel.com>
>
> Add functions for Rx/Tx ring enable/disable. Instead of resetting the
> whole device, only the affected ring is disabled or enabled.
>
> This plumbing is used in later commits, when zero-copy AF_XDP support
> is introduced.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>

Thanks!
Tested-by: William Tu <u9012063@gmail.com>

^ permalink raw reply

* Re: [PATCH net] ipv6: revert degradation in IPv6 Ready Logo test results
From: David Miller @ 2018-10-02 18:26 UTC (permalink / raw)
  To: mmanning; +Cc: netdev, fw
In-Reply-To: <20181002114030.18152-1-mmanning@vyatta.att-mail.com>

From: Mike Manning <mmanning@vyatta.att-mail.com>
Date: Tue,  2 Oct 2018 12:40:30 +0100

> This reverts commit 0ed4229b08c1 ("ipv6: defrag: drop non-last frags
> smaller than min mtu"). While one should not get fragments smaller than
> the IPv6 minimum MTU, not handling crafted packets in the TAHI IPv6
> conformance test suite (v6eval) for IPv6 Ready Logo results in 18
> failures representing over 5% of the score.
> 
> Cc: Florian Westphal <fw@strlen.de>
> Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>

Sorry, I'm not just going to blindly apply a patch because some
TAHI tests fail.

It's possible the TAHI tests are wrong, or that the specification
elements it is testing don't make any sense these days.

Allowing all kinds of random junk in the middle of the fragment queue
leads to lots of unnecessary cpu overhead and potential bugs, and it
triggerable remotely.

^ permalink raw reply

* Re: [PATCH v2 3/5] ixgbe: add AF_XDP zero-copy Rx support
From: William Tu @ 2018-10-02 18:26 UTC (permalink / raw)
  To: Björn Töpel
  Cc: jeffrey.t.kirsher, intel-wired-lan, Björn Töpel,
	Karlsson, Magnus, Magnus Karlsson, Alexei Starovoitov,
	Daniel Borkmann, Linux Kernel Network Developers,
	Jesper Dangaard Brouer, Test, Jakub Kicinski
In-Reply-To: <20181002080034.11754-4-bjorn.topel@gmail.com>

On Tue, Oct 2, 2018 at 1:01 AM Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> From: Björn Töpel <bjorn.topel@intel.com>
>
> This patch adds zero-copy Rx support for AF_XDP sockets. Instead of
> allocating buffers of type MEM_TYPE_PAGE_SHARED, the Rx frames are
> allocated as MEM_TYPE_ZERO_COPY when AF_XDP is enabled for a certain
> queue.
>
> All AF_XDP specific functions are added to a new file, ixgbe_xsk.c.
>
> Note that when AF_XDP zero-copy is enabled, the XDP action XDP_PASS
> will allocate a new buffer and copy the zero-copy frame prior passing
> it to the kernel stack.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---

Thanks!
Tested-by: William Tu <u9012063@gmail.com>

>  drivers/net/ethernet/intel/ixgbe/Makefile     |   3 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  27 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c  |  17 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  78 ++-
>  .../ethernet/intel/ixgbe/ixgbe_txrx_common.h  |  15 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 628 ++++++++++++++++++
>  6 files changed, 747 insertions(+), 21 deletions(-)
>  create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
>

^ permalink raw reply

* Re: [PATCH v2 5/5] ixgbe: add AF_XDP zero-copy Tx support
From: William Tu @ 2018-10-02 18:26 UTC (permalink / raw)
  To: Björn Töpel
  Cc: jeffrey.t.kirsher, intel-wired-lan, Björn Töpel,
	Karlsson, Magnus, Magnus Karlsson, Alexei Starovoitov,
	Daniel Borkmann, Linux Kernel Network Developers,
	Jesper Dangaard Brouer, Test, Jakub Kicinski
In-Reply-To: <20181002080034.11754-6-bjorn.topel@gmail.com>

On Tue, Oct 2, 2018 at 1:01 AM Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> From: Björn Töpel <bjorn.topel@intel.com>
>
> This patch adds zero-copy Tx support for AF_XDP sockets. It implements
> the ndo_xsk_async_xmit netdev ndo and performs all the Tx logic from a
> NAPI context. This means pulling egress packets from the Tx ring,
> placing the frames on the NIC HW descriptor ring and completing sent
> frames back to the application via the completion ring.
>
> The regular XDP Tx ring is used for AF_XDP as well. This rationale for
> this is as follows: XDP_REDIRECT guarantees mutual exclusion between
> different NAPI contexts based on CPU id. In other words, a netdev can
> XDP_REDIRECT to another netdev with a different NAPI context, since
> the operation is bound to a specific core and each core has its own
> hardware ring.
>
> As the AF_XDP Tx action is running in the same NAPI context and using
> the same ring, it will also be protected from XDP_REDIRECT actions
> with the exact same mechanism.
>
> As with AF_XDP Rx, all AF_XDP Tx specific functions are added to
> ixgbe_xsk.c.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---

Thanks!
Tested-by: William Tu <u9012063@gmail.com>

^ permalink raw reply

* Re: [PATCH v2 2/5] ixgbe: move common Rx functions to ixgbe_txrx_common.h
From: William Tu @ 2018-10-02 18:27 UTC (permalink / raw)
  To: Björn Töpel
  Cc: jeffrey.t.kirsher, intel-wired-lan, Björn Töpel,
	Karlsson, Magnus, Magnus Karlsson, Alexei Starovoitov,
	Daniel Borkmann, Linux Kernel Network Developers,
	Jesper Dangaard Brouer, Test, Jakub Kicinski
In-Reply-To: <20181002080034.11754-3-bjorn.topel@gmail.com>

On Tue, Oct 2, 2018 at 1:01 AM Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> From: Björn Töpel <bjorn.topel@intel.com>
>
> This patch prepares for the upcoming zero-copy Rx functionality, by
> moving/changing linkage of common functions, used both by the regular
> path and zero-copy path.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---

Thanks!
Tested-by: William Tu <u9012063@gmail.com>

^ permalink raw reply

* Re: [PATCH 1/1] net: fec: fix rare tx timeout
From: David Miller @ 2018-10-02 18:28 UTC (permalink / raw)
  To: rickard.andersson; +Cc: fugang.duan, netdev, rickaran
In-Reply-To: <20181002124932.14810-2-rickard.andersson@axis.com>

From: <rickard.andersson@axis.com>
Date: Tue, 2 Oct 2018 14:49:32 +0200

> From: Rickard x Andersson <rickaran@axis.com>
> 
> During certain heavy network loads TX could time out
> with TX ring dump.
> TX is sometimes never restarted after reaching
> "tx_stop_threshold" because function "fec_enet_tx_queue"
> only tests the first queue.
> 
> In addition the TX timeout callback function failed to
> recover because it also operated only on the first queue.
> 
> Signed-off-by: Rickard x Andersson <rickaran@axis.com>

Applied and queued up for -stable.

^ permalink raw reply

* Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module
From: Chenbo Feng @ 2018-10-02 18:28 UTC (permalink / raw)
  To: pablo
  Cc: zenczykowski, Chenbo Feng, netdev, netfilter-devel, kernel-team,
	Lorenzo Colitti
In-Reply-To: <20181002181546.b6zhizcl6lj2inu2@salvia>

On Tue, Oct 2, 2018 at 11:15 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Chenbo,
>
> On Tue, Oct 02, 2018 at 10:45:58AM -0700, Chenbo Feng wrote:
> > On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> [...]
> > Do you mean the remain field will be zeroed when copying the
> > xt_quota_info struct out of the kernel? I believe that is decided by
> > the usersize defined in struct xt_match and this patch set it to the
> > full struct size. So the whole xt_quota_info struct will be copied
> > into userspace including the field stores the remaining quota. The
> > userspace will not be aware of it if the ipatbles is not updated but
> > it should not modify it as well. I have tested the behavior with
> > net-next branch and it seems working. Am I missing something
> > recently updated?
>
> Hm, I see, I overlook that your patch removes this:
>
> -       .usersize   = offsetof(struct xt_quota_info, master),
>
> BTW, is iptables -D command working with your patch?
>
> Telling this because if .usersize is removed, then IIRC userspace
> compares this new remain field with userspace value and deletion will
> break.
>
> Patch that I was referring before is this one from Willem:
>
> commit f32815d21d4d8287336fb9cef4d2d9e0866214c2
> Author: Willem de Bruijn <willemb@google.com>
> Date:   Mon Jan 2 17:19:40 2017 -0500
>
>     xtables: add xt_match, xt_target and data copy_to_user functions
>
>     xt_entry_target, xt_entry_match and their private data may contain
>     kernel data.
> [...]
>     Private data is defined in xt_match and xt_target. All matches and
>     targets that maintain kernel data store this at the tail of their
>     private structure. Extend xt_match and xt_target with .usersize to
>     limit how many bytes of data are copied. The remainder is cleared.
>
> Let me know, thanks !

The delete operation is decided by the userspacesize defined in
userspace ipatbles. I think it is unrelated to the usersize we talk
about here. For old userspace iptables, the userspacesize is
offsetof(struct xt_quota_info, master) so it will not compare the rest
if the struct. And for new iptables we use offsetof(struct
xt_quota_info, remain). Either way the userspace does not consider the
remain field when comparing rules so we can do ipatbles rule deletion
with or without specifying --remain option or even specify --remain to
a wrong number. We decide to make it this way since the --remain field
is changing all the time when there is network traffic going on and
it's hard to compare the remaining quota for new ipatbles as well.

^ permalink raw reply

* Re: [PATCH v2 4/5] ixgbe: move common Tx functions to ixgbe_txrx_common.h
From: William Tu @ 2018-10-02 18:28 UTC (permalink / raw)
  To: Björn Töpel
  Cc: jeffrey.t.kirsher, intel-wired-lan, Björn Töpel,
	Karlsson, Magnus, Magnus Karlsson, Alexei Starovoitov,
	Daniel Borkmann, Linux Kernel Network Developers,
	Jesper Dangaard Brouer, Test, Jakub Kicinski
In-Reply-To: <20181002080034.11754-5-bjorn.topel@gmail.com>

On Tue, Oct 2, 2018 at 1:01 AM Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> From: Björn Töpel <bjorn.topel@intel.com>
>
> This patch prepares for the upcoming zero-copy Tx functionality by
> moving common functions used both by the regular path and zero-copy
> path.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
Thanks!
Tested-by: William Tu <u9012063@gmail.com>

^ permalink raw reply

* Re: [PATCH net-next 0/2] qed*: Driver support for 20G link speed.
From: David Miller @ 2018-10-02 18:29 UTC (permalink / raw)
  To: sudarsana.kalluru; +Cc: netdev, Michal.Kalderon
In-Reply-To: <20181002131612.10859-1-sudarsana.kalluru@cavium.com>

From: Sudarsana Reddy Kalluru <sudarsana.kalluru@cavium.com>
Date: Tue, 2 Oct 2018 06:16:10 -0700

> From: Sudarsana Reddy Kalluru <Sudarsana.Kalluru@cavium.com>
> 
> The patch series adds driver support for configuring/reading the 20G link
> speed.
> 
> Please consider applying this to "net-next".

Series applied.

^ permalink raw reply

* Re: [PATCH v2 0/5] Introducing ixgbe AF_XDP ZC support
From: Björn Töpel @ 2018-10-02 18:39 UTC (permalink / raw)
  To: William Tu, Björn Töpel
  Cc: jeffrey.t.kirsher, intel-wired-lan, Karlsson, Magnus,
	Magnus Karlsson, Alexei Starovoitov, Daniel Borkmann,
	Linux Kernel Network Developers, Jesper Dangaard Brouer, Test,
	Jakub Kicinski
In-Reply-To: <CALDO+SaKBCFZ5mUum09+FH1s_cEKkqTSSCAZhnFzHXHaNMmxdA@mail.gmail.com>

On 2018-10-02 20:23, William Tu wrote:
> On Tue, Oct 2, 2018 at 1:01 AM Björn Töpel <bjorn.topel@gmail.com> wrote:
>>
>> From: Björn Töpel <bjorn.topel@intel.com>
>>
>> Jeff: Please remove the v1 patches from your dev-queue!
>>
>> This patch set introduces zero-copy AF_XDP support for Intel's ixgbe
>> driver.
>>
>> The ixgbe zero-copy code is located in its own file ixgbe_xsk.[ch],
>> analogous to the i40e ZC support. Again, as in i40e, code paths have
>> been copied from the XDP path to the zero-copy path. Going forward we
>> will try to generalize more code between the AF_XDP ZC drivers, and
>> also reduce the heavy C&P.
>>
>> We have run some benchmarks on a dual socket system with two Broadwell
>> E5 2660 @ 2.0 GHz with hyperthreading turned off. Each socket has 14
>> cores which gives a total of 28, but only two cores are used in these
>> experiments. One for TR/RX and one for the user space application. The
>> memory is DDR4 @ 2133 MT/s (1067 MHz) and the size of each DIMM is
>> 8192MB and with 8 of those DIMMs in the system we have 64 GB of total
>> memory. The compiler used is GCC 7.3.0. The NIC is Intel
>> 82599ES/X520-2 10Gbit/s using the ixgbe driver.
>>
>> Below are the results in Mpps of the 82599ES/X520-2 NIC benchmark runs
>> for 64B and 1500B packets, generated by a commercial packet generator
>> HW blasting packets at full 10Gbit/s line rate. The results are with
>> retpoline and all other spectre and meltdown fixes.
>>
>> AF_XDP performance 64B packets:
>> Benchmark   XDP_DRV with zerocopy
>> rxdrop        14.7
>> txpush        14.6
>> l2fwd         11.1
>>
>> AF_XDP performance 1500B packets:
>> Benchmark   XDP_DRV with zerocopy
>> rxdrop        0.8
>> l2fwd         0.8
>>
>> XDP performance on our system as a base line.
>>
>> 64B packets:
>> XDP stats       CPU     Mpps       issue-pps
>> XDP-RX CPU      16      14.7       0
>>
>> 1500B packets:
>> XDP stats       CPU     Mpps       issue-pps
>> XDP-RX CPU      16      0.8        0
>>
>> The structure of the patch set is as follows:
>>
>> Patch 1: Introduce Rx/Tx ring enable/disable functionality
>> Patch 2: Preparatory patche to ixgbe driver code for RX
>> Patch 3: ixgbe zero-copy support for RX
>> Patch 4: Preparatory patch to ixgbe driver code for TX
>> Patch 5: ixgbe zero-copy support for TX
>>
>> Changes since v1:
>>
>> * Removed redundant AF_XDP precondition checks, pointed out by
>>    Jakub. Now, the preconditions are only checked at XDP enable time.
>> * Fixed a crash in the egress path, due to incorrect usage of
>>    ixgbe_ring queue_index member. In v2 a ring_idx back reference is
>>    introduced, and used in favor of queue_index. William reported the
>>    crash, and helped me smoke out the issue. Kudos!
> 
> Thanks! I tested this series and no more crash.

Thank you for spending time on this!

> The number is pretty good (*without* spectre and meltdown fixes)
> model name : Intel(R) Xeon(R) CPU E5-2440 v2 @ 1.90GHz, total 16 cores/
> 
> AF_XDP performance 64B packets:
> Benchmark   XDP_DRV with zerocopy
> rxdrop        20
> txpush        18
> l2fwd         20
>

What is 20 here? Given that 14.8Mpps is maximum for 64B@10Gbit/s for
one queue, is this multiple queues? Is this xdpsock or OvS with AF_XDP?


Cheers,
Björn

> Regards,
> William
> 

^ permalink raw reply

* Re: [net-next 0/9][pull request] 100GbE Intel Wired LAN Driver Updates 2018-10-02
From: David Miller @ 2018-10-02 18:41 UTC (permalink / raw)
  To: jeffrey.t.kirsher; +Cc: netdev, nhorman, sassmann, jogreene
In-Reply-To: <20181002152447.11175-1-jeffrey.t.kirsher@intel.com>

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Tue,  2 Oct 2018 08:24:38 -0700

> This series contains updates to ice driver only.

If there was a problem yo I'll solve it.
Check out the hook while my DJ revolves it.

Ice ice baby.
Ice ice baby.

Pulled, thanks Jeff.

^ permalink raw reply

* Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt_quota module
From: Pablo Neira Ayuso @ 2018-10-02 18:43 UTC (permalink / raw)
  To: Chenbo Feng
  Cc: zenczykowski, Chenbo Feng, netdev, netfilter-devel, kernel-team,
	Lorenzo Colitti
In-Reply-To: <CAMOXUJn6c+ybxks9siDn=fJo4mGBYk88LuD17BYnPBAPySufDg@mail.gmail.com>

On Tue, Oct 02, 2018 at 11:28:21AM -0700, Chenbo Feng wrote:
> On Tue, Oct 2, 2018 at 11:15 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > Hi Chenbo,
> >
> > On Tue, Oct 02, 2018 at 10:45:58AM -0700, Chenbo Feng wrote:
> > > On Tue, Oct 2, 2018 at 3:51 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > [...]
> > > Do you mean the remain field will be zeroed when copying the
> > > xt_quota_info struct out of the kernel? I believe that is decided by
> > > the usersize defined in struct xt_match and this patch set it to the
> > > full struct size. So the whole xt_quota_info struct will be copied
> > > into userspace including the field stores the remaining quota. The
> > > userspace will not be aware of it if the ipatbles is not updated but
> > > it should not modify it as well. I have tested the behavior with
> > > net-next branch and it seems working. Am I missing something
> > > recently updated?
> >
> > Hm, I see, I overlook that your patch removes this:
> >
> > -       .usersize   = offsetof(struct xt_quota_info, master),
> >
> > BTW, is iptables -D command working with your patch?
> >
> > Telling this because if .usersize is removed, then IIRC userspace
> > compares this new remain field with userspace value and deletion will
> > break.
> >
> > Patch that I was referring before is this one from Willem:
> >
> > commit f32815d21d4d8287336fb9cef4d2d9e0866214c2
> > Author: Willem de Bruijn <willemb@google.com>
> > Date:   Mon Jan 2 17:19:40 2017 -0500
> >
> >     xtables: add xt_match, xt_target and data copy_to_user functions
> >
> >     xt_entry_target, xt_entry_match and their private data may contain
> >     kernel data.
> > [...]
> >     Private data is defined in xt_match and xt_target. All matches and
> >     targets that maintain kernel data store this at the tail of their
> >     private structure. Extend xt_match and xt_target with .usersize to
> >     limit how many bytes of data are copied. The remainder is cleared.
> >
> > Let me know, thanks !
> 
> The delete operation is decided by the userspacesize defined in
> userspace ipatbles. I think it is unrelated to the usersize we talk
> about here. For old userspace iptables, the userspacesize is
> offsetof(struct xt_quota_info, master) so it will not compare the rest
> if the struct. And for new iptables we use offsetof(struct
> xt_quota_info, remain). Either way the userspace does not consider the
> remain field when comparing rules so we can do ipatbles rule deletion
> with or without specifying --remain option or even specify --remain to
> a wrong number. We decide to make it this way since the --remain field
> is changing all the time when there is network traffic going on and
> it's hard to compare the remaining quota for new ipatbles as well.

Thanks for explaining and your patience. Patch looks good then.

Thanks!

^ permalink raw reply

* Re: [PATCH v2 0/5] Introducing ixgbe AF_XDP ZC support
From: William Tu @ 2018-10-02 18:43 UTC (permalink / raw)
  To: Björn Töpel
  Cc: Björn Töpel, jeffrey.t.kirsher, intel-wired-lan,
	Karlsson, Magnus, Magnus Karlsson, Alexei Starovoitov,
	Daniel Borkmann, Linux Kernel Network Developers,
	Jesper Dangaard Brouer, Test, Jakub Kicinski
In-Reply-To: <b507fced-01ab-c109-a7b3-4ed302ca1c00@intel.com>

On Tue, Oct 2, 2018 at 11:39 AM Björn Töpel <bjorn.topel@intel.com> wrote:
>
> On 2018-10-02 20:23, William Tu wrote:
> > On Tue, Oct 2, 2018 at 1:01 AM Björn Töpel <bjorn.topel@gmail.com> wrote:
> >>
> >> From: Björn Töpel <bjorn.topel@intel.com>
> >>
> >> Jeff: Please remove the v1 patches from your dev-queue!
> >>
> >> This patch set introduces zero-copy AF_XDP support for Intel's ixgbe
> >> driver.
> >>
> >> The ixgbe zero-copy code is located in its own file ixgbe_xsk.[ch],
> >> analogous to the i40e ZC support. Again, as in i40e, code paths have
> >> been copied from the XDP path to the zero-copy path. Going forward we
> >> will try to generalize more code between the AF_XDP ZC drivers, and
> >> also reduce the heavy C&P.
> >>
> >> We have run some benchmarks on a dual socket system with two Broadwell
> >> E5 2660 @ 2.0 GHz with hyperthreading turned off. Each socket has 14
> >> cores which gives a total of 28, but only two cores are used in these
> >> experiments. One for TR/RX and one for the user space application. The
> >> memory is DDR4 @ 2133 MT/s (1067 MHz) and the size of each DIMM is
> >> 8192MB and with 8 of those DIMMs in the system we have 64 GB of total
> >> memory. The compiler used is GCC 7.3.0. The NIC is Intel
> >> 82599ES/X520-2 10Gbit/s using the ixgbe driver.
> >>
> >> Below are the results in Mpps of the 82599ES/X520-2 NIC benchmark runs
> >> for 64B and 1500B packets, generated by a commercial packet generator
> >> HW blasting packets at full 10Gbit/s line rate. The results are with
> >> retpoline and all other spectre and meltdown fixes.
> >>
> >> AF_XDP performance 64B packets:
> >> Benchmark   XDP_DRV with zerocopy
> >> rxdrop        14.7
> >> txpush        14.6
> >> l2fwd         11.1
> >>
> >> AF_XDP performance 1500B packets:
> >> Benchmark   XDP_DRV with zerocopy
> >> rxdrop        0.8
> >> l2fwd         0.8
> >>
> >> XDP performance on our system as a base line.
> >>
> >> 64B packets:
> >> XDP stats       CPU     Mpps       issue-pps
> >> XDP-RX CPU      16      14.7       0
> >>
> >> 1500B packets:
> >> XDP stats       CPU     Mpps       issue-pps
> >> XDP-RX CPU      16      0.8        0
> >>
> >> The structure of the patch set is as follows:
> >>
> >> Patch 1: Introduce Rx/Tx ring enable/disable functionality
> >> Patch 2: Preparatory patche to ixgbe driver code for RX
> >> Patch 3: ixgbe zero-copy support for RX
> >> Patch 4: Preparatory patch to ixgbe driver code for TX
> >> Patch 5: ixgbe zero-copy support for TX
> >>
> >> Changes since v1:
> >>
> >> * Removed redundant AF_XDP precondition checks, pointed out by
> >>    Jakub. Now, the preconditions are only checked at XDP enable time.
> >> * Fixed a crash in the egress path, due to incorrect usage of
> >>    ixgbe_ring queue_index member. In v2 a ring_idx back reference is
> >>    introduced, and used in favor of queue_index. William reported the
> >>    crash, and helped me smoke out the issue. Kudos!
> >
> > Thanks! I tested this series and no more crash.
>
> Thank you for spending time on this!
>
> > The number is pretty good (*without* spectre and meltdown fixes)
> > model name : Intel(R) Xeon(R) CPU E5-2440 v2 @ 1.90GHz, total 16 cores/
> >
> > AF_XDP performance 64B packets:
> > Benchmark   XDP_DRV with zerocopy
> > rxdrop        20
> > txpush        18
> > l2fwd         20
Sorry please ignore this number!
It's actually 2Mpps from xdpsock but that's because my sender only sends 2Mpps.
>
> What is 20 here? Given that 14.8Mpps is maximum for 64B@10Gbit/s for
> one queue, is this multiple queues? Is this xdpsock or OvS with AF_XDP?

I'm redoing the experiments with higher traffic rate, will report later..
William

^ permalink raw reply

* Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
From: Vijay Khemka @ 2018-10-02 18:56 UTC (permalink / raw)
  To: Justin.Lee1@Dell.com, sam@mendozajonas.com, joel@jms.id.au,
	linux-aspeed@lists.ozlabs.org, openbmc@lists.ozlabs.org,
	Sai Dasari, netdev@vger.kernel.org, christian@cmd.nu
In-Reply-To: <4876838166d140bab331280efcfd771d@AUSX13MPS306.AMER.DELL.COM>

Hi Justin,
Thanks for response. Please see my comments below.

-Vijay
    > > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
    > > index 7567ca63aae2..2f98533eba46 100644
    > > --- a/net/ncsi/ncsi-cmd.c
    > > +++ b/net/ncsi/ncsi-cmd.c
    > > @@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
    > >  	return 0;
    > >  }
    > >  
    > > +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
    > > +				struct ncsi_cmd_arg *nca)
    > > +{
    > > +	struct ncsi_cmd_oem_pkt *cmd;
    
    >OEM command doesn't not have the fixed data size. Should we use pointer instead?
    >struct ncsi_cmd_oem_pkt {
    >	struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
    >	__be32                  mfr_id;      /* Manufacture ID    */
    >	unsigned char           *data;       /* OEM Payload Data  */
    >};
    Yes, I agree that OEM command doesn't have fixed data but to map to skbuff structure, 
    I have defined above structure as per NCSI spec, We can certainly have a MAX_DATA_LEN define.

    > > +	unsigned int len;
    > > +
    > > +	len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
    > > +	if (nca->payload < 26)
    > > +		len += 26;
    
    >Why does it add 26? I knew the other place in ncsi_alloc_command() is also add 26.
    >If it is less than 26, then it should be a fixed size of structure ncsi_cmd_pkt (46), right?
    It adds 26 because It has already assigned len to hdr+4 which is 16+4 = 20 bytes. By 
    adding 26 it makes it to 46. I am just being consistent with other portion of code.

    > > +	else
    > > +		len += nca->payload;
    > > +
    > > +	cmd = skb_put_zero(skb, len);
    > > +	cmd->mfr_id = nca->dwords[0];
    > > +	memcpy(cmd->data, &nca->dwords[1], nca->payload - 4);
    
    >Netlink request is using the new nca->data pointer to pass the data as the request payload
    >is not the same size and some command payload is bigger than 16 bytes.
    >Will you consider to use the same data pointer? So, we don't need to have a checking here.
    >If the command is used less than 16 bytes, we can simply assigned &nca->bytes[0] to it.
    To keep original structure, we can change 16 bytes to MAX_DATA_LEN. Or I don't see any issue in 
    Copying data from data pointer from nca but user needs to be aware if it is less than 16 bytes then 
    use bytes or use data pointer. To keep it simple, we should simply define MAX_LEN.
    
    > > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
    > > index 91b4b66438df..1f338386810d 100644
    > > --- a/net/ncsi/ncsi-pkt.h
    > > +++ b/net/ncsi/ncsi-pkt.h
    > > @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
    > >  	unsigned char           pad[22];
    > >  };
    > >  
    > > +/* OEM Request Command as per NCSI Specification */
    > > +struct ncsi_cmd_oem_pkt {
    > > +	struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
    > > +	__be32                  mfr_id;      /* Manufacture ID    */
    > > +	unsigned char           data[64];    /* OEM Payload Data  */
    > > +	__be32                  checksum;    /* Checksum          */
    > > +};
    > > +
    > > +/* OEM Response Packet as per NCSI Specification */
    > > +struct ncsi_rsp_oem_pkt {
    > > +	struct ncsi_rsp_pkt_hdr rsp;         /* Command header    */
    > > +	__be32                  mfr_id;      /* Manufacture ID    */
    > > +	unsigned char           data[64];    /* Payload data      */
    > > +	__be32                  checksum;    /* Checksum          */
    > > +};
    > > +
    
    >OEM command doesn't not have the fixed response data size too.
    >Should we use pointer instead?

    Here also we can define MAX_DATA_LEN because data pointer won't map to skb directly.
    


^ permalink raw reply

* [PATCH net] bonding: fix warning message
From: Mahesh Bandewar @ 2018-10-02 19:14 UTC (permalink / raw)
  To: Jay Vosburgh, Andy Gospodarek, Veaceslav Falico, David Miller
  Cc: Mahesh Bandewar, Netdev, Eric Dumazet, Mahesh Bandewar

From: Mahesh Bandewar <maheshb@google.com>

RX queue config for bonding master could be different from its slave
device(s). With the commit 6a9e461f6fe4 ("bonding: pass link-local
packets to bonding master also."), the packet is reinjected into stack
with skb->dev as bonding master. This potentially triggers the
message:

   "bondX received packet on queue Y, but number of RX queues is Z"

whenever the queue that packet is received on is higher than the
numrxqueues on bonding master (Y > Z).

Fixes: 6a9e461f6fe4 ("bonding: pass link-local packets to bonding master also.")
Reported-by: John Sperbeck <jsperbeck@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
 drivers/net/bonding/bond_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c05c01a00755..ee28ec9e0aba 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1187,6 +1187,7 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
 
 		if (nskb) {
 			nskb->dev = bond->dev;
+			nskb->queue_mapping = 0;
 			netif_rx(nskb);
 		}
 		return RX_HANDLER_PASS;
-- 
2.19.0.605.g01d371f741-goog

^ permalink raw reply related

* Re: [PATCH RFC v2 net-next 03/25] netlink: introduce NLM_F_DUMP_PROPER_HDR flag
From: David Ahern @ 2018-10-02 19:20 UTC (permalink / raw)
  To: Jiri Benc; +Cc: Christian Brauner, netdev, davem, stephen
In-Reply-To: <20181002183018.65c41753@redhat.com>

On 10/2/18 10:30 AM, Jiri Benc wrote:
> On Tue, 2 Oct 2018 08:57:24 -0600, David Ahern wrote:
>> You can when you introduce a new option or a new flag that is required
>> to get new behavior like kernel side filtering.
> 
> Yes. That was what I tried with the patchset a few years back. It would
> be nice to revive the effort.
> 
>> I chose a netlink flag for consistency with NLM_F_DUMP_INTR and
>> NLM_F_DUMP_FILTERED. Both are netlink flags. This patch set fixes only
>> what is broken -- dumps.
> 
> When we're introducing better input checking in netlink (which is a
> good thing!), it would be good to do it consistently and have it
> generic across all operations.
> 

Thinking about this some more... a setsockopt to enable the new checks
would provide a definitive way for userspace to know if the feature is
supported or not.

As for expanding the scope to NEW (and maybe DEL) commands (which was
the point of your patch set 3 years ago), I think is an idealistic goal
that is not practical to implement. Trying to go through all commands
and cover all options and valid combinations to report back errors is a
herculean effort with little return on the time investment. New commands
and new features should certainly add rigid checks for valid
combinations, but a retrospective audit on existing commands is a time
sink. Perhaps new attributes can be checked (new command on old kernel);
seems like that can be covered by a simple change to nla_parse to handle
type > maxtype based on a new input flag.

Once the flag goes in, all of the commands to be affected by it have to
be done in the same release. I have done that for the dump commands
which is fairly easy considering the existing code.

^ permalink raw reply

* Re: [Patch net-next v3] net_sched: change tcf_del_walker() to take idrinfo->lock
From: Cong Wang @ 2018-10-02 19:23 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim,
	Vlad Buslov
In-Reply-To: <CAM_iQpVpmJ76vs4j0eaDWCJiXDjZA8Uj_ozS8gp5RZBpAV-zCQ@mail.gmail.com>

On Fri, Sep 28, 2018 at 9:47 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> Just FYI, the crash is fixed, but another RCU warning pops up after
> my fix. So I am still debugging it, it is related to the kfree_rcu() in
> tcf_chain_destroy().

Update: I tried very hard to make it working, the end result is ugly,
as I have to play with refcnt==1. I don't like it either. So, I am thinking
about to just convert the idrinfo->lock from a spinlock to a mutex,
which could save us a lot of troubles here.

^ permalink raw reply

* Re: [PATCH 1/1] bridge: remove BR_GROUPFWD_RESTRICTED for arbitrary forwarding of reserved addresses
From: Richard Weinberger @ 2018-10-02 19:30 UTC (permalink / raw)
  To: nikolay
  Cc: idosch, Stephen Hemminger, Florian Fainelli, Bernhard Thaler,
	David S. Miller, bridge, netdev, David Gstir, roopa
In-Reply-To: <102873a5-ed43-2bf7-9ef7-70a76c5b7f6e@cumulusnetworks.com>

On Tue, Oct 2, 2018 at 6:48 PM Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> > Is there a reason why this knob is not documented in Documentation/ABI/testing/sysfs-class-net?
> >
>
> Hmm, I don't think any of the bridge knobs are documented there or any other master device for that
> matter. Most of these options are documented in ip-link.8 man page from iproute2, it seems though
> this one I've missed to document. I'll prepare a patch for iproute2.

Well, /sys/class/net/<bridge iface>/bridge/group_fwd_mask is
documented, this confused me a little.

-- 
Thanks,
//richard

^ permalink raw reply

* linux-next: manual merge of the net-next tree with the net tree
From: Stephen Rothwell @ 2018-10-03  2:18 UTC (permalink / raw)
  To: David Miller, Networking
  Cc: Linux-Next Mailing List, Linux Kernel Mailing List, David Ahern,
	Christian Brauner

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

Hi all,

Today's linux-next merge of the net-next tree got a conflict in:

  net/core/rtnetlink.c

between commit:

  893626d6a353 ("rtnetlink: Fail dump if target netnsid is invalid")

from the net tree and commits:

  c383edc42403 ("rtnetlink: add rtnl_get_net_ns_capable()")
  7e4a8d5a93f6 ("rtnetlink: s/IFLA_IF_NETNSID/IFLA_TARGET_NETNSID/g")

from the net-next tree.

I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging.  You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.

-- 
Cheers,
Stephen Rothwell

diff --cc net/core/rtnetlink.c
index 7f37fe9c65a5,35162e1b06ad..000000000000
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@@ -1895,11 -1910,13 +1910,11 @@@ static int rtnl_dump_ifinfo(struct sk_b
  
  	if (nlmsg_parse(cb->nlh, hdrlen, tb, IFLA_MAX,
  			ifla_policy, NULL) >= 0) {
- 		if (tb[IFLA_IF_NETNSID]) {
- 			netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
- 			tgt_net = get_target_net(skb->sk, netnsid);
+ 		if (tb[IFLA_TARGET_NETNSID]) {
+ 			netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]);
+ 			tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid);
 -			if (IS_ERR(tgt_net)) {
 -				tgt_net = net;
 -				netnsid = -1;
 -			}
 +			if (IS_ERR(tgt_net))
 +				return PTR_ERR(tgt_net);
  		}
  
  		if (tb[IFLA_EXT_MASK])

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply

* [PATCH net] inet: make sure to grab rcu_read_lock before using ireq->ireq_opt
From: Eric Dumazet @ 2018-10-02 19:35 UTC (permalink / raw)
  To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet

Timer handlers do not imply rcu_read_lock(), so my recent fix
triggered a LOCKDEP warning when SYNACK is retransmit.

Lets add rcu_read_lock()/rcu_read_unlock() pairs around ireq->ireq_opt
usages instead of guessing what is done by callers, since it is
not worth the pain.

Get rid of ireq_opt_deref() helper since it hides the logic
without real benefit, since it is now a standard rcu_dereference().

Fixes: 1ad98e9d1bdf ("tcp/dccp: fix lockdep issue when SYN is backlogged")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Willem de Bruijn <willemb@google.com>
---
 include/net/inet_sock.h         | 5 -----
 net/dccp/ipv4.c                 | 4 +++-
 net/ipv4/inet_connection_sock.c | 5 ++++-
 net/ipv4/tcp_ipv4.c             | 4 +++-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index a8cd5cf9ff5b6ddc50bd2e70d3f9103afa32a3b5..a80fd0ac4563283246f4f53cea1ac0cd17b41dab 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -130,11 +130,6 @@ static inline int inet_request_bound_dev_if(const struct sock *sk,
 	return sk->sk_bound_dev_if;
 }
 
-static inline struct ip_options_rcu *ireq_opt_deref(const struct inet_request_sock *ireq)
-{
-	return rcu_dereference(ireq->ireq_opt);
-}
-
 struct inet_cork {
 	unsigned int		flags;
 	__be32			addr;
diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index b08feb219b44b67eadf408a33649d8c7ec9db2d0..8e08cea6f17866b5fb1619f570de747c6a837cbd 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -493,9 +493,11 @@ static int dccp_v4_send_response(const struct sock *sk, struct request_sock *req
 
 		dh->dccph_checksum = dccp_v4_csum_finish(skb, ireq->ir_loc_addr,
 							      ireq->ir_rmt_addr);
+		rcu_read_lock();
 		err = ip_build_and_send_pkt(skb, sk, ireq->ir_loc_addr,
 					    ireq->ir_rmt_addr,
-					    ireq_opt_deref(ireq));
+					    rcu_dereference(ireq->ireq_opt));
+		rcu_read_unlock();
 		err = net_xmit_eval(err);
 	}
 
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index dfd5009f96ef7111a593651a48f73c4a92c3ed15..15e7f7915a21e0fbce09d5d2c17d877eae499e03 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -544,7 +544,8 @@ struct dst_entry *inet_csk_route_req(const struct sock *sk,
 	struct ip_options_rcu *opt;
 	struct rtable *rt;
 
-	opt = ireq_opt_deref(ireq);
+	rcu_read_lock();
+	opt = rcu_dereference(ireq->ireq_opt);
 
 	flowi4_init_output(fl4, ireq->ir_iif, ireq->ir_mark,
 			   RT_CONN_FLAGS(sk), RT_SCOPE_UNIVERSE,
@@ -558,11 +559,13 @@ struct dst_entry *inet_csk_route_req(const struct sock *sk,
 		goto no_route;
 	if (opt && opt->opt.is_strictroute && rt->rt_uses_gateway)
 		goto route_err;
+	rcu_read_unlock();
 	return &rt->dst;
 
 route_err:
 	ip_rt_put(rt);
 no_route:
+	rcu_read_unlock();
 	__IP_INC_STATS(net, IPSTATS_MIB_OUTNOROUTES);
 	return NULL;
 }
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 44c09eddbb781c03da2417aaa925e360de01a6e9..cd426313a29819b34648086b551fe9390d8a0b0a 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -943,9 +943,11 @@ static int tcp_v4_send_synack(const struct sock *sk, struct dst_entry *dst,
 	if (skb) {
 		__tcp_v4_send_check(skb, ireq->ir_loc_addr, ireq->ir_rmt_addr);
 
+		rcu_read_lock();
 		err = ip_build_and_send_pkt(skb, sk, ireq->ir_loc_addr,
 					    ireq->ir_rmt_addr,
-					    ireq_opt_deref(ireq));
+					    rcu_dereference(ireq->ireq_opt));
+		rcu_read_unlock();
 		err = net_xmit_eval(err);
 	}
 
-- 
2.19.0.605.g01d371f741-goog

^ permalink raw reply related

* Re: [PATCH net-next v2 01/10] net: allow binding socket in a VRF when there's an unbound socket
From: David Ahern @ 2018-10-02 19:39 UTC (permalink / raw)
  To: Mike Manning, netdev; +Cc: Robert Shearman
In-Reply-To: <20181001084320.32453-2-mmanning@vyatta.att-mail.com>

On 10/1/18 2:43 AM, Mike Manning wrote:
> There is no easy way currently for applications that want to receive
> packets in the default VRF to be isolated from packets arriving in
> VRFs, which makes using VRF-unaware applications in a VRF-aware system
> a potential security risk.

please drop that paragraph from the commit message. It is misleading and
wrong.

Without VRF I can start ssh bound to wildcard address and port 22
allowing connections across any interface in the box. If I do not want
that sestup, I have options: e.g., bind ssh to the management address or
install netfilter rules. The same applies with VRF as I mentioned in the
v1 review.

You not liking the options or wanting another option is a different
reason for the change than claiming the current options are a security risk.

^ permalink raw reply

* Re: [PATCH net-next v2 04/10] net: provide a sysctl raw_l3mdev_accept for raw socket lookup with VRFs
From: David Ahern @ 2018-10-02 19:39 UTC (permalink / raw)
  To: Mike Manning, netdev
In-Reply-To: <20181001084320.32453-5-mmanning@vyatta.att-mail.com>

On 10/1/18 2:43 AM, Mike Manning wrote:
> Add a sysctl raw_l3mdev_accept to control raw socket lookup in a manner
> similar to use of tcp_l3mdev_accept for stream and of udp_l3mdev_accept
> for datagram sockets. Have this default to off as this is what users
> expect, given that there is no explicit mechanism to set unmodified
> VRF-unaware application into a default VRF.

It needs to default to on to avoid breaking existing users. With the
raw_l3mdev_accept sysctl set to 0 both 'ping -I <vrf> <addr>' and 'ping
-I <dev> <addr>' fail when they succeed today.

^ permalink raw reply

* Re: [net-next 0/9][pull request] 100GbE Intel Wired LAN Driver Updates 2018-10-02
From: Jeff Kirsher @ 2018-10-02 19:47 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, nhorman, sassmann, jogreene
In-Reply-To: <20181002.114109.2250028418917161969.davem@davemloft.net>

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

On Tue, 2018-10-02 at 11:41 -0700, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Tue,  2 Oct 2018 08:24:38 -0700
> 
> > This series contains updates to ice driver only.
> 
> If there was a problem yo I'll solve it.
> Check out the hook while my DJ revolves it.
> 
> Ice ice baby.
> Ice ice baby.

Thanks, now I can't get that song out of my head...

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* [Patch net-next] net_sched: convert idrinfo->lock from spinlock to a mutex
From: Cong Wang @ 2018-10-02 19:50 UTC (permalink / raw)
  To: netdev; +Cc: jiri, jhs, vladbu, idosch, Cong Wang, Jiri Pirko

In commit ec3ed293e766 ("net_sched: change tcf_del_walker() to take idrinfo->lock")
we move fl_hw_destroy_tmplt() to a workqueue to avoid blocking
with the spinlock held. Unfortunately, this causes a lot of
troubles here:

1. tcf_chain_destroy() could be called right after we queue the work
   but before the work runs. This is a use-after-free.

2. The chain refcnt is already 0, we can't even just hold it again.
   We can check refcnt==1 but it is ugly.

3. The chain with refcnt 0 is still visible in its block, which means
   it could be still found and used!

4. The block has a refcnt too, we can't hold it without introducing a
   proper API either.

We can make it working but the end result is ugly. Instead of wasting
time on reviewing it, let's just convert the troubling spinlock to
a mutex, which allows us to use non-atomic allocations too.

Fixes: ec3ed293e766 ("net_sched: change tcf_del_walker() to take idrinfo->lock")
Reported-by: Ido Schimmel <idosch@idosch.org>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/act_api.h  |  4 ++--
 net/sched/act_api.c    | 44 ++++++++++++++++++++++----------------------
 net/sched/cls_flower.c | 13 ++-----------
 3 files changed, 26 insertions(+), 35 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 1ddff3360592..05c7df41d737 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -13,7 +13,7 @@
 #include <net/netns/generic.h>
 
 struct tcf_idrinfo {
-	spinlock_t	lock;
+	struct mutex	lock;
 	struct idr	action_idr;
 };
 
@@ -117,7 +117,7 @@ int tc_action_net_init(struct tc_action_net *tn,
 	if (!tn->idrinfo)
 		return -ENOMEM;
 	tn->ops = ops;
-	spin_lock_init(&tn->idrinfo->lock);
+	mutex_init(&tn->idrinfo->lock);
 	idr_init(&tn->idrinfo->action_idr);
 	return err;
 }
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 3c7c23421885..55153da00278 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -104,11 +104,11 @@ static int __tcf_action_put(struct tc_action *p, bool bind)
 {
 	struct tcf_idrinfo *idrinfo = p->idrinfo;
 
-	if (refcount_dec_and_lock(&p->tcfa_refcnt, &idrinfo->lock)) {
+	if (refcount_dec_and_mutex_lock(&p->tcfa_refcnt, &idrinfo->lock)) {
 		if (bind)
 			atomic_dec(&p->tcfa_bindcnt);
 		idr_remove(&idrinfo->action_idr, p->tcfa_index);
-		spin_unlock(&idrinfo->lock);
+		mutex_unlock(&idrinfo->lock);
 
 		tcf_action_cleanup(p);
 		return 1;
@@ -200,7 +200,7 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
 	struct tc_action *p;
 	unsigned long id = 1;
 
-	spin_lock(&idrinfo->lock);
+	mutex_lock(&idrinfo->lock);
 
 	s_i = cb->args[0];
 
@@ -235,7 +235,7 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
 	if (index >= 0)
 		cb->args[0] = index + 1;
 
-	spin_unlock(&idrinfo->lock);
+	mutex_unlock(&idrinfo->lock);
 	if (n_i) {
 		if (act_flags & TCA_FLAG_LARGE_DUMP_ON)
 			cb->args[1] = n_i;
@@ -277,18 +277,18 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
 	if (nla_put_string(skb, TCA_KIND, ops->kind))
 		goto nla_put_failure;
 
-	spin_lock(&idrinfo->lock);
+	mutex_lock(&idrinfo->lock);
 	idr_for_each_entry_ul(idr, p, id) {
 		ret = tcf_idr_release_unsafe(p);
 		if (ret == ACT_P_DELETED) {
 			module_put(ops->owner);
 			n_i++;
 		} else if (ret < 0) {
-			spin_unlock(&idrinfo->lock);
+			mutex_unlock(&idrinfo->lock);
 			goto nla_put_failure;
 		}
 	}
-	spin_unlock(&idrinfo->lock);
+	mutex_unlock(&idrinfo->lock);
 
 	if (nla_put_u32(skb, TCA_FCNT, n_i))
 		goto nla_put_failure;
@@ -324,13 +324,13 @@ int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
 	struct tcf_idrinfo *idrinfo = tn->idrinfo;
 	struct tc_action *p;
 
-	spin_lock(&idrinfo->lock);
+	mutex_lock(&idrinfo->lock);
 	p = idr_find(&idrinfo->action_idr, index);
 	if (IS_ERR(p))
 		p = NULL;
 	else if (p)
 		refcount_inc(&p->tcfa_refcnt);
-	spin_unlock(&idrinfo->lock);
+	mutex_unlock(&idrinfo->lock);
 
 	if (p) {
 		*a = p;
@@ -345,10 +345,10 @@ static int tcf_idr_delete_index(struct tcf_idrinfo *idrinfo, u32 index)
 	struct tc_action *p;
 	int ret = 0;
 
-	spin_lock(&idrinfo->lock);
+	mutex_lock(&idrinfo->lock);
 	p = idr_find(&idrinfo->action_idr, index);
 	if (!p) {
-		spin_unlock(&idrinfo->lock);
+		mutex_unlock(&idrinfo->lock);
 		return -ENOENT;
 	}
 
@@ -358,7 +358,7 @@ static int tcf_idr_delete_index(struct tcf_idrinfo *idrinfo, u32 index)
 
 			WARN_ON(p != idr_remove(&idrinfo->action_idr,
 						p->tcfa_index));
-			spin_unlock(&idrinfo->lock);
+			mutex_unlock(&idrinfo->lock);
 
 			tcf_action_cleanup(p);
 			module_put(owner);
@@ -369,7 +369,7 @@ static int tcf_idr_delete_index(struct tcf_idrinfo *idrinfo, u32 index)
 		ret = -EPERM;
 	}
 
-	spin_unlock(&idrinfo->lock);
+	mutex_unlock(&idrinfo->lock);
 	return ret;
 }
 
@@ -431,10 +431,10 @@ void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
 {
 	struct tcf_idrinfo *idrinfo = tn->idrinfo;
 
-	spin_lock(&idrinfo->lock);
+	mutex_lock(&idrinfo->lock);
 	/* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
 	WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
-	spin_unlock(&idrinfo->lock);
+	mutex_unlock(&idrinfo->lock);
 }
 EXPORT_SYMBOL(tcf_idr_insert);
 
@@ -444,10 +444,10 @@ void tcf_idr_cleanup(struct tc_action_net *tn, u32 index)
 {
 	struct tcf_idrinfo *idrinfo = tn->idrinfo;
 
-	spin_lock(&idrinfo->lock);
+	mutex_lock(&idrinfo->lock);
 	/* Remove ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
 	WARN_ON(!IS_ERR(idr_remove(&idrinfo->action_idr, index)));
-	spin_unlock(&idrinfo->lock);
+	mutex_unlock(&idrinfo->lock);
 }
 EXPORT_SYMBOL(tcf_idr_cleanup);
 
@@ -465,14 +465,14 @@ int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
 	int ret;
 
 again:
-	spin_lock(&idrinfo->lock);
+	mutex_lock(&idrinfo->lock);
 	if (*index) {
 		p = idr_find(&idrinfo->action_idr, *index);
 		if (IS_ERR(p)) {
 			/* This means that another process allocated
 			 * index but did not assign the pointer yet.
 			 */
-			spin_unlock(&idrinfo->lock);
+			mutex_unlock(&idrinfo->lock);
 			goto again;
 		}
 
@@ -485,7 +485,7 @@ int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
 		} else {
 			*a = NULL;
 			ret = idr_alloc_u32(&idrinfo->action_idr, NULL, index,
-					    *index, GFP_ATOMIC);
+					    *index, GFP_KERNEL);
 			if (!ret)
 				idr_replace(&idrinfo->action_idr,
 					    ERR_PTR(-EBUSY), *index);
@@ -494,12 +494,12 @@ int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
 		*index = 1;
 		*a = NULL;
 		ret = idr_alloc_u32(&idrinfo->action_idr, NULL, index,
-				    UINT_MAX, GFP_ATOMIC);
+				    UINT_MAX, GFP_KERNEL);
 		if (!ret)
 			idr_replace(&idrinfo->action_idr, ERR_PTR(-EBUSY),
 				    *index);
 	}
-	spin_unlock(&idrinfo->lock);
+	mutex_unlock(&idrinfo->lock);
 	return ret;
 }
 EXPORT_SYMBOL(tcf_idr_check_alloc);
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 92dd5071a708..9aada2d0ef06 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -79,7 +79,6 @@ struct fl_flow_tmplt {
 	struct fl_flow_key mask;
 	struct flow_dissector dissector;
 	struct tcf_chain *chain;
-	struct rcu_work rwork;
 };
 
 struct cls_fl_head {
@@ -1438,20 +1437,12 @@ static void *fl_tmplt_create(struct net *net, struct tcf_chain *chain,
 	return ERR_PTR(err);
 }
 
-static void fl_tmplt_destroy_work(struct work_struct *work)
-{
-	struct fl_flow_tmplt *tmplt = container_of(to_rcu_work(work),
-						 struct fl_flow_tmplt, rwork);
-
-	fl_hw_destroy_tmplt(tmplt->chain, tmplt);
-	kfree(tmplt);
-}
-
 static void fl_tmplt_destroy(void *tmplt_priv)
 {
 	struct fl_flow_tmplt *tmplt = tmplt_priv;
 
-	tcf_queue_work(&tmplt->rwork, fl_tmplt_destroy_work);
+	fl_hw_destroy_tmplt(tmplt->chain, tmplt);
+	kfree(tmplt);
 }
 
 static int fl_dump_key_val(struct sk_buff *skb,
-- 
2.14.4

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox