| Message ID | 20171013105259.9728-1-christoffer.dall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 13, 2017 at 12:00:50PM +0100, Marc Zyngier wrote: > On 13/10/17 11:52, Christoffer Dall wrote: > > We currently allocate an entry dynamically, but we never check if the > > allocation actually succeeded. We actually don't need a dynamic > > allocation, because we know the maximum size of an ITS table entry, so > > we can simply use an allocation on the stack. > > > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > virt/kvm/arm/vgic/vgic-its.c | 19 ++++++++----------- > > 1 file changed, 8 insertions(+), 11 deletions(-) > > > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > > index f51c1e1..555f42f 100644 > > --- a/virt/kvm/arm/vgic/vgic-its.c > > +++ b/virt/kvm/arm/vgic/vgic-its.c > > @@ -176,6 +176,7 @@ static const struct vgic_its_abi its_table_abi_versions[] = { > > }; > > > > #define NR_ITS_ABIS ARRAY_SIZE(its_table_abi_versions) > > +#define MAX_ENTRY_SIZE 8 /* Max Entry size across all ABI versions */ > > > > inline const struct vgic_its_abi *vgic_its_get_abi(struct vgic_its *its) > > { > > @@ -1801,37 +1802,33 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry, > > static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz, > > int start_id, entry_fn_t fn, void *opaque) > > { > > - void *entry = kzalloc(esz, GFP_KERNEL); > > struct kvm *kvm = its->dev->kvm; > > unsigned long len = size; > > int id = start_id; > > gpa_t gpa = base; > > + char entry[MAX_ENTRY_SIZE]; > > Nit: you can drop the memset below if you initialize immediately, and > GCC supporting dynamic arrays allows you to write this: > > char entry[esz] = { 0, }; > > so that you don't have to add the MAX_ENTRY_SIZE. > Using a dynamic sized array is a great idea, but trying to initalize it at the same time gives me: error: variable-sized object may not be initialized Is there a trick I'm unfamiliar with? > > int ret; > > > > + memset(entry, 0, MAX_ENTRY_SIZE); > > + > > while (len > 0) { > > int next_offset; > > size_t byte_offset; > > > > ret = kvm_read_guest(kvm, gpa, entry, esz); > > if (ret) > > - goto out; > > + return ret; > > > > next_offset = fn(its, id, entry, opaque); > > - if (next_offset <= 0) { > > - ret = next_offset; > > - goto out; > > - } > > + if (next_offset <= 0) > > + return next_offset; > > > > byte_offset = next_offset * esz; > > id += next_offset; > > gpa += byte_offset; > > len -= byte_offset; > > } > > - ret = 1; > > - > > -out: > > - kfree(entry); > > - return ret; > > + return 1; > > } > > > > /** > > > > Otherwise: > > Acked-by: Marc Zyngier <marc.zyngier@arm.com> > Thanks! -Christoffer
On 13/10/17 12:57, Christoffer Dall wrote: > On Fri, Oct 13, 2017 at 12:00:50PM +0100, Marc Zyngier wrote: >> On 13/10/17 11:52, Christoffer Dall wrote: >>> We currently allocate an entry dynamically, but we never check if the >>> allocation actually succeeded. We actually don't need a dynamic >>> allocation, because we know the maximum size of an ITS table entry, so >>> we can simply use an allocation on the stack. >>> >>> Cc: <stable@vger.kernel.org> >>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> >>> --- >>> virt/kvm/arm/vgic/vgic-its.c | 19 ++++++++----------- >>> 1 file changed, 8 insertions(+), 11 deletions(-) >>> >>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >>> index f51c1e1..555f42f 100644 >>> --- a/virt/kvm/arm/vgic/vgic-its.c >>> +++ b/virt/kvm/arm/vgic/vgic-its.c >>> @@ -176,6 +176,7 @@ static const struct vgic_its_abi its_table_abi_versions[] = { >>> }; >>> >>> #define NR_ITS_ABIS ARRAY_SIZE(its_table_abi_versions) >>> +#define MAX_ENTRY_SIZE 8 /* Max Entry size across all ABI versions */ >>> >>> inline const struct vgic_its_abi *vgic_its_get_abi(struct vgic_its *its) >>> { >>> @@ -1801,37 +1802,33 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry, >>> static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz, >>> int start_id, entry_fn_t fn, void *opaque) >>> { >>> - void *entry = kzalloc(esz, GFP_KERNEL); >>> struct kvm *kvm = its->dev->kvm; >>> unsigned long len = size; >>> int id = start_id; >>> gpa_t gpa = base; >>> + char entry[MAX_ENTRY_SIZE]; >> >> Nit: you can drop the memset below if you initialize immediately, and >> GCC supporting dynamic arrays allows you to write this: >> >> char entry[esz] = { 0, }; >> >> so that you don't have to add the MAX_ENTRY_SIZE. >> > > Using a dynamic sized array is a great idea, but trying to initalize it > at the same time gives me: > > error: variable-sized object may not be initialized > > Is there a trick I'm unfamiliar with? No, it is just that these two things are mutually incompatible, and I didn't realize it. You have to choose one or the other... :-( I suggest you keep the memset... Thanks, M.
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index f51c1e1..555f42f 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -176,6 +176,7 @@ static const struct vgic_its_abi its_table_abi_versions[] = { }; #define NR_ITS_ABIS ARRAY_SIZE(its_table_abi_versions) +#define MAX_ENTRY_SIZE 8 /* Max Entry size across all ABI versions */ inline const struct vgic_its_abi *vgic_its_get_abi(struct vgic_its *its) { @@ -1801,37 +1802,33 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry, static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz, int start_id, entry_fn_t fn, void *opaque) { - void *entry = kzalloc(esz, GFP_KERNEL); struct kvm *kvm = its->dev->kvm; unsigned long len = size; int id = start_id; gpa_t gpa = base; + char entry[MAX_ENTRY_SIZE]; int ret; + memset(entry, 0, MAX_ENTRY_SIZE); + while (len > 0) { int next_offset; size_t byte_offset; ret = kvm_read_guest(kvm, gpa, entry, esz); if (ret) - goto out; + return ret; next_offset = fn(its, id, entry, opaque); - if (next_offset <= 0) { - ret = next_offset; - goto out; - } + if (next_offset <= 0) + return next_offset; byte_offset = next_offset * esz; id += next_offset; gpa += byte_offset; len -= byte_offset; } - ret = 1; - -out: - kfree(entry); - return ret; + return 1; } /**
We currently allocate an entry dynamically, but we never check if the allocation actually succeeded. We actually don't need a dynamic allocation, because we know the maximum size of an ITS table entry, so we can simply use an allocation on the stack. Cc: <stable@vger.kernel.org> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- virt/kvm/arm/vgic/vgic-its.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)