| Message ID | 20250108114321.128249-1-lulie@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] udp: Make rehash4 independent in udp_lib_rehash() | expand |
Some errors with CONFIG_BASE_SMALL, will fix in the next version. On 2025/1/8 19:43, Philo Lu wrote: > As discussed in [0], rehash4 could be missed in udp_lib_rehash() when > udp hash4 changes while hash2 doesn't change. This patch fixes this by > moving rehash4 codes out of rehash2 checking, and then rehash2 and > rehash4 are done separately. > > By doing this, we no longer need to call rehash4 explicitly in > udp_lib_hash4(), as the rehash callback in __ip4_datagram_connect takes > it. Thus, now udp_lib_hash4() returns directly if the sk is already > hashed. > > Note that uhash4 may fail to work under consecutive connect(<dst > address>) calls because rehash() is not called with every connect(). To > overcome this, connect(<AF_UNSPEC>) needs to be called after the next > connect to a new destination. > > [0] > https://lore.kernel.org/all/4761e466ab9f7542c68cdc95f248987d127044d2.1733499715.git.pabeni@redhat.com/ > > Fixes: 78c91ae2c6de ("ipv4/udp: Add 4-tuple hash for connected socket") > Suggested-by: Paolo Abeni <pabeni@redhat.com> > Signed-off-by: Philo Lu <lulie@linux.alibaba.com> > --- > net/ipv4/udp.c | 85 +++++++++++++++++++++++++------------------------- > 1 file changed, 42 insertions(+), 43 deletions(-) > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > index e8953e88efef9..154a1bea071b8 100644 > --- a/net/ipv4/udp.c > +++ b/net/ipv4/udp.c > @@ -533,30 +533,6 @@ static struct sock *udp4_lib_lookup4(const struct net *net, > return NULL; > } > > -/* In hash4, rehash can happen in connect(), where hash4_cnt keeps unchanged. */ > -static void udp_rehash4(struct udp_table *udptable, struct sock *sk, > - u16 newhash4) > -{ > - struct udp_hslot *hslot4, *nhslot4; > - > - hslot4 = udp_hashslot4(udptable, udp_sk(sk)->udp_lrpa_hash); > - nhslot4 = udp_hashslot4(udptable, newhash4); > - udp_sk(sk)->udp_lrpa_hash = newhash4; > - > - if (hslot4 != nhslot4) { > - spin_lock_bh(&hslot4->lock); > - hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_lrpa_node); > - hslot4->count--; > - spin_unlock_bh(&hslot4->lock); > - > - spin_lock_bh(&nhslot4->lock); > - hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_lrpa_node, > - &nhslot4->nulls_head); > - nhslot4->count++; > - spin_unlock_bh(&nhslot4->lock); > - } > -} > - > static void udp_unhash4(struct udp_table *udptable, struct sock *sk) > { > struct udp_hslot *hslot2, *hslot4; > @@ -582,15 +558,13 @@ void udp_lib_hash4(struct sock *sk, u16 hash) > struct net *net = sock_net(sk); > struct udp_table *udptable; > > - /* Connected udp socket can re-connect to another remote address, > - * so rehash4 is needed. > + /* Connected udp socket can re-connect to another remote address, which > + * will be handled by rehash. Thus no need to redo hash4 here. > */ > - udptable = net->ipv4.udp_table; > - if (udp_hashed4(sk)) { > - udp_rehash4(udptable, sk, hash); > + if (udp_hashed4(sk)) > return; > - } > > + udptable = net->ipv4.udp_table; > hslot = udp_hashslot(udptable, net, udp_sk(sk)->udp_port_hash); > hslot2 = udp_hashslot2(udptable, udp_sk(sk)->udp_portaddr_hash); > hslot4 = udp_hashslot4(udptable, hash); > @@ -2170,17 +2144,17 @@ EXPORT_SYMBOL(udp_lib_unhash); > void udp_lib_rehash(struct sock *sk, u16 newhash, u16 newhash4) > { > if (sk_hashed(sk)) { > + struct udp_hslot *hslot, *hslot2, *nhslot2, *hslot4, *nhslot4; > struct udp_table *udptable = udp_get_table_prot(sk); > - struct udp_hslot *hslot, *hslot2, *nhslot2; > > + hslot = udp_hashslot(udptable, sock_net(sk), > + udp_sk(sk)->udp_port_hash); > hslot2 = udp_hashslot2(udptable, udp_sk(sk)->udp_portaddr_hash); > nhslot2 = udp_hashslot2(udptable, newhash); > udp_sk(sk)->udp_portaddr_hash = newhash; > > if (hslot2 != nhslot2 || > rcu_access_pointer(sk->sk_reuseport_cb)) { > - hslot = udp_hashslot(udptable, sock_net(sk), > - udp_sk(sk)->udp_port_hash); > /* we must lock primary chain too */ > spin_lock_bh(&hslot->lock); > if (rcu_access_pointer(sk->sk_reuseport_cb)) > @@ -2199,18 +2173,43 @@ void udp_lib_rehash(struct sock *sk, u16 newhash, u16 newhash4) > spin_unlock(&nhslot2->lock); > } > > - if (udp_hashed4(sk)) { > - udp_rehash4(udptable, sk, newhash4); > + spin_unlock_bh(&hslot->lock); > + } > > - if (hslot2 != nhslot2) { > - spin_lock(&hslot2->lock); > - udp_hash4_dec(hslot2); > - spin_unlock(&hslot2->lock); > + /* Now process hash4 if necessary: > + * (1) update hslot4; > + * (2) update hslot2->hash4_cnt. > + * Note that hslot2/hslot4 should be checked separately, as > + * either of them may change with the other unchanged. > + */ > + if (udp_hashed4(sk)) { > + hslot4 = udp_hashslot4(udptable, > + udp_sk(sk)->udp_lrpa_hash); > + nhslot4 = udp_hashslot4(udptable, newhash4); > + udp_sk(sk)->udp_lrpa_hash = newhash4; > > - spin_lock(&nhslot2->lock); > - udp_hash4_inc(nhslot2); > - spin_unlock(&nhslot2->lock); > - } > + spin_lock_bh(&hslot->lock); > + if (hslot4 != nhslot4) { > + spin_lock(&hslot4->lock); > + hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_lrpa_node); > + hslot4->count--; > + spin_unlock(&hslot4->lock); > + > + spin_lock(&nhslot4->lock); > + hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_lrpa_node, > + &nhslot4->nulls_head); > + nhslot4->count++; > + spin_unlock(&nhslot4->lock); > + } > + > + if (hslot2 != nhslot2) { > + spin_lock(&hslot2->lock); > + udp_hash4_dec(hslot2); > + spin_unlock(&hslot2->lock); > + > + spin_lock(&nhslot2->lock); > + udp_hash4_inc(nhslot2); > + spin_unlock(&nhslot2->lock); > } > spin_unlock_bh(&hslot->lock); > }
Hi Philo, kernel test robot noticed the following build errors: [auto build test ERROR on net/main] url: https://github.com/intel-lab-lkp/linux/commits/Philo-Lu/udp-Make-rehash4-independent-in-udp_lib_rehash/20250108-194559 base: net/main patch link: https://lore.kernel.org/r/20250108114321.128249-1-lulie%40linux.alibaba.com patch subject: [PATCH net] udp: Make rehash4 independent in udp_lib_rehash() config: openrisc-randconfig-r073-20250109 (https://download.01.org/0day-ci/archive/20250110/202501100021.rFUCRD3l-lkp@intel.com/config) compiler: or1k-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250110/202501100021.rFUCRD3l-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202501100021.rFUCRD3l-lkp@intel.com/ All error/warnings (new ones prefixed by >>): net/ipv4/udp.c: In function 'udp_lib_rehash': >> net/ipv4/udp.c:2187:58: error: 'struct udp_sock' has no member named 'udp_lrpa_hash' 2187 | udp_sk(sk)->udp_lrpa_hash); | ^~ net/ipv4/udp.c:2189:35: error: 'struct udp_sock' has no member named 'udp_lrpa_hash' 2189 | udp_sk(sk)->udp_lrpa_hash = newhash4; | ^~ >> net/ipv4/udp.c:2194:69: error: 'struct udp_sock' has no member named 'udp_lrpa_node' 2194 | hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_lrpa_node); | ^~ net/ipv4/udp.c:2199:69: error: 'struct udp_sock' has no member named 'udp_lrpa_node' 2199 | hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_lrpa_node, | ^~ net/ipv4/udp.c: At top level: >> net/ipv4/udp.c:491:13: warning: 'udp_rehash4' defined but not used [-Wunused-function] 491 | static void udp_rehash4(struct udp_table *udptable, struct sock *sk, | ^~~~~~~~~~~ vim +2187 net/ipv4/udp.c 2140 2141 /* 2142 * inet_rcv_saddr was changed, we must rehash secondary hash 2143 */ 2144 void udp_lib_rehash(struct sock *sk, u16 newhash, u16 newhash4) 2145 { 2146 if (sk_hashed(sk)) { 2147 struct udp_hslot *hslot, *hslot2, *nhslot2, *hslot4, *nhslot4; 2148 struct udp_table *udptable = udp_get_table_prot(sk); 2149 2150 hslot = udp_hashslot(udptable, sock_net(sk), 2151 udp_sk(sk)->udp_port_hash); 2152 hslot2 = udp_hashslot2(udptable, udp_sk(sk)->udp_portaddr_hash); 2153 nhslot2 = udp_hashslot2(udptable, newhash); 2154 udp_sk(sk)->udp_portaddr_hash = newhash; 2155 2156 if (hslot2 != nhslot2 || 2157 rcu_access_pointer(sk->sk_reuseport_cb)) { 2158 /* we must lock primary chain too */ 2159 spin_lock_bh(&hslot->lock); 2160 if (rcu_access_pointer(sk->sk_reuseport_cb)) 2161 reuseport_detach_sock(sk); 2162 2163 if (hslot2 != nhslot2) { 2164 spin_lock(&hslot2->lock); 2165 hlist_del_init_rcu(&udp_sk(sk)->udp_portaddr_node); 2166 hslot2->count--; 2167 spin_unlock(&hslot2->lock); 2168 2169 spin_lock(&nhslot2->lock); 2170 hlist_add_head_rcu(&udp_sk(sk)->udp_portaddr_node, 2171 &nhslot2->head); 2172 nhslot2->count++; 2173 spin_unlock(&nhslot2->lock); 2174 } 2175 2176 spin_unlock_bh(&hslot->lock); 2177 } 2178 2179 /* Now process hash4 if necessary: 2180 * (1) update hslot4; 2181 * (2) update hslot2->hash4_cnt. 2182 * Note that hslot2/hslot4 should be checked separately, as 2183 * either of them may change with the other unchanged. 2184 */ 2185 if (udp_hashed4(sk)) { 2186 hslot4 = udp_hashslot4(udptable, > 2187 udp_sk(sk)->udp_lrpa_hash); 2188 nhslot4 = udp_hashslot4(udptable, newhash4); 2189 udp_sk(sk)->udp_lrpa_hash = newhash4; 2190 2191 spin_lock_bh(&hslot->lock); 2192 if (hslot4 != nhslot4) { 2193 spin_lock(&hslot4->lock); > 2194 hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_lrpa_node); 2195 hslot4->count--; 2196 spin_unlock(&hslot4->lock); 2197 2198 spin_lock(&nhslot4->lock); 2199 hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_lrpa_node, 2200 &nhslot4->nulls_head); 2201 nhslot4->count++; 2202 spin_unlock(&nhslot4->lock); 2203 } 2204 2205 if (hslot2 != nhslot2) { 2206 spin_lock(&hslot2->lock); 2207 udp_hash4_dec(hslot2); 2208 spin_unlock(&hslot2->lock); 2209 2210 spin_lock(&nhslot2->lock); 2211 udp_hash4_inc(nhslot2); 2212 spin_unlock(&nhslot2->lock); 2213 } 2214 spin_unlock_bh(&hslot->lock); 2215 } 2216 } 2217 } 2218 EXPORT_SYMBOL(udp_lib_rehash); 2219
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index e8953e88efef9..154a1bea071b8 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -533,30 +533,6 @@ static struct sock *udp4_lib_lookup4(const struct net *net, return NULL; } -/* In hash4, rehash can happen in connect(), where hash4_cnt keeps unchanged. */ -static void udp_rehash4(struct udp_table *udptable, struct sock *sk, - u16 newhash4) -{ - struct udp_hslot *hslot4, *nhslot4; - - hslot4 = udp_hashslot4(udptable, udp_sk(sk)->udp_lrpa_hash); - nhslot4 = udp_hashslot4(udptable, newhash4); - udp_sk(sk)->udp_lrpa_hash = newhash4; - - if (hslot4 != nhslot4) { - spin_lock_bh(&hslot4->lock); - hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_lrpa_node); - hslot4->count--; - spin_unlock_bh(&hslot4->lock); - - spin_lock_bh(&nhslot4->lock); - hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_lrpa_node, - &nhslot4->nulls_head); - nhslot4->count++; - spin_unlock_bh(&nhslot4->lock); - } -} - static void udp_unhash4(struct udp_table *udptable, struct sock *sk) { struct udp_hslot *hslot2, *hslot4; @@ -582,15 +558,13 @@ void udp_lib_hash4(struct sock *sk, u16 hash) struct net *net = sock_net(sk); struct udp_table *udptable; - /* Connected udp socket can re-connect to another remote address, - * so rehash4 is needed. + /* Connected udp socket can re-connect to another remote address, which + * will be handled by rehash. Thus no need to redo hash4 here. */ - udptable = net->ipv4.udp_table; - if (udp_hashed4(sk)) { - udp_rehash4(udptable, sk, hash); + if (udp_hashed4(sk)) return; - } + udptable = net->ipv4.udp_table; hslot = udp_hashslot(udptable, net, udp_sk(sk)->udp_port_hash); hslot2 = udp_hashslot2(udptable, udp_sk(sk)->udp_portaddr_hash); hslot4 = udp_hashslot4(udptable, hash); @@ -2170,17 +2144,17 @@ EXPORT_SYMBOL(udp_lib_unhash); void udp_lib_rehash(struct sock *sk, u16 newhash, u16 newhash4) { if (sk_hashed(sk)) { + struct udp_hslot *hslot, *hslot2, *nhslot2, *hslot4, *nhslot4; struct udp_table *udptable = udp_get_table_prot(sk); - struct udp_hslot *hslot, *hslot2, *nhslot2; + hslot = udp_hashslot(udptable, sock_net(sk), + udp_sk(sk)->udp_port_hash); hslot2 = udp_hashslot2(udptable, udp_sk(sk)->udp_portaddr_hash); nhslot2 = udp_hashslot2(udptable, newhash); udp_sk(sk)->udp_portaddr_hash = newhash; if (hslot2 != nhslot2 || rcu_access_pointer(sk->sk_reuseport_cb)) { - hslot = udp_hashslot(udptable, sock_net(sk), - udp_sk(sk)->udp_port_hash); /* we must lock primary chain too */ spin_lock_bh(&hslot->lock); if (rcu_access_pointer(sk->sk_reuseport_cb)) @@ -2199,18 +2173,43 @@ void udp_lib_rehash(struct sock *sk, u16 newhash, u16 newhash4) spin_unlock(&nhslot2->lock); } - if (udp_hashed4(sk)) { - udp_rehash4(udptable, sk, newhash4); + spin_unlock_bh(&hslot->lock); + } - if (hslot2 != nhslot2) { - spin_lock(&hslot2->lock); - udp_hash4_dec(hslot2); - spin_unlock(&hslot2->lock); + /* Now process hash4 if necessary: + * (1) update hslot4; + * (2) update hslot2->hash4_cnt. + * Note that hslot2/hslot4 should be checked separately, as + * either of them may change with the other unchanged. + */ + if (udp_hashed4(sk)) { + hslot4 = udp_hashslot4(udptable, + udp_sk(sk)->udp_lrpa_hash); + nhslot4 = udp_hashslot4(udptable, newhash4); + udp_sk(sk)->udp_lrpa_hash = newhash4; - spin_lock(&nhslot2->lock); - udp_hash4_inc(nhslot2); - spin_unlock(&nhslot2->lock); - } + spin_lock_bh(&hslot->lock); + if (hslot4 != nhslot4) { + spin_lock(&hslot4->lock); + hlist_nulls_del_init_rcu(&udp_sk(sk)->udp_lrpa_node); + hslot4->count--; + spin_unlock(&hslot4->lock); + + spin_lock(&nhslot4->lock); + hlist_nulls_add_head_rcu(&udp_sk(sk)->udp_lrpa_node, + &nhslot4->nulls_head); + nhslot4->count++; + spin_unlock(&nhslot4->lock); + } + + if (hslot2 != nhslot2) { + spin_lock(&hslot2->lock); + udp_hash4_dec(hslot2); + spin_unlock(&hslot2->lock); + + spin_lock(&nhslot2->lock); + udp_hash4_inc(nhslot2); + spin_unlock(&nhslot2->lock); } spin_unlock_bh(&hslot->lock); }
As discussed in [0], rehash4 could be missed in udp_lib_rehash() when udp hash4 changes while hash2 doesn't change. This patch fixes this by moving rehash4 codes out of rehash2 checking, and then rehash2 and rehash4 are done separately. By doing this, we no longer need to call rehash4 explicitly in udp_lib_hash4(), as the rehash callback in __ip4_datagram_connect takes it. Thus, now udp_lib_hash4() returns directly if the sk is already hashed. Note that uhash4 may fail to work under consecutive connect(<dst address>) calls because rehash() is not called with every connect(). To overcome this, connect(<AF_UNSPEC>) needs to be called after the next connect to a new destination. [0] https://lore.kernel.org/all/4761e466ab9f7542c68cdc95f248987d127044d2.1733499715.git.pabeni@redhat.com/ Fixes: 78c91ae2c6de ("ipv4/udp: Add 4-tuple hash for connected socket") Suggested-by: Paolo Abeni <pabeni@redhat.com> Signed-off-by: Philo Lu <lulie@linux.alibaba.com> --- net/ipv4/udp.c | 85 +++++++++++++++++++++++++------------------------- 1 file changed, 42 insertions(+), 43 deletions(-)