padata: Use RCU when fetching pd from do_serial
diff mbox series

Message ID 20190716125704.l2jolyyd3bue6hhn@gondor.apana.org.au
State Superseded
Headers show
Series
  • padata: Use RCU when fetching pd from do_serial
Related show

Commit Message

Herbert Xu July 16, 2019, 12:57 p.m. UTC
On Tue, Jul 16, 2019 at 01:14:10PM +0200, Steffen Klassert wrote:
>
> Maybe we can fix it if we call padata_free_pd() from
> padata_serial_worker() when it sent out the last object.

How about using RCU?

We still need to fix up the refcnt if it's supposed to limit the
overall number of outstanding requests.

---8<---
The function padata_do_serial uses parallel_data without obeying
the RCU rules around its life-cycle.  This means that a concurrent
padata_replace call can result in a crash.

This patch fixes it by using RCU just as we do in padata_do_parallel.

Fixes: 16295bec6398 ("padata: Generic parallelization/...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

Herbert Xu July 16, 2019, 1:09 p.m. UTC | #1
On Tue, Jul 16, 2019 at 08:57:04PM +0800, Herbert Xu wrote:
>
> How about using RCU?
> 
> We still need to fix up the refcnt if it's supposed to limit the
> overall number of outstanding requests.

Hmm, it doesn't work because the refcnt is attached to the old
pd.  That shouldn't be a problem though as we could simply ignore
the refcnt in padata_flush_queue.

However, I think this leads to another bug in that pcrypt doesn't
support dm-crypt properly.  It never does the backlog stuff and
therefore can't guarantee reliable processing which dm-crypt requires.

Is it intentional to only allow pcrypt for IPsec?

Cheers,
Peter Zijlstra July 16, 2019, 1:15 p.m. UTC | #2
On Tue, Jul 16, 2019 at 08:57:04PM +0800, Herbert Xu wrote:
> On Tue, Jul 16, 2019 at 01:14:10PM +0200, Steffen Klassert wrote:
> >
> > Maybe we can fix it if we call padata_free_pd() from
> > padata_serial_worker() when it sent out the last object.
> 
> How about using RCU?
> 
> We still need to fix up the refcnt if it's supposed to limit the
> overall number of outstanding requests.
> 
> ---8<---
> The function padata_do_serial uses parallel_data without obeying
> the RCU rules around its life-cycle.  This means that a concurrent
> padata_replace call can result in a crash.
> 
> This patch fixes it by using RCU just as we do in padata_do_parallel.
> 
> Fixes: 16295bec6398 ("padata: Generic parallelization/...")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

> diff --git a/kernel/padata.c b/kernel/padata.c
> index 2d2fddbb7a4c..fb5dd1210d2b 100644
> --- a/kernel/padata.c
> +++ b/kernel/padata.c
> @@ -128,7 +128,7 @@ int padata_do_parallel(struct padata_instance *pinst,
>  
>  	err = 0;
>  	atomic_inc(&pd->refcnt);
> -	padata->pd = pd;
> +	padata->inst = pinst;
>  	padata->cb_cpu = cb_cpu;
>  
>  	target_cpu = padata_cpu_hash(pd);
> @@ -367,7 +368,7 @@ void padata_do_serial(struct padata_priv *padata)
>  	struct parallel_data *pd;
>  	int reorder_via_wq = 0;
>  
> -	pd = padata->pd;
> +	pd = rcu_dereference_bh(padata->inst->pd);
>  
>  	cpu = get_cpu();
>  

That's weird for not having a matching assign and lacking comments to
explain that.
Herbert Xu July 16, 2019, 1:18 p.m. UTC | #3
On Tue, Jul 16, 2019 at 03:15:20PM +0200, Peter Zijlstra wrote:
>
> > @@ -367,7 +368,7 @@ void padata_do_serial(struct padata_priv *padata)
> >  	struct parallel_data *pd;
> >  	int reorder_via_wq = 0;
> >  
> > -	pd = padata->pd;
> > +	pd = rcu_dereference_bh(padata->inst->pd);
> >  
> >  	cpu = get_cpu();
> >  
> 
> That's weird for not having a matching assign and lacking comments to
> explain that.

There is a matching rcu_assign_pointer.  But we should add some
RCU markers.

Or perhaps you're misreading the level of indirections :)

Cheers,
Peter Zijlstra July 16, 2019, 1:50 p.m. UTC | #4
On Tue, Jul 16, 2019 at 09:18:07PM +0800, Herbert Xu wrote:
> On Tue, Jul 16, 2019 at 03:15:20PM +0200, Peter Zijlstra wrote:
> >
> > > @@ -367,7 +368,7 @@ void padata_do_serial(struct padata_priv *padata)
> > >  	struct parallel_data *pd;
> > >  	int reorder_via_wq = 0;
> > >  
> > > -	pd = padata->pd;
> > > +	pd = rcu_dereference_bh(padata->inst->pd);
> > >  
> > >  	cpu = get_cpu();
> > >  
> > 
> > That's weird for not having a matching assign and lacking comments to
> > explain that.
> 
> There is a matching rcu_assign_pointer.  But we should add some
> RCU markers.
> 
> Or perhaps you're misreading the level of indirections :)

Could well be, I'm not much familiar with this code; I'll look more
careful.
Herbert Xu July 16, 2019, 1:52 p.m. UTC | #5
On Tue, Jul 16, 2019 at 03:50:10PM +0200, Peter Zijlstra wrote:
>
> Could well be, I'm not much familiar with this code; I'll look more
> careful.

My patch is broken anyway.  We can't just switch pd structures
as the code relies on going to exactly the same CPU on the starting
pd to ensure ordering.

Cheers,
Steffen Klassert July 17, 2019, 8:28 a.m. UTC | #6
On Tue, Jul 16, 2019 at 09:09:28PM +0800, Herbert Xu wrote:
> 
> However, I think this leads to another bug in that pcrypt doesn't
> support dm-crypt properly.  It never does the backlog stuff and
> therefore can't guarantee reliable processing which dm-crypt requires.
> 
> Is it intentional to only allow pcrypt for IPsec?

I had a patch to support crypto backlog some years ago,
but testing with dm-crypt did not show any performance
improvement. So I decided to just skip that patch because
it added code for no need.
Herbert Xu July 17, 2019, 8:47 a.m. UTC | #7
On Wed, Jul 17, 2019 at 10:28:15AM +0200, Steffen Klassert wrote:
>
> I had a patch to support crypto backlog some years ago,
> but testing with dm-crypt did not show any performance
> improvement. So I decided to just skip that patch because
> it added code for no need.

Well pcrypt is part of the API so it needs to obey the rules.
So even if it wouldn't make sense to use dm-crypt with pcrypt
it still needs to do the right thing.

Thanks,`
Steffen Klassert July 17, 2019, 8:53 a.m. UTC | #8
On Wed, Jul 17, 2019 at 04:47:40PM +0800, Herbert Xu wrote:
> On Wed, Jul 17, 2019 at 10:28:15AM +0200, Steffen Klassert wrote:
> >
> > I had a patch to support crypto backlog some years ago,
> > but testing with dm-crypt did not show any performance
> > improvement. So I decided to just skip that patch because
> > it added code for no need.
> 
> Well pcrypt is part of the API so it needs to obey the rules.
> So even if it wouldn't make sense to use dm-crypt with pcrypt
> it still needs to do the right thing.

Ok, makes sense. The old patch still exists:

https://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk.git/commit/?h=net-next-pcrypt&id=5909a88ccef6f4c78fe9969160155a8f0ce8fee7

Let me see if I can respin it...

Patch
diff mbox series

diff --git a/include/linux/padata.h b/include/linux/padata.h
index 5d13d25da2c8..952f6514dd72 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -35,7 +35,7 @@ 
  * struct padata_priv -  Embedded to the users data structure.
  *
  * @list: List entry, to attach to the padata lists.
- * @pd: Pointer to the internal control structure.
+ * @inst: Pointer to the overall control structure.
  * @cb_cpu: Callback cpu for serializatioon.
  * @cpu: Cpu for parallelization.
  * @seq_nr: Sequence number of the parallelized data object.
@@ -45,7 +45,7 @@ 
  */
 struct padata_priv {
 	struct list_head	list;
-	struct parallel_data	*pd;
+	struct padata_instance	*inst;
 	int			cb_cpu;
 	int			cpu;
 	int			info;
diff --git a/kernel/padata.c b/kernel/padata.c
index 2d2fddbb7a4c..fb5dd1210d2b 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -128,7 +128,7 @@  int padata_do_parallel(struct padata_instance *pinst,
 
 	err = 0;
 	atomic_inc(&pd->refcnt);
-	padata->pd = pd;
+	padata->inst = pinst;
 	padata->cb_cpu = cb_cpu;
 
 	target_cpu = padata_cpu_hash(pd);
@@ -367,7 +368,7 @@  void padata_do_serial(struct padata_priv *padata)
 	struct parallel_data *pd;
 	int reorder_via_wq = 0;
 
-	pd = padata->pd;
+	pd = rcu_dereference_bh(padata->inst->pd);
 
 	cpu = get_cpu();