Hello Alexei,
On Mon, Jan 13, 2025 at 01:30:45PM -0800, Alexei Starovoitov wrote:
> On Sun, Jan 12, 2025 at 8:08 PM Ming Lei <tom.leiming@xxxxxxxxx> wrote:
> >
> > Hello Alexei,
> >
> > Thanks for your comments!
> >
> > On Thu, Jan 09, 2025 at 05:43:12PM -0800, Alexei Starovoitov wrote:
> > > On Tue, Jan 7, 2025 at 4:08 AM Ming Lei <tom.leiming@xxxxxxxxx> wrote:
> > > > +
> > > > +/* Return true if io cmd is queued, otherwise forward it to userspace */
> > > > +bool ublk_run_bpf_handler(struct ublk_queue *ubq, struct request *req,
> > > > + queue_io_cmd_t cb)
> > > > +{
> > > > + ublk_bpf_return_t ret;
> > > > + struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
> > > > + struct ublksrv_io_desc *iod = ublk_get_iod(ubq, req->tag);
> > > > + struct ublk_bpf_io *bpf_io = &data->bpf_data;
> > > > + const unsigned long total = iod->nr_sectors << 9;
> > > > + unsigned int done = 0;
> > > > + bool res = true;
> > > > + int err;
> > > > +
> > > > + if (!test_bit(UBLK_BPF_IO_PREP, &bpf_io->flags))
> > > > + ublk_bpf_prep_io(bpf_io, iod);
> > > > +
> > > > + do {
> > > > + enum ublk_bpf_disposition rc;
> > > > + unsigned int bytes;
> > > > +
> > > > + ret = cb(bpf_io, done);
> > >
> > > High level observation...
> > > I suspect forcing all sturct_ops callbacks to have only these
> > > two arguments and packing args into ublk_bpf_io
> > > will be limiting in the long term.
> >
> > There are three callbacks defined, and only the two with same type for
> > queuing io commands are covered in this function.
> >
> > But yes, callback type belongs to API, which should be designed
> > carefully, and I will think about further.
> >
> > >
> > > And this part of api would need to be redesigned,
> > > but since it's not an uapi... not a big deal.
> > >
> > > > + rc = ublk_bpf_get_disposition(ret);
> > > > +
> > > > + if (rc == UBLK_BPF_IO_QUEUED)
> > > > + goto exit;
> > > > +
> > > > + if (rc == UBLK_BPF_IO_REDIRECT)
> > > > + break;
> > >
> > > Same point about return value processing...
> > > Each struct_ops callback could have had its own meaning
> > > of retvals.
> > > I suspect it would have been more flexible and more powerful
> > > this way.
> >
> > Yeah, I agree, just the 3rd callback of release_io_cmd_t isn't covered
> > in this function.
> >
> > >
> > > Other than that bpf plumbing looks good.
> > >
> > > There is an issue with leaking allocated memory in bpf_aio_alloc kfunc
> > > (it probably should be KF_ACQUIRE)
> >
> > It is one problem which troubles me too:
> >
> > - another callback of struct_ops/bpf_aio_complete_cb is guaranteed to be
> > called after the 'struct bpf_aio' instance is submitted via kfunc
> > bpf_aio_submit(), and it is supposed to be freed from
> > struct_ops/bpf_aio_complete_cb
> >
> > - but the following verifier failure is triggered if bpf_aio_alloc and
> > bpf_aio_release are marked as KF_ACQUIRE & KF_RELEASE.
> >
> > ```
> > libbpf: prog 'ublk_loop_comp_cb': -- BEGIN PROG LOAD LOG --
> > Global function ublk_loop_comp_cb() doesn't return scalar. Only those are supported.
> > ```
>
> That's odd.
> Adding KF_ACQ/REL to bpf_aio_alloc/release kfuncs shouldn't affect
> verification of ublk_loop_comp_cb() prog. It's fine for it to stay 'void'
> return.
> You probably made it global function and that's was the reason for this
> verifier error. Global funcs have to return scalar for now.
> We can relax this restriction if necessary.
Looks marking ublk_loop_comp_cb() as static doesn't work:
[root@ktest-40 ublk]# make
CLNG-BPF ublk_loop.bpf.o
GEN-SKEL ublk_loop.skel.h
libbpf: relocation against STT_SECTION in non-exec section is not supported!
Error: failed to link '/root/git/linux/tools/testing/selftests/ublk/ublk_loop.bpf.o': Invalid argument (22)
But seems not big deal because we can change its return type to 'int'.
>
> >
> > Here 'struct bpf_aio' instance isn't stored in map, and it is provided
> > from struct_ops callback(bpf_aio_complete_cb), I appreciate you may share
> > any idea about how to let KF_ACQUIRE/KF_RELEASE cover the usage here.
>
> This is so that:
>
> ublk_loop_comp_cb ->
> ublk_loop_comp_and_release_aio ->
> bpf_aio_release
>
> would properly recognize that ref to aio is dropped?
>
> Currently the verifier doesn't support that,
> but there is work in progress to add this feature:
>
> https://lore.kernel.org/bpf/20241220195619.2022866-2-amery.hung@xxxxxxxxx/
>
> then in cfi_stabs annotated bio argument in bpf_aio_complete_cb()
> as "struct bpf_aio *aio__ref"
>
> Then the verifier will recognize that callback argument
> comes refcounted and the prog has to call KF_RELEASE kfunc on it.
This looks one very nice feature, thanks for sharing it!
I tried to apply the above patch and patch 3 on next tree and pass 'aio__ref' to the
callback cfi_stabs, but still failed:
[root@ktest-40 ublk]# ./test_loop_01.sh
libbpf: prog 'ublk_loop_comp_cb': BPF program load failed: -EINVAL
libbpf: prog 'ublk_loop_comp_cb': -- BEGIN PROG LOAD LOG --
0: R1=ctx() R10=fp0
; int BPF_PROG(ublk_loop_comp_cb, struct bpf_aio *aio, long ret) @ ublk_loop.c:34
0: (79) r7 = *(u64 *)(r1 +8) ; R1=ctx() R7_w=scalar()
1: (79) r6 = *(u64 *)(r1 +0)
func 'bpf_aio_complete_cb' arg0 has btf_id 37354 type STRUCT 'bpf_aio'
2: R1=ctx() R6_w=trusted_ptr_bpf_aio()
; struct ublk_bpf_io *io = ublk_bpf_acquire_io_from_aio(aio); @ ublk_loop.c:24
2: (bf) r1 = r6 ; R1_w=trusted_ptr_bpf_aio() R6_w=trusted_ptr_bpf_aio()
3: (85) call ublk_bpf_acquire_io_from_aio#43231 ; R0_w=ptr_ublk_bpf_io(ref_obj_id=1) refs=1
4: (bf) r8 = r0 ; R0_w=ptr_ublk_bpf_io(ref_obj_id=1) R8_w=ptr_ublk_bpf_io(ref_obj_id=1) refs=1
; ublk_bpf_complete_io(io, ret); @ ublk_loop.c:26
5: (bf) r1 = r8 ; R1_w=ptr_ublk_bpf_io(ref_obj_id=1) R8_w=ptr_ublk_bpf_io(ref_obj_id=1) refs=1
6: (bc) w2 = w7 ; R2_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) R7_w=scalar() refs=1
7: (85) call ublk_bpf_complete_io#43241 ; refs=1
; ublk_bpf_release_io_from_aio(io); @ ublk_loop.c:27
8: (bf) r1 = r8 ; R1_w=ptr_ublk_bpf_io(ref_obj_id=1) R8=ptr_ublk_bpf_io(ref_obj_id=1) refs=1
9: (85) call ublk_bpf_release_io_from_aio#43257 ;
; ublk_bpf_dettach_and_complete_aio(aio); @ ublk_loop.c:29
10: (bf) r1 = r6 ; R1_w=trusted_ptr_bpf_aio() R6=trusted_ptr_bpf_aio()
11: (85) call ublk_bpf_dettach_and_complete_aio#43245 ;
; bpf_aio_release(aio); @ ublk_loop.c:30
12: (bf) r1 = r6 ; R1_w=trusted_ptr_bpf_aio() R6=trusted_ptr_bpf_aio()
13: (85) call bpf_aio_release#95841
release kernel function bpf_aio_release expects refcounted PTR_TO_BTF_ID
processed 14 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
-- END PROG LOAD LOG --
libbpf: prog 'ublk_loop_comp_cb': failed to load: -EINVAL
libbpf: failed to load object 'ublk_loop.bpf.o'
fail to load bpf obj from ublk_loop.bpf.o
fail to register bpf prog loop ublk_loop.bpf.o
Thanks,
Ming
