| Message ID | 20210128124229.78315-1-kuniyu@amazon.co.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v4,net-next] net: Remove redundant calls of sk_tx_queue_clear(). | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: keescook@chromium.org pabeni@redhat.com jakub@cloudflare.com daniel@iogearbox.net bjorn@kernel.org linmiaohe@huawei.com kafai@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 5 this patch: 5 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 23 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5 this patch: 5 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 1/28/2021 2:42 PM, Kuniyuki Iwashima 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. > > 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. > > v4: > * Fix typo in the changelog (runtime -> compile-time) > > v3: https://lore.kernel.org/netdev/20210128021905.57471-1-kuniyu@amazon.co.jp/ > * 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/20210127132215.10842-1-kuniyu@amazon.co.jp/ > * Remove Reviewed-by: tag > > v1: https://lore.kernel.org/netdev/20210127125018.7059-1-kuniyu@amazon.co.jp/ > Sorry for not pointing this out earlier, but shouldn't the changelog come after the --- separator? Unless you want it to appear as part of the commit message. Other than that, I think now I'm fine with the patch. Acked-by: Tariq Toukan <tariqt@nvidia.com> Thanks, Tariq > CC: Tariq Toukan <tariqt@mellanox.com> > CC: Boris Pismenny <borisp@mellanox.com> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > --- > net/core/sock.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > 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; >
From: Tariq Toukan <ttoukan.linux@gmail.com> Date: Thu, 28 Jan 2021 15:09:51 +0200 > On 1/28/2021 2:42 PM, Kuniyuki Iwashima 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. > > > > 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. > > > > v4: > > * Fix typo in the changelog (runtime -> compile-time) > > > > v3: https://lore.kernel.org/netdev/20210128021905.57471-1-kuniyu@amazon.co.jp/ > > * 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/20210127132215.10842-1-kuniyu@amazon.co.jp/ > > * Remove Reviewed-by: tag > > > > v1: https://lore.kernel.org/netdev/20210127125018.7059-1-kuniyu@amazon.co.jp/ > > > > Sorry for not pointing this out earlier, but shouldn't the changelog > come after the --- separator? Unless you want it to appear as part of > the commit message. > > Other than that, I think now I'm fine with the patch. > > Acked-by: Tariq Toukan <tariqt@nvidia.com> > > Thanks, > Tariq Oh, I didn't know that useful behaviour, thank you! I will respin with your Acked-by tag. > > CC: Tariq Toukan <tariqt@mellanox.com> > > CC: Boris Pismenny <borisp@mellanox.com> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> > > --- > > net/core/sock.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > 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; > >
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;
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. v4: * Fix typo in the changelog (runtime -> compile-time) v3: https://lore.kernel.org/netdev/20210128021905.57471-1-kuniyu@amazon.co.jp/ * 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/20210127132215.10842-1-kuniyu@amazon.co.jp/ * Remove Reviewed-by: tag v1: https://lore.kernel.org/netdev/20210127125018.7059-1-kuniyu@amazon.co.jp/ CC: Tariq Toukan <tariqt@mellanox.com> CC: Boris Pismenny <borisp@mellanox.com> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> --- net/core/sock.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)