Re: [v2 PATCH 1/16] crypto: skcipher - Add skcipher walk interface

[Eric Biggers <ebiggers@google.com>]

Hi Herbert,

On Sun, Nov 13, 2016 at 07:45:32PM +0800, Herbert Xu wrote:
> +int skcipher_walk_done(struct skcipher_walk *walk, int err)
> +{
> +	unsigned int nbytes = 0;
> +	unsigned int n = 0;
> +
> +	if (likely(err >= 0)) {
> +		n = walk->nbytes - err;
> +		nbytes = walk->total - n;
> +	}
> +
> +	if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
> +				    SKCIPHER_WALK_SLOW |
> +				    SKCIPHER_WALK_COPY |
> +				    SKCIPHER_WALK_DIFF)))) {
> +unmap_src:
> +		skcipher_unmap_src(walk);

Isn't it wrong to unmap the buffers when err < 0?  They won't have been mapped
yet, e.g. if the 'skcipher_walk_done(walk, -EINVAL)' case is hit.

> +	/* Short-circuit for the common/fast path. */
> +	if (!(((unsigned long)walk->iv ^ (unsigned long)walk->oiv) |
> +	     ((unsigned long)walk->buffer ^ (unsigned long)walk->page) |
> +	     (unsigned long)walk->page))
> +		goto out;

This is really saying that the IV wasn't copied and that walk->buffer and
walk->page are both NULL.  But if walk->buffer is NULL then the IV wasn't
copied.  So this can be simplified to:

	if (!((unsigned long)walk->buffer | (unsigned long)walk->page))
		goto out;

> +int skcipher_walk_next(struct skcipher_walk *walk)
> +{

Shouldn't this be static?  There's even a static declaration earlier in the
file.

> +static int skcipher_next_slow(struct skcipher_walk *walk)
> +{
> +	bool phys = walk->flags & SKCIPHER_WALK_PHYS;
> +	unsigned alignmask = walk->alignmask;
> +	unsigned bsize = walk->chunksize;
...
> +	walk->nbytes = bsize;

skcipher_next_slow() is always setting up 'chunksize' bytes to be processed.
Isn't this wrong in the 'blocksize < chunksize' case because fewer than
'chunksize' bytes may need to be processed?

> +static inline void *skcipher_map(struct scatter_walk *walk)
> +{
> +	struct page *page = scatterwalk_page(walk);
> +
> +	return (PageHighMem(page) ? kmap_atomic(page) : page_address(page)) +
> +	       offset_in_page(walk->offset);
> +}

Is the PageHighMem() optimization really worthwhile?  It will skip kmap_atomic()
and hence preempt_disable() for non-highmem pages, so it may make it harder to
find bugs where users are sleeping when they shouldn't be.  It also seems
inconsistent that skcipher_map() will do this optimization but scatterwalk_map()
won't.

> +		if (!walk->page) {
> +			gfp_t gfp = skcipher_walk_gfp(walk);
> +
> +			walk->page = (void *)__get_free_page(gfp);
> +			if (!walk->page)
> +				goto slow_path;
> +		}
> +
> +		walk->nbytes = min_t(unsigned, n,
> +				     PAGE_SIZE - offset_in_page(walk->page));

walk->page will be page-aligned, so there's no need for offset_in_page().  Also,
'n' has already been clamped to at most one page.  So AFAICS this can simply be
'walk->nbytes = n;'.

> +int skcipher_walk_done(struct skcipher_walk *walk, int err);
...
> +void skcipher_walk_complete(struct skcipher_walk *walk, int err);

The naming is confusing: "done" vs. "complete".  They can easily be mixed up.
Would it make more sense to have something with "async" in the name for the
second one, like skcipher_walk_async_done()?

Eric