On Sun, May 05, 2019 at 11:06:04AM +0300, Gal Pressman wrote:
> On 03-May-19 15:20, Jason Gunthorpe wrote:
> > On Fri, May 03, 2019 at 12:37:13PM +0300, Gal Pressman wrote:
> >> On 02-May-19 16:55, Jason Gunthorpe wrote:
> >>> On Wed, May 01, 2019 at 01:48:20PM +0300, Gal Pressman wrote:
> >>>
> >>>> +static struct efa_comp_ctx *efa_com_submit_admin_cmd(struct efa_com_admin_queue *aq,
> >>>> + struct efa_admin_aq_entry *cmd,
> >>>> + size_t cmd_size_in_bytes,
> >>>> + struct efa_admin_acq_entry *comp,
> >>>> + size_t comp_size_in_bytes)
> >>>> +{
> >>>> + struct efa_comp_ctx *comp_ctx;
> >>>> +
> >>>> + spin_lock(&aq->sq.lock);
> >>>> + if (!test_bit(EFA_AQ_STATE_RUNNING_BIT, &aq->state)) {
> >>>> + ibdev_err(aq->efa_dev, "Admin queue is closed\n");
> >>>> + spin_unlock(&aq->sq.lock);
> >>>> + return ERR_PTR(-ENODEV);
> >>>> + }
> >>>> +
> >>>> + comp_ctx = __efa_com_submit_admin_cmd(aq, cmd, cmd_size_in_bytes, comp,
> >>>> + comp_size_in_bytes);
> >>>> + spin_unlock(&aq->sq.lock);
> >>>> + if (IS_ERR(comp_ctx))
> >>>> + clear_bit(EFA_AQ_STATE_RUNNING_BIT, &aq->state);
> >>>> +
> >>>> + return comp_ctx;
> >>>> +}
> >>>
> >>> [..]
> >>>
> >>>> +static void efa_com_admin_flush(struct efa_com_dev *edev)
> >>>> +{
> >>>> + struct efa_com_admin_queue *aq = &edev->aq;
> >>>> +
> >>>> + clear_bit(EFA_AQ_STATE_RUNNING_BIT, &aq->state);
> >>>
> >>> This scheme looks use after free racey to me..
> >>
> >> The running bit stops new admin commands from being submitted, clearly the exact
> >> moment in which the bit is cleared is "racy" to submission of admin commands but
> >> that is taken care of.
> >>
> >> The submission of an admin command is atomic as it is guarded by an admin queue
> >> lock.
> >> The same lock is acquired by this flow as well when flushing the admin queue.
> >> After all admin commands have been aborted and we know for sure that
> >> no new
> >
> > The problem is the 'abort' does nothing to ensure parallel threads are
> > no longer touching this memory,
>
> Which memory? The user threads touch the user allocated buffers which are not
> being freed on admin queue destroy.
The memory the other thread is touching is freed a few lines below in
a devm_kfree. The apparent purpose of this code is to make the other
thread stop but does it wrong.
I can't make sense of what this is supposed to be. Either there are
other threads running in parallel and it is just the wrong way to
barrier threads or there are no other threads at this point and this
code doesn't do anything..
Jason
