Re: [Qemu-devel] [PATCH v5 6/7] block: add generic full disk encryption driver

[Daniel P. Berrange]

On Fri, Mar 18, 2016 at 03:37:12PM +0000, Daniel P. Berrange wrote:
> On Fri, Mar 18, 2016 at 04:19:07PM +0100, Kevin Wolf wrote:
> > Am 18.03.2016 um 15:45 hat Daniel P. Berrange geschrieben:
> > > On Fri, Mar 18, 2016 at 01:09:35PM +0100, Kevin Wolf wrote:
> > > > Am 17.03.2016 um 18:51 hat Daniel P. Berrange geschrieben:
> > > > > +        ret = bdrv_co_readv(bs->file->bs,
> > > > > +                            payload_offset + sector_num,
> > > > > +                            cur_nr_sectors, &hd_qiov);
> > > > > +        qemu_co_mutex_lock(&crypto->lock);
> > > > > +        if (ret < 0) {
> > > > > +            goto cleanup;
> > > > > +        }
> > > > > +
> > > > > +        if (qcrypto_block_decrypt(crypto->block,
> > > > > +                                  sector_num,
> > > > > +                                  cipher_data, cur_nr_sectors * 512,
> > > > > +                                  NULL) < 0) {
> > > > > +            ret = -1;
> > > > 
> > > > Need a real -errno code here.
> > > > 
> > > > > +            goto cleanup;
> > > > > +        }
> > > > 
> > > > ...nor is there one between here and the end of the function.
> > > > 
> > > > So what does this CoMutex protect? If qcrypto_block_decrypt() needs this
> > > > for some reason (it doesn't seem to be touching anything that isn't per
> > > > request, but maybe I'm missing something), would it be clearer to put
> > > > the locking only around that call?
> > > 
> > > This just a result of me blindly copying the locking pattern from
> > > qcow2.c qcow2_co_readv() method without really understanding what
> > > it was protecting.
> > 
> > qcow2 protects a few fields in BDRVQcow2State and metadata that is used
> > and possibly modified by requests. For example, after reading in some
> > metadata, another request could make changes that invalidate it, and we
> > need to protect against that.
> > 
> > I don't see that the crypto driver relies on any global (i.e. not
> > per-request) state either in memory or on disk, except for things that
> > are never changed after open, so the lock might not be needed.
> 
> Actually it does have global state - the QCryptoCipher object that's
> into the QCryptoBlock object must not be used concurrently by multiple
> threads, as each thread will need to initialize different IV data.
> 
> > > If it not possible for two calls to bdrv_co_readv() to run in
> > > parallel, then I can drop this mutex.
> > 
> > They can. The obvious yield point where a coroutine switch can happen is
> > the bdrv_co_readv() call above (but you already unlock for that one).
> > Unless qcrypto_block_decrypt() does some I/O internally, we can't have
> > any other yield points.
> 
> Ok, so we do need the mutex then to protect the cipher object state
> against concurrent use.

Ignore this comment & the one above. Since we're using coroutines the
qcrypto_block_decrypt() calls can't ever be running truely concurrently.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|