drbd: Convert from ahash to shash

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 <[email protected]>

---
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(-)

drivers/block/drbd/drbd_int.h +7 -6

index bc4ed2ed40a2..97d8e290c2be 100644
@@ -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);

drivers/block/drbd/drbd_main.c +7 -7

index a80809bd3057..ccb54791d39c 100644
@@ -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);
 

drivers/block/drbd/drbd_nl.c +12 -27

index b4f02768ba47..d15703b1ffe8 100644
@@ -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.  */

drivers/block/drbd/drbd_receiver.c +18 -17

index be9450f5ad1c..76243e9ef277 100644
@@ -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;
 }

drivers/block/drbd/drbd_worker.c +32 -32

index 5e793dd7adfb..0b41fb0181a6 100644
@@ -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

Quoted 7 lines · Kees Cook

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.

Quoted 4 lines

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

Quoted 457 lines

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

Signed-off-by Kees Cook <[email protected]>

---
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(-)

drivers/block/drbd/drbd_int.h +7 -6

index bc4ed2ed40a2..97d8e290c2be 100644
@@ -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);

drivers/block/drbd/drbd_main.c +7 -7

index a80809bd3057..ccb54791d39c 100644
@@ -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);
 

drivers/block/drbd/drbd_nl.c +12 -27

index b4f02768ba47..d15703b1ffe8 100644
@@ -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.  */

drivers/block/drbd/drbd_receiver.c +18 -17

index be9450f5ad1c..76243e9ef277 100644
@@ -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;
 }

drivers/block/drbd/drbd_worker.c +32 -32

index 5e793dd7adfb..0b41fb0181a6 100644
@@ -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

Quoted 3 lines

--
Kees Cook
Pixel Security

--
Kees Cook
Pixel Security

Quoted 8 lines · Kees Cook

Quoted 7 lines · Kees Cook

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 14 lines · Kees Cook

Quoted 13 lines · Kees Cook

Quoted 11 lines · Kees Cook

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.

Quoted 2 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 15 lines · Lars Ellenberg

Quoted 14 lines · Kees Cook

Quoted 13 lines · Kees Cook

Quoted 11 lines · Kees Cook

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.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

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?