MIME-Version: 1.0
References: <20190228155823.24749-1-greearb@candelatech.com>
 <CABvG-CUTn3aHAUUvQ6P+BtaWPtrBeeO8b3vfm5w=hB753_+g0g@mail.gmail.com> <c67eeb9a-3444-dd49-58eb-4e633063fd0e@candelatech.com>
In-Reply-To: <c67eeb9a-3444-dd49-58eb-4e633063fd0e@candelatech.com>
From:   =?UTF-8?Q?Micha=C5=82_Kazior?= <kazikcz@gmail.com>
Date:   Thu, 28 Feb 2019 22:31:58 +0100
Message-ID: <CABvG-CVCZpKHuUufVZqAUWUwDYxJvUNSm9c=eRWMcT4GbAtRig@mail.gmail.com>
Subject: Re: [RFC] ath10k: Fix DMA errors related to beacons (CT FW only)
To:     Ben Greear <greearb@candelatech.com>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Thu, 28 Feb 2019 at 22:11, Ben Greear <greearb@candelatech.com> wrote:
> On 2/28/19 10:54 AM, Micha=C5=82 Kazior wrote:
> > On Thu, 28 Feb 2019 at 16:59, <greearb@candelatech.com> wrote:
> >>
> >> From: Ben Greear <greearb@candelatech.com>
[...]
> >> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wirel=
ess/ath/ath10k/mac.c
> >> index 154dcdabc48a..02a8efa2e783 100644
> >> --- a/drivers/net/wireless/ath/ath10k/mac.c
> >> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> > [...]
> >> @@ -6147,6 +6151,16 @@ static void ath10k_remove_interface(struct ieee=
80211_hw *hw,
> >>                  ath10k_warn(ar, "failed to stop spectral for vdev %i:=
 %d\n",
> >>                              arvif->vdev_id, ret);
> >>
> >> +       if (test_bit(ATH10K_FW_FEATURE_BEACON_TX_CB_CT,
> >> +                     ar->running_fw->fw_file.fw_features)) {
> >> +               int time_left;
> >> +
> >> +               time_left =3D wait_for_completion_timeout(&arvif->beac=
on_tx_done, (3 * HZ));
> >> +               if (!time_left)
> >> +                       ath10k_warn(ar, "WARNING: failed to wait for b=
eacon tx callback for vdev %i: %d\n",
> >> +                                   arvif->vdev_id, ret);
> >> +       }
> >
> > I think this can race against wmi tx credits replenishment and maybe
> > other wmi commands if they get issued (not many of them I suppose
> > though). The ordering would need to be something like this:
> >
> > cpu0                   cpu1
> >                         ath10k_wmi_op_ep_tx_credits
> >                         ieee80211_iterate_active_interfaces_atomic
> >                         ath10k_wmi_beacon_send_ref_nowait
> >                         # preempted
> > ieee80211_do_stop
> > clear sdata running
> > drv_remove_interface
> > wait_for_completion_timeout
> > ath10k_mac_vif_beacon_cleanup
> > free/unmap
> >                         gen_beacon_dma(bcn->xxx)
> >                         reinit_completion()
> >                         ath10k_wmi_cmd_send_nowait()
> >
> > Interestingly, this also shows it has been a possible use-after-free
> > without your patch. This is probably a unlikely scenario because
> > there's a disproportionate amount of code between these flows for this
> > to happen.
>
> Thanks for the thorough review.  One thing, the firmware
> cannot guarantee it can send the event (due to potential WMI event buffer
> exhaustion).  It would be rare that you got unlucky enough to hit the
> case where FW ran out of msg buffers right as you were deleting an
> interface though.

Right.


> As far as I can tell, my patch at least does not make the use-after-free
> scenario worse, so I'm tempted to ignore that for now.

Correct. Your patch doesn't make it worse.


> > There's also a memory leak because ath10k_mac_vif_beacon_free() exits
> > without freeing arvif->beacon if beacon_state is SENDING, which it
> > will be in in the above race case.
>
> Ok, this could be fixed by definitely freeing the mem in the remove_inter=
face
> logic, and is not new to my patch, right?

It's not new to your patch. It's just something I've noticed in the
codebase while reviewing your patch. Freeing it is going to be tricky
with the current design though.


> > With the per-skb beaconing case (which is dead code by the way it
> > seems..) iommu wouldn't be happy either because we'd hand over an
> > unmapped paddr to ath10k_wmi_cmd_send_nowait(). That's assuming kernel
> > didn't crash due to use-after-free on arvif->beacon (stored in `bcn`
> > on stack) before that >
> > This feels like a candidate for rcu if you wanted to have it fixed
> > neatly. It would fix both use-after-free and the locking conundrum -
> > ath10k_remove_interface() could NULL the pointer, call_rcu() and
> > wait_for_completion() in that call handler, and when its called back
> > unmap/free the beacon. This would require a refactor to how beacon
> > stuff is stored/maintained - a helper structure that would need carry
> > the beacon stuff + rcu_head, and this structure would be allocated and
> > assigned with rcu_assign_pointer() to a pointer in ath10k_vif. But I
> > feel like it's asking for unbound allocations or other bugs,
> > especially since I'm...
>
> This callback only happens at all with CT fw, and I definitely don't want=
 to
> deal with an out-of-tree rcu patch.  And, you are not guaranteed to get
> the callback as previously mentioned.  So I think rcu might be overkill
> for this?

You'd still need wait_for_completion with a timeout so even if you
don't get the beacon tx completion it'd still (have to) work. However
*it is* an overkill because the arvif->beacon can be simply removed
making the entire problem go away without adding extra logic.


Micha=C5=82