[PATCH 1/1] vhost: Protect the virtqueue from being cleared whilst still in use

2022-03-01 Thread Lee Jones 
vhost_vsock_handle_tx_kick() already holds the mutex during its call
to vhost_get_vq_desc().  All we have to do is take the same lock
during virtqueue clean-up and we mitigate the reported issues.

Link: https://syzkaller.appspot.com/bug?extid=279432d30d825e63ba00

Cc: 
Reported-by: syzbot+adc3cb32385586bec...@syzkaller.appspotmail.com
Signed-off-by: Lee Jones 
---
 drivers/vhost/vhost.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 59edb5a1ffe28..bbaff6a5e21b8 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -693,6 +693,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
int i;
 
for (i = 0; i < dev->nvqs; ++i) {
+   mutex_lock(>vqs[i]->mutex);
if (dev->vqs[i]->error_ctx)
eventfd_ctx_put(dev->vqs[i]->error_ctx);
if (dev->vqs[i]->kick)
@@ -700,6 +701,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
if (dev->vqs[i]->call_ctx.ctx)
eventfd_ctx_put(dev->vqs[i]->call_ctx.ctx);
vhost_vq_reset(dev, dev->vqs[i]);
+   mutex_unlock(>vqs[i]->mutex);
}
vhost_dev_free_iovecs(dev);
if (dev->log_ctx)
-- 
2.35.1.574.g5d30c73bfb-goog

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 4/4] virtio-crypto: rename skcipher algs

2022-03-01 Thread zhenwei pi 
Suggested by Gonglei, rename virtio_crypto_algs.c to
virtio_crypto_skcipher_algs.c. Also minor changes for function name.
Thus the function of source files get clear: skcipher services in
virtio_crypto_skcipher_algs.c and akcipher services in
virtio_crypto_akcipher_algs.c.

Signed-off-by: zhenwei pi 
---
 drivers/crypto/virtio/Makefile| 2 +-
 drivers/crypto/virtio/virtio_crypto_common.h  | 4 ++--
 drivers/crypto/virtio/virtio_crypto_mgr.c | 8 
 ...virtio_crypto_algs.c => virtio_crypto_skcipher_algs.c} | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)
 rename drivers/crypto/virtio/{virtio_crypto_algs.c => 
virtio_crypto_skcipher_algs.c} (99%)

diff --git a/drivers/crypto/virtio/Makefile b/drivers/crypto/virtio/Makefile
index f2b839473d61..bfa6cbae342e 100644
--- a/drivers/crypto/virtio/Makefile
+++ b/drivers/crypto/virtio/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio_crypto.o
 virtio_crypto-objs := \
-   virtio_crypto_algs.o \
+   virtio_crypto_skcipher_algs.o \
virtio_crypto_akcipher_algs.o \
virtio_crypto_mgr.o \
virtio_crypto_core.o
diff --git a/drivers/crypto/virtio/virtio_crypto_common.h 
b/drivers/crypto/virtio/virtio_crypto_common.h
index 214f9a6fcf84..e693d4ee83a6 100644
--- a/drivers/crypto/virtio/virtio_crypto_common.h
+++ b/drivers/crypto/virtio/virtio_crypto_common.h
@@ -130,8 +130,8 @@ static inline int virtio_crypto_get_current_node(void)
return node;
 }
 
-int virtio_crypto_algs_register(struct virtio_crypto *vcrypto);
-void virtio_crypto_algs_unregister(struct virtio_crypto *vcrypto);
+int virtio_crypto_skcipher_algs_register(struct virtio_crypto *vcrypto);
+void virtio_crypto_skcipher_algs_unregister(struct virtio_crypto *vcrypto);
 int virtio_crypto_akcipher_algs_register(struct virtio_crypto *vcrypto);
 void virtio_crypto_akcipher_algs_unregister(struct virtio_crypto *vcrypto);
 
