* [PATCH] vhost: zero vhost_vsock memory on allocation
@ 2019-03-27 0:56 Vitaly Mayatskikh
2019-03-27 8:47 ` Michal Hocko
2019-03-27 16:49 ` Stefan Hajnoczi
0 siblings, 2 replies; 8+ messages in thread
From: Vitaly Mayatskikh @ 2019-03-27 0:56 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Michal Hocko, linux-kernel
This fixes OOPS when using under-initialized vhost_vsock object.
The code had a combo of kzalloc plus vmalloc as a fallback
initially, but it has been replaced by plain kvmalloc in
commit 6c5ab6511f71 ("mm: support __GFP_REPEAT in kvmalloc_node for >32kB")
OOPS is easy to reproduce with open/ioctl after trashing the RAM.
Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
drivers/vhost/vsock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index bb5fc0e..9e7cb13 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -512,7 +512,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
/* This struct is large and allocation could fail, fall back to vmalloc
* if there is no other way.
*/
- vsock = kvmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
+ vsock = kvzalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
if (!vsock)
return -ENOMEM;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] vhost: zero vhost_vsock memory on allocation
2019-03-27 0:56 [PATCH] vhost: zero vhost_vsock memory on allocation Vitaly Mayatskikh
@ 2019-03-27 8:47 ` Michal Hocko
2019-03-27 16:49 ` Stefan Hajnoczi
1 sibling, 0 replies; 8+ messages in thread
From: Michal Hocko @ 2019-03-27 8:47 UTC (permalink / raw)
To: Vitaly Mayatskikh; +Cc: Stefan Hajnoczi, linux-kernel
On Tue 26-03-19 20:56:14, Vitaly Mayatskikh wrote:
> This fixes OOPS when using under-initialized vhost_vsock object.
>
> The code had a combo of kzalloc plus vmalloc as a fallback
> initially, but it has been replaced by plain kvmalloc in
> commit 6c5ab6511f71 ("mm: support __GFP_REPEAT in kvmalloc_node for >32kB")
>
> OOPS is easy to reproduce with open/ioctl after trashing the RAM.
Sorry for the screw up, that was certainly not inteded effect of the
patch.
Fixes: 6c5ab6511f71 ("mm: support __GFP_REPEAT in kvmalloc_node for >32kB")
Cc: stable # 4.12+
> Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Thanks for catching that.
> ---
> drivers/vhost/vsock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index bb5fc0e..9e7cb13 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -512,7 +512,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
> /* This struct is large and allocation could fail, fall back to vmalloc
> * if there is no other way.
> */
> - vsock = kvmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> + vsock = kvzalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> if (!vsock)
> return -ENOMEM;
>
> --
> 1.8.3.1
>
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vhost: zero vhost_vsock memory on allocation
2019-03-27 0:56 [PATCH] vhost: zero vhost_vsock memory on allocation Vitaly Mayatskikh
2019-03-27 8:47 ` Michal Hocko
@ 2019-03-27 16:49 ` Stefan Hajnoczi
2019-03-27 17:08 ` Vitaly Mayatskih
1 sibling, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2019-03-27 16:49 UTC (permalink / raw)
To: Vitaly Mayatskikh; +Cc: Michal Hocko, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]
On Tue, Mar 26, 2019 at 08:56:14PM -0400, Vitaly Mayatskikh wrote:
> This fixes OOPS when using under-initialized vhost_vsock object.
>
> The code had a combo of kzalloc plus vmalloc as a fallback
> initially, but it has been replaced by plain kvmalloc in
> commit 6c5ab6511f71 ("mm: support __GFP_REPEAT in kvmalloc_node for >32kB")
>
> OOPS is easy to reproduce with open/ioctl after trashing the RAM.
Which field was accessed before initialization?
I ask because the situation is now unclear since code remains that
assumes vsock is *not* zero-initialized:
vsock->guest_cid = 0; /* no CID assigned yet */
atomic_set(&vsock->queued_replies, 0);
If we're going to zalloc, let's get rid of explicit zero
initializations. Or let's use kvmalloc() and fix the uninitialized
access. Mixing both is confusing.
> Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
> ---
> drivers/vhost/vsock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index bb5fc0e..9e7cb13 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -512,7 +512,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
> /* This struct is large and allocation could fail, fall back to vmalloc
> * if there is no other way.
> */
> - vsock = kvmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> + vsock = kvzalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> if (!vsock)
> return -ENOMEM;
>
> --
> 1.8.3.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vhost: zero vhost_vsock memory on allocation
2019-03-27 16:49 ` Stefan Hajnoczi
@ 2019-03-27 17:08 ` Vitaly Mayatskih
2019-03-28 16:36 ` Stefan Hajnoczi
0 siblings, 1 reply; 8+ messages in thread
From: Vitaly Mayatskih @ 2019-03-27 17:08 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Michal Hocko, linux-kernel
On Wed, Mar 27, 2019 at 12:49 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Which field was accessed before initialization?
>
> I ask because the situation is now unclear since code remains that
> assumes vsock is *not* zero-initialized:
>
> vsock->guest_cid = 0; /* no CID assigned yet */
>
> atomic_set(&vsock->queued_replies, 0);
It was hash.
> If we're going to zalloc, let's get rid of explicit zero
> initializations. Or let's use kvmalloc() and fix the uninitialized
> access. Mixing both is confusing.
I would go with zalloc, since it's easier to read and it prevents
further situations like this.
A zalloc was there originally (not in fallback though).
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vhost: zero vhost_vsock memory on allocation
2019-03-27 17:08 ` Vitaly Mayatskih
@ 2019-03-28 16:36 ` Stefan Hajnoczi
2019-03-28 17:08 ` Vitaly Mayatskih
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2019-03-28 16:36 UTC (permalink / raw)
To: Vitaly Mayatskih; +Cc: Michal Hocko, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]
On Wed, Mar 27, 2019 at 01:08:53PM -0400, Vitaly Mayatskih wrote:
> On Wed, Mar 27, 2019 at 12:49 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > Which field was accessed before initialization?
> >
> > I ask because the situation is now unclear since code remains that
> > assumes vsock is *not* zero-initialized:
> >
> > vsock->guest_cid = 0; /* no CID assigned yet */
> >
> > atomic_set(&vsock->queued_replies, 0);
>
> It was hash.
vsock->hash is only read if vsock->guest_cid has already been set and
hence vsock->hash has been initialized too. I don't see where the
problem is.
Was your tree missing commit a72b69dc083a931422cc8a5e33841aff7d5312f2
("vhost/vsock: fix uninitialized vhost_vsock->guest_cid")?
Are you sure the crash can be reproduced on linux.git/master?
Sorry for insisting on so much information but I want to make I fully
understand the issue you encountered.
> > If we're going to zalloc, let's get rid of explicit zero
> > initializations. Or let's use kvmalloc() and fix the uninitialized
> > access. Mixing both is confusing.
>
> I would go with zalloc, since it's easier to read and it prevents
> further situations like this.
> A zalloc was there originally (not in fallback though).
Sounds good. Please send a v2 that also removes the explicit zero
initialization since it's no longer needed with zalloc.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vhost: zero vhost_vsock memory on allocation
2019-03-28 16:36 ` Stefan Hajnoczi
@ 2019-03-28 17:08 ` Vitaly Mayatskih
2019-03-28 17:10 ` [PATCH] vhost/vsock: initialize vhost_vsock->hash Vitaly Mayatskikh
0 siblings, 1 reply; 8+ messages in thread
From: Vitaly Mayatskih @ 2019-03-28 17:08 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Michal Hocko, linux-kernel
On Thu, Mar 28, 2019 at 12:36 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> vsock->hash is only read if vsock->guest_cid has already been set and
> hence vsock->hash has been initialized too. I don't see where the
> problem is.
>
> Was your tree missing commit a72b69dc083a931422cc8a5e33841aff7d5312f2
> ("vhost/vsock: fix uninitialized vhost_vsock->guest_cid")?
>
> Are you sure the crash can be reproduced on linux.git/master?
You are right: we hit it on 4.14.91 while the guest_cid fix was
backported in 4.14.93.
Thus not a real issue. Sorry for confusion, vhost is something still new to me.
> Sounds good. Please send a v2 that also removes the explicit zero
> initialization since it's no longer needed with zalloc.
I changed my mind, all the rest is initialized explicitly, let's keep
it that way.
Will send the patch shortly.
--
wbr, Vitaly
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] vhost/vsock: initialize vhost_vsock->hash
2019-03-28 17:08 ` Vitaly Mayatskih
@ 2019-03-28 17:10 ` Vitaly Mayatskikh
2019-04-05 10:24 ` Stefan Hajnoczi
0 siblings, 1 reply; 8+ messages in thread
From: Vitaly Mayatskikh @ 2019-03-28 17:10 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: linux-kernel
There's no current valid use case when uninitialized hash can be read
before being written, however let's keep every vhost_vsock field
initialized just for clarity.
Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
drivers/vhost/vsock.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index bb5fc0e..f43f4ed 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -537,6 +537,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
spin_lock_init(&vsock->send_pkt_list_lock);
INIT_LIST_HEAD(&vsock->send_pkt_list);
vhost_work_init(&vsock->send_pkt_work, vhost_transport_send_pkt_work);
+ INIT_HLIST_NODE(&vsock->hash);
return 0;
out:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] vhost/vsock: initialize vhost_vsock->hash
2019-03-28 17:10 ` [PATCH] vhost/vsock: initialize vhost_vsock->hash Vitaly Mayatskikh
@ 2019-04-05 10:24 ` Stefan Hajnoczi
0 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2019-04-05 10:24 UTC (permalink / raw)
To: Vitaly Mayatskikh; +Cc: linux-kernel, Michael S. Tsirkin
[-- Attachment #1: Type: text/plain, Size: 942 bytes --]
On Thu, Mar 28, 2019 at 01:10:57PM -0400, Vitaly Mayatskikh wrote:
> There's no current valid use case when uninitialized hash can be read
> before being written, however let's keep every vhost_vsock field
> initialized just for clarity.
>
> Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
> ---
> drivers/vhost/vsock.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index bb5fc0e..f43f4ed 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -537,6 +537,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
> spin_lock_init(&vsock->send_pkt_list_lock);
> INIT_LIST_HEAD(&vsock->send_pkt_list);
> vhost_work_init(&vsock->send_pkt_work, vhost_transport_send_pkt_work);
> + INIT_HLIST_NODE(&vsock->hash);
> return 0;
>
> out:
> --
> 1.8.3.1
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-05 10:24 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-27 0:56 [PATCH] vhost: zero vhost_vsock memory on allocation Vitaly Mayatskikh
2019-03-27 8:47 ` Michal Hocko
2019-03-27 16:49 ` Stefan Hajnoczi
2019-03-27 17:08 ` Vitaly Mayatskih
2019-03-28 16:36 ` Stefan Hajnoczi
2019-03-28 17:08 ` Vitaly Mayatskih
2019-03-28 17:10 ` [PATCH] vhost/vsock: initialize vhost_vsock->hash Vitaly Mayatskikh
2019-04-05 10:24 ` Stefan Hajnoczi
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).