On Wed, Nov 09, 2022 at 02:03:29PM +0100, Nicolai Stange wrote:
> Daniel Jordan <daniel.m.jordan@xxxxxxxxxx> writes:
>
> > On Wed, Oct 19, 2022 at 10:37:08AM +0200, Nicolai Stange wrote:
> >> Even though the parallel_data "pd" instance passed to padata_reorder() is
> >> guaranteed to exist as per the reference held by its callers, the same is
> >> not true for the associated padata_shell, pd->ps. More specifically, once
> >> the last padata_priv request has been completed, either at entry from
> >> padata_reorder() or concurrently to it, the padata API users are well
> >> within their right to free the padata_shell instance.
> >
> > The synchronize_rcu change seems to make padata_reorder safe from freed
> > ps's with the exception of a straggler reorder_work. For that, I think
> > something like this hybrid of your code and mine is enough to plug the
> > hole. It's on top of 1-2 and my hunk from 3. It has to take an extra
> > ref on pd, but only in the rare case where the reorder work is used.
> > Thoughts?
> >
> > diff --git a/kernel/padata.c b/kernel/padata.c
> > index cd6740ae6629..f14c256a0ee3 100644
> > --- a/kernel/padata.c
> > +++ b/kernel/padata.c
> > @@ -277,7 +277,7 @@ static struct padata_priv *padata_find_next(struct parallel_data *pd,
> >
> > static void padata_reorder(struct parallel_data *pd)
> > {
> > - struct padata_instance *pinst = pd->ps->pinst;
> > + struct padata_instance *pinst;
> > int cb_cpu;
> > struct padata_priv *padata;
> > struct padata_serial_queue *squeue;
> > @@ -314,7 +314,7 @@ static void padata_reorder(struct parallel_data *pd)
> > list_add_tail(&padata->list, &squeue->serial.list);
> > spin_unlock(&squeue->serial.lock);
> >
> > - queue_work_on(cb_cpu, pinst->serial_wq, &squeue->work);
> > + queue_work_on(cb_cpu, pd->ps->pinst->serial_wq, &squeue->work);
> > }
> >
> > spin_unlock_bh(&pd->lock);
> > @@ -330,8 +330,10 @@ static void padata_reorder(struct parallel_data *pd)
> > smp_mb();
> >
> > reorder = per_cpu_ptr(pd->reorder_list, pd->cpu);
> > - if (!list_empty(&reorder->list) && padata_find_next(pd, false))
> > - queue_work(pinst->serial_wq, &pd->reorder_work);
> > + if (!list_empty(&reorder->list) && padata_find_next(pd, false)) {
> > + if (queue_work(pd->ps->pinst->serial_wq, &pd->reorder_work))
> > + padata_get_pd(pd);
>
> As the reorder_work can start running immediately after having been
> submitted, wouldn't it be more correct to do something like
>
> padata_get_pd(pd);
> if (!queue_work(pd->ps->pinst->serial_wq, &pd->reorder_work))
> padata_put_pd(pd);
>
> ?
Yes, that's better, and all the above can go in your next version too.
