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@xxxxxxxxxxxxxxx> > > Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx> > > --- > > 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@xxxxxxx> > Thanks! -Christoffer