netdev - Re: [External] Re: [PATCH net-next v4 2/3] sock: add MSG_ZEROCOPY notification mechanism based on msg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1b693647-bccf-48b3-8010-abdb91a32594@bytedance.com>
Date: Sat, 1 Jun 2024 15:52:24 -0700
From: Zijian Zhang <zijianzhang@...edance.com>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, edumazet@...gle.com,
 willemdebruijn.kernel@...il.com, cong.wang@...edance.com,
 xiaochun.lu@...edance.com
Subject: Re: [External] Re: [PATCH net-next v4 2/3] sock: add MSG_ZEROCOPY
 notification mechanism based on msg_control

On 6/1/24 3:37 AM, Simon Horman wrote:
> On Tue, May 28, 2024 at 09:21:02PM +0000, zijianzhang@...edance.com wrote:
>> From: Zijian Zhang <zijianzhang@...edance.com>
>>
>> The MSG_ZEROCOPY flag enables copy avoidance for socket send calls.
>> However, zerocopy is not a free lunch. Apart from the management of user
>> pages, the combination of poll + recvmsg to receive notifications incurs
>> unignorable overhead in the applications. The overhead of such sometimes
>> might be more than the CPU savings from zerocopy. We try to solve this
>> problem with a new notification mechanism based on msgcontrol.
>> This new mechanism aims to reduce the overhead associated with receiving
>> notifications by embedding them directly into user arguments passed with
>> each sendmsg control message. By doing so, we can significantly reduce
>> the complexity and overhead for managing notifications. In an ideal
>> pattern, the user will keep calling sendmsg with SCM_ZC_NOTIFICATION
>> msg_control, and the notification will be delivered as soon as possible.
>>
>> Signed-off-by: Zijian Zhang <zijianzhang@...edance.com>
>> Signed-off-by: Xiaochun Lu <xiaochun.lu@...edance.com>
> 
> ...
> 
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index 521e6373d4f7..21239469d75c 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -2847,6 +2847,74 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>>   	case SCM_RIGHTS:
>>   	case SCM_CREDENTIALS:
>>   		break;
>> +	case SCM_ZC_NOTIFICATION: {
>> +		int ret, i = 0;
>> +		int cmsg_data_len, zc_info_elem_num;
>> +		void __user	*usr_addr;
>> +		struct zc_info_elem zc_info_kern[SOCK_ZC_INFO_MAX];
>> +		unsigned long flags;
>> +		struct sk_buff_head *q, local_q;
>> +		struct sk_buff *skb, *tmp;
>> +		struct sock_exterr_skb *serr;
> 
> Hi Zijian Zhang, Xiaochun Lu, all,
> 
> When compiling on ARM (32bit) with multi_v7_defconfig using clang-18
> I see the following warning:
> 
> .../sock.c:2808:5: warning: stack frame size (1664) exceeds limit (1024) in '__sock_cmsg_send' [-Wframe-larger-than]
>   2808 | int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
> 
> I expect this is mostly explained by the addition of zc_info_kern above.
> 

Nice catch, thanks for the info!

>> +
>> +		if (!sock_flag(sk, SOCK_ZEROCOPY) || sk->sk_family == PF_RDS)
>> +			return -EINVAL;
>> +
>> +		cmsg_data_len = cmsg->cmsg_len - sizeof(struct cmsghdr);
>> +		if (cmsg_data_len % sizeof(struct zc_info_elem))
>> +			return -EINVAL;
>> +
>> +		zc_info_elem_num = cmsg_data_len / sizeof(struct zc_info_elem);
>> +		if (!zc_info_elem_num || zc_info_elem_num > SOCK_ZC_INFO_MAX)
>> +			return -EINVAL;
>> +
>> +		if (in_compat_syscall())
>> +			usr_addr = compat_ptr(*(compat_uptr_t *)CMSG_DATA(cmsg));
>> +		else
>> +			usr_addr = (void __user *)*(void **)CMSG_DATA(cmsg);
>> +		if (!access_ok(usr_addr, cmsg_data_len))
>> +			return -EFAULT;
>> +
>> +		q = &sk->sk_error_queue;
>> +		skb_queue_head_init(&local_q);
>> +		spin_lock_irqsave(&q->lock, flags);
>> +		skb = skb_peek(q);
>> +		while (skb && i < zc_info_elem_num) {
>> +			struct sk_buff *skb_next = skb_peek_next(skb, q);
>> +
>> +			serr = SKB_EXT_ERR(skb);
>> +			if (serr->ee.ee_errno == 0 &&
>> +			    serr->ee.ee_origin == SO_EE_ORIGIN_ZEROCOPY) {
>> +				zc_info_kern[i].hi = serr->ee.ee_data;
>> +				zc_info_kern[i].lo = serr->ee.ee_info;
>> +				zc_info_kern[i].zerocopy = !(serr->ee.ee_code
>> +								& SO_EE_CODE_ZEROCOPY_COPIED);
>> +				__skb_unlink(skb, q);
>> +				__skb_queue_tail(&local_q, skb);
>> +				i++;
>> +			}
>> +			skb = skb_next;
>> +		}
>> +		spin_unlock_irqrestore(&q->lock, flags);
>> +
>> +		ret = copy_to_user(usr_addr,
>> +				   zc_info_kern,
>> +					i * sizeof(struct zc_info_elem));
>> +
>> +		if (unlikely(ret)) {
>> +			spin_lock_irqsave(&q->lock, flags);
>> +			skb_queue_reverse_walk_safe(&local_q, skb, tmp) {
>> +				__skb_unlink(skb, &local_q);
>> +				__skb_queue_head(q, skb);
>> +			}
>> +			spin_unlock_irqrestore(&q->lock, flags);
>> +			return -EFAULT;
>> +		}
>> +
>> +		while ((skb = __skb_dequeue(&local_q)))
>> +			consume_skb(skb);
>> +		break;
>> +	}
>>   	default:
>>   		return -EINVAL;
>>   	}
>> -- 
>> 2.20.1
>>
>>