| Message ID | 20200603101131.2107303-1-efremov@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Use vmemdup_user() | expand |
On 03/06/20 12:11, Denis Efremov wrote: > Replace opencoded alloc and copy with vmemdup_user(). > > Signed-off-by: Denis Efremov <efremov@linux.com> > --- > Looks like these are the only places in KVM that are suitable for > vmemdup_user(). > > arch/x86/kvm/cpuid.c | 17 +++++++---------- > virt/kvm/kvm_main.c | 19 ++++++++----------- > 2 files changed, 15 insertions(+), 21 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 901cd1fdecd9..27438a2bdb62 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, > r = -E2BIG; > if (cpuid->nent > KVM_MAX_CPUID_ENTRIES) > goto out; > - r = -ENOMEM; > if (cpuid->nent) { > - cpuid_entries = > - vmalloc(array_size(sizeof(struct kvm_cpuid_entry), > - cpuid->nent)); > - if (!cpuid_entries) > - goto out; > - r = -EFAULT; > - if (copy_from_user(cpuid_entries, entries, > - cpuid->nent * sizeof(struct kvm_cpuid_entry))) > + cpuid_entries = vmemdup_user(entries, > + array_size(sizeof(struct kvm_cpuid_entry), > + cpuid->nent)); > + if (IS_ERR(cpuid_entries)) { > + r = PTR_ERR(cpuid_entries); > goto out; > + } > } > for (i = 0; i < cpuid->nent; i++) { > vcpu->arch.cpuid_entries[i].function = cpuid_entries[i].function; > @@ -212,8 +209,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, > kvm_x86_ops.cpuid_update(vcpu); > r = kvm_update_cpuid(vcpu); > > + kvfree(cpuid_entries); > out: > - vfree(cpuid_entries); > return r; > } > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 731c1e517716..46a3743e95ff 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -3722,21 +3722,18 @@ static long kvm_vm_ioctl(struct file *filp, > if (routing.flags) > goto out; > if (routing.nr) { > - r = -ENOMEM; > - entries = vmalloc(array_size(sizeof(*entries), > - routing.nr)); > - if (!entries) > - goto out; > - r = -EFAULT; > urouting = argp; > - if (copy_from_user(entries, urouting->entries, > - routing.nr * sizeof(*entries))) > - goto out_free_irq_routing; > + entries = vmemdup_user(urouting->entries, > + array_size(sizeof(*entries), > + routing.nr)); > + if (IS_ERR(entries)) { > + r = PTR_ERR(entries); > + goto out; > + } > } > r = kvm_set_irq_routing(kvm, entries, routing.nr, > routing.flags); > -out_free_irq_routing: > - vfree(entries); > + kvfree(entries); > break; > } > #endif /* CONFIG_HAVE_KVM_IRQ_ROUTING */ > Queued, thanks. Paolo
On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <efremov@linux.com> wrote: > > Replace opencoded alloc and copy with vmemdup_user(). > > Signed-off-by: Denis Efremov <efremov@linux.com> > --- > Looks like these are the only places in KVM that are suitable for > vmemdup_user(). > > arch/x86/kvm/cpuid.c | 17 +++++++---------- > virt/kvm/kvm_main.c | 19 ++++++++----------- > 2 files changed, 15 insertions(+), 21 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 901cd1fdecd9..27438a2bdb62 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, > r = -E2BIG; > if (cpuid->nent > KVM_MAX_CPUID_ENTRIES) > goto out; > - r = -ENOMEM; > if (cpuid->nent) { > - cpuid_entries = > - vmalloc(array_size(sizeof(struct kvm_cpuid_entry), > - cpuid->nent)); > - if (!cpuid_entries) > - goto out; > - r = -EFAULT; > - if (copy_from_user(cpuid_entries, entries, > - cpuid->nent * sizeof(struct kvm_cpuid_entry))) > + cpuid_entries = vmemdup_user(entries, > + array_size(sizeof(struct kvm_cpuid_entry), > + cpuid->nent)); Does this break memcg accounting? I ask, because I'm really not sure.
On Thu 17-06-21 17:25:04, Jim Mattson wrote: > On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <efremov@linux.com> wrote: > > > > Replace opencoded alloc and copy with vmemdup_user(). > > > > Signed-off-by: Denis Efremov <efremov@linux.com> > > --- > > Looks like these are the only places in KVM that are suitable for > > vmemdup_user(). > > > > arch/x86/kvm/cpuid.c | 17 +++++++---------- > > virt/kvm/kvm_main.c | 19 ++++++++----------- > > 2 files changed, 15 insertions(+), 21 deletions(-) > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index 901cd1fdecd9..27438a2bdb62 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, > > r = -E2BIG; > > if (cpuid->nent > KVM_MAX_CPUID_ENTRIES) > > goto out; > > - r = -ENOMEM; > > if (cpuid->nent) { > > - cpuid_entries = > > - vmalloc(array_size(sizeof(struct kvm_cpuid_entry), > > - cpuid->nent)); > > - if (!cpuid_entries) > > - goto out; > > - r = -EFAULT; > > - if (copy_from_user(cpuid_entries, entries, > > - cpuid->nent * sizeof(struct kvm_cpuid_entry))) > > + cpuid_entries = vmemdup_user(entries, > > + array_size(sizeof(struct kvm_cpuid_entry), > > + cpuid->nent)); > > Does this break memcg accounting? I ask, because I'm really not sure. What do you mean by that? The original code uses plain vmalloc so the allocation is not memcg accounted (please note that __GFP_ACCOUNT needs to be specified explicitly). vmemdup_user is the same in that regards.
On Thu, Jun 17, 2021 at 11:00 PM Michal Hocko <mhocko@suse.com> wrote: > > On Thu 17-06-21 17:25:04, Jim Mattson wrote: > > On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <efremov@linux.com> wrote: > > > > > > Replace opencoded alloc and copy with vmemdup_user(). > > > > > > Signed-off-by: Denis Efremov <efremov@linux.com> > > > --- > > > Looks like these are the only places in KVM that are suitable for > > > vmemdup_user(). > > > > > > arch/x86/kvm/cpuid.c | 17 +++++++---------- > > > virt/kvm/kvm_main.c | 19 ++++++++----------- > > > 2 files changed, 15 insertions(+), 21 deletions(-) > > > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > > index 901cd1fdecd9..27438a2bdb62 100644 > > > --- a/arch/x86/kvm/cpuid.c > > > +++ b/arch/x86/kvm/cpuid.c > > > @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, > > > r = -E2BIG; > > > if (cpuid->nent > KVM_MAX_CPUID_ENTRIES) > > > goto out; > > > - r = -ENOMEM; > > > if (cpuid->nent) { > > > - cpuid_entries = > > > - vmalloc(array_size(sizeof(struct kvm_cpuid_entry), > > > - cpuid->nent)); > > > - if (!cpuid_entries) > > > - goto out; > > > - r = -EFAULT; > > > - if (copy_from_user(cpuid_entries, entries, > > > - cpuid->nent * sizeof(struct kvm_cpuid_entry))) > > > + cpuid_entries = vmemdup_user(entries, > > > + array_size(sizeof(struct kvm_cpuid_entry), > > > + cpuid->nent)); > > > > Does this break memcg accounting? I ask, because I'm really not sure. > > What do you mean by that? The original code uses plain vmalloc so the > allocation is not memcg accounted (please note that __GFP_ACCOUNT needs > to be specified explicitly). vmemdup_user is the same in that regards. I asked, because I wasn't sure if plain vmalloc was accounted or not. In any case, these allocations *should* be accounted, shouldn't they?
On Fri 18-06-21 09:53:53, Jim Mattson wrote: > On Thu, Jun 17, 2021 at 11:00 PM Michal Hocko <mhocko@suse.com> wrote: > > > > On Thu 17-06-21 17:25:04, Jim Mattson wrote: > > > On Wed, Jun 3, 2020 at 3:10 AM Denis Efremov <efremov@linux.com> wrote: > > > > > > > > Replace opencoded alloc and copy with vmemdup_user(). > > > > > > > > Signed-off-by: Denis Efremov <efremov@linux.com> > > > > --- > > > > Looks like these are the only places in KVM that are suitable for > > > > vmemdup_user(). > > > > > > > > arch/x86/kvm/cpuid.c | 17 +++++++---------- > > > > virt/kvm/kvm_main.c | 19 ++++++++----------- > > > > 2 files changed, 15 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > > > index 901cd1fdecd9..27438a2bdb62 100644 > > > > --- a/arch/x86/kvm/cpuid.c > > > > +++ b/arch/x86/kvm/cpuid.c > > > > @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, > > > > r = -E2BIG; > > > > if (cpuid->nent > KVM_MAX_CPUID_ENTRIES) > > > > goto out; > > > > - r = -ENOMEM; > > > > if (cpuid->nent) { > > > > - cpuid_entries = > > > > - vmalloc(array_size(sizeof(struct kvm_cpuid_entry), > > > > - cpuid->nent)); > > > > - if (!cpuid_entries) > > > > - goto out; > > > > - r = -EFAULT; > > > > - if (copy_from_user(cpuid_entries, entries, > > > > - cpuid->nent * sizeof(struct kvm_cpuid_entry))) > > > > + cpuid_entries = vmemdup_user(entries, > > > > + array_size(sizeof(struct kvm_cpuid_entry), > > > > + cpuid->nent)); > > > > > > Does this break memcg accounting? I ask, because I'm really not sure. > > > > What do you mean by that? The original code uses plain vmalloc so the > > allocation is not memcg accounted (please note that __GFP_ACCOUNT needs > > to be specified explicitly). vmemdup_user is the same in that regards. > > I asked, because I wasn't sure if plain vmalloc was accounted or not. > > In any case, these allocations *should* be accounted, shouldn't they? This is more of a question to maintainers. Are these objects easy to request by userspace without any bounds?
On 18/06/21 19:04, Michal Hocko wrote: > On Fri 18-06-21 09:53:53, Jim Mattson wrote: >> In any case, these allocations *should* be accounted, shouldn't they? > > This is more of a question to maintainers. Are these objects easy to > request by userspace without any bounds? This particular one need not be accounted because the allocation only lasts for the duration of the ioctl. The allocation below in kvm_vcpu_ioctl_set_cpuid e2 = kvmalloc_array(cpuid->nent, sizeof(*e2), GFP_KERNEL_ACCOUNT); is long term and is already accounted for. kvm_vcpu_ioctl_set_cpuid2 should also use kvmalloc_array and GFP_KERNEL_ACCOUNT. However, it wasn't doing so before this patch went in, either. Paolo
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 901cd1fdecd9..27438a2bdb62 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -182,17 +182,14 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, r = -E2BIG; if (cpuid->nent > KVM_MAX_CPUID_ENTRIES) goto out; - r = -ENOMEM; if (cpuid->nent) { - cpuid_entries = - vmalloc(array_size(sizeof(struct kvm_cpuid_entry), - cpuid->nent)); - if (!cpuid_entries) - goto out; - r = -EFAULT; - if (copy_from_user(cpuid_entries, entries, - cpuid->nent * sizeof(struct kvm_cpuid_entry))) + cpuid_entries = vmemdup_user(entries, + array_size(sizeof(struct kvm_cpuid_entry), + cpuid->nent)); + if (IS_ERR(cpuid_entries)) { + r = PTR_ERR(cpuid_entries); goto out; + } } for (i = 0; i < cpuid->nent; i++) { vcpu->arch.cpuid_entries[i].function = cpuid_entries[i].function; @@ -212,8 +209,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, kvm_x86_ops.cpuid_update(vcpu); r = kvm_update_cpuid(vcpu); + kvfree(cpuid_entries); out: - vfree(cpuid_entries); return r; } diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 731c1e517716..46a3743e95ff 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -3722,21 +3722,18 @@ static long kvm_vm_ioctl(struct file *filp, if (routing.flags) goto out; if (routing.nr) { - r = -ENOMEM; - entries = vmalloc(array_size(sizeof(*entries), - routing.nr)); - if (!entries) - goto out; - r = -EFAULT; urouting = argp; - if (copy_from_user(entries, urouting->entries, - routing.nr * sizeof(*entries))) - goto out_free_irq_routing; + entries = vmemdup_user(urouting->entries, + array_size(sizeof(*entries), + routing.nr)); + if (IS_ERR(entries)) { + r = PTR_ERR(entries); + goto out; + } } r = kvm_set_irq_routing(kvm, entries, routing.nr, routing.flags); -out_free_irq_routing: - vfree(entries); + kvfree(entries); break; } #endif /* CONFIG_HAVE_KVM_IRQ_ROUTING */
Replace opencoded alloc and copy with vmemdup_user(). Signed-off-by: Denis Efremov <efremov@linux.com> --- Looks like these are the only places in KVM that are suitable for vmemdup_user(). arch/x86/kvm/cpuid.c | 17 +++++++---------- virt/kvm/kvm_main.c | 19 ++++++++----------- 2 files changed, 15 insertions(+), 21 deletions(-)