Re: [RFC PATCH v1 07/10] bpf: add kfuncs and helpers support for file dynptrs

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 10/3/25 19:38, Andrii Nakryiko wrote:
> On Fri, Oct 3, 2025 at 9:04 AM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> Add support for file dynptr.
>>
>> Introduce struct bpf_dynptr_file_impl to hold internal state for file
>> dynptrs, with 64-bit size and offset support.
>>
>> Introduce lifecycle management kfuncs:
>>    - bpf_dynptr_from_file() for initialization
>>    - bpf_dynptr_file_discard() for destruction
>>
>> Extend existing helpers to support file dynptrs in:
>>    - bpf_dynptr_read()
>>    - bpf_dynptr_slice()
>>
>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>> ---
>>   kernel/bpf/helpers.c | 97 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 95 insertions(+), 2 deletions(-)
>>
> [...]
>
>> +static int bpf_file_fetch_bytes(struct bpf_dynptr_file_impl *df, u64 offset, void *buf, u64 len)
>> +{
>> +       const void *ptr;
>> +
>> +       if (!buf || len == 0)
> len == 0 doesn't return error for LOCAL and RINGBUF (at least), I
> don't think we should deviate. Just treat len == 0 as no-op.
>
>> +               return -EINVAL;
>> +
>> +       df->freader.buf = buf;
>> +       df->freader.buf_sz = len;
>> +       ptr = freader_fetch(&df->freader, offset + df->offset, len);
>> +       if (!ptr)
>> +               return df->freader.err;
>> +
>> +       if (ptr != buf) /* Force copying into the buffer */
>> +               memcpy(buf, ptr, len);
>> +
>> +       return 0;
>> +}
>> +
>>   void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data,
>>                       enum bpf_dynptr_type type, u32 offset, u32 size)
>>   {
>> @@ -1782,6 +1832,8 @@ static int __bpf_dynptr_read(void *dst, u64 len, const struct bpf_dynptr_kern *s
>>          case BPF_DYNPTR_TYPE_SKB_META:
>>                  memmove(dst, bpf_skb_meta_pointer(src->data, src->offset + offset), len);
>>                  return 0;
>> +       case BPF_DYNPTR_TYPE_FILE:
>> +               return bpf_file_fetch_bytes(src->data, offset, dst, len);
>>          default:
>>                  WARN_ONCE(true, "bpf_dynptr_read: unknown dynptr type %d\n", type);
>>                  return -EFAULT;
>> @@ -2177,6 +2229,35 @@ void bpf_rb_root_free(const struct btf_field *field, void *rb_root,
>>          }
>>   }
>>
>> +enum bpf_is_sleepable {
>> +       MAY_SLEEP,
>> +       MAY_NOT_SLEEP,
>> +};
> might be a bit of an overkill to have enum for this, but I don't feel
> strongly about this
>
>> +
>> +static int make_file_dynptr(struct file *file, u32 flags, enum bpf_is_sleepable sleepable,
>> +                           struct bpf_dynptr_kern *ptr)
>> +{
> nit: put it right before bpf_dynptr_from_file()?
No problem, but that makes patch look uglier, bpf_dynptr_from_file is 
substituted with make_file_dynptr.
>
>> +       struct bpf_dynptr_file_impl *state;
>> +
>> +       /* flags is currently unsupported */
>> +       if (flags) {
>> +               bpf_dynptr_set_null(ptr);
>> +               return -EINVAL;
>> +       }
>> +
>> +       state = bpf_mem_alloc(&bpf_global_ma, sizeof(struct bpf_dynptr_file_impl));
>> +       if (!state) {
>> +               bpf_dynptr_set_null(ptr);
>> +               return -ENOMEM;
>> +       }
>> +       state->offset = 0;
>> +       state->size = U64_MAX; /* Don't restrict size, as file may change anyways */
>> +       freader_init_from_file(&state->freader, NULL, 0, file, sleepable == MAY_SLEEP);
>> +       bpf_dynptr_init(ptr, state, BPF_DYNPTR_TYPE_FILE, 0, 0);
>> +       bpf_dynptr_set_rdonly(ptr);
>> +       return 0;
>> +}
>> +
>>   __bpf_kfunc_start_defs();
>>
>>   __bpf_kfunc void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign)
>> @@ -2720,6 +2801,9 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr *p, u64 offset,
>>          }
>>          case BPF_DYNPTR_TYPE_SKB_META:
>>                  return bpf_skb_meta_pointer(ptr->data, ptr->offset + offset);
>> +       case BPF_DYNPTR_TYPE_FILE:
>> +               err = bpf_file_fetch_bytes(ptr->data, offset, buffer__opt, buffer__szk);
>> +               return err ? NULL : buffer__opt;
>>          default:
>>                  WARN_ONCE(true, "unknown dynptr type %d\n", type);
>>                  return NULL;
>> @@ -2814,7 +2898,7 @@ __bpf_kfunc int bpf_dynptr_adjust(const struct bpf_dynptr *p, u64 start, u64 end
>>          if (start > size || end > size)
>>                  return -ERANGE;
>>
>> -       ptr->offset += start;
>> +       bpf_dynptr_advance_offset(ptr, start);
>>          bpf_dynptr_set_size(ptr, end - start);
>>
>>          return 0;
>> @@ -4201,11 +4285,20 @@ __bpf_kfunc int bpf_task_work_schedule_resume(struct task_struct *task, struct b
>>
>>   __bpf_kfunc int bpf_dynptr_from_file(struct file *file, u32 flags, struct bpf_dynptr *ptr__uninit)
>>   {
>> -       return 0;
>> +       return make_file_dynptr(file, flags, MAY_NOT_SLEEP, (struct bpf_dynptr_kern *)ptr__uninit);
>>   }
>>
>>   __bpf_kfunc int bpf_dynptr_file_discard(struct bpf_dynptr *dynptr)
>>   {
>> +       struct bpf_dynptr_kern *ptr = (struct bpf_dynptr_kern *)dynptr;
>> +       struct bpf_dynptr_file_impl *df;
>> +
>> +       if (bpf_dynptr_get_type(ptr) == BPF_DYNPTR_TYPE_INVALID)
>> +               return 0;
> nit: let's just do what dynptr_read does:
>
> if (!dynptr->data)
>      return 0;
Thanks for nits, I'll apply them.
>> +
>> +       df = ptr->data;
>> +       freader_cleanup(&df->freader);
>> +       bpf_mem_free(&bpf_global_ma, df);
>>          return 0;
>>   }
>>
>> --
>> 2.51.0
>>