| 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: <0d5145b231d9b7c8d2f32277ce5ab56bb1859bff.camel@redhat.com>
Date: Mon, 27 Nov 2023 10:59:03 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>, "David S. Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
<kuba@...nel.org>
Cc: Ivan Babrou <ivan@...udflare.com>, Kuniyuki Iwashima
<kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 4/4] af_unix: Try to run GC async.
On Wed, 2023-11-22 at 17:47 -0800, Kuniyuki Iwashima wrote:
> If more than 16000 inflight AF_UNIX sockets exist and the garbage
> collector is not running, unix_(dgram|stream)_sendmsg() call unix_gc().
> Also, they wait for unix_gc() to complete.
>
> In unix_gc(), all inflight AF_UNIX sockets are traversed at least once,
> and more if they are the GC candidate. Thus, sendmsg() significantly
> slows down with too many inflight AF_UNIX sockets.
>
> However, if a process sends data with no AF_UNIX FD, the sendmsg() call
> does not need to wait for GC. After this change, only the process that
> meets the condition below will be blocked under such a situation.
>
> 1) cmsg contains AF_UNIX socket
> 2) more than 32 AF_UNIX sent by the same user are still inflight
>
> Note that even a sendmsg() call that does not meet the condition but has
> AF_UNIX FD will be blocked later in unix_scm_to_skb() by the spinlock,
> but we allow that as a bonus for sane users.
>
> The results below are the time spent in unix_dgram_sendmsg() sending 1
> byte of data with no FD 4096 times on a host where 32K inflight AF_UNIX
> sockets exist.
>
> Without series: the sane sendmsg() needs to wait gc unreasonably.
>
> $ sudo /usr/share/bcc/tools/funclatency -p 11165 unix_dgram_sendmsg
> Tracing 1 functions for "unix_dgram_sendmsg"... Hit Ctrl-C to end.
> ^C
> nsecs : count distribution
> [...]
> 524288 -> 1048575 : 0 | |
> 1048576 -> 2097151 : 3881 |****************************************|
> 2097152 -> 4194303 : 214 |** |
> 4194304 -> 8388607 : 1 | |
>
> avg = 1825567 nsecs, total: 7477526027 nsecs, count: 4096
>
> With series: the sane sendmsg() can finish much faster.
>
> $ sudo /usr/share/bcc/tools/funclatency -p 8702 unix_dgram_sendmsg
> Tracing 1 functions for "unix_dgram_sendmsg"... Hit Ctrl-C to end.
> ^C
> nsecs : count distribution
> [...]
> 128 -> 255 : 0 | |
> 256 -> 511 : 4092 |****************************************|
> 512 -> 1023 : 2 | |
> 1024 -> 2047 : 0 | |
> 2048 -> 4095 : 0 | |
> 4096 -> 8191 : 1 | |
> 8192 -> 16383 : 1 | |
>
> avg = 410 nsecs, total: 1680510 nsecs, count: 4096
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
> include/net/af_unix.h | 2 +-
> include/net/scm.h | 1 +
> net/core/scm.c | 5 +++++
> net/unix/af_unix.c | 6 ++++--
> net/unix/garbage.c | 10 ++++++++--
> 5 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index c628d30ceb19..f8e654d418e6 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -13,7 +13,7 @@ void unix_notinflight(struct user_struct *user, struct file *fp);
> void unix_destruct_scm(struct sk_buff *skb);
> void io_uring_destruct_scm(struct sk_buff *skb);
> void unix_gc(void);
> -void wait_for_unix_gc(void);
> +void wait_for_unix_gc(struct scm_fp_list *fpl);
> struct unix_sock *unix_get_socket(struct file *filp);
> struct sock *unix_peer_get(struct sock *sk);
>
> diff --git a/include/net/scm.h b/include/net/scm.h
> index e8c76b4be2fe..1ff6a2855064 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -24,6 +24,7 @@ struct scm_creds {
>
> struct scm_fp_list {
> short count;
> + short count_unix;
> short max;
> struct user_struct *user;
> struct file *fp[SCM_MAX_FD];
> diff --git a/net/core/scm.c b/net/core/scm.c
> index 880027ecf516..c1aae77d120b 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -35,6 +35,7 @@
> #include <net/compat.h>
> #include <net/scm.h>
> #include <net/cls_cgroup.h>
> +#include <net/af_unix.h>
>
>
> /*
> @@ -105,6 +106,10 @@ static int scm_fp_copy(struct cmsghdr *cmsg, struct scm_fp_list **fplp)
> return -EBADF;
> *fpp++ = file;
> fpl->count++;
> +#if IS_ENABLED(CONFIG_UNIX)
> + if (unix_get_socket(file))
> + fpl->count_unix++;
> +#endif
> }
>
> if (!fpl->user)
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 1e6f5aaf1cc9..bbad3959751d 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1925,11 +1925,12 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> long timeo;
> int err;
>
> - wait_for_unix_gc();
> err = scm_send(sock, msg, &scm, false);
> if (err < 0)
> return err;
>
> + wait_for_unix_gc(scm.fp);
> +
> err = -EOPNOTSUPP;
> if (msg->msg_flags&MSG_OOB)
> goto out;
> @@ -2201,11 +2202,12 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
> bool fds_sent = false;
> int data_len;
>
> - wait_for_unix_gc();
> err = scm_send(sock, msg, &scm, false);
> if (err < 0)
> return err;
>
> + wait_for_unix_gc(scm.fp);
> +
> err = -EOPNOTSUPP;
> if (msg->msg_flags & MSG_OOB) {
> #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> index 8bc93a7e745f..73091d6b7fc4 100644
> --- a/net/unix/garbage.c
> +++ b/net/unix/garbage.c
> @@ -184,8 +184,9 @@ static void inc_inflight_move_tail(struct unix_sock *u)
> }
>
> #define UNIX_INFLIGHT_TRIGGER_GC 16000
> +#define UNIX_INFLIGHT_SANE_USER 32
I don't have any relevant usage stats for unix sockets, but out of
sheer ignorance on my side '32' looks a bit low. Why/how did you pick
such value?
> -void wait_for_unix_gc(void)
> +void wait_for_unix_gc(struct scm_fp_list *fpl)
> {
> /* If number of inflight sockets is insane, kick a
> * garbage collect right now.
> @@ -195,7 +196,12 @@ void wait_for_unix_gc(void)
> if (READ_ONCE(unix_tot_inflight) > UNIX_INFLIGHT_TRIGGER_GC)
> queue_work(system_unbound_wq, &unix_gc_work);
>
> - flush_work(&unix_gc_work);
> + /* Penalise users who want to send AF_UNIX sockets
> + * but whose sockets have not been received yet.
> + */
> + if (fpl && fpl->count_unix &&
> + READ_ONCE(fpl->user->unix_inflight) > UNIX_INFLIGHT_SANE_USER)
> + flush_work(&unix_gc_work);
flush_work() will be called even when 'unix_tot_inflight' is (much)
less then 'UNIX_INFLIGHT_TRIGGER_GC'. Could that cause some regressions
for workload with moderated numbers of fd in flights, where the GC was
never triggered before this series?
Thanks!
Paolo
Powered by blists - more mailing lists