On Mon, 28 Oct 2019 23:11:50 +0100, Björn Töpel wrote: > On Mon, 28 Oct 2019 at 18:55, Jakub Kicinski > <jakub.kicinski@xxxxxxxxxxxxx> wrote: > > > > On Fri, 25 Oct 2019 09:18:40 +0200, Björn Töpel wrote: > > > From: Björn Töpel <bjorn.topel@xxxxxxxxx> > > > > > > Prior this commit, the array storing XDP socket instances were stored > > > in a separate allocated array of the XSKMAP. Now, we store the sockets > > > as a flexible array member in a similar fashion as the arraymap. Doing > > > so, we do less pointer chasing in the lookup. > > > > > > Signed-off-by: Björn Töpel <bjorn.topel@xxxxxxxxx> > > > > Thanks for the re-spin. > > > > > diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c > > > index 82a1ffe15dfa..a83e92fe2971 100644 > > > --- a/kernel/bpf/xskmap.c > > > +++ b/kernel/bpf/xskmap.c > > > > > @@ -92,44 +93,35 @@ static struct bpf_map *xsk_map_alloc(union bpf_attr *attr) > > > attr->map_flags & ~(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)) > > > return ERR_PTR(-EINVAL); > > > > > > - m = kzalloc(sizeof(*m), GFP_USER); > > > - if (!m) > > > + numa_node = bpf_map_attr_numa_node(attr); > > > + size = struct_size(m, xsk_map, attr->max_entries); > > > + cost = size + array_size(sizeof(*m->flush_list), num_possible_cpus()); > > > > Now we didn't use array_size() previously because the sum here may > > overflow. > > > > We could use __ab_c_size() here, the name is probably too ugly to use > > directly and IDK what we'd have to name such a accumulation helper... > > > > So maybe just make cost and size a u64 and we should be in the clear. > > > > Hmm, but both: > int bpf_map_charge_init(struct bpf_map_memory *mem, size_t size); > void *bpf_map_area_alloc(size_t size, int numa_node); > pass size as size_t, so casting to u64 doesn't really help on 32-bit > systems, right? Yup :( IOW looks like the overflows will not be caught on 32bit machines in all existing code that does the (u64) cast. I hope I'm wrong there. > Wdyt about simply adding: > if (cost < size) > return ERR_PTR(-EINVAL) > after the cost calculation for explicit overflow checking? We'd need that for all users of these helpers. Could it perhaps makes sense to pass "alloc_size" and "extra_cost" as separate size_t to bpf_map_charge_init() and then we can do the overflow checking there, centrally? We can probably get rid of all the u64 casting too at that point, and use standard overflow helpers, yuppie :) > So, if size's struct_size overflows, the allocation will fail. > And we'll catch the cost overflow with the if-statement, no? > > Another option is changing the size_t in bpf_map_... to u64. Maybe > that's better, since arraymap and devmap uses u64 for cost/size.