diff --git a/drivers/crypto/virtio/virtio_crypto_mgr.c 
b/drivers/crypto/virtio/virtio_crypto_mgr.c
index 1cb92418b321..70e778aac0f2 100644
--- a/drivers/crypto/virtio/virtio_crypto_mgr.c
+++ b/drivers/crypto/virtio/virtio_crypto_mgr.c
@@ -237,14 +237,14 @@ struct virtio_crypto *virtcrypto_get_dev_node(int node, 
uint32_t service,
  */
 int virtcrypto_dev_start(struct virtio_crypto *vcrypto)
 {
-   if (virtio_crypto_algs_register(vcrypto)) {
-   pr_err("virtio_crypto: Failed to register crypto algs\n");
+   if (virtio_crypto_skcipher_algs_register(vcrypto)) {
+   pr_err("virtio_crypto: Failed to register crypto skcipher 
algs\n");
return -EFAULT;
}
 
if (virtio_crypto_akcipher_algs_register(vcrypto)) {
pr_err("virtio_crypto: Failed to register crypto akcipher 
algs\n");
-   virtio_crypto_algs_unregister(vcrypto);
+   virtio_crypto_skcipher_algs_unregister(vcrypto);
return -EFAULT;
}
 
@@ -263,7 +263,7 @@ int virtcrypto_dev_start(struct virtio_crypto *vcrypto)
  */
 void virtcrypto_dev_stop(struct virtio_crypto *vcrypto)
 {
-   virtio_crypto_algs_unregister(vcrypto);
+   virtio_crypto_skcipher_algs_unregister(vcrypto);
virtio_crypto_akcipher_algs_unregister(vcrypto);
 }
 
diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c 
b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
similarity index 99%
rename from drivers/crypto/virtio/virtio_crypto_algs.c
rename to drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
index 583c0b535d13..a618c46a52b8 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
@@ -613,7 +613,7 @@ static struct virtio_crypto_algo virtio_crypto_algs[] = { {
},
 } };
 
-int virtio_crypto_algs_register(struct virtio_crypto *vcrypto)
+int virtio_crypto_skcipher_algs_register(struct virtio_crypto *vcrypto)
 {
int ret = 0;
int i = 0;
@@ -644,7 +644,7 @@ int virtio_crypto_algs_register(struct virtio_crypto 
*vcrypto)
return ret;
 }
 
-void virtio_crypto_algs_unregister(struct virtio_crypto *vcrypto)
+void virtio_crypto_skcipher_algs_unregister(struct virtio_crypto *vcrypto)
 {
int i = 0;
 
-- 
2.20.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 3/4] virtio-crypto: implement RSA algorithm

2022-03-01 Thread zhenwei pi 
Support rsa & pkcs1pad(rsa,sha1) with priority 150.

Test with QEMU built-in backend, it works fine.
1, The self-test framework of crypto layer works fine in guest kernel
2, Test with Linux guest(with asym support), the following script
test(note that pkey_XXX is supported only in a newer version of keyutils):
  - both public key & private key
  - create/close session
  - encrypt/decrypt/sign/verify basic driver operation
  - also test with kernel crypto layer(pkey add/query)

All the cases work fine.

rm -rf *.der *.pem *.pfx
modprobe pkcs8_key_parser # if CONFIG_PKCS8_PRIVATE_KEY_PARSER=m
rm -rf /tmp/data
dd if=/dev/random of=/tmp/data count=1 bs=226

openssl req -nodes -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -subj 
"/C=CN/ST=BJ/L=HD/O=qemu/OU=dev/CN=qemu/emailAddress=q...@qemu.org"
openssl pkcs8 -in key.pem -topk8 -nocrypt -outform DER -out key.der
openssl x509 -in cert.pem -inform PEM -outform DER -out cert.der

PRIV_KEY_ID=`cat key.der | keyctl padd asymmetric test_priv_key @s`
echo "priv key id = "$PRIV_KEY_ID
PUB_KEY_ID=`cat cert.der | keyctl padd asymmetric test_pub_key @s`
echo "pub key id = "$PUB_KEY_ID

keyctl pkey_query $PRIV_KEY_ID 0
keyctl pkey_query $PUB_KEY_ID 0

echo "Enc with priv key..."
keyctl pkey_encrypt $PRIV_KEY_ID 0 /tmp/data enc=pkcs1 >/tmp/enc.priv
echo "Dec with pub key..."
keyctl pkey_decrypt $PRIV_KEY_ID 0 /tmp/enc.priv enc=pkcs1 >/tmp/dec
cmp /tmp/data /tmp/dec

echo "Sign with priv key..."
keyctl pkey_sign $PRIV_KEY_ID 0 /tmp/data enc=pkcs1 hash=sha1 > /tmp/sig
echo "Verify with pub key..."
keyctl pkey_verify $PRIV_KEY_ID 0 /tmp/data /tmp/sig enc=pkcs1 hash=sha1

echo "Enc with pub key..."
keyctl pkey_encrypt $PUB_KEY_ID 0 /tmp/data enc=pkcs1 >/tmp/enc.pub
echo "Dec with priv key..."
keyctl pkey_decrypt $PRIV_KEY_ID 0 /tmp/enc.pub enc=pkcs1 >/tmp/dec
cmp /tmp/data /tmp/dec

echo "Verify with pub key..."
keyctl pkey_verify $PUB_KEY_ID 0 /tmp/data /tmp/sig enc=pkcs1 hash=sha1

[1 compiling warning during development]
Reported-by: kernel test robot 

Co-developed-by: lei he 
Signed-off-by: lei he 
Signed-off-by: zhenwei pi 
---
 drivers/crypto/virtio/Makefile|   1 +
 .../virtio/virtio_crypto_akcipher_algs.c  | 585 ++
 drivers/crypto/virtio/virtio_crypto_common.h  |   3 +
 drivers/crypto/virtio/virtio_crypto_core.c|   6 +-
 drivers/crypto/virtio/virtio_crypto_mgr.c |  11 +
 5 files changed, 605 insertions(+), 1 deletion(-)
 create mode 100644 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c

diff --git a/drivers/crypto/virtio/Makefile b/drivers/crypto/virtio/Makefile
index cbffa135..f2b839473d61 100644
--- a/drivers/crypto/virtio/Makefile
+++ b/drivers/crypto/virtio/Makefile
@@ -2,5 +2,6 @@
 obj-$(CONFIG_CRYPTO_DEV_VIRTIO) += virtio_crypto.o
 virtio_crypto-objs := \
virtio_crypto_algs.o \
+   virtio_crypto_akcipher_algs.o \
virtio_crypto_mgr.o \
virtio_crypto_core.o
diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c 
b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
new file mode 100644
index ..f3ec9420215e
--- /dev/null
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -0,0 +1,585 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+ /* Asymmetric algorithms supported by virtio crypto device
+  *
+  * Authors: zhenwei pi 
+  *  lei he 
+  *
+  * Copyright 2022 Bytedance CO., LTD.
+  */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include "virtio_crypto_common.h"
+
+struct virtio_crypto_rsa_ctx {
+   MPI n;
+};
+
+struct virtio_crypto_akcipher_ctx {
+   struct crypto_engine_ctx enginectx;
+   struct virtio_crypto *vcrypto;
+   struct crypto_akcipher *tfm;
+   bool session_valid;
+   __u64 session_id;
+   union {
+   struct virtio_crypto_rsa_ctx rsa_ctx;
+   };
+};
+
+struct virtio_crypto_akcipher_request {
+   struct virtio_crypto_request base;
+   struct virtio_crypto_akcipher_ctx *akcipher_ctx;
+   struct akcipher_request *akcipher_req;
+   void *src_buf;
+   void *dst_buf;
+   uint32_t opcode;
+};
+
+struct virtio_crypto_akcipher_algo {
+   uint32_t algonum;
+   uint32_t service;
+   unsigned int active_devs;
+   struct akcipher_alg algo;
+};
+
+static DEFINE_MUTEX(algs_lock);
+
+static void virtio_crypto_akcipher_finalize_req(
+   struct virtio_crypto_akcipher_request *vc_akcipher_req,
+   struct akcipher_request *req, int err)
+{
+   virtcrypto_clear_request(_akcipher_req->base);
+
+   crypto_finalize_akcipher_request(vc_akcipher_req->base.dataq->engine, 
req, err);
+}
+
+static void virtio_crypto_dataq_akcipher_callback(struct virtio_crypto_request 
*vc_req, int len)
+{
+   struct virtio_crypto_akcipher_request *vc_akcipher_req =
+   container_of(vc_req, struct virtio_crypto_akcipher_request, 
base);
+   struct akcipher_request *akcipher_req;
+   int

[PATCH v3 2/4] virtio-crypto: introduce akcipher service

2022-03-01 Thread zhenwei pi 
Introduce asymmetric service definition, asymmetric operations and
several well known algorithms.

Co-developed-by: lei he 
Signed-off-by: lei he 
Signed-off-by: zhenwei pi 
---
 include/uapi/linux/virtio_crypto.h | 81 +-
 1 file changed, 80 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_crypto.h 
b/include/uapi/linux/virtio_crypto.h
index 1166a49084b0..71a54a6849ca 100644
--- a/include/uapi/linux/virtio_crypto.h
+++ b/include/uapi/linux/virtio_crypto.h
@@ -37,6 +37,7 @@
 #define VIRTIO_CRYPTO_SERVICE_HASH   1
 #define VIRTIO_CRYPTO_SERVICE_MAC2
 #define VIRTIO_CRYPTO_SERVICE_AEAD   3
+#define VIRTIO_CRYPTO_SERVICE_AKCIPHER 4
 
 #define VIRTIO_CRYPTO_OPCODE(service, op)   (((service) << 8) | (op))
 
@@ -57,6 +58,10 @@ struct virtio_crypto_ctrl_header {
   VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
 #define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
   VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
+#define VIRTIO_CRYPTO_AKCIPHER_CREATE_SESSION \
+  VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x04)
+#define VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION \
+  VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x05)
__le32 opcode;
__le32 algo;
__le32 flag;
@@ -180,6 +185,58 @@ struct virtio_crypto_aead_create_session_req {
__u8 padding[32];
 };
 
+struct virtio_crypto_rsa_session_para {
+#define VIRTIO_CRYPTO_RSA_RAW_PADDING   0
+#define VIRTIO_CRYPTO_RSA_PKCS1_PADDING 1
+   __le32 padding_algo;
+
+#define VIRTIO_CRYPTO_RSA_NO_HASH   0
+#define VIRTIO_CRYPTO_RSA_MD2   1
+#define VIRTIO_CRYPTO_RSA_MD3   2
+#define VIRTIO_CRYPTO_RSA_MD4   3
+#define VIRTIO_CRYPTO_RSA_MD5   4
+#define VIRTIO_CRYPTO_RSA_SHA1  5
+#define VIRTIO_CRYPTO_RSA_SHA2566
+#define VIRTIO_CRYPTO_RSA_SHA3847
+#define VIRTIO_CRYPTO_RSA_SHA5128
+#define VIRTIO_CRYPTO_RSA_SHA2249
+   __le32 hash_algo;
+};
+
+struct virtio_crypto_ecdsa_session_para {
+#define VIRTIO_CRYPTO_CURVE_UNKNOWN   0
+#define VIRTIO_CRYPTO_CURVE_NIST_P192 1
+#define VIRTIO_CRYPTO_CURVE_NIST_P224 2
+#define VIRTIO_CRYPTO_CURVE_NIST_P256 3
+#define VIRTIO_CRYPTO_CURVE_NIST_P384 4
+#define VIRTIO_CRYPTO_CURVE_NIST_P521 5
+   __le32 curve_id;
+   __le32 padding;
+};
+
+struct virtio_crypto_akcipher_session_para {
+#define VIRTIO_CRYPTO_NO_AKCIPHER0
+#define VIRTIO_CRYPTO_AKCIPHER_RSA   1
+#define VIRTIO_CRYPTO_AKCIPHER_DSA   2
+#define VIRTIO_CRYPTO_AKCIPHER_ECDSA 3
+   __le32 algo;
+
+#define VIRTIO_CRYPTO_AKCIPHER_KEY_TYPE_PUBLIC  1
+#define VIRTIO_CRYPTO_AKCIPHER_KEY_TYPE_PRIVATE 2
+   __le32 keytype;
+   __le32 keylen;
+
+   union {
+   struct virtio_crypto_rsa_session_para rsa;
+   struct virtio_crypto_ecdsa_session_para ecdsa;
+   } u;
+};
+
+struct virtio_crypto_akcipher_create_session_req {
+   struct virtio_crypto_akcipher_session_para para;
+   __u8 padding[36];
+};
+
 struct virtio_crypto_alg_chain_session_para {
 #define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER  1
 #define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH  2
@@ -247,6 +304,8 @@ struct virtio_crypto_op_ctrl_req {
mac_create_session;
struct virtio_crypto_aead_create_session_req
aead_create_session;
+   struct virtio_crypto_akcipher_create_session_req
+   akcipher_create_session;
struct virtio_crypto_destroy_session_req
destroy_session;
__u8 padding[56];
@@ -266,6 +325,14 @@ struct virtio_crypto_op_header {
VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
 #define VIRTIO_CRYPTO_AEAD_DECRYPT \
VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
+#define VIRTIO_CRYPTO_AKCIPHER_ENCRYPT \
+   VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x00)
+#define VIRTIO_CRYPTO_AKCIPHER_DECRYPT \
+   VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x01)
+#define VIRTIO_CRYPTO_AKCIPHER_SIGN \
+   VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x02)
+#define VIRTIO_CRYPTO_AKCIPHER_VERIFY \
+   VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AKCIPHER, 0x03)
__le32 opcode;
/* algo should be service-specific algorithms */
__le32 algo;
@@ -390,6 +457,16 @@ struct virtio_crypto_aead_data_req {
__u8 padding[32];
 };
 
+struct virtio_crypto_akcipher_para {
+   __le32 src_data_len;
+   __le32 dst_data_len;
+};
+
+struct virtio_crypto_akcipher_data_req {
+   struct virtio_crypto_akcipher_para para;
+   __u8 padding[40];
+};
+
 /* The request of the data virtqueue's packet */
 struct virtio_crypto_op_data_req {
struct virtio_crypto_op_header header;
@@ -399,6 +476,7 @@ struct virtio_crypto_op_data_req {
struct virtio_crypto_hash_data_req hash_req;
struct virtio_crypto_mac_data_req mac_req;

[PATCH v3 1/4] virtio_crypto: Introduce VIRTIO_CRYPTO_NOSPC

2022-03-01 Thread zhenwei pi 
Base on the lastest virtio crypto spec, define VIRTIO_CRYPTO_NOSPC.

Reviewed-by: Gonglei 
Signed-off-by: zhenwei pi 
---
 include/uapi/linux/virtio_crypto.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/virtio_crypto.h 
b/include/uapi/linux/virtio_crypto.h
index a03932f10565..1166a49084b0 100644
--- a/include/uapi/linux/virtio_crypto.h
+++ b/include/uapi/linux/virtio_crypto.h
@@ -408,6 +408,7 @@ struct virtio_crypto_op_data_req {
 #define VIRTIO_CRYPTO_BADMSG2
 #define VIRTIO_CRYPTO_NOTSUPP   3
 #define VIRTIO_CRYPTO_INVSESS   4 /* Invalid session id */
+#define VIRTIO_CRYPTO_NOSPC 5 /* no free session ID */
 
 /* The accelerator hardware is ready */
 #define VIRTIO_CRYPTO_S_HW_READY  (1 << 0)
-- 
2.20.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 0/4] Introduce akcipher service for virtio-crypto

2022-03-01 Thread zhenwei pi 
v2 -> v3:
  Rename virtio_crypto_algs.c to virtio_crypto_skcipher_algs.c, and
minor changes of function name.
  Minor changes in virtio_crypto_akcipher_algs.c: no need to copy from
buffer if opcode is verify.

v1 -> v2:
  Fix 1 compiling warning reported by kernel test robot 
  Put "__le32 akcipher_algo;" instead of "__le32 reserve;" field of
struct virtio_crypto_config directly without size change.
  Add padding in struct virtio_crypto_ecdsa_session_para to keep
64-bit alignment.
  Remove irrelevant change by code format alignment.

  Also CC crypto gurus Herbert and linux-cry...@vger.kernel.org.

  Test with QEMU(patched by the v2 version), works fine.

v1:
  Introduce akcipher service, implement RSA algorithm, and a minor fix.

zhenwei pi (4):
  virtio_crypto: Introduce VIRTIO_CRYPTO_NOSPC
  virtio-crypto: introduce akcipher service
  virtio-crypto: implement RSA algorithm
  virtio-crypto: rename skcipher algs

 drivers/crypto/virtio/Makefile|   3 +-
 .../virtio/virtio_crypto_akcipher_algs.c  | 585 ++
 drivers/crypto/virtio/virtio_crypto_common.h  |   7 +-
 drivers/crypto/virtio/virtio_crypto_core.c|   6 +-
 drivers/crypto/virtio/virtio_crypto_mgr.c |  15 +-
 ...o_algs.c => virtio_crypto_skcipher_algs.c} |   4 +-
 include/uapi/linux/virtio_crypto.h|  82 ++-
 7 files changed, 693 insertions(+), 9 deletions(-)
 create mode 100644 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
 rename drivers/crypto/virtio/{virtio_crypto_algs.c => 
virtio_crypto_skcipher_algs.c} (99%)

-- 
2.20.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: Assign discard_granularity

2022-03-01 Thread Martin K. Petersen 


Akihiko,

> I'd like to leave this patch as is since I cannot deny the possibility
> that the host has a different alignment offset for discarding and
> other operations.

That's correct.

SCSI explicitly reports separate values for physical block alignment and
"discard" alignment. The queue limits reflect this distinction.

While it is not super common for these two to be different, it does
happen. There are several disk arrays that do not have an internal
allocation unit (discard granularity) which is a power of two, for
instance.

I am not particularly happy that we have to deal with two distinct types
of alignment in the stack but that is the reality of the hardware we
have to support.

-- 
Martin K. Petersen  Oracle Linux Engineering
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq()

2022-03-01 Thread Michael S. Tsirkin 
On Mon, Feb 28, 2022 at 02:57:20PM +0800, Xie Yongji wrote:
> Currently we have a BUG_ON() to make sure the number of sg
> list does not exceed queue_max_segments() in virtio_queue_rq().
> However, the block layer uses queue_max_discard_segments()
> instead of queue_max_segments() to limit the sg list for
> discard requests. So the BUG_ON() might be triggered if
> virtio-blk device reports a larger value for max discard
> segment than queue_max_segments().

Hmm the spec does not say what should happen if max_discard_seg
exceeds seg_max. Is this the config you have in mind? how do you
create it?

> To fix it, let's simply
> remove the BUG_ON() which has become unnecessary after commit
> 02746e26c39e("virtio-blk: avoid preallocating big SGL for data").
> And the unused vblk->sg_elems can also be removed together.
> 
> Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
> Suggested-by: Christoph Hellwig 
> Signed-off-by: Xie Yongji 
> ---
>  drivers/block/virtio_blk.c | 10 +-
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index c443cd64fc9b..a43eb1813cec 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -76,9 +76,6 @@ struct virtio_blk {
>*/
>   refcount_t refs;
>  
> - /* What host tells us, plus 2 for header & tailer. */
> - unsigned int sg_elems;
> -
>   /* Ida index - used to track minor number allocations. */
>   int index;
>  
> @@ -322,8 +319,6 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx 
> *hctx,
>   blk_status_t status;
>   int err;
>  
> - BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems);
> -
>   status = virtblk_setup_cmd(vblk->vdev, req, vbr);
>   if (unlikely(status))
>   return status;
> @@ -783,8 +778,6 @@ static int virtblk_probe(struct virtio_device *vdev)
>   /* Prevent integer overflows and honor max vq size */
>   sg_elems = min_t(u32, sg_elems, VIRTIO_BLK_MAX_SG_ELEMS - 2);
>  
> - /* We need extra sg elements at head and tail. */
> - sg_elems += 2;
>   vdev->priv = vblk = kmalloc(sizeof(*vblk), GFP_KERNEL);
>   if (!vblk) {
>   err = -ENOMEM;
> @@ -796,7 +789,6 @@ static int virtblk_probe(struct virtio_device *vdev)
>   mutex_init(>vdev_mutex);
>  
>   vblk->vdev = vdev;
> - vblk->sg_elems = sg_elems;
>  
>   INIT_WORK(>config_work, virtblk_config_changed_work);
>  
> @@ -853,7 +845,7 @@ static int virtblk_probe(struct virtio_device *vdev)
>   set_disk_ro(vblk->disk, 1);
>  
>   /* We can handle whatever the host told us to handle. */
> - blk_queue_max_segments(q, vblk->sg_elems-2);
> + blk_queue_max_segments(q, sg_elems);
>  
>   /* No real sector limit. */
>   blk_queue_max_hw_sectors(q, -1U);
> -- 
> 2.20.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] virtio-blk: Remove BUG_ON() in virtio_queue_rq()

2022-03-01 Thread Christoph Hellwig 
Looks good,

Reviewed-by: Christoph Hellwig 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: PING: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm

2022-03-01 Thread Gonglei (Arei) via Virtualization 


> -Original Message-
> From: zhenwei pi [mailto:pizhen...@bytedance.com]
> Sent: Tuesday, March 1, 2022 6:26 PM
> To: Gonglei (Arei) 
> Cc: jasow...@redhat.com; m...@redhat.com;
> virtualization@lists.linux-foundation.org; linux-cry...@vger.kernel.org;
> linux-ker...@vger.kernel.org; helei.si...@bytedance.com;
> herb...@gondor.apana.org.au; kernel test robot 
> Subject: PING: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm
> 
> PING!
> 
> Hi, Lei
> I also take a look at other crypto drivers qat/ccp/hisilicon, they separate
> akcipher/skcipher algo. If you consider that reusing
> virtio_crypto_algs_register/unregister seems better, I will try to merge them
> into a single function.
> 

I'm fine with separating them in different c files. Then should we rename 
virtio_crypto_algs.c
to virtio_crypto_skcipher_algo.c?


Regards,
-Gonglei

> On 2/23/22 6:17 PM, zhenwei pi wrote:
> >
> > On 2/18/22 11:12 AM, zhenwei pi wrote:
>  +void virtio_crypto_akcipher_algs_unregister(struct virtio_crypto
>  +*vcrypto) {
>  +    int i = 0;
>  +
>  +    mutex_lock(_lock);
>  +
>  +    for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs); i++)
>  +{
>  +    uint32_t service = virtio_crypto_akcipher_algs[i].service;
>  +    uint32_t algonum = virtio_crypto_akcipher_algs[i].algonum;
>  +
>  +    if (virtio_crypto_akcipher_algs[i].active_devs == 0 ||
>  +    !virtcrypto_algo_is_supported(vcrypto, service,
>  +algonum))
>  +    continue;
>  +
>  +    if (virtio_crypto_akcipher_algs[i].active_devs == 1)
>  +
> 
>  crypto_unregister_akcipher(_crypto_akcipher_algs[i].algo);
>  +
>  +    virtio_crypto_akcipher_algs[i].active_devs--;
>  +    }
>  +
>  +    mutex_unlock(_lock);
>  +}
> >>>
> >>> Why don't you reuse the virtio_crypto_algs_register/unregister
> >>> functions?
> >>> The current code is too repetitive. Maybe we don't need create the
> >>> new file virtio_crypto_akcipher_algo.c because we had
> >>> virtio_crypto_algs.c which includes all algorithms.
> >>>
> >>
> >> Yes, this looks similar to virtio_crypto_algs_register/unregister.
> >>
> >> Let's look at the difference:
> >> struct virtio_crypto_akcipher_algo {
> >>  uint32_t algonum;
> >>  uint32_t service;
> >>  unsigned int active_devs;
> >>  struct akcipher_alg algo;
> >> };
> >>
> >> struct virtio_crypto_algo {
> >>  uint32_t algonum;
> >>  uint32_t service;
> >>  unsigned int active_devs;
> >>  struct skcipher_alg algo; /* akcipher_alg VS skcipher_alg */
> >> };
> >>
> >> If reusing virtio_crypto_algs_register/unregister, we need to modify
> >> the data structure like this:
> >> struct virtio_crypto_akcipher_algo {
> >>  uint32_t algonum;
> >>  uint32_t service;    /* use service to distinguish
> >> akcipher/skcipher */
> >>  unsigned int active_devs;
> >>  union {
> >>  struct skcipher_alg skcipher;
> >>  struct akcipher_alg akcipher;
> >>  } alg;
> >> };
> >>
> >> int virtio_crypto_akcipher_algs_register(struct virtio_crypto
> >> *vcrypto) {
> >>  ...
> >>  for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs);
> >> i++) {
> >>  uint32_t service =
> >> virtio_crypto_akcipher_algs[i].service;
> >>  ...
> >>  /* test service type then call
> >> crypto_register_akcipher/crypto_register_skcipher */
> >>  if (service == VIRTIO_CRYPTO_SERVICE_AKCIPHER)
> >> crypto_register_akcipher(_crypto_akcipher_algs[i].algo.akciphe
> >> r);
> >>  else
> >> crypto_register_skcipher(_crypto_skcipher_algs[i].algo.skciphe
> >> r);
> >>  ...
> >>  }
> >>  ...
> >> }
> >>
> >> Also test service type and call
> >> crypto_unregister_skcipher/crypto_unregister_akcipher.
> >>
> >> This gets unclear from current v2 version.
> >>
> >> On the other hand, the kernel side prefers to separate skcipher and
> >> akcipher(separated header files and implementations).
> >>
> >
> 
> --
> zhenwei pi
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

