diff mbox series

[v1,net-next] af_unix: Remove dead code in unix_stream_read_generic().

Message ID 20240529144648.68591-1-kuniyu@amazon.com (mailing list archive)
State Accepted
Commit b5c089880723b2c18531c40e445235bd646a51d1
Delegated to: Netdev Maintainers
Headers show
Series [v1,net-next] af_unix: Remove dead code in unix_stream_read_generic(). | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 908 this patch: 908
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 906 this patch: 906
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 912 this patch: 912
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 48 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-05-30--06-00 (tests: 1042)

Commit Message

Kuniyuki Iwashima May 29, 2024, 2:46 p.m. UTC 
When splice() support was added in commit 2b514574f7e8 ("net:
af_unix: implement splice for stream af_unix sockets"), we had
to release unix_sk(sk)->readlock (current iolock) before calling
splice_to_pipe().

Due to the unlock, commit 73ed5d25dce0 ("af-unix: fix use-after-free
with concurrent readers while splicing") added a safeguard in
unix_stream_read_generic(); we had to bump the skb refcount before
calling ->recv_actor() and then check if the skb was consumed by a
concurrent reader.

However, the pipe side locking was refactored, and since commit
25869262ef7a ("skb_splice_bits(): get rid of callback"), we can
call splice_to_pipe() without releasing unix_sk(sk)->iolock.

Now, the skb is always alive after the ->recv_actor() callback,
so let's remove the unnecessary drop_skb logic.

This is mostly the revert of commit 73ed5d25dce0 ("af-unix: fix
use-after-free with concurrent readers while splicing").

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

Comments

patchwork-bot+netdevbpf@kernel.org June 1, 2024, 11:40 p.m. UTC | #1 
Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 29 May 2024 07:46:48 -0700 you wrote:
> When splice() support was added in commit 2b514574f7e8 ("net:
> af_unix: implement splice for stream af_unix sockets"), we had
> to release unix_sk(sk)->readlock (current iolock) before calling
> splice_to_pipe().
> 
> Due to the unlock, commit 73ed5d25dce0 ("af-unix: fix use-after-free
> with concurrent readers while splicing") added a safeguard in
> unix_stream_read_generic(); we had to bump the skb refcount before
> calling ->recv_actor() and then check if the skb was consumed by a
> concurrent reader.
> 
> [...]

Here is the summary with links:
  - [v1,net-next] af_unix: Remove dead code in unix_stream_read_generic().
    https://git.kernel.org/netdev/net-next/c/b5c089880723

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 902148212ad3..f4cc8b9bf82d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -654,8 +654,8 @@  static void unix_release_sock(struct sock *sk, int embrion)
 	while ((skb = skb_dequeue(&sk->sk_receive_queue)) != NULL) {
 		if (state == TCP_LISTEN)
 			unix_release_sock(skb->sk, 1);
+
 		/* passed fds are erased in the kfree_skb hook	      */
-		UNIXCB(skb).consumed = skb->len;
 		kfree_skb(skb);
 	}
 
@@ -2704,9 +2704,8 @@  static int unix_stream_read_generic(struct unix_stream_read_state *state,
 	skip = max(sk_peek_offset(sk, flags), 0);
 
 	do {
-		int chunk;
-		bool drop_skb;
 		struct sk_buff *skb, *last;
+		int chunk;
 
 redo:
 		unix_state_lock(sk);
@@ -2802,11 +2801,7 @@  static int unix_stream_read_generic(struct unix_stream_read_state *state,
 		}
 
 		chunk = min_t(unsigned int, unix_skb_len(skb) - skip, size);
-		skb_get(skb);
 		chunk = state->recv_actor(skb, skip, chunk, state);
-		drop_skb = !unix_skb_len(skb);
-		/* skb is only safe to use if !drop_skb */
-		consume_skb(skb);
 		if (chunk < 0) {
 			if (copied == 0)
 				copied = -EFAULT;
@@ -2815,18 +2810,6 @@  static int unix_stream_read_generic(struct unix_stream_read_state *state,
 		copied += chunk;
 		size -= chunk;
 
-		if (drop_skb) {
-			/* the skb was touched by a concurrent reader;
-			 * we should not expect anything from this skb
-			 * anymore and assume it invalid - we can be
-			 * sure it was dropped from the socket queue
-			 *
-			 * let's report a short read
-			 */
-			err = 0;
-			break;
-		}
-
 		/* Mark read part of skb as used */
 		if (!(flags & MSG_PEEK)) {
 			UNIXCB(skb).consumed += chunk;