Re: [RFT PATCH v2] crypto: arm64/gcm - implement native driver using v8 Crypto Extensions

[Ard Biesheuvel]

On 30 June 2017 at 11:32, Ard Biesheuvel  wrote:
> Currently, the AES-GCM implementation for arm64 systems that support the
> ARMv8 Crypto Extensions is based on the generic GCM module, which combines
> the AES-CTR implementation using AES instructions with the PMULL based
> GHASH driver. This is suboptimal, given the fact that the input data needs
> to be loaded twice, once for the encryption and again for the MAC
> calculation.
>
> On Cortex-A57 (r1p2) and other recent cores that implement micro-op fusing
> for the AES instructions, AES executes at less than 1 cycle per byte, which
> means that any cycles wasted on loading the data twice hurt even more.
>
> So implement a new GCM driver that combines the AES and PMULL instructions
> at the block level. This improves performance on Cortex-A57 by ~27% (from


37% not 27%

> 3.5 cpb to 2.6 cpb)
>
> Signed-off-by: Ard Biesheuvel 
> ---
> v2: - rebase onto non-upstream arm64 SIMD refactoring branch
>   
> (https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm64-gcm)
> - implement non-SIMD fallback
> - remove accelerated AES routines from setkey() path
> - use be32() accessors instead of open-coded array assignments
> - remove redundant round key loads
>
> Raw numbers measured on a 2GHz AMD Overdrive B1 can be found after he patch.
>
>  arch/arm64/crypto/Kconfig |   3 +-
>  arch/arm64/crypto/ghash-ce-core.S | 175 
>  arch/arm64/crypto/ghash-ce-glue.c | 436 ++--
>  3 files changed, 587 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm64/crypto/Kconfig b/arch/arm64/crypto/Kconfig
> index a669dedc8767..3e5b39b79fb9 100644
> --- a/arch/arm64/crypto/Kconfig
> +++ b/arch/arm64/crypto/Kconfig
> @@ -29,10 +29,11 @@ config CRYPTO_SHA2_ARM64_CE
> select CRYPTO_SHA256_ARM64
>
>  config CRYPTO_GHASH_ARM64_CE
> -   tristate "GHASH (for GCM chaining mode) using ARMv8 Crypto Extensions"
> +   tristate "GHASH/AES-GCM using ARMv8 Crypto Extensions"
> depends on KERNEL_MODE_NEON
> select CRYPTO_HASH
> select CRYPTO_GF128MUL
> +   select CRYPTO_AES
>
>  config CRYPTO_CRCT10DIF_ARM64_CE
> tristate "CRCT10DIF digest algorithm using PMULL instructions"
> diff --git a/arch/arm64/crypto/ghash-ce-core.S 
> b/arch/arm64/crypto/ghash-ce-core.S
> index f0bb9f0b524f..cb22459eba85 100644
> --- a/arch/arm64/crypto/ghash-ce-core.S
> +++ b/arch/arm64/crypto/ghash-ce-core.S
> @@ -77,3 +77,178 @@ CPU_LE( rev64   T1.16b, T1.16b  )
> st1 {XL.2d}, [x1]
> ret
>  ENDPROC(pmull_ghash_update)
> +
> +   KS  .reqv8
> +   CTR .reqv9
> +   INP .reqv10
> +
> +   .macro  load_round_keys, rounds, rk
> +   cmp \rounds, #12
> +   blo f   /* 128 bits */
> +   beq f   /* 192 bits */
> +   ld1 {v17.4s-v18.4s}, [\rk], #32
> +:  ld1 {v19.4s-v20.4s}, [\rk], #32
> +:  ld1 {v21.4s-v24.4s}, [\rk], #64
> +   ld1 {v25.4s-v28.4s}, [\rk], #64
> +   ld1 {v29.4s-v31.4s}, [\rk]
> +   .endm
> +
> +   .macro  enc_round, state, key
> +   aese\state\().16b, \key\().16b
> +   aesmc   \state\().16b, \state\().16b
> +   .endm
> +
> +   .macro  enc_block, state, rounds
> +   cmp \rounds, #12
> +   b.lof   /* 128 bits */
> +   b.eqf   /* 192 bits */
> +   enc_round   \state, v17
> +   enc_round   \state, v18
> +:  enc_round   \state, v19
> +   enc_round   \state, v20
> +:  .irpkey, v21, v22, v23, v24, v25, v26, v27, v28, v29
> +   enc_round   \state, \key
> +   .endr
> +   aese\state\().16b, v30.16b
> +   eor \state\().16b, \state\().16b, v31.16b
> +   .endm
> +
> +   .macro  pmull_gcm_do_crypt, enc
> +   ld1 {SHASH.2d}, [x4]
> +   ld1 {XL.2d}, [x1]
> +   ldr x8, [x5, #8]// load lower counter
> +
> +   moviMASK.16b, #0xe1
> +   ext SHASH2.16b, SHASH.16b, SHASH.16b, #8
> +CPU_LE(rev x8, x8  )
> +   shl MASK.2d, MASK.2d, #57
> +   eor SHASH2.16b, SHASH2.16b, SHASH.16b
> +
> +   .if \enc == 1
> +   ld1 {KS.16b}, [x7]
> +   .endif
> +
> +0: ld1 {CTR.8b}, [x5]  // load upper counter
> +   ld1 {INP.16b}, [x3], #16
> +   rev x9, x8
> +   add x8, x8, #1
> +   sub w0, w0, #1
> +   ins CTR.d[1], x9// set lower counter
> +
> +   .if

Re: [PATCH v3 4/4] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Logan Gunthorpe]

