Skip to content

Commit

Permalink
af_unix: Try not to hold unix_gc_lock during accept().
Browse files Browse the repository at this point in the history
Commit dcf70df ("af_unix: Fix up unix_edge.successor for embryo
socket.") added spin_lock(&unix_gc_lock) in accept() path, and it
caused regression in a stress test as reported by kernel test robot.

If the embryo socket is not part of the inflight graph, we need not
hold the lock.

To decide that in O(1) time and avoid the regression in the normal
use case,

  1. add a new stat unix_sk(sk)->scm_stat.nr_unix_fds

  2. count the number of inflight AF_UNIX sockets in the receive
     queue under unix_state_lock()

  3. move unix_update_edges() call under unix_state_lock()

  4. avoid locking if nr_unix_fds is 0 in unix_update_edges()

Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202404101427.92a08551-oliver.sang@intel.com
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Link: https://lore.kernel.org/r/20240413021928.20946-1-kuniyu@amazon.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
  • Loading branch information
Kuniyuki Iwashima authored and Paolo Abeni committed Apr 16, 2024
1 parent e918c7b commit fd86344
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 5 deletions.
1 change: 1 addition & 0 deletions include/net/af_unix.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ struct unix_skb_parms {

struct scm_stat {
atomic_t nr_fds;
unsigned long nr_unix_fds;
};

#define UNIXCB(skb) (*(struct unix_skb_parms *)&((skb)->cb))
Expand Down
2 changes: 1 addition & 1 deletion net/unix/af_unix.c
Original file line number Diff line number Diff line change
Expand Up @@ -1719,12 +1719,12 @@ static int unix_accept(struct socket *sock, struct socket *newsock, int flags,
}

tsk = skb->sk;
unix_update_edges(unix_sk(tsk));
skb_free_datagram(sk, skb);
wake_up_interruptible(&unix_sk(sk)->peer_wait);

/* attach accepted sock to socket */
unix_state_lock(tsk);
unix_update_edges(unix_sk(tsk));
newsock->state = SS_CONNECTED;
unix_sock_inherit_flags(sock, newsock);
sock_graft(tsk, newsock);
Expand Down
20 changes: 16 additions & 4 deletions net/unix/garbage.c
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver)
unix_add_edge(fpl, edge);
} while (i < fpl->count_unix);

receiver->scm_stat.nr_unix_fds += fpl->count_unix;
WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + fpl->count_unix);
out:
WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight + fpl->count);
Expand All @@ -222,6 +223,7 @@ void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver)

void unix_del_edges(struct scm_fp_list *fpl)
{
struct unix_sock *receiver;
int i = 0;

spin_lock(&unix_gc_lock);
Expand All @@ -235,6 +237,8 @@ void unix_del_edges(struct scm_fp_list *fpl)
unix_del_edge(fpl, edge);
} while (i < fpl->count_unix);

receiver = fpl->edges[0].successor;
receiver->scm_stat.nr_unix_fds -= fpl->count_unix;
WRITE_ONCE(unix_tot_inflight, unix_tot_inflight - fpl->count_unix);
out:
WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight - fpl->count);
Expand All @@ -246,10 +250,18 @@ void unix_del_edges(struct scm_fp_list *fpl)

void unix_update_edges(struct unix_sock *receiver)
{
spin_lock(&unix_gc_lock);
unix_update_graph(unix_sk(receiver->listener)->vertex);
receiver->listener = NULL;
spin_unlock(&unix_gc_lock);
/* nr_unix_fds is only updated under unix_state_lock().
* If it's 0 here, the embryo socket is not part of the
* inflight graph, and GC will not see it, so no lock needed.
*/
if (!receiver->scm_stat.nr_unix_fds) {
receiver->listener = NULL;
} else {
spin_lock(&unix_gc_lock);
unix_update_graph(unix_sk(receiver->listener)->vertex);
receiver->listener = NULL;
spin_unlock(&unix_gc_lock);
}
}

int unix_prepare_fpl(struct scm_fp_list *fpl)
Expand Down

0 comments on commit fd86344

Please sign in to comment.