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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Michal Hocko <[email protected]>
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
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 <[email protected]>
> ---
> 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
>
On Wed, Mar 27, 2019 at 12:49 PM Stefan Hajnoczi <[email protected]> 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).
On Wed, Mar 27, 2019 at 01:08:53PM -0400, Vitaly Mayatskih wrote:
> On Wed, Mar 27, 2019 at 12:49 PM Stefan Hajnoczi <[email protected]> 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.
On Thu, Mar 28, 2019 at 12:36 PM Stefan Hajnoczi <[email protected]> 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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>