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);
?
Otherwise the patch looks good to me.
Thanks!
Nicolai
> + }
> }
>
> static void invoke_padata_reorder(struct work_struct *work)
> @@ -342,6 +344,7 @@ static void invoke_padata_reorder(struct work_struct *work)
> pd = container_of(work, struct parallel_data, reorder_work);
> padata_reorder(pd);
> local_bh_enable();
> + padata_put_pd(pd);
> }
>
> static void padata_serial_worker(struct work_struct *serial_work)
>
--
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
(HRB 36809, AG Nürnberg)
