Re: [PATCH 2/4] rust: io_uring: introduce rust abstraction for io-uring cmd

[Sidong Yang <sidong.yang@furiosa.ai>]

On Mon, Jul 21, 2025 at 06:28:09PM +0200, Benno Lossin wrote:
> On Mon Jul 21, 2025 at 5:47 PM CEST, Sidong Yang wrote:
> > On Mon, Jul 21, 2025 at 11:04:31AM -0400, Caleb Sander Mateos wrote:
> >> On Mon, Jul 21, 2025 at 1:23 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >> > On Sun, Jul 20, 2025 at 03:10:28PM -0400, Caleb Sander Mateos wrote:
> >> > > On Sat, Jul 19, 2025 at 10:34 AM Sidong Yang <sidong.yang@furiosa.ai> wrote:
> >> > > > +    }
> >> > > > +
> >> > > > +    // Called by consumers of io_uring_cmd, if they originally returned -EIOCBQUEUED upon receiving the command
> >> > > > +    #[inline]
> >> > > > +    pub fn done(self, ret: isize, res2: u64, issue_flags: u32) {
> >> > >
> >> > > I don't think it's safe to move io_uring_cmd. io_uring_cmd_done(), for
> >> > > example, calls cmd_to_io_kiocb() to turn struct io_uring_cmd *ioucmd
> >> > > into struct io_kiocb *req via a pointer cast. And struct io_kiocb's
> >> > > definitely need to be pinned in memory. For example,
> >> > > io_req_normal_work_add() inserts the struct io_kiocb into a linked
> >> > > list. Probably some sort of pinning is necessary for IoUringCmd.
> >> >
> >> > Understood, Normally the users wouldn't create IoUringCmd than use borrowed cmd
> >> > in uring_cmd() callback. How about change to &mut self and also uring_cmd provides
> >> > &mut IoUringCmd for arg.
> >> 
> >> I'm still a little worried about exposing &mut IoUringCmd without
> >> pinning. It would allow swapping the fields of two IoUringCmd's (and
> >> therefore struct io_uring_cmd's), for example. If a struct
> >> io_uring_cmd belongs to a struct io_kiocb linked into task_list,
> >> swapping it with another struct io_uring_cmd would result in
> >> io_uring_cmd_work() being invoked on the wrong struct io_uring_cmd.
> >> Maybe it would be okay if IoUringCmd had an invariant that the struct
> >> io_uring_cmd is not on the task work list. But I would feel safer with
> >> using Pin<&mut IoUringCmd>. I don't have much experience with Rust in
> >> the kernel, though, so I would welcome other opinions.
> >
> > I've thought about this deeply. You're right. exposing &mut without
> > pinning make it unsafe.
> 
> > User also can make *mut and memmove to anywhere without unsafe block.
> 
> How so? Using `*mut T` always needs unsafe.

You're right. Please forget about this.
> 
> > It's safest to get NonNull from from_raw and it returns
> > Pin<&mut IoUringCmd>.
> 
> I don't think you need `NonNull<T>`.

NonNull<T> gurantees that it's not null. It could be also dangling but it's
safer than *mut T. Could you tell me why I don't need it?

> 
> > from_raw() name is weird. it should be from_nonnnull()? Also, done()
> > would get Pin<&mut Self>.
> 
> That sounds reasonable.
> 
> Are you certain that it's an exclusive reference?

As far as I know, yes.

Thanks,
Sidong
> 
> ---
> Cheers,
> Benno