On 30/06/17 12:18 AM, Horia Geantă wrote:
> On 6/29/2017 7:10 PM, Logan Gunthorpe wrote:
>> From: Horia Geantă 
>>
>> We can now make use of the io-64-nonatomic-hi-lo header to always
> Typo: we are using io-64-nonatomic-lo-hi, not hi-lo.

Yup, thanks. I will fix this for a v4. I've also noticed a couple other
mistakes I need to fix.

Logan

Re: [PATCH] crypto: ccp-platform: print error message on platform_get_irq failure

[Gary R Hook]

On 06/30/2017 12:59 AM, Gustavo A. R. Silva wrote:

Print error message on platform_get_irq failure before return.

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/crypto/ccp/ccp-platform.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ccp/ccp-platform.c
b/drivers/crypto/ccp/ccp-platform.c
index e26969e..6020c4a 100644
--- a/drivers/crypto/ccp/ccp-platform.c
+++ b/drivers/crypto/ccp/ccp-platform.c
@@ -66,8 +66,10 @@ static int ccp_get_irq(struct ccp_device *ccp)
 int ret;

 ret = platform_get_irq(pdev, 0);
-   if (ret < 0)
+   if (ret < 0) {
+   dev_notice(dev, "unable to get IRQ (%d)\n", ret);
 return ret;
+   }


Good find.

