Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 26 Jul 2022 00:13:37 -0400
From:   Mike Snitzer <snitzer@redhat.com>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     Mike Snitzer <snitzer@kernel.org>, linux-kernel@vger.kernel.org,
        Nathan Huckleberry <nhuck@google.com>, dm-devel@redhat.com,
        Sami Tolvanen <samitolvanen@google.com>,
        Alasdair Kergon <agk@redhat.com>
Subject: Re: [PATCH 3/3] dm-verity: Add try_verify_in_tasklet
Message-ID: <Yt9p8VupWC0ZlFv4@redhat.com>
References: <20220722093823.4158756-1-nhuck@google.com>
 <20220722093823.4158756-4-nhuck@google.com>
 <Yt9KTzXUeA8xAiGv@redhat.com>
 <Yt9aGRXNNPGZFvld@sol.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yt9aGRXNNPGZFvld@sol.localdomain>
Precedence: bulk

On Mon, Jul 25 2022 at 11:06P -0400,
Eric Biggers <ebiggers@kernel.org> wrote:

> On Mon, Jul 25, 2022 at 09:58:39PM -0400, Mike Snitzer wrote:
> > 
> > > @@ -1156,7 +1217,7 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
> > >  		goto bad;
> > >  	}
> > >  
> > > -	v->tfm = crypto_alloc_ahash(v->alg_name, 0, 0);
> > > +	v->tfm = crypto_alloc_ahash(v->alg_name, 0, CRYPTO_ALG_ASYNC);
> > >  	if (IS_ERR(v->tfm)) {
> > >  		ti->error = "Cannot initialize hash function";
> > >  		r = PTR_ERR(v->tfm);
> > 
> > This hunk that adds the CRYPTO_ALG_ASYNC flag _seems_ unrelated.
> 
> I believe it's needed to ensure that only a synchronous algorithm is allocated,
> so that verity_hash_update() doesn't have to sleep during the tasklet.  It
> should be conditional on v->use_tasklet, though.

Ah yes, it is a mask, that makes sense.

I can now see why it was being set unconditionally given dm-verity's
optional ctr args aren't processed until after the crypto_alloc_ahash() call. 
And of course verity_parse_opt_args() depends on non-optional args
related to the tfm.... gah!

Do you have a sense for what the implications are for always setting
CRYPTO_ALG_ASYNC like Nathan had? Will it disallow certain tfm that
may already be in use by some users?

> > @@ -321,14 +320,12 @@ static int verity_verify_level(struct dm_verity *v, struct dm_verity_io *io,
> >  		if (likely(memcmp(verity_io_real_digest(v, io), want_digest,
> >  				  v->digest_size) == 0))
> >  			aux->hash_verified = 1;
> > -		else if (io->in_tasklet) {
> > +		else if (io->in_tasklet)
> >  			/*
> >  			 * FEC code cannot be run in a tasklet since it may
> > -			 * sleep.  We need to resume execution in a work-queue
> > -			 * to handle FEC.
> > +			 * sleep, so fallback to using a work-queue.
> >  			 */
> >  			return -EAGAIN;
> > -		}
> 
> 
> Doesn't this need to be:
> 
> 			r = -EAGAIN;
> 			goto release_ret_r;

Yes, good catch.

Thanks,
Mike