| 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
| ||
|
Date: Mon, 27 Nov 2023 15:00:03 -0800
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <pabeni@...hat.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <ivan@...udflare.com>,
<kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.com>,
<netdev@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 4/4] af_unix: Try to run GC async.
From: Paolo Abeni <pabeni@...hat.com>
Date: Mon, 27 Nov 2023 10:59:03 +0100
> 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?
My take was that the peer should receive the fds in timely manner so that
no one will be punished, but I admit 32 is small enough, which can be
reached by a single SCM_RIGHTS (SCM_MAX_FD == 253) cmsg. So, probably we
can bump it up to 1024 or 2048 (> (4 or 8) * SCM_MAX_FD).
> > -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?
Ah exactly, I'll add work_pending() in v3.
if (!fpl || !fpl->count_unix)
return
if (work_pending(&unix_gc_work) &&
READ_ONCE(fpl->user->unix_inflight) > UNIX_INFLIGHT_SANE_USER)
flush_work(&unix_gc_work)
Thanks!
Powered by blists - more mailing lists