I'm all for better and more messages, but I'd like to see more detail 
here, and in the
later dev_notice(). Can we have the messages better reflect the failure 
points?




 ccp->irq = ret;
 ret = request_irq(ccp->irq, ccp->vdata->perform->irqhandler, 0,
--
2.5.0

Re: [PATCH 1/2] crypto: Make hwrng choose rng source by quality.

[Harald Freudenberger]

On 06/30/2017 07:27 AM, PrasannaKumar Muralidharan wrote:
> Hi Harald,
>
> Can you split this patch into two? One patch to choose rng based on
> the quality and another for not overriding user decided rng.
>
> Some more minor comments below.
>
> On 29 June 2017 at 15:33, Harald Freudenberger
>  wrote:
>> The hwrng core implementation currently doesn't consider the
>> quality field of the struct hwrng. So the first registered rng
>> is the winner and further rng sources even with much better
>> quality are ignored.
>>
>> The behavior should be that always the best rng with the highest
>> quality rate should be used as current rng source. Only if the
>> user explicitly chooses a rng source (via writing a rng name
>> to /sys/class/misc/hw_random) the decision for the best quality
>> should be suppressed.
>>
>> This patch makes hwrng always hold a list of registered rng
>> sources sorted decreasing by quality. On registration of a new
>> hwrng source the list is updated and if the current rng source
>> was not chosen by user and the new rng provides better quality
>> set as new current rng source. Similar on unregistration of an
>> rng, if it was the current used rng source the one with the
>> next highest quality is used. If a rng source has been set via
>> sysfs from userland as long as this one doesn't unregister
>> it is kept as current rng regardless of registration of 'better'
>> rng sources.
> Nice to see the patch. This is indeed required.
>
>> Signed-off-by: Harald Freudenberger 
>> ---
>>  drivers/char/hw_random/core.c | 31 +--
>>  1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
>> index 503a41d..7fe47f8 100644
>> --- a/drivers/char/hw_random/core.c
>> +++ b/drivers/char/hw_random/core.c
>> @@ -28,7 +28,10 @@
>>  #define RNG_MODULE_NAME"hw_random"
>>
>>  static struct hwrng *current_rng;
>> +/* the current rng has been explicitly chosen by user via sysfs */
>> +static int cur_rng_set_by_user;
> Letting the user know that the current rng was selected based on user
> input would be a good option I guess. Any thoughts on this?
>
>>  static struct task_struct *hwrng_fill;
>> +/* list of registered rngs, sorted decending by quality */
>>  static LIST_HEAD(rng_list);
>>  /* Protects rng_list and current_rng */
>>  static DEFINE_MUTEX(rng_mutex);
>> @@ -308,6 +311,8 @@ static ssize_t hwrng_attr_current_store(struct device 
>> *dev,
>> break;
>> }
>> }
>> +   if (!err)
>> +   cur_rng_set_by_user = 1;
> This can be put inside the loop. The if condition will go away in that case.
>
>> mutex_unlock(_mutex);
>>
>> return err ? : len;
>> @@ -417,6 +422,7 @@ int hwrng_register(struct hwrng *rng)
>>  {
>> int err = -EINVAL;
>> struct hwrng *old_rng, *tmp;
>> +   struct list_head *ptr;
> Any better name instead of ptr?
>
>> if (!rng->name || (!rng->data_read && !rng->read))
>> goto out;
>> @@ -432,14 +438,26 @@ int hwrng_register(struct hwrng *rng)
>> init_completion(>cleanup_done);
>> complete(>cleanup_done);
>>
>> +   /* rng_list is sorted by decreasing quality */
>> +   list_for_each(ptr, _list) {
>> +   tmp = list_entry(ptr, struct hwrng, list);
>> +   if (tmp->quality < rng->quality)
>> +   break;
>> +   }
>> +   list_add_tail(>list, ptr);
>> +
>> old_rng = current_rng;
>> err = 0;
>> -   if (!old_rng) {
>> +   if (!old_rng ||
>> +   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
>> +   /*
>> +* Set new rng as current if no current rng or rng was
>> +* not chosen by user and the new one has better quality.
>> +*/
>> err = set_current_rng(rng);
>> if (err)
>> goto out_unlock;
>> }
>> -   list_add_tail(>list, _list);
>>
>> if (old_rng && !rng->init) {
>> /*
>> @@ -466,12 +484,13 @@ void hwrng_unregister(struct hwrng *rng)
>> list_del(>list);
>> if (current_rng == rng) {
>> drop_current_rng();
>> +   cur_rng_set_by_user = 0;
>> +   /* rng_list is sorted by quality, use the best (=first) one 
>> */
>> if (!list_empty(_list)) {
>> -   struct hwrng *tail;
>> -
>> -   tail = list_entry(rng_list.prev, struct hwrng, list);
>> +   struct hwrng *new_rng;
>>
>> -   set_current_rng(tail);
>> +   new_rng = list_entry(rng_list.next, struct hwrng, 
>> list);
>> +   set_current_rng(new_rng);
>> }
>> }
>>
>> --
>> 2.7.4
>>
> This patch looks good. I 

[RFT PATCH v2] crypto: arm64/gcm - implement native driver using v8 Crypto Extensions

[Ard Biesheuvel]

Currently, the AES-GCM implementation for arm64 systems that support the
ARMv8 Crypto Extensions is based on the generic GCM module, which combines
the AES-CTR implementation using AES instructions with the PMULL based
GHASH driver. This is suboptimal, given the fact that the input data needs
to be loaded twice, once for the encryption and again for the MAC
calculation.

On Cortex-A57 (r1p2) and other recent cores that implement micro-op fusing
for the AES instructions, AES executes at less than 1 cycle per byte, which
means that any cycles wasted on loading the data twice hurt even more.

So implement a new GCM driver that combines the AES and PMULL instructions
at the block level. This improves performance on Cortex-A57 by ~27% (from
3.5 cpb to 2.6 cpb)

Signed-off-by: Ard Biesheuvel 
---
v2: - rebase onto non-upstream arm64 SIMD refactoring branch
  
(https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=arm64-gcm)
- implement non-SIMD fallback
- remove accelerated AES routines from setkey() path
- use be32() accessors instead of open-coded array assignments
- remove redundant round key loads

Raw numbers measured on a 2GHz AMD Overdrive B1 can be found after he patch.

 arch/arm64/crypto/Kconfig |   3 +-
 arch/arm64/crypto/ghash-ce-core.S | 175 
 arch/arm64/crypto/ghash-ce-glue.c | 436 ++--
 3 files changed, 587 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/crypto/Kconfig b/arch/arm64/crypto/Kconfig
index a669dedc8767..3e5b39b79fb9 100644
--- a/arch/arm64/crypto/Kconfig
+++ b/arch/arm64/crypto/Kconfig
@@ -29,10 +29,11 @@ config CRYPTO_SHA2_ARM64_CE
select CRYPTO_SHA256_ARM64
 
 config CRYPTO_GHASH_ARM64_CE
-   tristate "GHASH (for GCM chaining mode) using ARMv8 Crypto Extensions"
+   tristate "GHASH/AES-GCM using ARMv8 Crypto Extensions"
depends on KERNEL_MODE_NEON
select CRYPTO_HASH
select CRYPTO_GF128MUL
+   select CRYPTO_AES
 
 config CRYPTO_CRCT10DIF_ARM64_CE
tristate "CRCT10DIF digest algorithm using PMULL instructions"
diff --git a/arch/arm64/crypto/ghash-ce-core.S 
b/arch/arm64/crypto/ghash-ce-core.S
index f0bb9f0b524f..cb22459eba85 100644
--- a/arch/arm64/crypto/ghash-ce-core.S
+++ b/arch/arm64/crypto/ghash-ce-core.S
@@ -77,3 +77,178 @@ CPU_LE( rev64   T1.16b, T1.16b  )
st1 {XL.2d}, [x1]
ret
 ENDPROC(pmull_ghash_update)
+
+   KS  .reqv8
+   CTR .reqv9
+   INP .reqv10
+
+   .macro  load_round_keys, rounds, rk
+   cmp \rounds, #12
+   blo f   /* 128 bits */
+   beq f   /* 192 bits */
+   ld1 {v17.4s-v18.4s}, [\rk], #32
+:  ld1 {v19.4s-v20.4s}, [\rk], #32
+:  ld1 {v21.4s-v24.4s}, [\rk], #64
+   ld1 {v25.4s-v28.4s}, [\rk], #64
+   ld1 {v29.4s-v31.4s}, [\rk]
+   .endm
+
+   .macro  enc_round, state, key
+   aese\state\().16b, \key\().16b
+   aesmc   \state\().16b, \state\().16b
+   .endm
+
+   .macro  enc_block, state, rounds
+   cmp \rounds, #12
+   b.lof   /* 128 bits */
+   b.eqf   /* 192 bits */
+   enc_round   \state, v17
+   enc_round   \state, v18
+:  enc_round   \state, v19
+   enc_round   \state, v20
+:  .irpkey, v21, v22, v23, v24, v25, v26, v27, v28, v29
+   enc_round   \state, \key
+   .endr
+   aese\state\().16b, v30.16b
+   eor \state\().16b, \state\().16b, v31.16b
+   .endm
+
+   .macro  pmull_gcm_do_crypt, enc
+   ld1 {SHASH.2d}, [x4]
+   ld1 {XL.2d}, [x1]
+   ldr x8, [x5, #8]// load lower counter
+
+   moviMASK.16b, #0xe1
+   ext SHASH2.16b, SHASH.16b, SHASH.16b, #8
+CPU_LE(rev x8, x8  )
+   shl MASK.2d, MASK.2d, #57
+   eor SHASH2.16b, SHASH2.16b, SHASH.16b
+
+   .if \enc == 1
+   ld1 {KS.16b}, [x7]
+   .endif
+
+0: ld1 {CTR.8b}, [x5]  // load upper counter
+   ld1 {INP.16b}, [x3], #16
+   rev x9, x8
+   add x8, x8, #1
+   sub w0, w0, #1
+   ins CTR.d[1], x9// set lower counter
+
+   .if \enc == 1
+   eor INP.16b, INP.16b, KS.16b// encrypt input
+   st1 {INP.16b}, [x2], #16
+   .endif
+
+   rev64   T1.16b, INP.16b
+
+   cmp w6, #12
+   b.ge2f  // AES-192/256?
+
+1: enc_round

[PATCH] crypto: omap-aes: fix error return code in omap_aes_probe()

[Gustavo A. R. Silva]

Propagate the return value of platform_get_irq on failure.

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/crypto/omap-aes.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/omap-aes.c b/drivers/crypto/omap-aes.c
index 5120a17..c376a3e 100644
--- a/drivers/crypto/omap-aes.c
+++ b/drivers/crypto/omap-aes.c
@@ -1095,6 +1095,7 @@ static int omap_aes_probe(struct platform_device *pdev)
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
dev_err(dev, "can't get IRQ resource\n");
+   err = irq;
goto err_irq;
}
 
-- 
2.5.0

[PATCH] crypto: mxc-scc: fix error code in mxc_scc_probe()

[Gustavo A. R. Silva]

Print and propagate the return value of platform_get_irq on failure.

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/crypto/mxc-scc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/mxc-scc.c b/drivers/crypto/mxc-scc.c
index ee4be1b0..e01c463 100644
--- a/drivers/crypto/mxc-scc.c
+++ b/drivers/crypto/mxc-scc.c
@@ -708,8 +708,8 @@ static int mxc_scc_probe(struct platform_device *pdev)
for (i = 0; i < 2; i++) {
irq = platform_get_irq(pdev, i);
if (irq < 0) {
-   dev_err(dev, "failed to get irq resource\n");
-   ret = -EINVAL;
+   dev_err(dev, "failed to get irq resource: %d\n", irq);
+   ret = irq;
goto err_out;
}
 
-- 
2.5.0

[PATCH] crypto: omap-des: fix error return code in omap_des_probe()

[Gustavo A. R. Silva]

Print and propagate the return value of platform_get_irq on failure.

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/crypto/omap-des.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/omap-des.c b/drivers/crypto/omap-des.c
index 0bcab00..d37c950 100644
--- a/drivers/crypto/omap-des.c
+++ b/drivers/crypto/omap-des.c
@@ -1023,7 +1023,8 @@ static int omap_des_probe(struct platform_device *pdev)
 
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
-   dev_err(dev, "can't get IRQ resource\n");
+   dev_err(dev, "can't get IRQ resource: %d\n", irq);
+   err = irq;
goto err_irq;
}
 
-- 
2.5.0

[PATCH] crypto: mxs-dcp: print error message on platform_get_irq failure

[Gustavo A. R. Silva]

Print error message on platform_get_irq failure before return.

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/crypto/mxs-dcp.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
index 625ee50..764be3e 100644
--- a/drivers/crypto/mxs-dcp.c
+++ b/drivers/crypto/mxs-dcp.c
@@ -908,12 +908,16 @@ static int mxs_dcp_probe(struct platform_device *pdev)
 
iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dcp_vmi_irq = platform_get_irq(pdev, 0);
-   if (dcp_vmi_irq < 0)
+   if (dcp_vmi_irq < 0) {
+   dev_err(dev, "Failed to get IRQ: (%d)!\n", dcp_vmi_irq);
return dcp_vmi_irq;
+   }
 
dcp_irq = platform_get_irq(pdev, 1);
-   if (dcp_irq < 0)
+   if (dcp_irq < 0) {
+   dev_err(dev, "Failed to get IRQ: (%d)!\n", dcp_irq);
return dcp_irq;
+   }
 
sdcp = devm_kzalloc(dev, sizeof(*sdcp), GFP_KERNEL);
if (!sdcp)
-- 
2.5.0

[PATCH] crypto: ccp-platform: print error message on platform_get_irq failure

[Gustavo A. R. Silva]

Print error message on platform_get_irq failure before return.

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/crypto/ccp/ccp-platform.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ccp/ccp-platform.c 
b/drivers/crypto/ccp/ccp-platform.c
index e26969e..6020c4a 100644
--- a/drivers/crypto/ccp/ccp-platform.c
+++ b/drivers/crypto/ccp/ccp-platform.c
@@ -66,8 +66,10 @@ static int ccp_get_irq(struct ccp_device *ccp)
int ret;
 
ret = platform_get_irq(pdev, 0);
-   if (ret < 0)
+   if (ret < 0) {
+   dev_notice(dev, "unable to get IRQ (%d)\n", ret);
return ret;
+   }
 
ccp->irq = ret;
ret = request_irq(ccp->irq, ccp->vdata->perform->irqhandler, 0,
-- 
2.5.0

[PATCH] crypto: mediatek: fix error return code in mtk_crypto_probe()

[Gustavo A. R. Silva]

Propagate the return value of platform_get_irq on failure.

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/crypto/mediatek/mtk-platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/mediatek/mtk-platform.c 
b/drivers/crypto/mediatek/mtk-platform.c
index 000b650..b182e94 100644
--- a/drivers/crypto/mediatek/mtk-platform.c
+++ b/drivers/crypto/mediatek/mtk-platform.c
@@ -500,7 +500,7 @@ static int mtk_crypto_probe(struct platform_device *pdev)
cryp->irq[i] = platform_get_irq(pdev, i);
if (cryp->irq[i] < 0) {
dev_err(cryp->dev, "no IRQ:%d resource info\n", i);
-   return -ENXIO;
+   return cryp->irq[i];
}
}
 
-- 
2.5.0

Re: [PATCH v3 4/4] crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64

[Horia Geantă]

On 6/29/2017 7:10 PM, Logan Gunthorpe wrote:
> From: Horia Geantă 
> 
> We can now make use of the io-64-nonatomic-hi-lo header to always
Typo: we are using io-64-nonatomic-lo-hi, not hi-lo.

Thanks,
Horia

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Nicholas A. Bellinger]

On Mon, 2017-06-26 at 19:38 +0200, Stephan Müller wrote:
> Am Montag, 26. Juni 2017, 03:23:09 CEST schrieb Nicholas A. Bellinger:
> 
> Hi Nicholas,
> 
> > Hi Stephan, Lee & Jason,
> > 
> > (Adding target-devel CC')
> > 
> > Apologies for coming late to the discussion.  Comments below.
> > 
> > On Sun, 2017-06-18 at 10:04 +0200, Stephan Müller wrote:
> > > Am Samstag, 17. Juni 2017, 05:45:57 CEST schrieb Lee Duncan:
> > > 
> > > Hi Lee,
> > > 
> > > > In your testing, how long might a process have to wait? Are we talking
> > > > seconds? Longer? What about timeouts?
> > > 
> > > In current kernels (starting with 4.8) this timeout should clear within a
> > > few seconds after boot.
> > > 
> > > In older kernels (pre 4.8), my KVM takes up to 90 seconds to reach that
> > > seeding point. I have heard that on IBM System Z this trigger point
> > > requires minutes to be reached.
> > 
> > I share the same concern as Lee wrt to introducing latency into the
> > existing iscsi-target login sequence.
> > 
> > Namely in the use-cases where a single node is supporting ~1K unique
> > iscsi-target IQNs, and fail-over + re-balancing of IQNs where 100s of
> > login attempts are expected to occur in parallel.
> > 
> > If environments exist that require non trivial amounts of time for RNG
> > seeding to be ready for iscsi-target usage, then enforcing this
> > requirement at iscsi login time can open up problems, especially when
> > iscsi host environments may be sensitive to login timeouts, I/O
> > timeouts, et al.
> > 
> > That said, I'd prefer to simply wait for RNG to be seeded at modprobe
> > iscsi_target_mod time, instead of trying to enforce randomness during
> > login.
> > 
> > This way, those of use who have distributed storage platforms can know
> > to avoid putting a node into a state to accept iscsi-target IQN export
> > migration, before modprobe iscsi_target_mod has successfully loaded and
> > RNG seeding has been confirmed.
> > 
> > WDYT..?
> 
> We may have a chicken and egg problem when the wait is at the modprobe time. 
> Assume the modprobe happens during initramfs time to get access to the root 
> file system. In this case, you entire boot process will lock up for an 
> indefinite amount of time. The reason is that in order to obtain events 
> detected by the RNG, devices need to be initialized and working. Such devices 
> commonly start working after the the root partition is mounted as it contains 
> all drivers, all configuration information etc.
> 
> Note, during the development of my /dev/random implementation, I added the 
> getrandom-like blocking behavior to /dev/urandom (which is the equivalent to 
> Jason's patch except that it applies to user space). The boot process locked 
> up since systemd wanted data from /dev/urandom while it processed the 
> initramfs. As it did not get any, the boot process did not commence that 
> could 
> deliver new events to be picked up by the RNG.
> 
> As I do not have such an iscsi system, I cannot test Jason's patch. But maybe 
> the mentioned chicken-and-egg problem I mentioned above is already visible 
> with the current patch as it will lead to a blocking of the mounting of the 
> root partition in case the root partition is on an iscsi target.

AFAIK, there are no distro initramfs dependencies for iscsi_target_mod,
and every environment I've ever seen loads iscsi_target_mod after
switching to the real rootfs.

For an iscsi initiator that might not been the case, especially if the
rootfs is running atop a iscsi LUN.

But at least for iscsi-target mode, any blocking during modprobe waiting
for RNG seeding would happen outside of initramfs.