PING: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm

2022-03-01 Thread zhenwei pi 

PING!

Hi, Lei
I also take a look at other crypto drivers qat/ccp/hisilicon, they
separate akcipher/skcipher algo. If you consider that reusing
virtio_crypto_algs_register/unregister seems better, I will try to merge
them into a single function.

On 2/23/22 6:17 PM, zhenwei pi wrote:


On 2/18/22 11:12 AM, zhenwei pi wrote:

+void virtio_crypto_akcipher_algs_unregister(struct virtio_crypto
+*vcrypto) {
+    int i = 0;
+
+    mutex_lock(_lock);
+
+    for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs); i++) {
+    uint32_t service = virtio_crypto_akcipher_algs[i].service;
+    uint32_t algonum = virtio_crypto_akcipher_algs[i].algonum;
+
+    if (virtio_crypto_akcipher_algs[i].active_devs == 0 ||
+    !virtcrypto_algo_is_supported(vcrypto, service, algonum))
+    continue;
+
+    if (virtio_crypto_akcipher_algs[i].active_devs == 1)
+
crypto_unregister_akcipher(_crypto_akcipher_algs[i].algo);
+
+    virtio_crypto_akcipher_algs[i].active_devs--;
+    }
+
+    mutex_unlock(_lock);
+}


