Re: [dm-devel] [PATCH v5 12/19] dm: move dm-verity to generic async completion

[Gilad Ben-Yossef]

On Sat, Aug 19, 2017 at 11:08 PM, Mikulas Patocka  wrote:
>
>
>
> On Mon, 14 Aug 2017, Gilad Ben-Yossef wrote:
>
> > dm-verity is starting async. crypto ops and waiting for them to complete.
> > Move it over to generic code doing the same.
> >
> > This also fixes a possible data coruption bug created by the
> > use of wait_for_completion_interruptible() without dealing
> > correctly with an interrupt aborting the wait prior to the
> > async op finishing.
>
> What is the exact problem there? The interruptible sleep is called from a
> workqueue and workqueues have all signals blocked. Are signals unblocked
> for some reason there?


I should have used "potential" rather then "possible". My bad.

My point was that the use of wait_for_completion_interruptible() is wrong
because we are not really ready to handle a signal *if* it arrives.

Yes, since we are being called from a workqueue this will not happen, but:

a. we (or rather I, since I wrote the offending code...) shouldn't call
*_interruptible() in a function that is executing with signals masked
unconditionally. It makes not sense.

b. someone might move the context this is executing in in the future, or make
some other change that will enable signals and create a very subtle bug.

>
>
> Should there be another patch for stable kernels that fixes the
> interruptible sleep?


No, for the reason you pointed yourself - signals are blocked here, so this
is a potential bug only but can't be triggered.

I already sent a patch set fixing similar places that had the
same issue and it went into stable - e.g.
https://patchwork.kernel.org/patch/9781693/

Thanks,
Gilad

