6.1-stable review patch. If anyone has any objections, please let me know.
------------------
From: Kuniyuki Iwashima kuniyu@amazon.com
[ Upstream commit 8594d9b85c07f05e431bd07e895c2a3ad9b85d6f ]
Since introduced, OOB skb holds an additional reference count with no special reason and caused many issues.
Also, kfree_skb() and consume_skb() are used to decrement the count, which is confusing.
Let's drop the unnecessary skb_get() in queue_oob() and corresponding kfree_skb(), consume_skb(), and skb_unref().
Now unix_sk(sk)->oob_skb is just a pointer to skb in the receive queue, so special handing is no longer needed in GC.
Signed-off-by: Kuniyuki Iwashima kuniyu@amazon.com Link: https://patch.msgid.link/20240816233921.57800-1-kuniyu@amazon.com Signed-off-by: Jakub Kicinski kuba@kernel.org Stable-dep-of: 32ca245464e1 ("af_unix: Don't leave consecutive consumed OOB skbs.") Signed-off-by: Sasha Levin sashal@kernel.org --- net/unix/af_unix.c | 27 +++++---------------------- net/unix/garbage.c | 24 +++--------------------- 2 files changed, 8 insertions(+), 43 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 79b783a70c87d..9ef6011a055b1 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -604,10 +604,7 @@ static void unix_release_sock(struct sock *sk, int embrion) unix_state_unlock(sk);
#if IS_ENABLED(CONFIG_AF_UNIX_OOB) - if (u->oob_skb) { - kfree_skb(u->oob_skb); - u->oob_skb = NULL; - } + u->oob_skb = NULL; #endif
wake_up_interruptible_all(&u->peer_wait); @@ -2133,13 +2130,9 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other }
maybe_add_creds(skb, sock, other); - skb_get(skb); - scm_stat_add(other, skb);
spin_lock(&other->sk_receive_queue.lock); - if (ousk->oob_skb) - consume_skb(ousk->oob_skb); WRITE_ONCE(ousk->oob_skb, skb); __skb_queue_tail(&other->sk_receive_queue, skb); spin_unlock(&other->sk_receive_queue.lock); @@ -2640,8 +2633,6 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state)
if (!(state->flags & MSG_PEEK)) WRITE_ONCE(u->oob_skb, NULL); - else - skb_get(oob_skb);
spin_unlock(&sk->sk_receive_queue.lock); unix_state_unlock(sk); @@ -2651,8 +2642,6 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state) if (!(state->flags & MSG_PEEK)) UNIXCB(oob_skb).consumed += 1;
- consume_skb(oob_skb); - mutex_unlock(&u->iolock);
if (chunk < 0) @@ -2680,12 +2669,10 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, if (copied) { skb = NULL; } else if (!(flags & MSG_PEEK)) { - if (sock_flag(sk, SOCK_URGINLINE)) { - WRITE_ONCE(u->oob_skb, NULL); - consume_skb(skb); - } else { + WRITE_ONCE(u->oob_skb, NULL); + + if (!sock_flag(sk, SOCK_URGINLINE)) { __skb_unlink(skb, &sk->sk_receive_queue); - WRITE_ONCE(u->oob_skb, NULL); unlinked_skb = skb; skb = skb_peek(&sk->sk_receive_queue); } @@ -2696,10 +2683,7 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
spin_unlock(&sk->sk_receive_queue.lock);
- if (unlinked_skb) { - WARN_ON_ONCE(skb_unref(unlinked_skb)); - kfree_skb(unlinked_skb); - } + kfree_skb(unlinked_skb); } return skb; } @@ -2742,7 +2726,6 @@ static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor) unix_state_unlock(sk);
if (drop) { - WARN_ON_ONCE(skb_unref(skb)); kfree_skb(skb); return -EAGAIN; } diff --git a/net/unix/garbage.c b/net/unix/garbage.c index 23efb78fe9ef4..0068e758be4dd 100644 --- a/net/unix/garbage.c +++ b/net/unix/garbage.c @@ -337,23 +337,6 @@ static bool unix_vertex_dead(struct unix_vertex *vertex) return true; }
-enum unix_recv_queue_lock_class { - U_RECVQ_LOCK_NORMAL, - U_RECVQ_LOCK_EMBRYO, -}; - -static void unix_collect_queue(struct unix_sock *u, struct sk_buff_head *hitlist) -{ - skb_queue_splice_init(&u->sk.sk_receive_queue, hitlist); - -#if IS_ENABLED(CONFIG_AF_UNIX_OOB) - if (u->oob_skb) { - WARN_ON_ONCE(skb_unref(u->oob_skb)); - u->oob_skb = NULL; - } -#endif -} - static void unix_collect_skb(struct list_head *scc, struct sk_buff_head *hitlist) { struct unix_vertex *vertex; @@ -375,13 +358,12 @@ static void unix_collect_skb(struct list_head *scc, struct sk_buff_head *hitlist skb_queue_walk(queue, skb) { struct sk_buff_head *embryo_queue = &skb->sk->sk_receive_queue;
- /* listener -> embryo order, the inversion never happens. */ - spin_lock_nested(&embryo_queue->lock, U_RECVQ_LOCK_EMBRYO); - unix_collect_queue(unix_sk(skb->sk), hitlist); + spin_lock(&embryo_queue->lock); + skb_queue_splice_init(embryo_queue, hitlist); spin_unlock(&embryo_queue->lock); } } else { - unix_collect_queue(u, hitlist); + skb_queue_splice_init(queue, hitlist); }
spin_unlock(&queue->lock);