[PATCH v7 1/8] Talitos: Support for async_tx XOR offload

[Geanta Neag Horia Ioan-B05471]

On Fri, 31 Aug 2012 06:08:05 +0300, Liu Qiang-B32616 <B32616 at freescale.com>
wrote:
> > -----Original Message-----
> > From: Geanta Neag Horia Ioan-B05471
> > Sent: Thursday, August 30, 2012 10:23 PM
> > To: Liu Qiang-B32616; linux-crypto at vger.kernel.org;
> > dan.j.williams at gmail.com; herbert at gondor.hengli.com.au;
> > davem at davemloft.net; linux-kernel at vger.kernel.org; linuxppc-
> > dev at lists.ozlabs.org
> > Cc: Li Yang-R58472; Phillips Kim-R1AAHA; vinod.koul at intel.com;
> > dan.j.williams at intel.com; arnd at arndb.de; gregkh at linuxfoundation.org; Liu
> > Qiang-B32616
> > Subject: RE: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload
> >
> > On Thu, 9 Aug 2012 11:20:48 +0300, qiang.liu at freescale.com wrote:
> > > From: Qiang Liu <qiang.liu at freescale.com>
> > >
> > > Expose Talitos's XOR functionality to be used for RAID parity
> > > calculation via the Async_tx layer.
> > >
> > > Cc: Herbert Xu <herbert at gondor.apana.org.au>
> > > Cc: David S. Miller <davem at davemloft.net>
> > > Signed-off-by: Dipen Dudhat <Dipen.Dudhat at freescale.com>
> > > Signed-off-by: Maneesh Gupta <Maneesh.Gupta at freescale.com>
> > > Signed-off-by: Kim Phillips <kim.phillips at freescale.com>
> > > Signed-off-by: Vishnu Suresh <Vishnu at freescale.com>
> > > Signed-off-by: Qiang Liu <qiang.liu at freescale.com>
> > > ---
> > >  drivers/crypto/Kconfig   |    9 +
> > >  drivers/crypto/talitos.c |  413
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/crypto/talitos.h |   53 ++++++
> > >  3 files changed, 475 insertions(+), 0 deletions(-)
> >

> > > +static int talitos_alloc_chan_resources(struct dma_chan *chan)
> > > +{
> > > +	struct talitos_xor_chan *xor_chan;
> > > +	struct talitos_xor_desc *desc;
> > > +	LIST_HEAD(tmp_list);
> > > +	int i;
> > > +
> > > +	xor_chan = container_of(chan, struct talitos_xor_chan, common);
> > > +
> > > +	if (!list_empty(&xor_chan->free_desc))
> > > +		return xor_chan->total_desc;
> > > +
> > > +	for (i = 0; i < TALITOS_MAX_DESCRIPTOR_NR; i++) {
> > > +		desc = talitos_xor_alloc_descriptor(xor_chan,
> > > +				GFP_KERNEL | GFP_DMA);
> >
> > talitos_xor_alloc_descriptor() is called here without holding
> > the xor_chan->desc_lock and it increments xor_chan->total_desc.
> > Isn't this an issue ?
> 
> No, please refer to the code as below,
> +	list_add_tail(&desc->node, &tmp_list);
> 
> The list is temporary list, it will be merged to xor_chan->free_desc in next step, here is protected
> by lock,
> +	spin_lock_bh(&xor_chan->desc_lock);
> +	list_splice_init(&tmp_list, &xor_chan->free_desc);
> +	spin_unlock_bh(&xor_chan->desc_lock);

I was not referring to the list, but to xor_chan->total_desc variable.
The following access:
talitos_alloc_chan_resources()->talitos_xor_alloc_descriptor()->total_desc++
is not protected by the xor_chan->desc_lock.