On Wed, May 11, 2022 at 5:05 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 5/10/22 12:42 AM, Joanne Koong wrote:
> [...]
> > @@ -6498,6 +6523,11 @@ struct bpf_timer {
> > __u64 :64;
> > } __attribute__((aligned(8)));
> >
> > +struct bpf_dynptr {
> > + __u64 :64;
> > + __u64 :64;
> > +} __attribute__((aligned(8)));
> > +
> > struct bpf_sysctl {
> > __u32 write; /* Sysctl is being read (= 0) or written (= 1).
> > * Allows 1,2,4-byte read, but no write.
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 8a2398ac14c2..a4272e9239ea 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1396,6 +1396,77 @@ const struct bpf_func_proto bpf_kptr_xchg_proto = {
> > .arg2_btf_id = BPF_PTR_POISON,
> > };
> >
> > +void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data, enum bpf_dynptr_type type,
> > + u32 offset, u32 size)
> > +{
> > + ptr->data = data;
> > + ptr->offset = offset;
> > + ptr->size = size;
> > + bpf_dynptr_set_type(ptr, type);
> > +}
> > +
> > +void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr)
> > +{
> > + memset(ptr, 0, sizeof(*ptr));
> > +}
> > +
> > +BPF_CALL_3(bpf_dynptr_alloc, u32, size, u64, flags, struct bpf_dynptr_kern *, ptr)
> > +{
> > + gfp_t gfp_flags = GFP_ATOMIC;
>
> nit: should also have __GFP_NOWARN
>
> I presume mem accounting cannot be done on this one given there is no real "ownership"
> of this piece of mem?
While we figure out the details of memory accounting for allocations,
I will defer the malloc parts of this patchset to the 2nd dynptr
patchset. I will resubmit v5 without malloc-type dynptrs
>
> Was planning to run some more local tests tomorrow, but from glance at selftest side
> I haven't seen sanity checks like these:
>
> bpf_dynptr_alloc(8, 0, &ptr);
> data = bpf_dynptr_data(&ptr, 0, 0);
> bpf_dynptr_put(&ptr);
> *(__u8 *)data = 23;
>
> How is this prevented? I think you do a ptr id check in the is_dynptr_ref_function
> check on the acquire function, but with above use, would our data pointer escape, or
> get invalidated via last put?
>
> Thanks,
> Daniel
