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] [thread-next>] [day] [month] [year] [list]
Message-ID: <8e98eb7f-5263-48ca-86be-dfc43c135385@bytedance.com>
Date: Mon, 1 Jul 2024 12:58:25 -0700
From: Zijian Zhang <zijianzhang@...edance.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, netdev@...r.kernel.org
Cc: edumazet@...gle.com, cong.wang@...edance.com, xiaochun.lu@...edance.com
Subject: Re: [PATCH net-next v6 3/4] sock: add MSG_ZEROCOPY notification
 mechanism based on msg_control

On 6/30/24 7:44 AM, Willem de Bruijn wrote:
> zijianzhang@ 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>
>> ---
>>   arch/alpha/include/uapi/asm/socket.h  |  2 ++
>>   arch/mips/include/uapi/asm/socket.h   |  2 ++
>>   arch/parisc/include/uapi/asm/socket.h |  2 ++
>>   arch/sparc/include/uapi/asm/socket.h  |  2 ++
>>   include/linux/socket.h                |  2 +-
>>   include/uapi/asm-generic/socket.h     |  2 ++
>>   include/uapi/linux/socket.h           | 10 +++++++
>>   net/core/sock.c                       | 42 +++++++++++++++++++++++++++
>>   8 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
>> index e94f621903fe..7761a4e0ea2c 100644
>> --- a/arch/alpha/include/uapi/asm/socket.h
>> +++ b/arch/alpha/include/uapi/asm/socket.h
>> @@ -140,6 +140,8 @@
>>   #define SO_PASSPIDFD		76
>>   #define SO_PEERPIDFD		77
>>   
>> +#define SCM_ZC_NOTIFICATION 78
>> +
>>   #if !defined(__KERNEL__)
>>   
>>   #if __BITS_PER_LONG == 64
>> diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h
>> index 60ebaed28a4c..89edc51380f0 100644
>> --- a/arch/mips/include/uapi/asm/socket.h
>> +++ b/arch/mips/include/uapi/asm/socket.h
>> @@ -151,6 +151,8 @@
>>   #define SO_PASSPIDFD		76
>>   #define SO_PEERPIDFD		77
>>   
>> +#define SCM_ZC_NOTIFICATION 78
>> +
>>   #if !defined(__KERNEL__)
>>   
>>   #if __BITS_PER_LONG == 64
>> diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h
>> index be264c2b1a11..2911b43e6a9d 100644
>> --- a/arch/parisc/include/uapi/asm/socket.h
>> +++ b/arch/parisc/include/uapi/asm/socket.h
>> @@ -132,6 +132,8 @@
>>   #define SO_PASSPIDFD		0x404A
>>   #define SO_PEERPIDFD		0x404B
>>   
>> +#define SCM_ZC_NOTIFICATION 0x404C
>> +
>>   #if !defined(__KERNEL__)
>>   
>>   #if __BITS_PER_LONG == 64
>> diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h
>> index 682da3714686..dc045e87cc8e 100644
>> --- a/arch/sparc/include/uapi/asm/socket.h
>> +++ b/arch/sparc/include/uapi/asm/socket.h
>> @@ -133,6 +133,8 @@
>>   #define SO_PASSPIDFD             0x0055
>>   #define SO_PEERPIDFD             0x0056
>>   
>> +#define SCM_ZC_NOTIFICATION 0x0057
>> +
>>   #if !defined(__KERNEL__)
>>   
>>   
>> diff --git a/include/linux/socket.h b/include/linux/socket.h
>> index 35adc30c9db6..f2f013166525 100644
>> --- a/include/linux/socket.h
>> +++ b/include/linux/socket.h
>> @@ -170,7 +170,7 @@ static inline struct cmsghdr * cmsg_nxthdr (struct msghdr *__msg, struct cmsghdr
>>   
>>   static inline bool cmsg_copy_to_user(struct cmsghdr *__cmsg)
>>   {
>> -	return 0;
>> +	return __cmsg->cmsg_type == SCM_ZC_NOTIFICATION;
>>   }
>>   
>>   static inline size_t msg_data_left(struct msghdr *msg)
>> diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
>> index 8ce8a39a1e5f..7474c8a244bc 100644
>> --- a/include/uapi/asm-generic/socket.h
>> +++ b/include/uapi/asm-generic/socket.h
>> @@ -135,6 +135,8 @@
>>   #define SO_PASSPIDFD		76
>>   #define SO_PEERPIDFD		77
>>   
>> +#define SCM_ZC_NOTIFICATION 78
>> +
>>   #if !defined(__KERNEL__)
>>   
>>   #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
>> diff --git a/include/uapi/linux/socket.h b/include/uapi/linux/socket.h
>> index d3fcd3b5ec53..26bee6291c6c 100644
>> --- a/include/uapi/linux/socket.h
>> +++ b/include/uapi/linux/socket.h
>> @@ -2,6 +2,8 @@
>>   #ifndef _UAPI_LINUX_SOCKET_H
>>   #define _UAPI_LINUX_SOCKET_H
>>   
>> +#include <linux/types.h>
>> +
>>   /*
>>    * Desired design of maximum size and alignment (see RFC2553)
>>    */
>> @@ -35,4 +37,12 @@ struct __kernel_sockaddr_storage {
>>   #define SOCK_TXREHASH_DISABLED	0
>>   #define SOCK_TXREHASH_ENABLED	1
>>   
>> +#define SOCK_ZC_INFO_MAX 16
>> +
>> +struct zc_info_elem {
>> +	__u32 lo;
>> +	__u32 hi;
>> +	__u8 zerocopy;
>> +};
>> +
>>   #endif /* _UAPI_LINUX_SOCKET_H */
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index 4a766a91ff5c..1b2ce72e1338 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -2863,6 +2863,48 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>>   	case SCM_RIGHTS:
>>   	case SCM_CREDENTIALS:
>>   		break;
>> +	case SCM_ZC_NOTIFICATION: {
>> +		struct zc_info_elem *zc_info_kern = CMSG_DATA(cmsg);
>> +		int cmsg_data_len, zc_info_elem_num;
>> +		struct sock_exterr_skb *serr;
>> +		struct sk_buff_head *q;
>> +		unsigned long flags;
>> +		struct sk_buff *skb;
>> +		int i = 0;
>> +
>> +		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;
>> +
>> +		q = &sk->sk_error_queue;
>> +		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);
>> +				consume_skb(skb);
>> +			}
>> +			skb = skb_next;
>> +			i++;
>> +		}
> 
> How will userspace know the number of entries written? Since the
> cmsg_len is not updated, is the expectation that the CMSG_DATA is zero
> initialized by the user and the list will be zero-element terminated?
> 

Since zero-element is a valid value(the first sendmsg with non-zerocopy)
in our case, zero-element terminated might not be feasible.

In current selftest, the user initializes zerocopy field to a invalid
value, and upon returning iterate the array until the invalid value. It
is very dependent to the user, I think it's not good enough.

I agree that updating cmsg_len is better.

>> +		spin_unlock_irqrestore(&q->lock, flags);
>> +		break;
>> +	}
>>   	default:
>>   		return -EINVAL;
>>   	}
>> -- 
>> 2.20.1
>>
> 
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