| 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
| ||
|
Message-ID: <b2dd03f7-34de-4a56-a727-8ec2effa2288@bytedance.com>
Date: Thu, 25 Jul 2024 17:01:49 -0700
From: Zijian Zhang <zijianzhang@...edance.com>
To: Mina Almasry <almasrymina@...gle.com>
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 v7 2/3] sock: add MSG_ZEROCOPY
notification mechanism based on msg_control
On 7/25/24 2:59 PM, Mina Almasry wrote:
> On Mon, Jul 08, 2024 at 09:04:04PM +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. We try to mitigate this overhead
>> with a new notification mechanism based on msg_control. Leveraging the
>> general framework to copy cmsgs to the user space, we copy zerocopy
>> notifications to the user upon returning of sendmsgs.
>>
>> 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 | 13 ++++++++
>> net/core/sock.c | 46 +++++++++++++++++++++++++++
>> 8 files changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
>> index e94f621903fe..7c32d9dbe47f 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..3f7fade998cb 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..77f5bee0fdc9 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..eb44fc515b45 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 75461812a7a3..6f1b791e2de8 100644
>> --- a/include/linux/socket.h
>> +++ b/include/linux/socket.h
>> @@ -171,7 +171,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..02e9159c7944 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..ab361f30f3a6 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,15 @@ struct __kernel_sockaddr_storage {
>> #define SOCK_TXREHASH_DISABLED 0
>> #define SOCK_TXREHASH_ENABLED 1
>>
>> +struct zc_info_elem {
>> + __u32 lo;
>> + __u32 hi;
>> + __u8 zerocopy;
>
> Some docs please on what each of these are, if possible. Sorry if the repeated
> requests are annoying.
>
> In particular I'm a bit confused why the zerocopy field is there. Looking at
> the code, is this always set to 1?
>
```
hi = serr->ee_data;
lo = serr->ee_info;
zerocopy = !(serr->ee_code & SO_EE_CODE_ZEROCOPY_COPIED);
```
In the original method, the above code means one notification for
sendmsg id [lo, hi], with zerocopy=n/y to denote if the zerocopy is
reverted back to copy.
So the zerocopy field aligns the same meaning of
!(serr->ee_code & SO_EE_CODE_ZEROCOPY_COPIED) in the original method.
Sorry for the confusion, I will add more docs to explain it.
>> +};
>> +
>> +struct zc_info {
>> + __u32 size;
>> + struct zc_info_elem arr[];
>> +};
>> +
>> #endif /* _UAPI_LINUX_SOCKET_H */
>> diff --git a/net/core/sock.c b/net/core/sock.c
>> index efb30668dac3..e0b5162233d3 100644
>> --- a/net/core/sock.c
>> +++ b/net/core/sock.c
>> @@ -2863,6 +2863,52 @@ int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
>> case SCM_RIGHTS:
>> case SCM_CREDENTIALS:
>> break;
>> + case SCM_ZC_NOTIFICATION: {
>> + struct zc_info *zc_info = CMSG_DATA(cmsg);
>> + struct zc_info_elem *zc_info_arr;
>> + struct sock_exterr_skb *serr;
>> + int cmsg_data_len, i = 0;
>> + struct sk_buff_head *q;
>> + unsigned long flags;
>> + struct sk_buff *skb;
>> + u32 zc_info_size;
>> +
>> + 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))
>> + return -EINVAL;
>> +
>> + zc_info_size = zc_info->size;
>> + zc_info_arr = zc_info->arr;
>
> Annoying nit: To be honest zc_info->size isn't much longer to type than
> zc_info_size, so I would have not added local variables.
>
Agree, nice catch!
>> + if (cmsg_data_len != sizeof(struct zc_info) +
>> + zc_info_size * sizeof(struct zc_info_elem))
>> + return -EINVAL;
>> +
>> + q = &sk->sk_error_queue;
>> + spin_lock_irqsave(&q->lock, flags);
>> + skb = skb_peek(q);
>> + while (skb && i < zc_info_size) {
>> + 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_arr[i].hi = serr->ee.ee_data;
>> + zc_info_arr[i].lo = serr->ee.ee_info;
>> + zc_info_arr[i].zerocopy = !(serr->ee.ee_code
>> + & SO_EE_CODE_ZEROCOPY_COPIED);
>> + __skb_unlink(skb, q);
>> + consume_skb(skb);
>> + i++;
>> + }
>> + skb = skb_next;
>> + }
>> + spin_unlock_irqrestore(&q->lock, flags);
>
> I wonder if you should drop the spin lock in the middle of this loop somehow,
> otherwise you may end up spinning for a very long time while the spinlock held
> and irq disabled.
>
> IIRC zc_info_size is user input, right? Maybe you should limit zc_info_size to
> 16 entries or something. So the user doesn't end up passing 100000 as
> zc_info_size and making the kernel loop for a long time here.
>
Thanks for the suggestion, totally agree, I should limit the
zc_info_size.
>> + zc_info->size = i;
>> + break;
>> + }
>> default:
>> return -EINVAL;
>> }
>> --
>> 2.20.1
>>
Powered by blists - more mailing lists