>
>
> Mikulas
>
> > Signed-off-by: Gilad Ben-Yossef 
> > ---
> >  drivers/md/dm-verity-target.c | 81 
> > +++
> >  drivers/md/dm-verity.h|  5 ---
> >  2 files changed, 20 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> > index 79f18d4..8df08a8 100644
> > --- a/drivers/md/dm-verity-target.c
> > +++ b/drivers/md/dm-verity-target.c
> > @@ -92,74 +92,33 @@ static sector_t verity_position_at_level(struct 
> > dm_verity *v, sector_t block,
> >   return block >> (level * v->hash_per_block_bits);
> >  }
> >
> > -/*
> > - * Callback function for asynchrnous crypto API completion notification
> > - */
> > -static void verity_op_done(struct crypto_async_request *base, int err)
> > -{
> > - struct verity_result *res = (struct verity_result *)base->data;
> > -
> > - if (err == -EINPROGRESS)
> > - return;
> > -
> > - res->err = err;
> > - complete(>completion);
> > -}
> > -
> > -/*
> > - * Wait for async crypto API callback
> > - */
> > -static inline int verity_complete_op(struct verity_result *res, int ret)
> > -{
> > - switch (ret) {
> > - case 0:
> > - break;
> > -
> > - case -EINPROGRESS:
> > - case -EBUSY:
> > - ret = wait_for_completion_interruptible(>completion);
> > - if (!ret)
> > - ret = res->err;
> > - reinit_completion(>completion);
> > - break;
> > -
> > - default:
> > - DMERR("verity_wait_hash: crypto op submission failed: %d", 
> > ret);
> > - }
> > -
> > - if (unlikely(ret < 0))
> > - DMERR("verity_wait_hash: crypto op failed: %d", ret);
> > -
> > - return ret;
> > -}
> > -
> >  static int verity_hash_update(struct dm_verity *v, struct ahash_request 
> > *req,
> >   const u8 *data, size_t len,
> > - struct verity_result *res)
> > + struct crypto_wait *wait)
> >  {
> >   struct scatterlist sg;
> >
> >   sg_init_one(, data, len);
> >   ahash_request_set_crypt(req, , NULL, len);
> >
> > - return verity_complete_op(res, crypto_ahash_update(req));
> > + return crypto_wait_req(crypto_ahash_update(req), wait);
> >  }
> >
> >  /*
> >   * Wrapper for crypto_ahash_init, which handles verity salting.
> >   */
> >  static int verity_hash_init(struct dm_verity *v, struct ahash_request *req,
> > - struct verity_result *res)
> > + struct crypto_wait *wait)
> >  {
> >   int r;
> >
> >   ahash_request_set_tfm(req, v->tfm);
> >   ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP |
> >   CRYPTO_TFM_REQ_MAY_BACKLOG,
> > - verity_op_done, (void *)res);
> > - init_completion(>completion);
> > + crypto_req_done, (void *)wait);
> > + crypto_init_wait(wait);
> >
> > - r = verity_complete_op(res, crypto_ahash_init(req));
> > + r = crypto_wait_req(crypto_ahash_init(req), wait);
> >
> >   if (unlikely(r < 0)) {
> >   DMERR("crypto_ahash_init failed: %d", r);
> > @@ -167,18 +126,18 @@ static 

Re: [dm-devel] [PATCH v5 12/19] dm: move dm-verity to generic async completion

[Mikulas Patocka]

On Mon, 14 Aug 2017, Gilad Ben-Yossef wrote:

> dm-verity is starting async. crypto ops and waiting for them to complete.
> Move it over to generic code doing the same.
> 
> This also fixes a possible data coruption bug created by the
> use of wait_for_completion_interruptible() without dealing
> correctly with an interrupt aborting the wait prior to the
> async op finishing.

What is the exact problem there? The interruptible sleep is called from a 
workqueue and workqueues have all signals blocked. Are signals unblocked 
for some reason there?

Should there be another patch for stable kernels that fixes the 
interruptible sleep?

Mikulas

> Signed-off-by: Gilad Ben-Yossef 
> ---
>  drivers/md/dm-verity-target.c | 81 
> +++
>  drivers/md/dm-verity.h|  5 ---
>  2 files changed, 20 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 79f18d4..8df08a8 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -92,74 +92,33 @@ static sector_t verity_position_at_level(struct dm_verity 
> *v, sector_t block,
>   return block >> (level * v->hash_per_block_bits);
>  }
>  
> -/*
> - * Callback function for asynchrnous crypto API completion notification
> - */
> -static void verity_op_done(struct crypto_async_request *base, int err)
> -{
> - struct verity_result *res = (struct verity_result *)base->data;
> -
> - if (err == -EINPROGRESS)
> - return;
> -
> - res->err = err;
> - complete(>completion);
> -}
> -
> -/*
> - * Wait for async crypto API callback
> - */
> -static inline int verity_complete_op(struct verity_result *res, int ret)
> -{
> - switch (ret) {
> - case 0:
> - break;
> -
> - case -EINPROGRESS:
> - case -EBUSY:
> - ret = wait_for_completion_interruptible(>completion);
> - if (!ret)
> - ret = res->err;
> - reinit_completion(>completion);
> - break;
> -
> - default:
> - DMERR("verity_wait_hash: crypto op submission failed: %d", ret);
> - }
> -
> - if (unlikely(ret < 0))
> - DMERR("verity_wait_hash: crypto op failed: %d", ret);
> -
> - return ret;
> -}
> -
>  static int verity_hash_update(struct dm_verity *v, struct ahash_request *req,
>   const u8 *data, size_t len,
> - struct verity_result *res)
> + struct crypto_wait *wait)
>  {
>   struct scatterlist sg;
>  
>   sg_init_one(, data, len);
>   ahash_request_set_crypt(req, , NULL, len);
>  
> - return verity_complete_op(res, crypto_ahash_update(req));
> + return crypto_wait_req(crypto_ahash_update(req), wait);
>  }
>  
>  /*
>   * Wrapper for crypto_ahash_init, which handles verity salting.
>   */
>  static int verity_hash_init(struct dm_verity *v, struct ahash_request *req,
> - struct verity_result *res)
> + struct crypto_wait *wait)
>  {
>   int r;
>  
>   ahash_request_set_tfm(req, v->tfm);
>   ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP |
>   CRYPTO_TFM_REQ_MAY_BACKLOG,
> - verity_op_done, (void *)res);
> - init_completion(>completion);
> + crypto_req_done, (void *)wait);
> + crypto_init_wait(wait);
>  
> - r = verity_complete_op(res, crypto_ahash_init(req));
> + r = crypto_wait_req(crypto_ahash_init(req), wait);
>  
>   if (unlikely(r < 0)) {
>   DMERR("crypto_ahash_init failed: %d", r);
> @@ -167,18 +126,18 @@ static int verity_hash_init(struct dm_verity *v, struct 
> ahash_request *req,
>   }
>  
>   if (likely(v->salt_size && (v->version >= 1)))
> - r = verity_hash_update(v, req, v->salt, v->salt_size, res);
> + r = verity_hash_update(v, req, v->salt, v->salt_size, wait);
>  
>   return r;
>  }
>  
>  static int verity_hash_final(struct dm_verity *v, struct ahash_request *req,
> -  u8 *digest, struct verity_result *res)
> +  u8 *digest, struct crypto_wait *wait)
>  {
>   int r;
>  
>   if (unlikely(v->salt_size && (!v->version))) {
> - r = verity_hash_update(v, req, v->salt, v->salt_size, res);
> + r = verity_hash_update(v, req, v->salt, v->salt_size, wait);
>  
>   if (r < 0) {
>   DMERR("verity_hash_final failed updating salt: %d", r);
> @@ -187,7 +146,7 @@ static int verity_hash_final(struct dm_verity *v, struct 
> ahash_request *req,
>   }
>  
>   ahash_request_set_crypt(req, NULL, digest, 0);
> - r = verity_complete_op(res, crypto_ahash_final(req));
> + r = crypto_wait_req(crypto_ahash_final(req), wait);
>  out:
>   return r;
>