Message-ID: <CAHS8izPsK_+WffSBiaEEc7cb44dapure=L=1zhLWkjxAy9cpwA@mail.gmail.com>
Date: Fri, 26 Jul 2024 10:00:15 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Zijian Zhang <zijianzhang@...edance.com>
Cc: netdev@...r.kernel.org, edumazet@...gle.com, 
	willemdebruijn.kernel@...il.com, cong.wang@...edance.com, 
	xiaochun.lu@...edance.com
Subject: Re: [PATCH net-next v7 1/3] sock: support copying cmsgs to the user
 space in sendmsg

On Thu, Jul 25, 2024 at 4:51 PM Zijian Zhang <zijianzhang@...edance.com> wrote:
...
> >> -static int ____sys_sendmsg(struct socket *sock, struct msghdr *msg_sys,
> >> -                       unsigned int flags, struct used_address *used_address,
> >> +static int sendmsg_copy_cmsg_to_user(struct msghdr *msg_sys,
> >> +                                 struct user_msghdr __user *umsg)
> >> +{
> >> +    struct compat_msghdr __user *umsg_compat =
> >> +                            (struct compat_msghdr __user *)umsg;
> >> +    unsigned int flags = msg_sys->msg_flags;
> >> +    struct msghdr msg_user = *msg_sys;
> >> +    unsigned long cmsg_ptr;
> >> +    struct cmsghdr *cmsg;
> >> +    int err;
> >> +
> >> +    msg_user.msg_control_is_user = true;
> >> +    msg_user.msg_control_user = umsg->msg_control;
> >> +    cmsg_ptr = (unsigned long)msg_user.msg_control;
> >> +    for_each_cmsghdr(cmsg, msg_sys) {
> >> +            if (!CMSG_OK(msg_sys, cmsg))
> >> +                    break;
> >> +            if (cmsg_copy_to_user(cmsg))
> >> +                    put_cmsg(&msg_user, cmsg->cmsg_level, cmsg->cmsg_type,
> >> +                             cmsg->cmsg_len - sizeof(*cmsg), CMSG_DATA(cmsg));
> >
> > put_cmsg() can fail as far as I can tell. Any reason we don't have to check for
> > failure here?
> >
> > What happens when these failures happen. Do we end up putting the ZC
> > notification later, or is the zc notification lost forever because we did not
> > detect the failure to put_cmsg() it?
> >
>
> That's a good question,
>
> The reason why I don't have check here is that I refered to net/socket.c
> and sock.c. It turns out there is no failure check for put_cmsgs in
> these files.
>
> For example, in sock_recv_errqueue, it invokes put_cmsg without check,
> and kfree_skb anyway. In this case, if put_cmsg fails, we will lose the
> information forever. I find cases where sock_recv_errqueue is used for
> TX_TIMESTAMP. Maybe loss for timestamp is okay?
>
> However, I find that sock_recv_errqueue is also used in rds_recvmsg to
> receive the zc notifications for rds socket. The zc notification could
> also be lost forever in this case?
>
> Not sure if anyone knows the reason why there is no failure check for
> put_cmsg in net/socket.c and sock.c?
>

I don't know to be honest. I think it's fine for the put_cmsg() to
fail and the notification to be delivered later. However I'm not sure
it's OK for the notification to be lost permanently because of an
error?

For timestamp I can see it not being a big deal if the notification is
lost. For ZC notifications, I think the normal flow is that the
application holds onto the TX buffer until it receives the
notification. If the notification is lost because of an error,
wouldn't that cause a permanent memory leak in the application?

My humble opinion is try as much as possible to either fully deliver
the notification or to save the notification for a future syscall, but
not to lose it. But, I see that no other reviewers are calling this
out, so maybe it's not a big deal and you shouldn't change anything.

> > This may be a lack of knowledge on my part, but i'm very confused that
> > msg_control_copy_to_user is set to false here, and then checked below, and it's
> > not touched in between. How could it evaluate to true below? Is it because something
> > overwrites the value in msg_sys between this set and the check?
> >
> > If something is overwriting it, is the initialization to false necessary?
> > I don't see other fields of msg_sys initialized this way.
> >
>
> ```
> msg_sys->msg_control_copy_to_user = false;
> ...
> err = __sock_sendmsg(sock, msg_sys); -> __sock_cmsg_send
> ...
> if (msg && msg_sys->msg_control_copy_to_user && err >= 0)
> ```
>
> The msg_control_copy_to_user maybe updated by the cmsg handler in
> the function __sock_cmsg_send. In patch 2/3, we have
> msg_control_copy_to_user updated to true in SCM_ZC_NOTIFICATION
> handler.
>
> As for the initialization,
>
> msg_sys is allocated from the kernel stack, if we don't initialize
> it to false, it might be randomly true, even though there is no
> cmsg wants to be copied back.
>
> Why is there only one initialization here? The existing bit
> msg_control_is_user only get initialized where the following code
> path will use it. msg_control_is_user is initialized in multiple
> locations in net/socket.c. However, In function hidp_send_frame,
> msg_control_is_user is not initialized, because the following path will
> not use this bit.
>
> We only initialize msg_control_copy_to_user in function
> ____sys_sendmsg, because only in this function will we check this bit.
>
> If the initialization here makes people confused, I will add some docs.
>

Thanks for the explanation. This looks correct to me now, no need to
add docs. I just missed the intention.

> >>
> >>      if (msg_sys->msg_controllen > INT_MAX)
> >>              goto out;
> >> @@ -2594,6 +2630,14 @@ static int ____sys_sendmsg(struct socket *sock, struct msghdr *msg_sys,
> >>                             used_address->name_len);
> >>      }
> >>
> >> +    if (msg && msg_sys->msg_control_copy_to_user && err >= 0) {
> >> +            ssize_t len = err;
> >> +
> >> +            err = sendmsg_copy_cmsg_to_user(msg_sys, msg);
> >> +            if (!err)
> >> +                    err = len;
> >
> > I'm a bit surprised there isn't any cleanup here if copying the cmsg to user
> > fails. It seems that that __sock_sendmsg() is executed, then if we fail here,
> > we just return an error without unrolling what __sock_sendmsg() did. Why is
> > this ok?
> >
> > Should sendmsg_copy_cmsg_to_user() be done before __sock_sendms() with a goto
> > out if it fails?
> >
>
> I did this refering to ____sys_recvmsg, in this function, if __put_user
> fails, we do not unroll what sock_recvmsg did, and return the error code
> of __put_user.
>
> Before __sock_sendmsg, the content of msg_control is not updated by the
> function __sock_cmsg_send, so sendmsg_copy_cmsg_to_user at this time
> might be not expected.
>

I see. I don't think sendmsg_copy_cmsg_to_user() should unroll
__sock_sendmsg(), but if possible for the notification not to be lost,
I think that would be an improvement.

-- 
Thanks,
Mina

Powered by blists - more mailing lists