On Wed, Jan 15, 2025 at 3:58 AM Ming Lei <tom.leiming@xxxxxxxxx> wrote:
>
> 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
>
Hi Ming,
Your stub function signature does not look quite right.
It should be <struct_ops_name>__<op_name>, hence:
static void bpf_aio_complete_ops__bpf_aio_complete_cb(struct bpf_aio
*io__ref, long ret)
For more detail, look at find_stub_func_proto().
Thanks,
Amery
>
>
> Thanks,
> Ming