Why don't you reuse the virtio_crypto_algs_register/unregister 
functions?
The current code is too repetitive. Maybe we don't need create the 
new file virtio_crypto_akcipher_algo.c

because we had virtio_crypto_algs.c which includes all algorithms.



Yes, this looks similar to virtio_crypto_algs_register/unregister.

Let's look at the difference:
struct virtio_crypto_akcipher_algo {
 uint32_t algonum;
 uint32_t service;
 unsigned int active_devs;
 struct akcipher_alg algo;
};

struct virtio_crypto_algo {
 uint32_t algonum;
 uint32_t service;
 unsigned int active_devs;
 struct skcipher_alg algo; /* akcipher_alg VS skcipher_alg */
};

If reusing virtio_crypto_algs_register/unregister, we need to modify 
the data structure like this:

struct virtio_crypto_akcipher_algo {
 uint32_t algonum;
 uint32_t service;    /* use service to distinguish 
akcipher/skcipher */

 unsigned int active_devs;
 union {
 struct skcipher_alg skcipher;
 struct akcipher_alg akcipher;
 } alg;
};

int virtio_crypto_akcipher_algs_register(struct virtio_crypto *vcrypto)
{
 ...
 for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs); i++) {
 uint32_t service = 
virtio_crypto_akcipher_algs[i].service;

 ...
 /* test service type then call 
crypto_register_akcipher/crypto_register_skcipher */

 if (service == VIRTIO_CRYPTO_SERVICE_AKCIPHER)
crypto_register_akcipher(_crypto_akcipher_algs[i].algo.akcipher);
 else
crypto_register_skcipher(_crypto_skcipher_algs[i].algo.skcipher);
 ...
 }
 ...
}

