On Tue, 25 Sep 2018 at 18:03, Jens Axboe <axboe@xxxxxxxxx> wrote: > > On 9/25/18 3:25 AM, Ard Biesheuvel wrote: > > On Mon, 24 Sep 2018 at 19:53, Kees Cook <keescook@xxxxxxxxxxxx> wrote: > >> > >> On Mon, Sep 24, 2018 at 4:52 AM, Ard Biesheuvel > >> <ard.biesheuvel@xxxxxxxxxx> wrote: > >>> On Wed, 19 Sep 2018 at 04:11, Kees Cook <keescook@xxxxxxxxxxxx> wrote: > >>>> @@ -119,7 +119,7 @@ cryptoloop_transfer(struct loop_device *lo, int cmd, > >>>> unsigned in_offs, out_offs; > >>>> int err; > >>>> > >>>> - skcipher_request_set_tfm(req, tfm); > >>>> + skcipher_request_set_sync_tfm(req, tfm); > >>>> skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, > >>>> NULL, NULL); > >>>> > >>> > >>> Does this work? > >> > >> Everything is a direct wrapper for existing types and functions, so I > >> wouldn't expect any functional change. I haven't been able to test > >> this particular interface, though. cryptoloop is very deprecated, > >> isn't it? > >> > > > > Ah yes, I managed to confuse myself there. This looks all fine to me. > > > > In any case, this is another example where we may decide to fix the > > code rather than retain the request allocation on the stack (but that > > is Jens's call ultimately, I suppose) > > > > diff --git a/drivers/block/cryptoloop.c b/drivers/block/cryptoloop.c > > index 7033a4beda66..5ed2167219ba 100644 > > --- a/drivers/block/cryptoloop.c > > +++ b/drivers/block/cryptoloop.c > > @@ -110,7 +110,7 @@ cryptoloop_transfer(struct loop_device *lo, int cmd, > > int size, sector_t IV) > > { > > struct crypto_skcipher *tfm = lo->key_data; > > - SKCIPHER_REQUEST_ON_STACK(req, tfm); > > + struct skcipher_request *req; > > struct scatterlist sg_out; > > struct scatterlist sg_in; > > > > @@ -119,7 +119,10 @@ cryptoloop_transfer(struct loop_device *lo, int cmd, > > unsigned in_offs, out_offs; > > int err; > > > > - skcipher_request_set_tfm(req, tfm); > > + req = skcipher_request_alloc(tfm, GFP_NOIO); > > + if (!req) > > + return -ENOMEM; > > Is this going to be reliable? ->transfer() is called when we're doing IO, > and you'd normally need a mempool backed allocation to make this safe > and guarantee forward progress. > As far as I can tell, this function is only called from lo_read_transfer/lo_write_transfer, both of which do an unconditional alloc_page(GFP_NOIO), which is why I assumed that kmalloc(GFP_NOIO) would be permissible in the same context. Are you saying this may not be the case?