[dm-devel] [PATCH v2] dm: verity target

[Mandeep Singh Baines]

Steffen Klassert (steffen.klassert at secunet.com) wrote:
> On Fri, Nov 11, 2011 at 09:34:44AM -0800, Mandeep Singh Baines wrote:
> >
> > +/**
> > + * dm_bht_compute_hash: hashes a page of data
> > + */
> > +static int dm_bht_compute_hash(struct dm_bht *bht, struct page *pg,
> > +			       unsigned int offset, u8 *digest)
> > +{
> > +	struct shash_desc *hash_desc = bht->hash_desc[smp_processor_id()];
> > +	void *data;
> > +	int err;
> 
> You don't need to have one shash_desc per cpu. As the shash interface is
> synchronous, you could either allocate a new shash_desc with every call
> to dm_bht_compute_hash, or just place the shash_desc with the context to
> the stack of this function. The easiest thing would be to add the
> shash transformation direct to dm_bht and then something like
> 
>         struct {
>                 struct shash_desc desc;        
>                 char ctx[crypto_shash_descsize(bht->tfm)];                                                
>         } hash_desc;
> 
> 	hash_desc.desc.tfm = bmt->tfm;
> 
> The intermediate and final digest values are stored to hash_desc.ctx.
> This is local, hence it's reentrant.
> 

Hi Steffen,

We use a hash per CPU to avoid allocating and freeing a context per
computation. We could allocate the context on the stack as you suggest.
However, we were worried about overflowing the stack. The context can be
quite large (close to 200 bytes for sha512).

We use a hash per CPU because there can be at most one hash in progress
on each CPU at any one time.

Regards,
Mandeep

> > +
> > +	/* Note, this is synchronous. */
> > +	if (crypto_shash_init(hash_desc)) {
> > +		DMCRIT("failed to reinitialize crypto hash (proc:%d)",
> > +			smp_processor_id());
> > +		return -EINVAL;
> > +	}
> > +	data = kmap_atomic(pg);
> > +	err = crypto_shash_update(hash_desc, data + offset, PAGE_SIZE);
> > +	kunmap_atomic(data);
> > +	if (err) {
> > +		DMCRIT("crypto_hash_update failed");
> > +		return -EINVAL;
> > +	}
> > +	if (crypto_shash_update(hash_desc, bht->salt, sizeof(bht->salt))) {
> > +		DMCRIT("crypto_hash_update failed");
> > +		return -EINVAL;
> > +	}
> > +	if (crypto_shash_final(hash_desc, digest)) {
> > +		DMCRIT("crypto_hash_final failed");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> [snip]
> 
> > + * TODO(wad): All hash storage memory is pre-allocated and freed once an
> > + * entire branch has been verified.
> > + */
> > +struct dm_bht {
> > +	/* Configured values */
> > +	int depth;  /* Depth of the tree including the root */
> > +	unsigned int block_count;  /* Number of blocks hashed */
> > +	unsigned int block_size;  /* Size of a hash block */
> > +	char hash_alg[CRYPTO_MAX_ALG_NAME];
> > +	unsigned char salt[DM_BHT_SALT_SIZE];
> > +
> > +	/* Computed values */
> > +	unsigned int node_count;  /* Data size (in hashes) for each entry */
> > +	unsigned int node_count_shift;  /* first bit set - 1 */
> > +	/* There is one per CPU so that verified can be simultaneous. */
> > +	struct shash_desc *hash_desc[NR_CPUS];  /* Container for the hash alg */
> 
> As mentioned above, you don't need to have one shash_desc per cpu.
> Replace this with a 'struct crypto_shash *tfm;' and add this transformation
> to the local hash_desc.
>