Also test service type and call 
crypto_unregister_skcipher/crypto_unregister_akcipher.


This gets unclear from current v2 version.

On the other hand, the kernel side prefers to separate skcipher and 
akcipher(separated header files and implementations).






--
zhenwei pi
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: Assign discard_granularity

2022-03-01 Thread Stefan Hajnoczi 
On Tue, Mar 01, 2022 at 02:43:55PM +0900, Akihiko Odaki wrote:
> On 2022/02/28 19:51, Stefan Hajnoczi wrote:
> > On Thu, Feb 24, 2022 at 06:38:02PM +0900, Akihiko Odaki wrote:
> > > Virtual I/O Device (VIRTIO) Version 1.1
> > > https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
> > > > discard_sector_alignment can be used by OS when splitting a request
> > > > based on alignment.
> > > 
> > > According to Documentation/ABI/stable/sysfs-block, the corresponding
> > > field in the kernel is, confusingly, discard_granularity, not
> > > discard_alignment.
> > 
> > Good catch, struct virtio_blk_config->discard_sector_alignment is Linux
> > q->limits.discard_granularity.
> > 
> > > 
> > > Signed-off-by: Akihiko Odaki 
> > > ---
> > >   drivers/block/virtio_blk.c | 4 +---
> > >   1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > index c443cd64fc9b..1fb3c89900e3 100644
> > > --- a/drivers/block/virtio_blk.c
> > > +++ b/drivers/block/virtio_blk.c
> > > @@ -913,11 +913,9 @@ static int virtblk_probe(struct virtio_device *vdev)
> > >   blk_queue_io_opt(q, blk_size * opt_io_size);
> > >   if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
> > > - q->limits.discard_granularity = blk_size;
> > > -
> > >   virtio_cread(vdev, struct virtio_blk_config,
> > >discard_sector_alignment, );
> > > - q->limits.discard_alignment = v ? v << SECTOR_SHIFT : 0;
> > 
> > Should we use struct virtio_blk_config->topology.alignment_offset
> > ("offset of first aligned logical block" and used for Linux
> > blk_queue_alignment_offset()) for q->limits.discard_alignment?
> 
> Maybe but I'm not sure. I had looked at the code of QEMU
> (commit 5c1ee569660d4a205dced9cb4d0306b907fb7599) but it apparently always
> sets 0 for virtio_blk_config->topology.alignment_offset.
> I don't have a hardware which requires discard_alignment either so I cannot
> test it.
> 
> I'd like to leave this patch as is since I cannot deny the possibility that
> the host has a different alignment offset for discarding and other
> operations.

