netdev - Re: [PATCH bpf 3/4] net/i40e: Fix concurrency issues between config flow and XSK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 6 Dec 2019 10:03:06 +0100
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Maxim Mikityanskiy <maximmi@...lanox.com>
Cc:     Björn Töpel <bjorn.topel@...el.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "David S. Miller" <davem@...emloft.net>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>
Subject: Re: [PATCH bpf 3/4] net/i40e: Fix concurrency issues between config
 flow and XSK

On Thu, 5 Dec 2019 at 16:52, Maxim Mikityanskiy <maximmi@...lanox.com> wrote:
>
> Use synchronize_rcu to wait until the XSK wakeup function finishes
> before destroying the resources it uses:
>
> 1. i40e_down already calls synchronize_rcu. On i40e_down either
> __I40E_VSI_DOWN or __I40E_CONFIG_BUSY is set. Check the latter in
> i40e_xsk_async_xmit (the former is already checked there).
>
> 2. After switching the XDP program, call synchronize_rcu to let
> i40e_xsk_async_xmit exit before the XDP program is freed.
>
> 3. Changing the number of channels brings the interface down (see
> i40e_prep_for_reset and i40e_pf_quiesce_all_vsi).
>
> 4. Disabling UMEM sets __I40E_CONFIG_BUSY, too.
>
> Signed-off-by: Maxim Mikityanskiy <maximmi@...lanox.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 7 +++++--
>  drivers/net/ethernet/intel/i40e/i40e_xsk.c  | 4 ++++
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 1ccabeafa44c..afa3a99e68e1 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -6823,8 +6823,8 @@ void i40e_down(struct i40e_vsi *vsi)
>         for (i = 0; i < vsi->num_queue_pairs; i++) {
>                 i40e_clean_tx_ring(vsi->tx_rings[i]);
>                 if (i40e_enabled_xdp_vsi(vsi)) {
> -                       /* Make sure that in-progress ndo_xdp_xmit
> -                        * calls are completed.
> +                       /* Make sure that in-progress ndo_xdp_xmit and
> +                        * ndo_xsk_async_xmit calls are completed.
>                          */
>                         synchronize_rcu();
>                         i40e_clean_tx_ring(vsi->xdp_rings[i]);
> @@ -12545,6 +12545,9 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
>                 i40e_prep_for_reset(pf, true);
>
>         old_prog = xchg(&vsi->xdp_prog, prog);
> +       if (old_prog)
> +               /* Wait until ndo_xsk_async_xmit completes. */
> +               synchronize_rcu();

This is not needed -- please correct me if I'm missing something! If
we're disabling XDP, the need_reset-clause will take care or the
proper synchronization. And we don't need to worry about old_prog for
the swap-XDP case, because bpf_prog_put() does cleanup with
call_rcu().


>
>         if (need_reset)
>                 i40e_reset_and_rebuild(pf, true, true);
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index d07e1a890428..f73cd917c44f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> @@ -787,8 +787,12 @@ int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
>  {
>         struct i40e_netdev_priv *np = netdev_priv(dev);
>         struct i40e_vsi *vsi = np->vsi;
> +       struct i40e_pf *pf = vsi->back;
>         struct i40e_ring *ring;
>
> +       if (test_bit(__I40E_CONFIG_BUSY, pf->state))
> +               return -ENETDOWN;
> +

You right that we need to check for BUSY, since the XDP ring might be
stale! Thanks for catching this! Can you respin this patch, with just
this hunk? (Unless I'm wrong! :-))



>         if (test_bit(__I40E_VSI_DOWN, vsi->state))
>                 return -ENETDOWN;
>
> --
> 2.20.1
>