Re: [PATCH v2 for-next 3/3] io_uring: support multishot in recvmsg

[Dylan Yudaken <dylany@fb.com>]

On Wed, 2022-07-13 at 06:48 -0600, Jens Axboe wrote:
> On 7/13/22 2:23 AM, Dylan Yudaken wrote:
> > diff --git a/io_uring/net.c b/io_uring/net.c
> > index 5bc3440a8290..56f734acced6 100644
> > --- a/io_uring/net.c
> > +++ b/io_uring/net.c
> > @@ -483,12 +491,15 @@ static inline void io_recv_prep_retry(struct
> > io_kiocb *req)
> >  }
> >  
> >  /*
> > - * Finishes io_recv
> > + * Finishes io_recv and io_recvmsg.
> >   *
> >   * Returns true if it is actually finished, or false if it should
> > run
> >   * again (for multishot).
> >   */
> > -static inline bool io_recv_finish(struct io_kiocb *req, int *ret,
> > unsigned int cflags)
> > +static inline bool io_recv_finish(struct io_kiocb *req,
> > +                                 int *ret,
> > +                                 unsigned int cflags,
> > +                                 bool multishot_finished)
> >  {
> >         if (!(req->flags & REQ_F_APOLL_MULTISHOT)) {
> 
> Minor nit, but this should look like:
> 
> static inline bool io_recv_finish(struct io_kiocb *req, int *ret,
>                                   unsigned int cflags, bool
> mshot_finished)
> > @@ -518,6 +529,104 @@ static inline bool io_recv_finish(struct
> > io_kiocb *req, int *ret, unsigned int c
> >         return true;
> >  }
> >  
> > +static int io_recvmsg_prep_multishot(
> > +       struct io_async_msghdr *kmsg,
> > +       struct io_sr_msg *sr,
> > +       void __user **buf,
> > +       size_t *len)
> > +{
> 
> Ditto on the function formating.
> 

will update both in a v3

> > +       unsigned long used = 0;
> > +
> > +       if (*len < sizeof(struct io_uring_recvmsg_out))
> > +               return -EFAULT;
> > +       used += sizeof(struct io_uring_recvmsg_out);
> > +
> > +       if (kmsg->namelen) {
> > +               if (kmsg->namelen + used > *len)
> > +                       return -EFAULT;
> > +               used += kmsg->namelen;
> > +       }
> > +       if (kmsg->controllen) {
> > +               if (kmsg->controllen + used > *len)
> > +                       return -EFAULT;
> > +               kmsg->msg.msg_control_user = (void *)((unsigned
> > long)*buf + used);
> > +               kmsg->msg.msg_controllen = kmsg->controllen;
> > +               used += kmsg->controllen;
> > +       }
> > +       if (used >= UINT_MAX)
> > +               return -EOVERFLOW;
> > +
> > +       sr->buf = *buf; /* stash for later copy */
> > +       *buf = (void *)((unsigned long)*buf + used);
> > +       *len -= used;
> > +       kmsg->payloadlen = *len;
> > +       return 0;
> > +}
> 
> Not sure if it's just me, but the *buf and casting is really hard to
> read here. Can we make that any clearer? Maybe cast to an unsigned
> long
> * at the top of change the buf argument to be that?

I think that should be doable. Not an (unsigned long *) though as it is
all for incrementing the pointer address, but probably an (unsigned
char *).

> 
> > +struct io_recvmsg_multishot_hdr {
> > +       struct io_uring_recvmsg_out msg;
> > +       struct sockaddr_storage addr;
> > +} __packed;
> 
> This __packed shouldn't be necessary, and I'm always a bit wary of
> adding it on kernel structures as if it's really needed, then we're
> most
> likely doing something wrong (and things will run slower, notably on
> some archs). Looks like you have a BUILD_BUG_ON() for this too, so
> we'd
> catch any potential issues here upfront.

Makes sense.

> 
> > +static int io_recvmsg_multishot(
> > +       struct socket *sock,
> > +       struct io_sr_msg *io,
> > +       struct io_async_msghdr *kmsg,
> > +       unsigned int flags,
> > +       bool *finished)
> > +{
> > +       int err;
> > +       int copy_len;
> > +       struct io_recvmsg_multishot_hdr hdr;
> > +
> > +       if (kmsg->namelen)
> > +               kmsg->msg.msg_name = &hdr.addr;
> > +       kmsg->msg.msg_flags = flags &
> > (MSG_CMSG_CLOEXEC|MSG_CMSG_COMPAT);
> > +       kmsg->msg.msg_namelen = 0;
> > +
> > +       if (sock->file->f_flags & O_NONBLOCK)
> > +               flags |= MSG_DONTWAIT;
> > +
> > +       err = sock_recvmsg(sock, &kmsg->msg, flags);
> > +       *finished = err <= 0;
> > +       if (err < 0)
> > +               return err;
> > +
> > +       hdr.msg = (struct io_uring_recvmsg_out) {
> > +               .controllen = kmsg->controllen - kmsg-
> > >msg.msg_controllen,
> > +               .flags = kmsg->msg.msg_flags & ~MSG_CMSG_COMPAT
> > +       };
> > +
> > +       hdr.msg.payloadlen = err;
> > +       if (err > kmsg->payloadlen)
> > +               err = kmsg->payloadlen;
> > +
> > +       copy_len = sizeof(struct io_uring_recvmsg_out);
> > +       if (kmsg->msg.msg_namelen > kmsg->namelen)
> > +               copy_len += kmsg->namelen;
> > +       else
> > +               copy_len += kmsg->msg.msg_namelen;
> > +
> > +       /*
> > +        *      "fromlen shall refer to the value before
> > truncation.."
> > +        *                      1003.1g
> > +        */
> > +       hdr.msg.namelen = kmsg->msg.msg_namelen;
> > +
> > +       /* ensure that there is no gap between hdr and
> > sockaddr_storage */
> > +       BUILD_BUG_ON(offsetof(struct io_recvmsg_multishot_hdr,
> > addr) !=
> > +                    sizeof(struct io_uring_recvmsg_out));
> > +       if (copy_to_user(io->buf, &hdr, copy_len)) {
> > +               *finished = true;
> > +               return -EFAULT;
> > +       }
> > +
> > +       return sizeof(struct io_uring_recvmsg_out) +
> > +               kmsg->namelen +
> > +               kmsg->controllen +
> > +               err;
> > +}
> 
>         return sizeof(struct io_uring_recvmsg_out) + kmsg->namelen +
>                         kmsg->controllen + err;
> 
> would be closer to the kernel style.
> 
> In general I'm not a big fan of the bool pointer 'finished'. But I
> also
> don't have a good suggestion on how to make it cleaner, so... Would
> be
> nice if we could just have an error return (< 0), and then return >=
> 0
> in two variants for MSHOT_OK and MSHOT_TERMINATE or something.

I will try for v3, but the issue is that we have 2 things to return:
how many bytes are copied to the output buffer, and if multishot is
finished, and these have to be kept separately as there is no way to
derive one from the other. 

> 
> > @@ -527,6 +636,7 @@ int io_recvmsg(struct io_kiocb *req, unsigned
> > int issue_flags)
> >         unsigned flags;
> >         int ret, min_ret = 0;
> >         bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> > +       bool multishot_finished = true;
> >  
> >         sock = sock_from_file(req->file);
> >         if (unlikely(!sock))
> > @@ -545,16 +655,29 @@ int io_recvmsg(struct io_kiocb *req, unsigned
> > int issue_flags)
> >             (sr->flags & IORING_RECVSEND_POLL_FIRST))
> >                 return io_setup_async_msg(req, kmsg, issue_flags);
> >  
> > +retry_multishot:
> >         if (io_do_buffer_select(req)) {
> >                 void __user *buf;
> > +               size_t len = sr->len;
> >  
> > -               buf = io_buffer_select(req, &sr->len, issue_flags);
> > +               buf = io_buffer_select(req, &len, issue_flags);
> >                 if (!buf)
> >                         return -ENOBUFS;
> > +
> > +               if (req->flags & REQ_F_APOLL_MULTISHOT) {
> > +                       ret = io_recvmsg_prep_multishot(kmsg, sr,
> > +                                                       &buf,
> > &len);
> > +
>                         ret = io_recvmsg_prep_multishot(kmsg, sr,
> &buf, &len);
> 
> Apart from these nits, looks pretty good to me.
>