Re: [PATCH bpf-next v6 12/13] selftests/bpf: Test using file dynptr after the reference on file is dropped

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

On 6/1/26 7:42 PM, Amery Hung wrote:
> On Mon, Jun 1, 2026 at 5:05 AM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
>>
>> On 5/29/26 2:49 AM, Amery Hung wrote:
>>> File dynptr and slice should be invalidated when the parent file's
>>> reference is dropped in the program. Without the verifier tracking
>>> dyntpr's parent referenced object, the dynptr would continute to be
>>> incorrectly used even if the underlying file is being tear down or gone.
>>>
>>> Signed-off-by: Amery Hung <ameryhung@gmail.com>
>>> ---
>>>  .../selftests/bpf/progs/file_reader_fail.c    | 60 +++++++++++++++++++
>>>  1 file changed, 60 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/bpf/progs/file_reader_fail.c b/tools/testing/selftests/bpf/progs/file_reader_fail.c
>>> index 0739620dea8a..d5fae5e4cf9a 100644
>>> --- a/tools/testing/selftests/bpf/progs/file_reader_fail.c
>>> +++ b/tools/testing/selftests/bpf/progs/file_reader_fail.c
>>> @@ -50,3 +50,63 @@ int xdp_no_dynptr_type(struct xdp_md *xdp)
>>>       bpf_dynptr_file_discard(&dynptr);
>>>       return 0;
>>>  }
>>> +
>>> +SEC("lsm/file_open")
>>> +__failure
>>> +__msg("Leaking reference id={{[0-9]+}} alloc_insn={{[0-9]+}}. Release it first.")
>>> +int use_file_dynptr_after_put_file(void *ctx)
>>> +{
>>> +     struct task_struct *task = bpf_get_current_task_btf();
>>> +     struct file *file = bpf_get_task_exe_file(task);
>>> +     struct bpf_dynptr dynptr;
>>> +     char buf[64];
>>> +
>>> +     if (!file)
>>> +             return 0;
>>> +
>>> +     if (bpf_dynptr_from_file(file, 0, &dynptr))
>>> +             goto out;
>>> +
>>> +     /* this should fail - file dynptr should be discarded first to prevent resource leak */
>>> +     bpf_put_file(file);
>>> +
>>> +     bpf_dynptr_read(buf, sizeof(buf), &dynptr, 0, 0);
>>> +     return 0;
>>> +
>>> +out:
>>> +     bpf_dynptr_file_discard(&dynptr);
>>> +     bpf_put_file(file);
>>> +     return 0;
>>> +}
>>> +
>>> +SEC("lsm/file_open")
>>> +__failure
>>> +__msg("Leaking reference id={{[0-9]+}} alloc_insn={{[0-9]+}}. Release it first.")
>>> +int use_file_dynptr_slice_after_put_file(void *ctx)
>>> +{
>>> +     struct task_struct *task = bpf_get_current_task_btf();
>>> +     struct file *file = bpf_get_task_exe_file(task);
>>> +     struct bpf_dynptr dynptr;
>>> +     char *data;
>>> +
>>> +     if (!file)
>>> +             return 0;
>>> +
>>> +     if (bpf_dynptr_from_file(file, 0, &dynptr))
>>> +             goto out;
>>> +
>>> +     data = bpf_dynptr_data(&dynptr, 0, 1);
>>
>> File dynptr is always read-only, so bpf_dynptr_data() always
>> returns NULL. Verifier does not know this (not sure if we should
>> address it). Maybe it makes sense to remove this call or substitute by
>> probe read, just to avoid confusing example.
>>
> 
> Thanks for taking a look at the selftests and pointing this out. Does
> replacing it with bpf_dynptr_slice() make sense to you?
>
 
That'll do, but bpf_dynptr_read() is more straightforward API for reading file
dynptr, as you always have to copy the data.

>>> +     if (!data)
>>> +             goto out;
>>> +
>>> +     /* this should fail - file dynptr should be discarded first to prevent resource leak */
>>> +     bpf_put_file(file);
>>> +
>>> +     *data = 'x';
>>> +     return 0;
>>> +
>>> +out:
>>> +     bpf_dynptr_file_discard(&dynptr);
>>> +     bpf_put_file(file);
>>> +     return 0;
>>> +}
>>