Re: [External] Re: [PATCH net-next v6 2/4] sock: support copy cmsg to userspace in TX path

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Zijian Zhang wrote:
> 
> 
> On 6/30/24 7:43 AM, Willem de Bruijn wrote:
> > zijianzhang@ wrote:
> >> From: Zijian Zhang <zijianzhang@bytedance.com>
> >>
> >> Since ____sys_sendmsg creates a kernel copy of msg_control and passes
> >> that to the callees, put_cmsg will write into this kernel buffer. If
> >> people want to piggyback some information like timestamps upon returning
> >> of sendmsg. ____sys_sendmsg will have to copy_to_user to the original buf,
> >> which is not supported. As a result, users typically have to call recvmsg
> >> on the ERRMSG_QUEUE of the socket, incurring extra system call overhead.
> >>
> >> This commit supports copying cmsg to userspace in TX path by introducing
> >> a flag MSG_CMSG_COPY_TO_USER in struct msghdr to guide the copy logic
> >> upon returning of ___sys_sendmsg.
> >>
> >> Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
> >> Signed-off-by: Xiaochun Lu <xiaochun.lu@bytedance.com>

> >>   		if (cmsg->cmsg_level != SOL_SOCKET)
> >>   			continue;
> >>   		ret = __sock_cmsg_send(sk, cmsg, sockc);
> ...
> >> +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 long cmsg_ptr = (unsigned long)umsg->msg_control;
> >> +	unsigned int flags = msg_sys->msg_flags;
> >> +	struct msghdr msg_user = *msg_sys;
> >> +	struct cmsghdr *cmsg;
> >> +	int err;
> >> +
> >> +	msg_user.msg_control = umsg->msg_control;
> >> +	msg_user.msg_control_is_user = true;
> >> +	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));
> >> +	}
> > 
> > Alternatively just copy the entire msg_control if any cmsg wants to
> > be copied back. The others will be unmodified. No need to iterate
> > then.
> > 
> 
> Copy the entire msg_control via copy_to_user does not take
> MSG_CMSG_COMPAT into account. I may have to use put_cmsg to deal
> with the compat version, and thus have to keep the for loop?

Good point. Okay, then this is pretty clean. Only returning the
cmsg that have been written to is actually quite nice.
 
> If so, I may keep the function cmsg_copy_to_user to avoid extra copy?
> 
> >> +
> >> +	err = __put_user((msg_sys->msg_flags & ~MSG_CMSG_COMPAT), COMPAT_FLAGS(umsg));
> >> +	if (err)
> >> +		return err;
> > 
> > Does this value need to be written?
> > 
> 
> I did this according to ____sys_recvmsg, maybe it's useful to export
> flag like MSG_CTRUNC to users?

Good point.