Re: [PATCH V3 05/12] io_uring: bpf: extend io_uring with bpf struct_ops

[Jens Axboe <axboe@kernel.dk>]

On 4/2/26 10:17 PM, Ming Lei wrote:
> On Wed, Mar 25, 2026 at 08:09:03PM -0600, Jens Axboe wrote:
>> On 3/24/26 10:37 AM, Ming Lei wrote:
>>>  int io_uring_bpf_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>  {
>>> +	struct uring_bpf_data *data = io_kiocb_to_cmd(req, struct uring_bpf_data);
>>> +	u32 opf = READ_ONCE(sqe->bpf_op_flags);
>>> +	unsigned char bpf_op = uring_bpf_get_op(opf);
>>> +	const struct uring_bpf_ops *ops;
>>> +
>>> +	if (unlikely(!(req->ctx->flags & IORING_SETUP_BPF_EXT)))
>>> +		goto fail;
>>> +
>>> +	if (bpf_op >= IO_RING_MAX_BPF_OPS)
>>> +		return -EINVAL;
>>> +
>>> +	ops = req->ctx->bpf_ext_ops[bpf_op].ops;
>>> +	data->opf = opf;
>>> +	data->ops = ops;
>>> +	if (ops && ops->prep_fn)
>>> +		return ops->prep_fn(data, sqe);
>>> +fail:
>>>  	return -EOPNOTSUPP;
>>>  }
>>
>> Any early exit should ensure 'data' is sane, so that the cleanup doesn't
>> potentially touch uninitialized crap. This is something that has bit us
>> in the past. Not an issue for this patch that adds the code, but it will
>> be once the next patch is applied. Better to clear ->opf/ops here
>> upfront, so that we never leave this function without 'data' being fully
>> initialized.
> 
> But ->cleanup() is only called in case of REQ_F_NEED_CLEANUP.
> 
> Or maybe you mean other cleanup instead of ->cleanup()?

I do mean ->cleanup() - what I'm trying to say here is that we've had
cases of REQ_F_NEED_CLEANUP being set late, and hence missing cleanup
for easily hit error conditions, and non-initialized data being exposed
in cleanup. It's very easy to miss for later patches that adds another
error condition. My recommendation is to fully initialize 'data' and set
REQ_F_NEED_CLEANUP early, which handily avoids that for future changes
too.

-- 
Jens Axboe