* [PATCH v7] drbd: Convert from ahash to shash
@ 2018-08-06 23:32 Kees Cook
2018-09-04 6:04 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2018-08-06 23:32 UTC (permalink / raw)
To: Lars Ellenberg
Cc: Philipp Reisner, Jens Axboe, linux-block, drbd-dev, linux-kernel
In preparing to remove all stack VLA usage from the kernel[1], this
removes the discouraged use of AHASH_REQUEST_ON_STACK in favor of
the smaller SHASH_DESC_ON_STACK by converting from ahash-wrapped-shash
to direct shash. By removing a layer of indirection this both improves
performance and reduces stack usage. The stack allocation will be made
a fixed size in a later patch to the crypto subsystem.
The bulk of the lines in this change are simple s/ahash/shash/, but the
main logic differences are in drbd_csum_ee() and drbd_csum_bio(), which
externalizes the page walking with k(un)map_atomic() instead of using
scattergather.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
I'm calling this "v7" just for general sanity over the naming of the
patch when it was part of the large crypto series of changes. Really,
it's v2.
v7: use kmap for possible highmem page addresses (Lars).
---
drivers/block/drbd/drbd_int.h | 13 +++---
drivers/block/drbd/drbd_main.c | 14 +++----
drivers/block/drbd/drbd_nl.c | 39 ++++++------------
drivers/block/drbd/drbd_receiver.c | 35 +++++++++--------
drivers/block/drbd/drbd_worker.c | 63 +++++++++++++++---------------
5 files changed, 76 insertions(+), 88 deletions(-)
diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h
index bc4ed2ed40a2..97d8e290c2be 100644
--- a/drivers/block/drbd/drbd_int.h
+++ b/drivers/block/drbd/drbd_int.h
@@ -726,10 +726,10 @@ struct drbd_connection {
struct list_head transfer_log; /* all requests not yet fully processed */
struct crypto_shash *cram_hmac_tfm;
- struct crypto_ahash *integrity_tfm; /* checksums we compute, updates protected by connection->data->mutex */
- struct crypto_ahash *peer_integrity_tfm; /* checksums we verify, only accessed from receiver thread */
- struct crypto_ahash *csums_tfm;
- struct crypto_ahash *verify_tfm;
+ struct crypto_shash *integrity_tfm; /* checksums we compute, updates protected by connection->data->mutex */
+ struct crypto_shash *peer_integrity_tfm; /* checksums we verify, only accessed from receiver thread */
+ struct crypto_shash *csums_tfm;
+ struct crypto_shash *verify_tfm;
void *int_dig_in;
void *int_dig_vv;
@@ -1533,8 +1533,9 @@ static inline void ov_out_of_sync_print(struct drbd_device *device)
}
-extern void drbd_csum_bio(struct crypto_ahash *, struct bio *, void *);
-extern void drbd_csum_ee(struct crypto_ahash *, struct drbd_peer_request *, void *);
+extern void drbd_csum_bio(struct crypto_shash *, struct bio *, void *);
+extern void drbd_csum_ee(struct crypto_shash *, struct drbd_peer_request *,
+ void *);
/* worker callbacks */
extern int w_e_end_data_req(struct drbd_work *, int);
extern int w_e_end_rsdata_req(struct drbd_work *, int);
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index a80809bd3057..ccb54791d39c 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -1377,7 +1377,7 @@ void drbd_send_ack_dp(struct drbd_peer_device *peer_device, enum drbd_packet cmd
struct p_data *dp, int data_size)
{
if (peer_device->connection->peer_integrity_tfm)
- data_size -= crypto_ahash_digestsize(peer_device->connection->peer_integrity_tfm);
+ data_size -= crypto_shash_digestsize(peer_device->connection->peer_integrity_tfm);
_drbd_send_ack(peer_device, cmd, dp->sector, cpu_to_be32(data_size),
dp->block_id);
}
@@ -1690,7 +1690,7 @@ int drbd_send_dblock(struct drbd_peer_device *peer_device, struct drbd_request *
sock = &peer_device->connection->data;
p = drbd_prepare_command(peer_device, sock);
digest_size = peer_device->connection->integrity_tfm ?
- crypto_ahash_digestsize(peer_device->connection->integrity_tfm) : 0;
+ crypto_shash_digestsize(peer_device->connection->integrity_tfm) : 0;
if (!p)
return -EIO;
@@ -1796,7 +1796,7 @@ int drbd_send_block(struct drbd_peer_device *peer_device, enum drbd_packet cmd,
p = drbd_prepare_command(peer_device, sock);
digest_size = peer_device->connection->integrity_tfm ?
- crypto_ahash_digestsize(peer_device->connection->integrity_tfm) : 0;
+ crypto_shash_digestsize(peer_device->connection->integrity_tfm) : 0;
if (!p)
return -EIO;
@@ -2561,11 +2561,11 @@ void conn_free_crypto(struct drbd_connection *connection)
{
drbd_free_sock(connection);
- crypto_free_ahash(connection->csums_tfm);
- crypto_free_ahash(connection->verify_tfm);
+ crypto_free_shash(connection->csums_tfm);
+ crypto_free_shash(connection->verify_tfm);
crypto_free_shash(connection->cram_hmac_tfm);
- crypto_free_ahash(connection->integrity_tfm);
- crypto_free_ahash(connection->peer_integrity_tfm);
+ crypto_free_shash(connection->integrity_tfm);
+ crypto_free_shash(connection->peer_integrity_tfm);
kfree(connection->int_dig_in);
kfree(connection->int_dig_vv);
diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index b4f02768ba47..d15703b1ffe8 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -2303,10 +2303,10 @@ check_net_options(struct drbd_connection *connection, struct net_conf *new_net_c
}
struct crypto {
- struct crypto_ahash *verify_tfm;
- struct crypto_ahash *csums_tfm;
+ struct crypto_shash *verify_tfm;
+ struct crypto_shash *csums_tfm;
struct crypto_shash *cram_hmac_tfm;
- struct crypto_ahash *integrity_tfm;
+ struct crypto_shash *integrity_tfm;
};
static int
@@ -2324,36 +2324,21 @@ alloc_shash(struct crypto_shash **tfm, char *tfm_name, int err_alg)
return NO_ERROR;
}
-static int
-alloc_ahash(struct crypto_ahash **tfm, char *tfm_name, int err_alg)
-{
- if (!tfm_name[0])
- return NO_ERROR;
-
- *tfm = crypto_alloc_ahash(tfm_name, 0, CRYPTO_ALG_ASYNC);
- if (IS_ERR(*tfm)) {
- *tfm = NULL;
- return err_alg;
- }
-
- return NO_ERROR;
-}
-
static enum drbd_ret_code
alloc_crypto(struct crypto *crypto, struct net_conf *new_net_conf)
{
char hmac_name[CRYPTO_MAX_ALG_NAME];
enum drbd_ret_code rv;
- rv = alloc_ahash(&crypto->csums_tfm, new_net_conf->csums_alg,
+ rv = alloc_shash(&crypto->csums_tfm, new_net_conf->csums_alg,
ERR_CSUMS_ALG);
if (rv != NO_ERROR)
return rv;
- rv = alloc_ahash(&crypto->verify_tfm, new_net_conf->verify_alg,
+ rv = alloc_shash(&crypto->verify_tfm, new_net_conf->verify_alg,
ERR_VERIFY_ALG);
if (rv != NO_ERROR)
return rv;
- rv = alloc_ahash(&crypto->integrity_tfm, new_net_conf->integrity_alg,
+ rv = alloc_shash(&crypto->integrity_tfm, new_net_conf->integrity_alg,
ERR_INTEGRITY_ALG);
if (rv != NO_ERROR)
return rv;
@@ -2371,9 +2356,9 @@ alloc_crypto(struct crypto *crypto, struct net_conf *new_net_conf)
static void free_crypto(struct crypto *crypto)
{
crypto_free_shash(crypto->cram_hmac_tfm);
- crypto_free_ahash(crypto->integrity_tfm);
- crypto_free_ahash(crypto->csums_tfm);
- crypto_free_ahash(crypto->verify_tfm);
+ crypto_free_shash(crypto->integrity_tfm);
+ crypto_free_shash(crypto->csums_tfm);
+ crypto_free_shash(crypto->verify_tfm);
}
int drbd_adm_net_opts(struct sk_buff *skb, struct genl_info *info)
@@ -2450,17 +2435,17 @@ int drbd_adm_net_opts(struct sk_buff *skb, struct genl_info *info)
rcu_assign_pointer(connection->net_conf, new_net_conf);
if (!rsr) {
- crypto_free_ahash(connection->csums_tfm);
+ crypto_free_shash(connection->csums_tfm);
connection->csums_tfm = crypto.csums_tfm;
crypto.csums_tfm = NULL;
}
if (!ovr) {
- crypto_free_ahash(connection->verify_tfm);
+ crypto_free_shash(connection->verify_tfm);
connection->verify_tfm = crypto.verify_tfm;
crypto.verify_tfm = NULL;
}
- crypto_free_ahash(connection->integrity_tfm);
+ crypto_free_shash(connection->integrity_tfm);
connection->integrity_tfm = crypto.integrity_tfm;
if (connection->cstate >= C_WF_REPORT_PARAMS && connection->agreed_pro_version >= 100)
/* Do this without trying to take connection->data.mutex again. */
diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
index be9450f5ad1c..76243e9ef277 100644
--- a/drivers/block/drbd/drbd_receiver.c
+++ b/drivers/block/drbd/drbd_receiver.c
@@ -1732,7 +1732,7 @@ static int receive_Barrier(struct drbd_connection *connection, struct packet_inf
}
/* quick wrapper in case payload size != request_size (write same) */
-static void drbd_csum_ee_size(struct crypto_ahash *h,
+static void drbd_csum_ee_size(struct crypto_shash *h,
struct drbd_peer_request *r, void *d,
unsigned int payload_size)
{
@@ -1769,7 +1769,7 @@ read_in_block(struct drbd_peer_device *peer_device, u64 id, sector_t sector,
digest_size = 0;
if (!trim && peer_device->connection->peer_integrity_tfm) {
- digest_size = crypto_ahash_digestsize(peer_device->connection->peer_integrity_tfm);
+ digest_size = crypto_shash_digestsize(peer_device->connection->peer_integrity_tfm);
/*
* FIXME: Receive the incoming digest into the receive buffer
* here, together with its struct p_data?
@@ -1905,7 +1905,7 @@ static int recv_dless_read(struct drbd_peer_device *peer_device, struct drbd_req
digest_size = 0;
if (peer_device->connection->peer_integrity_tfm) {
- digest_size = crypto_ahash_digestsize(peer_device->connection->peer_integrity_tfm);
+ digest_size = crypto_shash_digestsize(peer_device->connection->peer_integrity_tfm);
err = drbd_recv_all_warn(peer_device->connection, dig_in, digest_size);
if (err)
return err;
@@ -3540,7 +3540,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
int p_proto, p_discard_my_data, p_two_primaries, cf;
struct net_conf *nc, *old_net_conf, *new_net_conf = NULL;
char integrity_alg[SHARED_SECRET_MAX] = "";
- struct crypto_ahash *peer_integrity_tfm = NULL;
+ struct crypto_shash *peer_integrity_tfm = NULL;
void *int_dig_in = NULL, *int_dig_vv = NULL;
p_proto = be32_to_cpu(p->protocol);
@@ -3621,7 +3621,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
* change.
*/
- peer_integrity_tfm = crypto_alloc_ahash(integrity_alg, 0, CRYPTO_ALG_ASYNC);
+ peer_integrity_tfm = crypto_alloc_shash(integrity_alg, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(peer_integrity_tfm)) {
peer_integrity_tfm = NULL;
drbd_err(connection, "peer data-integrity-alg %s not supported\n",
@@ -3629,7 +3629,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
goto disconnect;
}
- hash_size = crypto_ahash_digestsize(peer_integrity_tfm);
+ hash_size = crypto_shash_digestsize(peer_integrity_tfm);
int_dig_in = kmalloc(hash_size, GFP_KERNEL);
int_dig_vv = kmalloc(hash_size, GFP_KERNEL);
if (!(int_dig_in && int_dig_vv)) {
@@ -3659,7 +3659,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
mutex_unlock(&connection->resource->conf_update);
mutex_unlock(&connection->data.mutex);
- crypto_free_ahash(connection->peer_integrity_tfm);
+ crypto_free_shash(connection->peer_integrity_tfm);
kfree(connection->int_dig_in);
kfree(connection->int_dig_vv);
connection->peer_integrity_tfm = peer_integrity_tfm;
@@ -3677,7 +3677,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
disconnect_rcu_unlock:
rcu_read_unlock();
disconnect:
- crypto_free_ahash(peer_integrity_tfm);
+ crypto_free_shash(peer_integrity_tfm);
kfree(int_dig_in);
kfree(int_dig_vv);
conn_request_state(connection, NS(conn, C_DISCONNECTING), CS_HARD);
@@ -3689,15 +3689,16 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
* return: NULL (alg name was "")
* ERR_PTR(error) if something goes wrong
* or the crypto hash ptr, if it worked out ok. */
-static struct crypto_ahash *drbd_crypto_alloc_digest_safe(const struct drbd_device *device,
+static struct crypto_shash *drbd_crypto_alloc_digest_safe(
+ const struct drbd_device *device,
const char *alg, const char *name)
{
- struct crypto_ahash *tfm;
+ struct crypto_shash *tfm;
if (!alg[0])
return NULL;
- tfm = crypto_alloc_ahash(alg, 0, CRYPTO_ALG_ASYNC);
+ tfm = crypto_alloc_shash(alg, 0, 0);
if (IS_ERR(tfm)) {
drbd_err(device, "Can not allocate \"%s\" as %s (reason: %ld)\n",
alg, name, PTR_ERR(tfm));
@@ -3750,8 +3751,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i
struct drbd_device *device;
struct p_rs_param_95 *p;
unsigned int header_size, data_size, exp_max_sz;
- struct crypto_ahash *verify_tfm = NULL;
- struct crypto_ahash *csums_tfm = NULL;
+ struct crypto_shash *verify_tfm = NULL;
+ struct crypto_shash *csums_tfm = NULL;
struct net_conf *old_net_conf, *new_net_conf = NULL;
struct disk_conf *old_disk_conf = NULL, *new_disk_conf = NULL;
const int apv = connection->agreed_pro_version;
@@ -3898,14 +3899,14 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i
if (verify_tfm) {
strcpy(new_net_conf->verify_alg, p->verify_alg);
new_net_conf->verify_alg_len = strlen(p->verify_alg) + 1;
- crypto_free_ahash(peer_device->connection->verify_tfm);
+ crypto_free_shash(peer_device->connection->verify_tfm);
peer_device->connection->verify_tfm = verify_tfm;
drbd_info(device, "using verify-alg: \"%s\"\n", p->verify_alg);
}
if (csums_tfm) {
strcpy(new_net_conf->csums_alg, p->csums_alg);
new_net_conf->csums_alg_len = strlen(p->csums_alg) + 1;
- crypto_free_ahash(peer_device->connection->csums_tfm);
+ crypto_free_shash(peer_device->connection->csums_tfm);
peer_device->connection->csums_tfm = csums_tfm;
drbd_info(device, "using csums-alg: \"%s\"\n", p->csums_alg);
}
@@ -3949,9 +3950,9 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i
mutex_unlock(&connection->resource->conf_update);
/* just for completeness: actually not needed,
* as this is not reached if csums_tfm was ok. */
- crypto_free_ahash(csums_tfm);
+ crypto_free_shash(csums_tfm);
/* but free the verify_tfm again, if csums_tfm did not work out */
- crypto_free_ahash(verify_tfm);
+ crypto_free_shash(verify_tfm);
conn_request_state(peer_device->connection, NS(conn, C_DISCONNECTING), CS_HARD);
return -EIO;
}
diff --git a/drivers/block/drbd/drbd_worker.c b/drivers/block/drbd/drbd_worker.c
index 5e793dd7adfb..0b41fb0181a6 100644
--- a/drivers/block/drbd/drbd_worker.c
+++ b/drivers/block/drbd/drbd_worker.c
@@ -295,60 +295,61 @@ void drbd_request_endio(struct bio *bio)
complete_master_bio(device, &m);
}
-void drbd_csum_ee(struct crypto_ahash *tfm, struct drbd_peer_request *peer_req, void *digest)
+void drbd_csum_ee(struct crypto_shash *tfm, struct drbd_peer_request *peer_req, void *digest)
{
- AHASH_REQUEST_ON_STACK(req, tfm);
- struct scatterlist sg;
+ SHASH_DESC_ON_STACK(desc, tfm);
struct page *page = peer_req->pages;
struct page *tmp;
unsigned len;
+ void *src;
- ahash_request_set_tfm(req, tfm);
- ahash_request_set_callback(req, 0, NULL, NULL);
+ desc->tfm = tfm;
+ desc->flags = 0;
- sg_init_table(&sg, 1);
- crypto_ahash_init(req);
+ crypto_shash_init(desc);
+ src = kmap_atomic(page);
while ((tmp = page_chain_next(page))) {
/* all but the last page will be fully used */
- sg_set_page(&sg, page, PAGE_SIZE, 0);
- ahash_request_set_crypt(req, &sg, NULL, sg.length);
- crypto_ahash_update(req);
+ crypto_shash_update(desc, src, PAGE_SIZE);
+ kunmap_atomic(src);
page = tmp;
+ src = kmap_atomic(page);
}
/* and now the last, possibly only partially used page */
len = peer_req->i.size & (PAGE_SIZE - 1);
- sg_set_page(&sg, page, len ?: PAGE_SIZE, 0);
- ahash_request_set_crypt(req, &sg, digest, sg.length);
- crypto_ahash_finup(req);
- ahash_request_zero(req);
+ crypto_shash_update(desc, src, len ?: PAGE_SIZE);
+ kunmap_atomic(src);
+
+ crypto_shash_final(desc, digest);
+ shash_desc_zero(desc);
}
-void drbd_csum_bio(struct crypto_ahash *tfm, struct bio *bio, void *digest)
+void drbd_csum_bio(struct crypto_shash *tfm, struct bio *bio, void *digest)
{
- AHASH_REQUEST_ON_STACK(req, tfm);
- struct scatterlist sg;
+ SHASH_DESC_ON_STACK(desc, tfm);
struct bio_vec bvec;
struct bvec_iter iter;
- ahash_request_set_tfm(req, tfm);
- ahash_request_set_callback(req, 0, NULL, NULL);
+ desc->tfm = tfm;
+ desc->flags = 0;
- sg_init_table(&sg, 1);
- crypto_ahash_init(req);
+ crypto_shash_init(desc);
bio_for_each_segment(bvec, bio, iter) {
- sg_set_page(&sg, bvec.bv_page, bvec.bv_len, bvec.bv_offset);
- ahash_request_set_crypt(req, &sg, NULL, sg.length);
- crypto_ahash_update(req);
+ u8 *src;
+
+ src = kmap_atomic(bvec.bv_page);
+ crypto_shash_update(desc, src + bvec.bv_offset, bvec.bv_len);
+ kunmap_atomic(src);
+
/* REQ_OP_WRITE_SAME has only one segment,
* checksum the payload only once. */
if (bio_op(bio) == REQ_OP_WRITE_SAME)
break;
}
- ahash_request_set_crypt(req, NULL, digest, 0);
- crypto_ahash_final(req);
- ahash_request_zero(req);
+ crypto_shash_final(desc, digest);
+ shash_desc_zero(desc);
}
/* MAYBE merge common code with w_e_end_ov_req */
@@ -367,7 +368,7 @@ static int w_e_send_csum(struct drbd_work *w, int cancel)
if (unlikely((peer_req->flags & EE_WAS_ERROR) != 0))
goto out;
- digest_size = crypto_ahash_digestsize(peer_device->connection->csums_tfm);
+ digest_size = crypto_shash_digestsize(peer_device->connection->csums_tfm);
digest = kmalloc(digest_size, GFP_NOIO);
if (digest) {
sector_t sector = peer_req->i.sector;
@@ -1205,7 +1206,7 @@ int w_e_end_csum_rs_req(struct drbd_work *w, int cancel)
* a real fix would be much more involved,
* introducing more locking mechanisms */
if (peer_device->connection->csums_tfm) {
- digest_size = crypto_ahash_digestsize(peer_device->connection->csums_tfm);
+ digest_size = crypto_shash_digestsize(peer_device->connection->csums_tfm);
D_ASSERT(device, digest_size == di->digest_size);
digest = kmalloc(digest_size, GFP_NOIO);
}
@@ -1255,7 +1256,7 @@ int w_e_end_ov_req(struct drbd_work *w, int cancel)
if (unlikely(cancel))
goto out;
- digest_size = crypto_ahash_digestsize(peer_device->connection->verify_tfm);
+ digest_size = crypto_shash_digestsize(peer_device->connection->verify_tfm);
digest = kmalloc(digest_size, GFP_NOIO);
if (!digest) {
err = 1; /* terminate the connection in case the allocation failed */
@@ -1327,7 +1328,7 @@ int w_e_end_ov_reply(struct drbd_work *w, int cancel)
di = peer_req->digest;
if (likely((peer_req->flags & EE_WAS_ERROR) == 0)) {
- digest_size = crypto_ahash_digestsize(peer_device->connection->verify_tfm);
+ digest_size = crypto_shash_digestsize(peer_device->connection->verify_tfm);
digest = kmalloc(digest_size, GFP_NOIO);
if (digest) {
drbd_csum_ee(peer_device->connection->verify_tfm, peer_req, digest);
--
2.17.1
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v7] drbd: Convert from ahash to shash
2018-08-06 23:32 [PATCH v7] drbd: Convert from ahash to shash Kees Cook
@ 2018-09-04 6:04 ` Kees Cook
2018-09-05 3:04 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2018-09-04 6:04 UTC (permalink / raw)
To: Lars Ellenberg; +Cc: Philipp Reisner, Jens Axboe, linux-block, drbd-dev, LKML
On Mon, Aug 6, 2018 at 4:32 PM, Kees Cook <keescook@chromium.org> wrote:
> In preparing to remove all stack VLA usage from the kernel[1], this
> removes the discouraged use of AHASH_REQUEST_ON_STACK in favor of
> the smaller SHASH_DESC_ON_STACK by converting from ahash-wrapped-shash
> to direct shash. By removing a layer of indirection this both improves
> performance and reduces stack usage. The stack allocation will be made
> a fixed size in a later patch to the crypto subsystem.
>
> The bulk of the lines in this change are simple s/ahash/shash/, but the
> main logic differences are in drbd_csum_ee() and drbd_csum_bio(), which
> externalizes the page walking with k(un)map_atomic() instead of using
> scattergather.
Hi Lars! How does this look to you? If you can Ack I assume Jens would
be able to take this.
Thanks!
-Kees
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> I'm calling this "v7" just for general sanity over the naming of the
> patch when it was part of the large crypto series of changes. Really,
> it's v2.
>
> v7: use kmap for possible highmem page addresses (Lars).
> ---
> drivers/block/drbd/drbd_int.h | 13 +++---
> drivers/block/drbd/drbd_main.c | 14 +++----
> drivers/block/drbd/drbd_nl.c | 39 ++++++------------
> drivers/block/drbd/drbd_receiver.c | 35 +++++++++--------
> drivers/block/drbd/drbd_worker.c | 63 +++++++++++++++---------------
> 5 files changed, 76 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h
> index bc4ed2ed40a2..97d8e290c2be 100644
> --- a/drivers/block/drbd/drbd_int.h
> +++ b/drivers/block/drbd/drbd_int.h
> @@ -726,10 +726,10 @@ struct drbd_connection {
> struct list_head transfer_log; /* all requests not yet fully processed */
>
> struct crypto_shash *cram_hmac_tfm;
> - struct crypto_ahash *integrity_tfm; /* checksums we compute, updates protected by connection->data->mutex */
> - struct crypto_ahash *peer_integrity_tfm; /* checksums we verify, only accessed from receiver thread */
> - struct crypto_ahash *csums_tfm;
> - struct crypto_ahash *verify_tfm;
> + struct crypto_shash *integrity_tfm; /* checksums we compute, updates protected by connection->data->mutex */
> + struct crypto_shash *peer_integrity_tfm; /* checksums we verify, only accessed from receiver thread */
> + struct crypto_shash *csums_tfm;
> + struct crypto_shash *verify_tfm;
> void *int_dig_in;
> void *int_dig_vv;
>
> @@ -1533,8 +1533,9 @@ static inline void ov_out_of_sync_print(struct drbd_device *device)
> }
>
>
> -extern void drbd_csum_bio(struct crypto_ahash *, struct bio *, void *);
> -extern void drbd_csum_ee(struct crypto_ahash *, struct drbd_peer_request *, void *);
> +extern void drbd_csum_bio(struct crypto_shash *, struct bio *, void *);
> +extern void drbd_csum_ee(struct crypto_shash *, struct drbd_peer_request *,
> + void *);
> /* worker callbacks */
> extern int w_e_end_data_req(struct drbd_work *, int);
> extern int w_e_end_rsdata_req(struct drbd_work *, int);
> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> index a80809bd3057..ccb54791d39c 100644
> --- a/drivers/block/drbd/drbd_main.c
> +++ b/drivers/block/drbd/drbd_main.c
> @@ -1377,7 +1377,7 @@ void drbd_send_ack_dp(struct drbd_peer_device *peer_device, enum drbd_packet cmd
> struct p_data *dp, int data_size)
> {
> if (peer_device->connection->peer_integrity_tfm)
> - data_size -= crypto_ahash_digestsize(peer_device->connection->peer_integrity_tfm);
> + data_size -= crypto_shash_digestsize(peer_device->connection->peer_integrity_tfm);
> _drbd_send_ack(peer_device, cmd, dp->sector, cpu_to_be32(data_size),
> dp->block_id);
> }
> @@ -1690,7 +1690,7 @@ int drbd_send_dblock(struct drbd_peer_device *peer_device, struct drbd_request *
> sock = &peer_device->connection->data;
> p = drbd_prepare_command(peer_device, sock);
> digest_size = peer_device->connection->integrity_tfm ?
> - crypto_ahash_digestsize(peer_device->connection->integrity_tfm) : 0;
> + crypto_shash_digestsize(peer_device->connection->integrity_tfm) : 0;
>
> if (!p)
> return -EIO;
> @@ -1796,7 +1796,7 @@ int drbd_send_block(struct drbd_peer_device *peer_device, enum drbd_packet cmd,
> p = drbd_prepare_command(peer_device, sock);
>
> digest_size = peer_device->connection->integrity_tfm ?
> - crypto_ahash_digestsize(peer_device->connection->integrity_tfm) : 0;
> + crypto_shash_digestsize(peer_device->connection->integrity_tfm) : 0;
>
> if (!p)
> return -EIO;
> @@ -2561,11 +2561,11 @@ void conn_free_crypto(struct drbd_connection *connection)
> {
> drbd_free_sock(connection);
>
> - crypto_free_ahash(connection->csums_tfm);
> - crypto_free_ahash(connection->verify_tfm);
> + crypto_free_shash(connection->csums_tfm);
> + crypto_free_shash(connection->verify_tfm);
> crypto_free_shash(connection->cram_hmac_tfm);
> - crypto_free_ahash(connection->integrity_tfm);
> - crypto_free_ahash(connection->peer_integrity_tfm);
> + crypto_free_shash(connection->integrity_tfm);
> + crypto_free_shash(connection->peer_integrity_tfm);
> kfree(connection->int_dig_in);
> kfree(connection->int_dig_vv);
>
> diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
> index b4f02768ba47..d15703b1ffe8 100644
> --- a/drivers/block/drbd/drbd_nl.c
> +++ b/drivers/block/drbd/drbd_nl.c
> @@ -2303,10 +2303,10 @@ check_net_options(struct drbd_connection *connection, struct net_conf *new_net_c
> }
>
> struct crypto {
> - struct crypto_ahash *verify_tfm;
> - struct crypto_ahash *csums_tfm;
> + struct crypto_shash *verify_tfm;
> + struct crypto_shash *csums_tfm;
> struct crypto_shash *cram_hmac_tfm;
> - struct crypto_ahash *integrity_tfm;
> + struct crypto_shash *integrity_tfm;
> };
>
> static int
> @@ -2324,36 +2324,21 @@ alloc_shash(struct crypto_shash **tfm, char *tfm_name, int err_alg)
> return NO_ERROR;
> }
>
> -static int
> -alloc_ahash(struct crypto_ahash **tfm, char *tfm_name, int err_alg)
> -{
> - if (!tfm_name[0])
> - return NO_ERROR;
> -
> - *tfm = crypto_alloc_ahash(tfm_name, 0, CRYPTO_ALG_ASYNC);
> - if (IS_ERR(*tfm)) {
> - *tfm = NULL;
> - return err_alg;
> - }
> -
> - return NO_ERROR;
> -}
> -
> static enum drbd_ret_code
> alloc_crypto(struct crypto *crypto, struct net_conf *new_net_conf)
> {
> char hmac_name[CRYPTO_MAX_ALG_NAME];
> enum drbd_ret_code rv;
>
> - rv = alloc_ahash(&crypto->csums_tfm, new_net_conf->csums_alg,
> + rv = alloc_shash(&crypto->csums_tfm, new_net_conf->csums_alg,
> ERR_CSUMS_ALG);
> if (rv != NO_ERROR)
> return rv;
> - rv = alloc_ahash(&crypto->verify_tfm, new_net_conf->verify_alg,
> + rv = alloc_shash(&crypto->verify_tfm, new_net_conf->verify_alg,
> ERR_VERIFY_ALG);
> if (rv != NO_ERROR)
> return rv;
> - rv = alloc_ahash(&crypto->integrity_tfm, new_net_conf->integrity_alg,
> + rv = alloc_shash(&crypto->integrity_tfm, new_net_conf->integrity_alg,
> ERR_INTEGRITY_ALG);
> if (rv != NO_ERROR)
> return rv;
> @@ -2371,9 +2356,9 @@ alloc_crypto(struct crypto *crypto, struct net_conf *new_net_conf)
> static void free_crypto(struct crypto *crypto)
> {
> crypto_free_shash(crypto->cram_hmac_tfm);
> - crypto_free_ahash(crypto->integrity_tfm);
> - crypto_free_ahash(crypto->csums_tfm);
> - crypto_free_ahash(crypto->verify_tfm);
> + crypto_free_shash(crypto->integrity_tfm);
> + crypto_free_shash(crypto->csums_tfm);
> + crypto_free_shash(crypto->verify_tfm);
> }
>
> int drbd_adm_net_opts(struct sk_buff *skb, struct genl_info *info)
> @@ -2450,17 +2435,17 @@ int drbd_adm_net_opts(struct sk_buff *skb, struct genl_info *info)
> rcu_assign_pointer(connection->net_conf, new_net_conf);
>
> if (!rsr) {
> - crypto_free_ahash(connection->csums_tfm);
> + crypto_free_shash(connection->csums_tfm);
> connection->csums_tfm = crypto.csums_tfm;
> crypto.csums_tfm = NULL;
> }
> if (!ovr) {
> - crypto_free_ahash(connection->verify_tfm);
> + crypto_free_shash(connection->verify_tfm);
> connection->verify_tfm = crypto.verify_tfm;
> crypto.verify_tfm = NULL;
> }
>
> - crypto_free_ahash(connection->integrity_tfm);
> + crypto_free_shash(connection->integrity_tfm);
> connection->integrity_tfm = crypto.integrity_tfm;
> if (connection->cstate >= C_WF_REPORT_PARAMS && connection->agreed_pro_version >= 100)
> /* Do this without trying to take connection->data.mutex again. */
> diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
> index be9450f5ad1c..76243e9ef277 100644
> --- a/drivers/block/drbd/drbd_receiver.c
> +++ b/drivers/block/drbd/drbd_receiver.c
> @@ -1732,7 +1732,7 @@ static int receive_Barrier(struct drbd_connection *connection, struct packet_inf
> }
>
> /* quick wrapper in case payload size != request_size (write same) */
> -static void drbd_csum_ee_size(struct crypto_ahash *h,
> +static void drbd_csum_ee_size(struct crypto_shash *h,
> struct drbd_peer_request *r, void *d,
> unsigned int payload_size)
> {
> @@ -1769,7 +1769,7 @@ read_in_block(struct drbd_peer_device *peer_device, u64 id, sector_t sector,
>
> digest_size = 0;
> if (!trim && peer_device->connection->peer_integrity_tfm) {
> - digest_size = crypto_ahash_digestsize(peer_device->connection->peer_integrity_tfm);
> + digest_size = crypto_shash_digestsize(peer_device->connection->peer_integrity_tfm);
> /*
> * FIXME: Receive the incoming digest into the receive buffer
> * here, together with its struct p_data?
> @@ -1905,7 +1905,7 @@ static int recv_dless_read(struct drbd_peer_device *peer_device, struct drbd_req
>
> digest_size = 0;
> if (peer_device->connection->peer_integrity_tfm) {
> - digest_size = crypto_ahash_digestsize(peer_device->connection->peer_integrity_tfm);
> + digest_size = crypto_shash_digestsize(peer_device->connection->peer_integrity_tfm);
> err = drbd_recv_all_warn(peer_device->connection, dig_in, digest_size);
> if (err)
> return err;
> @@ -3540,7 +3540,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
> int p_proto, p_discard_my_data, p_two_primaries, cf;
> struct net_conf *nc, *old_net_conf, *new_net_conf = NULL;
> char integrity_alg[SHARED_SECRET_MAX] = "";
> - struct crypto_ahash *peer_integrity_tfm = NULL;
> + struct crypto_shash *peer_integrity_tfm = NULL;
> void *int_dig_in = NULL, *int_dig_vv = NULL;
>
> p_proto = be32_to_cpu(p->protocol);
> @@ -3621,7 +3621,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
> * change.
> */
>
> - peer_integrity_tfm = crypto_alloc_ahash(integrity_alg, 0, CRYPTO_ALG_ASYNC);
> + peer_integrity_tfm = crypto_alloc_shash(integrity_alg, 0, CRYPTO_ALG_ASYNC);
> if (IS_ERR(peer_integrity_tfm)) {
> peer_integrity_tfm = NULL;
> drbd_err(connection, "peer data-integrity-alg %s not supported\n",
> @@ -3629,7 +3629,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
> goto disconnect;
> }
>
> - hash_size = crypto_ahash_digestsize(peer_integrity_tfm);
> + hash_size = crypto_shash_digestsize(peer_integrity_tfm);
> int_dig_in = kmalloc(hash_size, GFP_KERNEL);
> int_dig_vv = kmalloc(hash_size, GFP_KERNEL);
> if (!(int_dig_in && int_dig_vv)) {
> @@ -3659,7 +3659,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
> mutex_unlock(&connection->resource->conf_update);
> mutex_unlock(&connection->data.mutex);
>
> - crypto_free_ahash(connection->peer_integrity_tfm);
> + crypto_free_shash(connection->peer_integrity_tfm);
> kfree(connection->int_dig_in);
> kfree(connection->int_dig_vv);
> connection->peer_integrity_tfm = peer_integrity_tfm;
> @@ -3677,7 +3677,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
> disconnect_rcu_unlock:
> rcu_read_unlock();
> disconnect:
> - crypto_free_ahash(peer_integrity_tfm);
> + crypto_free_shash(peer_integrity_tfm);
> kfree(int_dig_in);
> kfree(int_dig_vv);
> conn_request_state(connection, NS(conn, C_DISCONNECTING), CS_HARD);
> @@ -3689,15 +3689,16 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
> * return: NULL (alg name was "")
> * ERR_PTR(error) if something goes wrong
> * or the crypto hash ptr, if it worked out ok. */
> -static struct crypto_ahash *drbd_crypto_alloc_digest_safe(const struct drbd_device *device,
> +static struct crypto_shash *drbd_crypto_alloc_digest_safe(
> + const struct drbd_device *device,
> const char *alg, const char *name)
> {
> - struct crypto_ahash *tfm;
> + struct crypto_shash *tfm;
>
> if (!alg[0])
> return NULL;
>
> - tfm = crypto_alloc_ahash(alg, 0, CRYPTO_ALG_ASYNC);
> + tfm = crypto_alloc_shash(alg, 0, 0);
> if (IS_ERR(tfm)) {
> drbd_err(device, "Can not allocate \"%s\" as %s (reason: %ld)\n",
> alg, name, PTR_ERR(tfm));
> @@ -3750,8 +3751,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i
> struct drbd_device *device;
> struct p_rs_param_95 *p;
> unsigned int header_size, data_size, exp_max_sz;
> - struct crypto_ahash *verify_tfm = NULL;
> - struct crypto_ahash *csums_tfm = NULL;
> + struct crypto_shash *verify_tfm = NULL;
> + struct crypto_shash *csums_tfm = NULL;
> struct net_conf *old_net_conf, *new_net_conf = NULL;
> struct disk_conf *old_disk_conf = NULL, *new_disk_conf = NULL;
> const int apv = connection->agreed_pro_version;
> @@ -3898,14 +3899,14 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i
> if (verify_tfm) {
> strcpy(new_net_conf->verify_alg, p->verify_alg);
> new_net_conf->verify_alg_len = strlen(p->verify_alg) + 1;
> - crypto_free_ahash(peer_device->connection->verify_tfm);
> + crypto_free_shash(peer_device->connection->verify_tfm);
> peer_device->connection->verify_tfm = verify_tfm;
> drbd_info(device, "using verify-alg: \"%s\"\n", p->verify_alg);
> }
> if (csums_tfm) {
> strcpy(new_net_conf->csums_alg, p->csums_alg);
> new_net_conf->csums_alg_len = strlen(p->csums_alg) + 1;
> - crypto_free_ahash(peer_device->connection->csums_tfm);
> + crypto_free_shash(peer_device->connection->csums_tfm);
> peer_device->connection->csums_tfm = csums_tfm;
> drbd_info(device, "using csums-alg: \"%s\"\n", p->csums_alg);
> }
> @@ -3949,9 +3950,9 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i
> mutex_unlock(&connection->resource->conf_update);
> /* just for completeness: actually not needed,
> * as this is not reached if csums_tfm was ok. */
> - crypto_free_ahash(csums_tfm);
> + crypto_free_shash(csums_tfm);
> /* but free the verify_tfm again, if csums_tfm did not work out */
> - crypto_free_ahash(verify_tfm);
> + crypto_free_shash(verify_tfm);
> conn_request_state(peer_device->connection, NS(conn, C_DISCONNECTING), CS_HARD);
> return -EIO;
> }
> diff --git a/drivers/block/drbd/drbd_worker.c b/drivers/block/drbd/drbd_worker.c
> index 5e793dd7adfb..0b41fb0181a6 100644
> --- a/drivers/block/drbd/drbd_worker.c
> +++ b/drivers/block/drbd/drbd_worker.c
> @@ -295,60 +295,61 @@ void drbd_request_endio(struct bio *bio)
> complete_master_bio(device, &m);
> }
>
> -void drbd_csum_ee(struct crypto_ahash *tfm, struct drbd_peer_request *peer_req, void *digest)
> +void drbd_csum_ee(struct crypto_shash *tfm, struct drbd_peer_request *peer_req, void *digest)
> {
> - AHASH_REQUEST_ON_STACK(req, tfm);
> - struct scatterlist sg;
> + SHASH_DESC_ON_STACK(desc, tfm);
> struct page *page = peer_req->pages;
> struct page *tmp;
> unsigned len;
> + void *src;
>
> - ahash_request_set_tfm(req, tfm);
> - ahash_request_set_callback(req, 0, NULL, NULL);
> + desc->tfm = tfm;
> + desc->flags = 0;
>
> - sg_init_table(&sg, 1);
> - crypto_ahash_init(req);
> + crypto_shash_init(desc);
>
> + src = kmap_atomic(page);
> while ((tmp = page_chain_next(page))) {
> /* all but the last page will be fully used */
> - sg_set_page(&sg, page, PAGE_SIZE, 0);
> - ahash_request_set_crypt(req, &sg, NULL, sg.length);
> - crypto_ahash_update(req);
> + crypto_shash_update(desc, src, PAGE_SIZE);
> + kunmap_atomic(src);
> page = tmp;
> + src = kmap_atomic(page);
> }
> /* and now the last, possibly only partially used page */
> len = peer_req->i.size & (PAGE_SIZE - 1);
> - sg_set_page(&sg, page, len ?: PAGE_SIZE, 0);
> - ahash_request_set_crypt(req, &sg, digest, sg.length);
> - crypto_ahash_finup(req);
> - ahash_request_zero(req);
> + crypto_shash_update(desc, src, len ?: PAGE_SIZE);
> + kunmap_atomic(src);
> +
> + crypto_shash_final(desc, digest);
> + shash_desc_zero(desc);
> }
>
> -void drbd_csum_bio(struct crypto_ahash *tfm, struct bio *bio, void *digest)
> +void drbd_csum_bio(struct crypto_shash *tfm, struct bio *bio, void *digest)
> {
> - AHASH_REQUEST_ON_STACK(req, tfm);
> - struct scatterlist sg;
> + SHASH_DESC_ON_STACK(desc, tfm);
> struct bio_vec bvec;
> struct bvec_iter iter;
>
> - ahash_request_set_tfm(req, tfm);
> - ahash_request_set_callback(req, 0, NULL, NULL);
> + desc->tfm = tfm;
> + desc->flags = 0;
>
> - sg_init_table(&sg, 1);
> - crypto_ahash_init(req);
> + crypto_shash_init(desc);
>
> bio_for_each_segment(bvec, bio, iter) {
> - sg_set_page(&sg, bvec.bv_page, bvec.bv_len, bvec.bv_offset);
> - ahash_request_set_crypt(req, &sg, NULL, sg.length);
> - crypto_ahash_update(req);
> + u8 *src;
> +
> + src = kmap_atomic(bvec.bv_page);
> + crypto_shash_update(desc, src + bvec.bv_offset, bvec.bv_len);
> + kunmap_atomic(src);
> +
> /* REQ_OP_WRITE_SAME has only one segment,
> * checksum the payload only once. */
> if (bio_op(bio) == REQ_OP_WRITE_SAME)
> break;
> }
> - ahash_request_set_crypt(req, NULL, digest, 0);
> - crypto_ahash_final(req);
> - ahash_request_zero(req);
> + crypto_shash_final(desc, digest);
> + shash_desc_zero(desc);
> }
>
> /* MAYBE merge common code with w_e_end_ov_req */
> @@ -367,7 +368,7 @@ static int w_e_send_csum(struct drbd_work *w, int cancel)
> if (unlikely((peer_req->flags & EE_WAS_ERROR) != 0))
> goto out;
>
> - digest_size = crypto_ahash_digestsize(peer_device->connection->csums_tfm);
> + digest_size = crypto_shash_digestsize(peer_device->connection->csums_tfm);
> digest = kmalloc(digest_size, GFP_NOIO);
> if (digest) {
> sector_t sector = peer_req->i.sector;
> @@ -1205,7 +1206,7 @@ int w_e_end_csum_rs_req(struct drbd_work *w, int cancel)
> * a real fix would be much more involved,
> * introducing more locking mechanisms */
> if (peer_device->connection->csums_tfm) {
> - digest_size = crypto_ahash_digestsize(peer_device->connection->csums_tfm);
> + digest_size = crypto_shash_digestsize(peer_device->connection->csums_tfm);
> D_ASSERT(device, digest_size == di->digest_size);
> digest = kmalloc(digest_size, GFP_NOIO);
> }
> @@ -1255,7 +1256,7 @@ int w_e_end_ov_req(struct drbd_work *w, int cancel)
> if (unlikely(cancel))
> goto out;
>
> - digest_size = crypto_ahash_digestsize(peer_device->connection->verify_tfm);
> + digest_size = crypto_shash_digestsize(peer_device->connection->verify_tfm);
> digest = kmalloc(digest_size, GFP_NOIO);
> if (!digest) {
> err = 1; /* terminate the connection in case the allocation failed */
> @@ -1327,7 +1328,7 @@ int w_e_end_ov_reply(struct drbd_work *w, int cancel)
> di = peer_req->digest;
>
> if (likely((peer_req->flags & EE_WAS_ERROR) == 0)) {
> - digest_size = crypto_ahash_digestsize(peer_device->connection->verify_tfm);
> + digest_size = crypto_shash_digestsize(peer_device->connection->verify_tfm);
> digest = kmalloc(digest_size, GFP_NOIO);
> if (digest) {
> drbd_csum_ee(peer_device->connection->verify_tfm, peer_req, digest);
> --
> 2.17.1
>
>
> --
> Kees Cook
> Pixel Security
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7] drbd: Convert from ahash to shash
2018-09-04 6:04 ` Kees Cook
@ 2018-09-05 3:04 ` Kees Cook
2018-09-05 8:33 ` Lars Ellenberg
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2018-09-05 3:04 UTC (permalink / raw)
To: Lars Ellenberg, Jens Axboe; +Cc: Philipp Reisner, linux-block, drbd-dev, LKML
On Mon, Sep 3, 2018 at 11:04 PM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Aug 6, 2018 at 4:32 PM, Kees Cook <keescook@chromium.org> wrote:
>> In preparing to remove all stack VLA usage from the kernel[1], this
>> removes the discouraged use of AHASH_REQUEST_ON_STACK in favor of
>> the smaller SHASH_DESC_ON_STACK by converting from ahash-wrapped-shash
>> to direct shash. By removing a layer of indirection this both improves
>> performance and reduces stack usage. The stack allocation will be made
>> a fixed size in a later patch to the crypto subsystem.
>>
>> The bulk of the lines in this change are simple s/ahash/shash/, but the
>> main logic differences are in drbd_csum_ee() and drbd_csum_bio(), which
>> externalizes the page walking with k(un)map_atomic() instead of using
>> scattergather.
>
> Hi Lars! How does this look to you? If you can Ack I assume Jens would
> be able to take this.
FWIW I've tested a simple drbd configuration before/after this change
and things seem to be working correctly.
-Kees
>
> Thanks!
>
> -Kees
>
>>
>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> I'm calling this "v7" just for general sanity over the naming of the
>> patch when it was part of the large crypto series of changes. Really,
>> it's v2.
>>
>> v7: use kmap for possible highmem page addresses (Lars).
>> ---
>> drivers/block/drbd/drbd_int.h | 13 +++---
>> drivers/block/drbd/drbd_main.c | 14 +++----
>> drivers/block/drbd/drbd_nl.c | 39 ++++++------------
>> drivers/block/drbd/drbd_receiver.c | 35 +++++++++--------
>> drivers/block/drbd/drbd_worker.c | 63 +++++++++++++++---------------
>> 5 files changed, 76 insertions(+), 88 deletions(-)
>>
>> diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h
>> index bc4ed2ed40a2..97d8e290c2be 100644
>> --- a/drivers/block/drbd/drbd_int.h
>> +++ b/drivers/block/drbd/drbd_int.h
>> @@ -726,10 +726,10 @@ struct drbd_connection {
>> struct list_head transfer_log; /* all requests not yet fully processed */
>>
>> struct crypto_shash *cram_hmac_tfm;
>> - struct crypto_ahash *integrity_tfm; /* checksums we compute, updates protected by connection->data->mutex */
>> - struct crypto_ahash *peer_integrity_tfm; /* checksums we verify, only accessed from receiver thread */
>> - struct crypto_ahash *csums_tfm;
>> - struct crypto_ahash *verify_tfm;
>> + struct crypto_shash *integrity_tfm; /* checksums we compute, updates protected by connection->data->mutex */
>> + struct crypto_shash *peer_integrity_tfm; /* checksums we verify, only accessed from receiver thread */
>> + struct crypto_shash *csums_tfm;
>> + struct crypto_shash *verify_tfm;
>> void *int_dig_in;
>> void *int_dig_vv;
>>
>> @@ -1533,8 +1533,9 @@ static inline void ov_out_of_sync_print(struct drbd_device *device)
>> }
>>
>>
>> -extern void drbd_csum_bio(struct crypto_ahash *, struct bio *, void *);
>> -extern void drbd_csum_ee(struct crypto_ahash *, struct drbd_peer_request *, void *);
>> +extern void drbd_csum_bio(struct crypto_shash *, struct bio *, void *);
>> +extern void drbd_csum_ee(struct crypto_shash *, struct drbd_peer_request *,
>> + void *);
>> /* worker callbacks */
>> extern int w_e_end_data_req(struct drbd_work *, int);
>> extern int w_e_end_rsdata_req(struct drbd_work *, int);
>> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
>> index a80809bd3057..ccb54791d39c 100644
>> --- a/drivers/block/drbd/drbd_main.c
>> +++ b/drivers/block/drbd/drbd_main.c
>> @@ -1377,7 +1377,7 @@ void drbd_send_ack_dp(struct drbd_peer_device *peer_device, enum drbd_packet cmd
>> struct p_data *dp, int data_size)
>> {
>> if (peer_device->connection->peer_integrity_tfm)
>> - data_size -= crypto_ahash_digestsize(peer_device->connection->peer_integrity_tfm);
>> + data_size -= crypto_shash_digestsize(peer_device->connection->peer_integrity_tfm);
>> _drbd_send_ack(peer_device, cmd, dp->sector, cpu_to_be32(data_size),
>> dp->block_id);
>> }
>> @@ -1690,7 +1690,7 @@ int drbd_send_dblock(struct drbd_peer_device *peer_device, struct drbd_request *
>> sock = &peer_device->connection->data;
>> p = drbd_prepare_command(peer_device, sock);
>> digest_size = peer_device->connection->integrity_tfm ?
>> - crypto_ahash_digestsize(peer_device->connection->integrity_tfm) : 0;
>> + crypto_shash_digestsize(peer_device->connection->integrity_tfm) : 0;
>>
>> if (!p)
>> return -EIO;
>> @@ -1796,7 +1796,7 @@ int drbd_send_block(struct drbd_peer_device *peer_device, enum drbd_packet cmd,
>> p = drbd_prepare_command(peer_device, sock);
>>
>> digest_size = peer_device->connection->integrity_tfm ?
>> - crypto_ahash_digestsize(peer_device->connection->integrity_tfm) : 0;
>> + crypto_shash_digestsize(peer_device->connection->integrity_tfm) : 0;
>>
>> if (!p)
>> return -EIO;
>> @@ -2561,11 +2561,11 @@ void conn_free_crypto(struct drbd_connection *connection)
>> {
>> drbd_free_sock(connection);
>>
>> - crypto_free_ahash(connection->csums_tfm);
>> - crypto_free_ahash(connection->verify_tfm);
>> + crypto_free_shash(connection->csums_tfm);
>> + crypto_free_shash(connection->verify_tfm);
>> crypto_free_shash(connection->cram_hmac_tfm);
>> - crypto_free_ahash(connection->integrity_tfm);
>> - crypto_free_ahash(connection->peer_integrity_tfm);
>> + crypto_free_shash(connection->integrity_tfm);
>> + crypto_free_shash(connection->peer_integrity_tfm);
>> kfree(connection->int_dig_in);
>> kfree(connection->int_dig_vv);
>>
>> diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
>> index b4f02768ba47..d15703b1ffe8 100644
>> --- a/drivers/block/drbd/drbd_nl.c
>> +++ b/drivers/block/drbd/drbd_nl.c
>> @@ -2303,10 +2303,10 @@ check_net_options(struct drbd_connection *connection, struct net_conf *new_net_c
>> }
>>
>> struct crypto {
>> - struct crypto_ahash *verify_tfm;
>> - struct crypto_ahash *csums_tfm;
>> + struct crypto_shash *verify_tfm;
>> + struct crypto_shash *csums_tfm;
>> struct crypto_shash *cram_hmac_tfm;
>> - struct crypto_ahash *integrity_tfm;
>> + struct crypto_shash *integrity_tfm;
>> };
>>
>> static int
>> @@ -2324,36 +2324,21 @@ alloc_shash(struct crypto_shash **tfm, char *tfm_name, int err_alg)
>> return NO_ERROR;
>> }
>>
>> -static int
>> -alloc_ahash(struct crypto_ahash **tfm, char *tfm_name, int err_alg)
>> -{
>> - if (!tfm_name[0])
>> - return NO_ERROR;
>> -
>> - *tfm = crypto_alloc_ahash(tfm_name, 0, CRYPTO_ALG_ASYNC);
>> - if (IS_ERR(*tfm)) {
>> - *tfm = NULL;
>> - return err_alg;
>> - }
>> -
>> - return NO_ERROR;
>> -}
>> -
>> static enum drbd_ret_code
>> alloc_crypto(struct crypto *crypto, struct net_conf *new_net_conf)
>> {
>> char hmac_name[CRYPTO_MAX_ALG_NAME];
>> enum drbd_ret_code rv;
>>
>> - rv = alloc_ahash(&crypto->csums_tfm, new_net_conf->csums_alg,
>> + rv = alloc_shash(&crypto->csums_tfm, new_net_conf->csums_alg,
>> ERR_CSUMS_ALG);
>> if (rv != NO_ERROR)
>> return rv;
>> - rv = alloc_ahash(&crypto->verify_tfm, new_net_conf->verify_alg,
>> + rv = alloc_shash(&crypto->verify_tfm, new_net_conf->verify_alg,
>> ERR_VERIFY_ALG);
>> if (rv != NO_ERROR)
>> return rv;
>> - rv = alloc_ahash(&crypto->integrity_tfm, new_net_conf->integrity_alg,
>> + rv = alloc_shash(&crypto->integrity_tfm, new_net_conf->integrity_alg,
>> ERR_INTEGRITY_ALG);
>> if (rv != NO_ERROR)
>> return rv;
>> @@ -2371,9 +2356,9 @@ alloc_crypto(struct crypto *crypto, struct net_conf *new_net_conf)
>> static void free_crypto(struct crypto *crypto)
>> {
>> crypto_free_shash(crypto->cram_hmac_tfm);
>> - crypto_free_ahash(crypto->integrity_tfm);
>> - crypto_free_ahash(crypto->csums_tfm);
>> - crypto_free_ahash(crypto->verify_tfm);
>> + crypto_free_shash(crypto->integrity_tfm);
>> + crypto_free_shash(crypto->csums_tfm);
>> + crypto_free_shash(crypto->verify_tfm);
>> }
>>
>> int drbd_adm_net_opts(struct sk_buff *skb, struct genl_info *info)
>> @@ -2450,17 +2435,17 @@ int drbd_adm_net_opts(struct sk_buff *skb, struct genl_info *info)
>> rcu_assign_pointer(connection->net_conf, new_net_conf);
>>
>> if (!rsr) {
>> - crypto_free_ahash(connection->csums_tfm);
>> + crypto_free_shash(connection->csums_tfm);
>> connection->csums_tfm = crypto.csums_tfm;
>> crypto.csums_tfm = NULL;
>> }
>> if (!ovr) {
>> - crypto_free_ahash(connection->verify_tfm);
>> + crypto_free_shash(connection->verify_tfm);
>> connection->verify_tfm = crypto.verify_tfm;
>> crypto.verify_tfm = NULL;
>> }
>>
>> - crypto_free_ahash(connection->integrity_tfm);
>> + crypto_free_shash(connection->integrity_tfm);
>> connection->integrity_tfm = crypto.integrity_tfm;
>> if (connection->cstate >= C_WF_REPORT_PARAMS && connection->agreed_pro_version >= 100)
>> /* Do this without trying to take connection->data.mutex again. */
>> diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
>> index be9450f5ad1c..76243e9ef277 100644
>> --- a/drivers/block/drbd/drbd_receiver.c
>> +++ b/drivers/block/drbd/drbd_receiver.c
>> @@ -1732,7 +1732,7 @@ static int receive_Barrier(struct drbd_connection *connection, struct packet_inf
>> }
>>
>> /* quick wrapper in case payload size != request_size (write same) */
>> -static void drbd_csum_ee_size(struct crypto_ahash *h,
>> +static void drbd_csum_ee_size(struct crypto_shash *h,
>> struct drbd_peer_request *r, void *d,
>> unsigned int payload_size)
>> {
>> @@ -1769,7 +1769,7 @@ read_in_block(struct drbd_peer_device *peer_device, u64 id, sector_t sector,
>>
>> digest_size = 0;
>> if (!trim && peer_device->connection->peer_integrity_tfm) {
>> - digest_size = crypto_ahash_digestsize(peer_device->connection->peer_integrity_tfm);
>> + digest_size = crypto_shash_digestsize(peer_device->connection->peer_integrity_tfm);
>> /*
>> * FIXME: Receive the incoming digest into the receive buffer
>> * here, together with its struct p_data?
>> @@ -1905,7 +1905,7 @@ static int recv_dless_read(struct drbd_peer_device *peer_device, struct drbd_req
>>
>> digest_size = 0;
>> if (peer_device->connection->peer_integrity_tfm) {
>> - digest_size = crypto_ahash_digestsize(peer_device->connection->peer_integrity_tfm);
>> + digest_size = crypto_shash_digestsize(peer_device->connection->peer_integrity_tfm);
>> err = drbd_recv_all_warn(peer_device->connection, dig_in, digest_size);
>> if (err)
>> return err;
>> @@ -3540,7 +3540,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
>> int p_proto, p_discard_my_data, p_two_primaries, cf;
>> struct net_conf *nc, *old_net_conf, *new_net_conf = NULL;
>> char integrity_alg[SHARED_SECRET_MAX] = "";
>> - struct crypto_ahash *peer_integrity_tfm = NULL;
>> + struct crypto_shash *peer_integrity_tfm = NULL;
>> void *int_dig_in = NULL, *int_dig_vv = NULL;
>>
>> p_proto = be32_to_cpu(p->protocol);
>> @@ -3621,7 +3621,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
>> * change.
>> */
>>
>> - peer_integrity_tfm = crypto_alloc_ahash(integrity_alg, 0, CRYPTO_ALG_ASYNC);
>> + peer_integrity_tfm = crypto_alloc_shash(integrity_alg, 0, CRYPTO_ALG_ASYNC);
>> if (IS_ERR(peer_integrity_tfm)) {
>> peer_integrity_tfm = NULL;
>> drbd_err(connection, "peer data-integrity-alg %s not supported\n",
>> @@ -3629,7 +3629,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
>> goto disconnect;
>> }
>>
>> - hash_size = crypto_ahash_digestsize(peer_integrity_tfm);
>> + hash_size = crypto_shash_digestsize(peer_integrity_tfm);
>> int_dig_in = kmalloc(hash_size, GFP_KERNEL);
>> int_dig_vv = kmalloc(hash_size, GFP_KERNEL);
>> if (!(int_dig_in && int_dig_vv)) {
>> @@ -3659,7 +3659,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
>> mutex_unlock(&connection->resource->conf_update);
>> mutex_unlock(&connection->data.mutex);
>>
>> - crypto_free_ahash(connection->peer_integrity_tfm);
>> + crypto_free_shash(connection->peer_integrity_tfm);
>> kfree(connection->int_dig_in);
>> kfree(connection->int_dig_vv);
>> connection->peer_integrity_tfm = peer_integrity_tfm;
>> @@ -3677,7 +3677,7 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
>> disconnect_rcu_unlock:
>> rcu_read_unlock();
>> disconnect:
>> - crypto_free_ahash(peer_integrity_tfm);
>> + crypto_free_shash(peer_integrity_tfm);
>> kfree(int_dig_in);
>> kfree(int_dig_vv);
>> conn_request_state(connection, NS(conn, C_DISCONNECTING), CS_HARD);
>> @@ -3689,15 +3689,16 @@ static int receive_protocol(struct drbd_connection *connection, struct packet_in
>> * return: NULL (alg name was "")
>> * ERR_PTR(error) if something goes wrong
>> * or the crypto hash ptr, if it worked out ok. */
>> -static struct crypto_ahash *drbd_crypto_alloc_digest_safe(const struct drbd_device *device,
>> +static struct crypto_shash *drbd_crypto_alloc_digest_safe(
>> + const struct drbd_device *device,
>> const char *alg, const char *name)
>> {
>> - struct crypto_ahash *tfm;
>> + struct crypto_shash *tfm;
>>
>> if (!alg[0])
>> return NULL;
>>
>> - tfm = crypto_alloc_ahash(alg, 0, CRYPTO_ALG_ASYNC);
>> + tfm = crypto_alloc_shash(alg, 0, 0);
>> if (IS_ERR(tfm)) {
>> drbd_err(device, "Can not allocate \"%s\" as %s (reason: %ld)\n",
>> alg, name, PTR_ERR(tfm));
>> @@ -3750,8 +3751,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i
>> struct drbd_device *device;
>> struct p_rs_param_95 *p;
>> unsigned int header_size, data_size, exp_max_sz;
>> - struct crypto_ahash *verify_tfm = NULL;
>> - struct crypto_ahash *csums_tfm = NULL;
>> + struct crypto_shash *verify_tfm = NULL;
>> + struct crypto_shash *csums_tfm = NULL;
>> struct net_conf *old_net_conf, *new_net_conf = NULL;
>> struct disk_conf *old_disk_conf = NULL, *new_disk_conf = NULL;
>> const int apv = connection->agreed_pro_version;
>> @@ -3898,14 +3899,14 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i
>> if (verify_tfm) {
>> strcpy(new_net_conf->verify_alg, p->verify_alg);
>> new_net_conf->verify_alg_len = strlen(p->verify_alg) + 1;
>> - crypto_free_ahash(peer_device->connection->verify_tfm);
>> + crypto_free_shash(peer_device->connection->verify_tfm);
>> peer_device->connection->verify_tfm = verify_tfm;
>> drbd_info(device, "using verify-alg: \"%s\"\n", p->verify_alg);
>> }
>> if (csums_tfm) {
>> strcpy(new_net_conf->csums_alg, p->csums_alg);
>> new_net_conf->csums_alg_len = strlen(p->csums_alg) + 1;
>> - crypto_free_ahash(peer_device->connection->csums_tfm);
>> + crypto_free_shash(peer_device->connection->csums_tfm);
>> peer_device->connection->csums_tfm = csums_tfm;
>> drbd_info(device, "using csums-alg: \"%s\"\n", p->csums_alg);
>> }
>> @@ -3949,9 +3950,9 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i
>> mutex_unlock(&connection->resource->conf_update);
>> /* just for completeness: actually not needed,
>> * as this is not reached if csums_tfm was ok. */
>> - crypto_free_ahash(csums_tfm);
>> + crypto_free_shash(csums_tfm);
>> /* but free the verify_tfm again, if csums_tfm did not work out */
>> - crypto_free_ahash(verify_tfm);
>> + crypto_free_shash(verify_tfm);
>> conn_request_state(peer_device->connection, NS(conn, C_DISCONNECTING), CS_HARD);
>> return -EIO;
>> }
>> diff --git a/drivers/block/drbd/drbd_worker.c b/drivers/block/drbd/drbd_worker.c
>> index 5e793dd7adfb..0b41fb0181a6 100644
>> --- a/drivers/block/drbd/drbd_worker.c
>> +++ b/drivers/block/drbd/drbd_worker.c
>> @@ -295,60 +295,61 @@ void drbd_request_endio(struct bio *bio)
>> complete_master_bio(device, &m);
>> }
>>
>> -void drbd_csum_ee(struct crypto_ahash *tfm, struct drbd_peer_request *peer_req, void *digest)
>> +void drbd_csum_ee(struct crypto_shash *tfm, struct drbd_peer_request *peer_req, void *digest)
>> {
>> - AHASH_REQUEST_ON_STACK(req, tfm);
>> - struct scatterlist sg;
>> + SHASH_DESC_ON_STACK(desc, tfm);
>> struct page *page = peer_req->pages;
>> struct page *tmp;
>> unsigned len;
>> + void *src;
>>
>> - ahash_request_set_tfm(req, tfm);
>> - ahash_request_set_callback(req, 0, NULL, NULL);
>> + desc->tfm = tfm;
>> + desc->flags = 0;
>>
>> - sg_init_table(&sg, 1);
>> - crypto_ahash_init(req);
>> + crypto_shash_init(desc);
>>
>> + src = kmap_atomic(page);
>> while ((tmp = page_chain_next(page))) {
>> /* all but the last page will be fully used */
>> - sg_set_page(&sg, page, PAGE_SIZE, 0);
>> - ahash_request_set_crypt(req, &sg, NULL, sg.length);
>> - crypto_ahash_update(req);
>> + crypto_shash_update(desc, src, PAGE_SIZE);
>> + kunmap_atomic(src);
>> page = tmp;
>> + src = kmap_atomic(page);
>> }
>> /* and now the last, possibly only partially used page */
>> len = peer_req->i.size & (PAGE_SIZE - 1);
>> - sg_set_page(&sg, page, len ?: PAGE_SIZE, 0);
>> - ahash_request_set_crypt(req, &sg, digest, sg.length);
>> - crypto_ahash_finup(req);
>> - ahash_request_zero(req);
>> + crypto_shash_update(desc, src, len ?: PAGE_SIZE);
>> + kunmap_atomic(src);
>> +
>> + crypto_shash_final(desc, digest);
>> + shash_desc_zero(desc);
>> }
>>
>> -void drbd_csum_bio(struct crypto_ahash *tfm, struct bio *bio, void *digest)
>> +void drbd_csum_bio(struct crypto_shash *tfm, struct bio *bio, void *digest)
>> {
>> - AHASH_REQUEST_ON_STACK(req, tfm);
>> - struct scatterlist sg;
>> + SHASH_DESC_ON_STACK(desc, tfm);
>> struct bio_vec bvec;
>> struct bvec_iter iter;
>>
>> - ahash_request_set_tfm(req, tfm);
>> - ahash_request_set_callback(req, 0, NULL, NULL);
>> + desc->tfm = tfm;
>> + desc->flags = 0;
>>
>> - sg_init_table(&sg, 1);
>> - crypto_ahash_init(req);
>> + crypto_shash_init(desc);
>>
>> bio_for_each_segment(bvec, bio, iter) {
>> - sg_set_page(&sg, bvec.bv_page, bvec.bv_len, bvec.bv_offset);
>> - ahash_request_set_crypt(req, &sg, NULL, sg.length);
>> - crypto_ahash_update(req);
>> + u8 *src;
>> +
>> + src = kmap_atomic(bvec.bv_page);
>> + crypto_shash_update(desc, src + bvec.bv_offset, bvec.bv_len);
>> + kunmap_atomic(src);
>> +
>> /* REQ_OP_WRITE_SAME has only one segment,
>> * checksum the payload only once. */
>> if (bio_op(bio) == REQ_OP_WRITE_SAME)
>> break;
>> }
>> - ahash_request_set_crypt(req, NULL, digest, 0);
>> - crypto_ahash_final(req);
>> - ahash_request_zero(req);
>> + crypto_shash_final(desc, digest);
>> + shash_desc_zero(desc);
>> }
>>
>> /* MAYBE merge common code with w_e_end_ov_req */
>> @@ -367,7 +368,7 @@ static int w_e_send_csum(struct drbd_work *w, int cancel)
>> if (unlikely((peer_req->flags & EE_WAS_ERROR) != 0))
>> goto out;
>>
>> - digest_size = crypto_ahash_digestsize(peer_device->connection->csums_tfm);
>> + digest_size = crypto_shash_digestsize(peer_device->connection->csums_tfm);
>> digest = kmalloc(digest_size, GFP_NOIO);
>> if (digest) {
>> sector_t sector = peer_req->i.sector;
>> @@ -1205,7 +1206,7 @@ int w_e_end_csum_rs_req(struct drbd_work *w, int cancel)
>> * a real fix would be much more involved,
>> * introducing more locking mechanisms */
>> if (peer_device->connection->csums_tfm) {
>> - digest_size = crypto_ahash_digestsize(peer_device->connection->csums_tfm);
>> + digest_size = crypto_shash_digestsize(peer_device->connection->csums_tfm);
>> D_ASSERT(device, digest_size == di->digest_size);
>> digest = kmalloc(digest_size, GFP_NOIO);
>> }
>> @@ -1255,7 +1256,7 @@ int w_e_end_ov_req(struct drbd_work *w, int cancel)
>> if (unlikely(cancel))
>> goto out;
>>
>> - digest_size = crypto_ahash_digestsize(peer_device->connection->verify_tfm);
>> + digest_size = crypto_shash_digestsize(peer_device->connection->verify_tfm);
>> digest = kmalloc(digest_size, GFP_NOIO);
>> if (!digest) {
>> err = 1; /* terminate the connection in case the allocation failed */
>> @@ -1327,7 +1328,7 @@ int w_e_end_ov_reply(struct drbd_work *w, int cancel)
>> di = peer_req->digest;
>>
>> if (likely((peer_req->flags & EE_WAS_ERROR) == 0)) {
>> - digest_size = crypto_ahash_digestsize(peer_device->connection->verify_tfm);
>> + digest_size = crypto_shash_digestsize(peer_device->connection->verify_tfm);
>> digest = kmalloc(digest_size, GFP_NOIO);
>> if (digest) {
>> drbd_csum_ee(peer_device->connection->verify_tfm, peer_req, digest);
>> --
>> 2.17.1
>>
>>
>> --
>> Kees Cook
>> Pixel Security
>
>
>
> --
> Kees Cook
> Pixel Security
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7] drbd: Convert from ahash to shash
2018-09-05 3:04 ` Kees Cook
@ 2018-09-05 8:33 ` Lars Ellenberg
2018-09-05 16:04 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Lars Ellenberg @ 2018-09-05 8:33 UTC (permalink / raw)
To: Kees Cook; +Cc: Jens Axboe, Philipp Reisner, linux-block, drbd-dev, LKML
On Tue, Sep 04, 2018 at 08:04:18PM -0700, Kees Cook wrote:
> On Mon, Sep 3, 2018 at 11:04 PM, Kees Cook <keescook@chromium.org> wrote:
> > On Mon, Aug 6, 2018 at 4:32 PM, Kees Cook <keescook@chromium.org> wrote:
> >> In preparing to remove all stack VLA usage from the kernel[1], this
> >> removes the discouraged use of AHASH_REQUEST_ON_STACK in favor of
> >> the smaller SHASH_DESC_ON_STACK by converting from ahash-wrapped-shash
> >> to direct shash. By removing a layer of indirection this both improves
> >> performance and reduces stack usage. The stack allocation will be made
> >> a fixed size in a later patch to the crypto subsystem.
> >>
> >> The bulk of the lines in this change are simple s/ahash/shash/, but the
> >> main logic differences are in drbd_csum_ee() and drbd_csum_bio(), which
> >> externalizes the page walking with k(un)map_atomic() instead of using
> >> scattergather.
> >
> > Hi Lars! How does this look to you? If you can Ack I assume Jens would
> > be able to take this.
Sure, I should have ACKed it a month ago already. As I said, I believe
you the crypto. And you added the kmap_atomic as I pointed out.
All good.
> FWIW I've tested a simple drbd configuration before/after this change
> and things seem to be working correctly.
You'd need "data-integrity-alg" set (or "verify-alg", and then have it
do an online-verify) to excercise the crypto stuff,
and you'd need a highmem system (are these still out there?)
to have the kmap not be a no-op. But I don't see any potential problem.
Thanks!
Lars
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7] drbd: Convert from ahash to shash
2018-09-05 8:33 ` Lars Ellenberg
@ 2018-09-05 16:04 ` Kees Cook
2018-09-05 16:10 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2018-09-05 16:04 UTC (permalink / raw)
To: Kees Cook, Jens Axboe, Philipp Reisner, linux-block, drbd-dev, LKML
On Wed, Sep 5, 2018 at 1:33 AM, Lars Ellenberg
<lars.ellenberg@linbit.com> wrote:
> On Tue, Sep 04, 2018 at 08:04:18PM -0700, Kees Cook wrote:
>> On Mon, Sep 3, 2018 at 11:04 PM, Kees Cook <keescook@chromium.org> wrote:
>> > On Mon, Aug 6, 2018 at 4:32 PM, Kees Cook <keescook@chromium.org> wrote:
>> >> In preparing to remove all stack VLA usage from the kernel[1], this
>> >> removes the discouraged use of AHASH_REQUEST_ON_STACK in favor of
>> >> the smaller SHASH_DESC_ON_STACK by converting from ahash-wrapped-shash
>> >> to direct shash. By removing a layer of indirection this both improves
>> >> performance and reduces stack usage. The stack allocation will be made
>> >> a fixed size in a later patch to the crypto subsystem.
>> >>
>> >> The bulk of the lines in this change are simple s/ahash/shash/, but the
>> >> main logic differences are in drbd_csum_ee() and drbd_csum_bio(), which
>> >> externalizes the page walking with k(un)map_atomic() instead of using
>> >> scattergather.
>> >
>> > Hi Lars! How does this look to you? If you can Ack I assume Jens would
>> > be able to take this.
>
> Sure, I should have ACKed it a month ago already. As I said, I believe
> you the crypto. And you added the kmap_atomic as I pointed out.
> All good.
Great, thanks! Jens, can you take this?
>> FWIW I've tested a simple drbd configuration before/after this change
>> and things seem to be working correctly.
>
> You'd need "data-integrity-alg" set (or "verify-alg", and then have it
> do an online-verify) to excercise the crypto stuff,
> and you'd need a highmem system (are these still out there?)
> to have the kmap not be a no-op. But I don't see any potential problem.
While I don't have a highmem system, I've confirmed that
data-integrity-alg and verify-alg are both working for me.
Thanks!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7] drbd: Convert from ahash to shash
2018-09-05 16:04 ` Kees Cook
@ 2018-09-05 16:10 ` Jens Axboe
0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2018-09-05 16:10 UTC (permalink / raw)
To: Kees Cook, Philipp Reisner, linux-block, drbd-dev, LKML
On 9/5/18 10:04 AM, Kees Cook wrote:
> On Wed, Sep 5, 2018 at 1:33 AM, Lars Ellenberg
> <lars.ellenberg@linbit.com> wrote:
>> On Tue, Sep 04, 2018 at 08:04:18PM -0700, Kees Cook wrote:
>>> On Mon, Sep 3, 2018 at 11:04 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> On Mon, Aug 6, 2018 at 4:32 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>> In preparing to remove all stack VLA usage from the kernel[1], this
>>>>> removes the discouraged use of AHASH_REQUEST_ON_STACK in favor of
>>>>> the smaller SHASH_DESC_ON_STACK by converting from ahash-wrapped-shash
>>>>> to direct shash. By removing a layer of indirection this both improves
>>>>> performance and reduces stack usage. The stack allocation will be made
>>>>> a fixed size in a later patch to the crypto subsystem.
>>>>>
>>>>> The bulk of the lines in this change are simple s/ahash/shash/, but the
>>>>> main logic differences are in drbd_csum_ee() and drbd_csum_bio(), which
>>>>> externalizes the page walking with k(un)map_atomic() instead of using
>>>>> scattergather.
>>>>
>>>> Hi Lars! How does this look to you? If you can Ack I assume Jens would
>>>> be able to take this.
>>
>> Sure, I should have ACKed it a month ago already. As I said, I believe
>> you the crypto. And you added the kmap_atomic as I pointed out.
>> All good.
>
> Great, thanks! Jens, can you take this?
Done, queued up for 4.20.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-05 16:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-06 23:32 [PATCH v7] drbd: Convert from ahash to shash Kees Cook
2018-09-04 6:04 ` Kees Cook
2018-09-05 3:04 ` Kees Cook
2018-09-05 8:33 ` Lars Ellenberg
2018-09-05 16:04 ` Kees Cook
2018-09-05 16:10 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).