* [PATCH v3 net-next] net: Remove redundant calls of sk_tx_queue_clear().
@ 2021-01-28 2:19 Kuniyuki Iwashima
2021-01-28 11:07 ` Tariq Toukan
0 siblings, 1 reply; 3+ messages in thread
From: Kuniyuki Iwashima @ 2021-01-28 2:19 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Eric Dumazet
Cc: Amit Shah, Kuniyuki Iwashima, Kuniyuki Iwashima, netdev,
linux-kernel, Tariq Toukan, Boris Pismenny
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 runtime 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.
v3:
* 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(-)
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;
--
2.17.2 (Apple Git-113)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3 net-next] net: Remove redundant calls of sk_tx_queue_clear().
2021-01-28 2:19 [PATCH v3 net-next] net: Remove redundant calls of sk_tx_queue_clear() Kuniyuki Iwashima
@ 2021-01-28 11:07 ` Tariq Toukan
2021-01-28 12:23 ` Kuniyuki Iwashima
0 siblings, 1 reply; 3+ messages in thread
From: Tariq Toukan @ 2021-01-28 11:07 UTC (permalink / raw)
To: Kuniyuki Iwashima, David S . Miller, Jakub Kicinski, Eric Dumazet
Cc: Amit Shah, Kuniyuki Iwashima, netdev, linux-kernel, Tariq Toukan,
Boris Pismenny
On 1/28/2021 4:19 AM, 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 runtime
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.
>
> v3:
> * 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(-)
>
> 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;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3 net-next] net: Remove redundant calls of sk_tx_queue_clear().
2021-01-28 11:07 ` Tariq Toukan
@ 2021-01-28 12:23 ` Kuniyuki Iwashima
0 siblings, 0 replies; 3+ messages in thread
From: Kuniyuki Iwashima @ 2021-01-28 12:23 UTC (permalink / raw)
To: ttoukan.linux
Cc: aams, borisp, davem, edumazet, kuba, kuni1840, kuniyu,
linux-kernel, netdev, tariqt
From: Tariq Toukan <ttoukan.linux@gmail.com>
Date: Thu, 28 Jan 2021 13:07:26 +0200
> On 1/28/2021 4:19 AM, 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 runtime
>
> compile-time
Oh, shame on me...
I'll fix it in the next spin.
Thank you,
Kuniyuki
> > 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.
> >
> > v3:
> > * 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(-)
> >
> > 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;
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-28 12:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-28 2:19 [PATCH v3 net-next] net: Remove redundant calls of sk_tx_queue_clear() Kuniyuki Iwashima
2021-01-28 11:07 ` Tariq Toukan
2021-01-28 12:23 ` Kuniyuki Iwashima
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).