[SRU focal/linux-oem-5.6 1/1] io_uring: fix missing msg_name assignment

[Thadeu Lima de Souza Cascardo]

On Fri, May 14, 2021 at 09:43:19AM -0400, Krzysztof Kozlowski wrote:
> On 14/05/2021 08:14, Thadeu Lima de Souza Cascardo wrote:
> > From: Pavel Begunkov <asml.silence at gmail.com>
> > 
> > BugLink: https://bugs.launchpad.net/bugs/1928028
> > 
> > Ensure to set msg.msg_name for the async portion of send/recvmsg,
> > as the header copy will copy to/from it.
> > 
> > Cc: stable at vger.kernel.org # v5.5+
> > Signed-off-by: Pavel Begunkov <asml.silence at gmail.com>
> > Signed-off-by: Jens Axboe <axboe at kernel.dk>
> > (backported from commit dd821e0c95a64b5923a0c57f07d3f7563553e756)
> > [cascardo: missing commit 52de1fe12240, there is no io_recvmsg_copy_hdr]
> > [cascardo: also, recvmsg paths do not set msg_name when copying msg,
> >  but use uaddr]
> 
> Do you refer here to the io_recvmsg_prep() and you state it does not
> need setting msg_name?

The original commit dd821e0c95a6 sets msg.name at io_recvmsg_copy_hdr, which is
not present in 5.6, and was introduced in commit 52de1fe12240. Which is,
indeed, called at io_recvmsg_prep and io_recvmsg. Though msg_name is set on
io_recvmsg, it is not necessary. And so, not necessary at io_recvmsg_prep
either.

Running a test with IOSQE_ASYNC flag and IORING_OP_RECVMSG, and a msg_name,
there is no such issue as the one we found out with IORING_OP_SENDMSG. Simply
because, as I stated, a uaddr is used.

>From net/socket.c:copy_msghdr_from_user, save_addr is used to signal this is
called from recvmsg, not sendmsg:

        if (save_addr)
                *save_addr = msg.msg_name;

        if (msg.msg_name && kmsg->msg_namelen) {
                if (!save_addr) {
                        err = move_addr_to_kernel(msg.msg_name,
                                                  kmsg->msg_namelen,
                                                  kmsg->msg_name);
                        if (err < 0)
                                return err;
                }

Then, on net/socket.c:____sys_recvmsg:

	struct sockaddr_storage addr;
[...]
        if (uaddr != NULL) {
                err = move_addr_to_user(&addr,
                                        msg_sys->msg_namelen, uaddr,
                                        uaddr_len);
                if (err < 0)
                        goto out;
        }

So, there is no need to set msg_name on the kmsg on recvmsg paths, but a uaddr
must be given, where a pointer to the user given msg_name will be saved and
used to copy the kernel stack-allocated address to.

Makes sense?
Cascardo.


> 
> 
> Best regards,
> Krzysztof
> 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>
> > ---
> >  fs/io_uring.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > index 8276c3c42894..387d779ab26b 100644
> > --- a/fs/io_uring.c
> > +++ b/fs/io_uring.c
> > @@ -3034,6 +3034,7 @@ static int io_sendmsg_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> >  	if (req->flags & REQ_F_NEED_CLEANUP)
> >  		return 0;
> >  
> > +	io->msg.msg.msg_name = &io->msg.addr;
> >  	io->msg.iov = io->msg.fast_iov;
> >  	ret = sendmsg_copy_msghdr(&io->msg.msg, sr->msg, sr->msg_flags,
> >  					&io->msg.iov);
> > 
>