[v5,net-next] net: Remove redundant calls of sk_tx_queue_clear().

Commit Message

Iwashima, Kuniyuki Jan. 28, 2021, 3:02 p.m. UTC

The commit 41b14fb8724d ("net: Do not clear the sock TX queue in
sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds
it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in
the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). On the
other hand, the original commit had already put sk_tx_queue_clear() in
sk_prot_alloc(): the callee of sk_alloc() and sk_clone_lock(). Thus
sk_tx_queue_clear() is called twice in each path.

If we remove sk_tx_queue_clear() in sk_alloc() and sk_clone_lock(), it
currently works well because (i) sk_tx_queue_mapping is defined between
sk_dontcopy_begin and sk_dontcopy_end, and (ii) sock_copy() called after
sk_prot_alloc() in sk_clone_lock() does not overwrite sk_tx_queue_mapping.
However, if we move sk_tx_queue_mapping out of the no copy area, it
introduces a bug unintentionally.

Therefore, this patch adds a compile-time check to take care of the order
of sock_copy() and sk_tx_queue_clear() and removes sk_tx_queue_clear() from
sk_prot_alloc() so that it does the only allocation and its callers
initialize fields.

CC: Tariq Toukan <[e-mail]>
CC: Boris Pismenny <[e-mail]>
Signed-off-by: Kuniyuki Iwashima <[e-mail]>
Acked-by: Tariq Toukan <[e-mail]>
---
v5:
* Move the changelog after the --- separator

v4: https://lore.kernel.org/netdev/[e-mail]/
* Fix typo in the changelog (runtime -> compile-time)

v3: https://lore.kernel.org/netdev/[e-mail]/
* Remove Fixes: tag
* Add BUILD_BUG_ON
* Remove sk_tx_queue_clear() from sk_prot_alloc()
  instead of sk_alloc() and sk_clone_lock()

v2: https://lore.kernel.org/netdev/[e-mail]/
* Remove Reviewed-by: tag

v1: https://lore.kernel.org/netdev/[e-mail]/

 net/core/sock.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

[e-mail] Jan. 30, 2021, 5:10 a.m. UTC | #1

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Fri, 29 Jan 2021 00:02:17 +0900 you wrote:
> The commit 41b14fb8724d ("net: Do not clear the sock TX queue in
> sk_set_socket()") removes sk_tx_queue_clear() from sk_set_socket() and adds
> it instead in sk_alloc() and sk_clone_lock() to fix an issue introduced in
> the commit e022f0b4a03f ("net: Introduce sk_tx_queue_mapping"). On the
> other hand, the original commit had already put sk_tx_queue_clear() in
> sk_prot_alloc(): the callee of sk_alloc() and sk_clone_lock(). Thus
> sk_tx_queue_clear() is called twice in each path.
> 
> [...]

Here is the summary with links:
  - [v5,net-next] net: Remove redundant calls of sk_tx_queue_clear().
    https://git.kernel.org/netdev/net-next/c/df610cd9163b

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Patch

diff --git a/net/core/sock.c b/net/core/sock.c
index bbcd4b97eddd..cfbd62a5e079 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1657,6 +1657,16 @@  static void sock_copy(struct sock *nsk, const struct sock *osk)
 #ifdef CONFIG_SECURITY_NETWORK
 	void *sptr = nsk->sk_security;
 #endif
+
+	/* If we move sk_tx_queue_mapping out of the private section,
+	 * we must check if sk_tx_queue_clear() is called after
+	 * sock_copy() in sk_clone_lock().
+	 */
+	BUILD_BUG_ON(offsetof(struct sock, sk_tx_queue_mapping) <
+		     offsetof(struct sock, sk_dontcopy_begin) ||
+		     offsetof(struct sock, sk_tx_queue_mapping) >=
+		     offsetof(struct sock, sk_dontcopy_end));
+
 	memcpy(nsk, osk, offsetof(struct sock, sk_dontcopy_begin));
 
 	memcpy(&nsk->sk_dontcopy_end, &osk->sk_dontcopy_end,
@@ -1690,7 +1700,6 @@  static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
 
 		if (!try_module_get(prot->owner))
 			goto out_free_sec;
-		sk_tx_queue_clear(sk);
 	}
 
 	return sk;