Fair enough. To do it properly we'd need to add a new configuration
space field to virtio-blk.

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] vdpa: fix use-after-free on vp_vdpa_remove

2022-03-01 Thread Stefano Garzarella 
On Tue, Mar 1, 2022 at 2:26 AM Yi Wang  wrote:
>
> From: Zhang Min 
>
> When vp_vdpa driver is unbind, vp_vdpa is freed in vdpa_unregister_device
> and then vp_vdpa->mdev.pci_dev is dereferenced in vp_modern_remove,
> triggering use-after-free.
>
> Call Trace of unbinding driver free vp_vdpa :
> do_syscall_64
>   vfs_write
> kernfs_fop_write_iter
>   device_release_driver_internal
> pci_device_remove
>   vp_vdpa_remove
> vdpa_unregister_device
>   kobject_release
> device_release
>   kfree
>
> Call Trace of dereference vp_vdpa->mdev.pci_dev:
> vp_modern_remove
>   pci_release_selected_regions
> pci_release_region
>   pci_resource_len
> pci_resource_end
>   (dev)->resource[(bar)].end

We can add the fixes tag:

Fixes: 64b9f64f80a6 ("vdpa: introduce virtio pci driver")

>
> Signed-off-by: Zhang Min 
> Signed-off-by: Yi Wang 
> ---
>  drivers/vdpa/virtio_pci/vp_vdpa.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c 
> b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index a57e381e830b..cce101e6a940 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -533,8 +533,8 @@ static void vp_vdpa_remove(struct pci_dev *pdev)
>  {
> struct vp_vdpa *vp_vdpa = pci_get_drvdata(pdev);
>
> -   vdpa_unregister_device(_vdpa->vdpa);
> vp_modern_remove(_vdpa->mdev);
> +   vdpa_unregister_device(_vdpa->vdpa);
>  }
>
>  static struct pci_driver vp_vdpa_driver = {
> --
> 2.27.0
>

The patch LGTM:

Reviewed-by: Stefano Garzarella 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization