* [PATCH v2 net-next] inet: add READ_ONCE(sk->sk_bound_dev_if) in INET_MATCH()
@ 2022-05-12 16:56 Eric Dumazet
2022-05-12 17:02 ` Oliver Hartkopp
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2022-05-12 16:56 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: netdev, Eric Dumazet, Eric Dumazet
From: Eric Dumazet <edumazet@google.com>
INET_MATCH() runs without holding a lock on the socket.
We probably need to annotate most reads.
This patch makes INET_MATCH() an inline function
to ease our changes.
v2:
We remove the 32bit version of it, as modern compilers
should generate the same code really, no need to
try to be smarter.
Also make 'struct net *net' the first argument.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Sent as a standalone patch to not spam netdev@ list.
include/net/inet_hashtables.h | 33 +++++++++++++++------------------
include/net/sock.h | 3 ---
net/ipv4/inet_hashtables.c | 15 +++++----------
net/ipv4/udp.c | 3 +--
4 files changed, 21 insertions(+), 33 deletions(-)
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 98e1ec1a14f0382d1f4f8e85fe5ac2a056d2d6bc..e44e410813d0f469131f54cf3372458a0340d5cf 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -295,7 +295,6 @@ static inline struct sock *inet_lookup_listener(struct net *net,
((__force __portpair)(((__u32)(__dport) << 16) | (__force __u32)(__be16)(__sport)))
#endif
-#if (BITS_PER_LONG == 64)
#ifdef __BIG_ENDIAN
#define INET_ADDR_COOKIE(__name, __saddr, __daddr) \
const __addrpair __name = (__force __addrpair) ( \
@@ -307,24 +306,22 @@ static inline struct sock *inet_lookup_listener(struct net *net,
(((__force __u64)(__be32)(__daddr)) << 32) | \
((__force __u64)(__be32)(__saddr)))
#endif /* __BIG_ENDIAN */
-#define INET_MATCH(__sk, __net, __cookie, __saddr, __daddr, __ports, __dif, __sdif) \
- (((__sk)->sk_portpair == (__ports)) && \
- ((__sk)->sk_addrpair == (__cookie)) && \
- (((__sk)->sk_bound_dev_if == (__dif)) || \
- ((__sk)->sk_bound_dev_if == (__sdif))) && \
- net_eq(sock_net(__sk), (__net)))
-#else /* 32-bit arch */
-#define INET_ADDR_COOKIE(__name, __saddr, __daddr) \
- const int __name __deprecated __attribute__((unused))
-#define INET_MATCH(__sk, __net, __cookie, __saddr, __daddr, __ports, __dif, __sdif) \
- (((__sk)->sk_portpair == (__ports)) && \
- ((__sk)->sk_daddr == (__saddr)) && \
- ((__sk)->sk_rcv_saddr == (__daddr)) && \
- (((__sk)->sk_bound_dev_if == (__dif)) || \
- ((__sk)->sk_bound_dev_if == (__sdif))) && \
- net_eq(sock_net(__sk), (__net)))
-#endif /* 64-bit arch */
+static inline bool INET_MATCH(struct net *net, const struct sock *sk,
+ const __addrpair cookie, const __portpair ports,
+ int dif, int sdif)
+{
+ int bound_dev_if;
+
+ if (!net_eq(sock_net(sk), net) ||
+ sk->sk_portpair != ports ||
+ sk->sk_addrpair != cookie)
+ return false;
+
+ /* Paired with WRITE_ONCE() from sock_bindtoindex_locked() */
+ bound_dev_if = READ_ONCE(sk->sk_bound_dev_if);
+ return bound_dev_if == dif || bound_dev_if == sdif;
+}
/* Sockets in TCP_CLOSE state are _always_ taken out of the hash, so we need
* not check it for lookups anymore, thanks Alexey. -DaveM
diff --git a/include/net/sock.h b/include/net/sock.h
index 73063c88a2499b31c1e8d25dc157d21f93b02bf5..01edfde4257d697f2a2c88ef704a3849af4e5305 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -161,9 +161,6 @@ typedef __u64 __bitwise __addrpair;
* for struct sock and struct inet_timewait_sock.
*/
struct sock_common {
- /* skc_daddr and skc_rcv_saddr must be grouped on a 8 bytes aligned
- * address on 64bit arches : cf INET_MATCH()
- */
union {
__addrpair skc_addrpair;
struct {
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index a5d57fa679caa47ec31ea4b1de3c45f93be4cd13..16a8440083f7e4bebd5de51ddb41b3d886b233cd 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -410,13 +410,11 @@ struct sock *__inet_lookup_established(struct net *net,
sk_nulls_for_each_rcu(sk, node, &head->chain) {
if (sk->sk_hash != hash)
continue;
- if (likely(INET_MATCH(sk, net, acookie,
- saddr, daddr, ports, dif, sdif))) {
+ if (likely(INET_MATCH(net, sk, acookie, ports, dif, sdif))) {
if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
goto out;
- if (unlikely(!INET_MATCH(sk, net, acookie,
- saddr, daddr, ports,
- dif, sdif))) {
+ if (unlikely(!INET_MATCH(net, sk, acookie,
+ ports, dif, sdif))) {
sock_gen_put(sk);
goto begin;
}
@@ -465,8 +463,7 @@ static int __inet_check_established(struct inet_timewait_death_row *death_row,
if (sk2->sk_hash != hash)
continue;
- if (likely(INET_MATCH(sk2, net, acookie,
- saddr, daddr, ports, dif, sdif))) {
+ if (likely(INET_MATCH(net, sk2, acookie, ports, dif, sdif))) {
if (sk2->sk_state == TCP_TIME_WAIT) {
tw = inet_twsk(sk2);
if (twsk_unique(sk, sk2, twp))
@@ -532,9 +529,7 @@ static bool inet_ehash_lookup_by_sk(struct sock *sk,
if (esk->sk_hash != sk->sk_hash)
continue;
if (sk->sk_family == AF_INET) {
- if (unlikely(INET_MATCH(esk, net, acookie,
- sk->sk_daddr,
- sk->sk_rcv_saddr,
+ if (unlikely(INET_MATCH(net, esk, acookie,
ports, dif, sdif))) {
return true;
}
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 9d5071c79c9599aa973b80869b7768a68a508cc2..53342ce17172722d51a5db34ca9f1d5c61fb82de 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2563,8 +2563,7 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
struct sock *sk;
udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
- if (INET_MATCH(sk, net, acookie, rmt_addr,
- loc_addr, ports, dif, sdif))
+ if (INET_MATCH(net, sk, acookie, ports, dif, sdif))
return sk;
/* Only check first socket in chain */
break;
--
2.36.0.550.gb090851708-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] inet: add READ_ONCE(sk->sk_bound_dev_if) in INET_MATCH()
2022-05-12 16:56 [PATCH v2 net-next] inet: add READ_ONCE(sk->sk_bound_dev_if) in INET_MATCH() Eric Dumazet
@ 2022-05-12 17:02 ` Oliver Hartkopp
2022-05-12 17:14 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Oliver Hartkopp @ 2022-05-12 17:02 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: netdev, Eric Dumazet
On 12.05.22 18:56, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> INET_MATCH() runs without holding a lock on the socket.
>
> We probably need to annotate most reads.
>
> This patch makes INET_MATCH() an inline function
> to ease our changes.
>
> v2:
>
> We remove the 32bit version of it, as modern compilers
> should generate the same code really, no need to
> try to be smarter.
>
> Also make 'struct net *net' the first argument.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>
> Sent as a standalone patch to not spam netdev@ list.
>
> include/net/inet_hashtables.h | 33 +++++++++++++++------------------
> include/net/sock.h | 3 ---
> net/ipv4/inet_hashtables.c | 15 +++++----------
> net/ipv4/udp.c | 3 +--
> 4 files changed, 21 insertions(+), 33 deletions(-)
>
> diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
> index 98e1ec1a14f0382d1f4f8e85fe5ac2a056d2d6bc..e44e410813d0f469131f54cf3372458a0340d5cf 100644
> --- a/include/net/inet_hashtables.h
> +++ b/include/net/inet_hashtables.h
> @@ -295,7 +295,6 @@ static inline struct sock *inet_lookup_listener(struct net *net,
> ((__force __portpair)(((__u32)(__dport) << 16) | (__force __u32)(__be16)(__sport)))
> #endif
>
> -#if (BITS_PER_LONG == 64)
> #ifdef __BIG_ENDIAN
> #define INET_ADDR_COOKIE(__name, __saddr, __daddr) \
> const __addrpair __name = (__force __addrpair) ( \
> @@ -307,24 +306,22 @@ static inline struct sock *inet_lookup_listener(struct net *net,
> (((__force __u64)(__be32)(__daddr)) << 32) | \
> ((__force __u64)(__be32)(__saddr)))
> #endif /* __BIG_ENDIAN */
> -#define INET_MATCH(__sk, __net, __cookie, __saddr, __daddr, __ports, __dif, __sdif) \
> - (((__sk)->sk_portpair == (__ports)) && \
> - ((__sk)->sk_addrpair == (__cookie)) && \
> - (((__sk)->sk_bound_dev_if == (__dif)) || \
> - ((__sk)->sk_bound_dev_if == (__sdif))) && \
> - net_eq(sock_net(__sk), (__net)))
> -#else /* 32-bit arch */
> -#define INET_ADDR_COOKIE(__name, __saddr, __daddr) \
> - const int __name __deprecated __attribute__((unused))
>
> -#define INET_MATCH(__sk, __net, __cookie, __saddr, __daddr, __ports, __dif, __sdif) \
> - (((__sk)->sk_portpair == (__ports)) && \
> - ((__sk)->sk_daddr == (__saddr)) && \
> - ((__sk)->sk_rcv_saddr == (__daddr)) && \
> - (((__sk)->sk_bound_dev_if == (__dif)) || \
> - ((__sk)->sk_bound_dev_if == (__sdif))) && \
> - net_eq(sock_net(__sk), (__net)))
> -#endif /* 64-bit arch */
> +static inline bool INET_MATCH(struct net *net, const struct sock *sk,
When you convert the #define into an inline function, wouldn't it be
more natural to name it lower caps?
static inline bool inet_match(struct net *net, ... )
Best,
Oliver
> + const __addrpair cookie, const __portpair ports,
> + int dif, int sdif)
> +{
> + int bound_dev_if;
> +
> + if (!net_eq(sock_net(sk), net) ||
> + sk->sk_portpair != ports ||
> + sk->sk_addrpair != cookie)
> + return false;
> +
> + /* Paired with WRITE_ONCE() from sock_bindtoindex_locked() */
> + bound_dev_if = READ_ONCE(sk->sk_bound_dev_if);
> + return bound_dev_if == dif || bound_dev_if == sdif;
> +}
>
> /* Sockets in TCP_CLOSE state are _always_ taken out of the hash, so we need
> * not check it for lookups anymore, thanks Alexey. -DaveM
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 73063c88a2499b31c1e8d25dc157d21f93b02bf5..01edfde4257d697f2a2c88ef704a3849af4e5305 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -161,9 +161,6 @@ typedef __u64 __bitwise __addrpair;
> * for struct sock and struct inet_timewait_sock.
> */
> struct sock_common {
> - /* skc_daddr and skc_rcv_saddr must be grouped on a 8 bytes aligned
> - * address on 64bit arches : cf INET_MATCH()
> - */
> union {
> __addrpair skc_addrpair;
> struct {
> diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
> index a5d57fa679caa47ec31ea4b1de3c45f93be4cd13..16a8440083f7e4bebd5de51ddb41b3d886b233cd 100644
> --- a/net/ipv4/inet_hashtables.c
> +++ b/net/ipv4/inet_hashtables.c
> @@ -410,13 +410,11 @@ struct sock *__inet_lookup_established(struct net *net,
> sk_nulls_for_each_rcu(sk, node, &head->chain) {
> if (sk->sk_hash != hash)
> continue;
> - if (likely(INET_MATCH(sk, net, acookie,
> - saddr, daddr, ports, dif, sdif))) {
> + if (likely(INET_MATCH(net, sk, acookie, ports, dif, sdif))) {
> if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> goto out;
> - if (unlikely(!INET_MATCH(sk, net, acookie,
> - saddr, daddr, ports,
> - dif, sdif))) {
> + if (unlikely(!INET_MATCH(net, sk, acookie,
> + ports, dif, sdif))) {
> sock_gen_put(sk);
> goto begin;
> }
> @@ -465,8 +463,7 @@ static int __inet_check_established(struct inet_timewait_death_row *death_row,
> if (sk2->sk_hash != hash)
> continue;
>
> - if (likely(INET_MATCH(sk2, net, acookie,
> - saddr, daddr, ports, dif, sdif))) {
> + if (likely(INET_MATCH(net, sk2, acookie, ports, dif, sdif))) {
> if (sk2->sk_state == TCP_TIME_WAIT) {
> tw = inet_twsk(sk2);
> if (twsk_unique(sk, sk2, twp))
> @@ -532,9 +529,7 @@ static bool inet_ehash_lookup_by_sk(struct sock *sk,
> if (esk->sk_hash != sk->sk_hash)
> continue;
> if (sk->sk_family == AF_INET) {
> - if (unlikely(INET_MATCH(esk, net, acookie,
> - sk->sk_daddr,
> - sk->sk_rcv_saddr,
> + if (unlikely(INET_MATCH(net, esk, acookie,
> ports, dif, sdif))) {
> return true;
> }
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 9d5071c79c9599aa973b80869b7768a68a508cc2..53342ce17172722d51a5db34ca9f1d5c61fb82de 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2563,8 +2563,7 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
> struct sock *sk;
>
> udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
> - if (INET_MATCH(sk, net, acookie, rmt_addr,
> - loc_addr, ports, dif, sdif))
> + if (INET_MATCH(net, sk, acookie, ports, dif, sdif))
> return sk;
> /* Only check first socket in chain */
> break;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] inet: add READ_ONCE(sk->sk_bound_dev_if) in INET_MATCH()
2022-05-12 17:02 ` Oliver Hartkopp
@ 2022-05-12 17:14 ` Eric Dumazet
2022-05-13 17:18 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2022-05-12 17:14 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, Paolo Abeni, netdev
On Thu, May 12, 2022 at 10:02 AM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> When you convert the #define into an inline function, wouldn't it be
> more natural to name it lower caps?
>
> static inline bool inet_match(struct net *net, ... )
Sure, it is only a matter for us to remember all the past/present
names, based on implementation details, especially at backport times.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] inet: add READ_ONCE(sk->sk_bound_dev_if) in INET_MATCH()
2022-05-12 17:14 ` Eric Dumazet
@ 2022-05-13 17:18 ` Jakub Kicinski
2022-05-13 18:48 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2022-05-13 17:18 UTC (permalink / raw)
To: Eric Dumazet
Cc: Oliver Hartkopp, Eric Dumazet, David S . Miller, Paolo Abeni, netdev
On Thu, 12 May 2022 10:14:23 -0700 Eric Dumazet wrote:
> On Thu, May 12, 2022 at 10:02 AM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> > When you convert the #define into an inline function, wouldn't it be
> > more natural to name it lower caps?
> >
> > static inline bool inet_match(struct net *net, ... )
>
> Sure, it is only a matter for us to remember all the past/present
> names, based on implementation details, especially at backport times.
We can apply as is if you prefer, but I'm not sure I follow TBH.
The prototype (arguments) of the function/macro have changed so there
is nothing to be gained from not changing the name AFAICT, no?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 net-next] inet: add READ_ONCE(sk->sk_bound_dev_if) in INET_MATCH()
2022-05-13 17:18 ` Jakub Kicinski
@ 2022-05-13 18:48 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2022-05-13 18:48 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Oliver Hartkopp, Eric Dumazet, David S . Miller, Paolo Abeni, netdev
On Fri, May 13, 2022 at 10:18 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 12 May 2022 10:14:23 -0700 Eric Dumazet wrote:
> > On Thu, May 12, 2022 at 10:02 AM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >
> > > When you convert the #define into an inline function, wouldn't it be
> > > more natural to name it lower caps?
> > >
> > > static inline bool inet_match(struct net *net, ... )
> >
> > Sure, it is only a matter for us to remember all the past/present
> > names, based on implementation details, especially at backport times.
>
> We can apply as is if you prefer, but I'm not sure I follow TBH.
> The prototype (arguments) of the function/macro have changed so there
> is nothing to be gained from not changing the name AFAICT, no?
This was applied by David, I will send the v2 of the series, and a
patch renaming INET_MATCH().
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-05-13 18:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12 16:56 [PATCH v2 net-next] inet: add READ_ONCE(sk->sk_bound_dev_if) in INET_MATCH() Eric Dumazet
2022-05-12 17:02 ` Oliver Hartkopp
2022-05-12 17:14 ` Eric Dumazet
2022-05-13 17:18 ` Jakub Kicinski
2022-05-13 18:48 ` Eric Dumazet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.