> On Mar 22, 2023, at 3:55 PM, Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 3/21/23 5:59 PM, Aditi Ghag wrote:
>>> On Mar 21, 2023, at 5:29 PM, Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>>>
>>> On 3/21/23 11:45 AM, Aditi Ghag wrote:
>>>> Previously, UDP listening sockets that bind'ed to a port
>>>> weren't getting properly destroyed via udp_abort.
>>>> Specifically, the sockets were left in the UDP hash table with
>>>> unset source port.
>>>> Fix the issue by unconditionally unhashing and resetting source
>>>> port for sockets that are getting destroyed. This would mean
>>>> that in case of sockets listening on wildcarded address and
>>>> on a specific address with a common port, users would have to
>>>> explicitly select the socket(s) they intend to destroy.
>>>> Signed-off-by: Aditi Ghag <aditi.ghag@xxxxxxxxxxxxx>
>>>> ---
>>>> net/ipv4/udp.c | 21 ++++++++++++++++++++-
>>>> 1 file changed, 20 insertions(+), 1 deletion(-)
>>>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>>>> index 02d357713838..a495ac88fcee 100644
>>>> --- a/net/ipv4/udp.c
>>>> +++ b/net/ipv4/udp.c
>>>> @@ -1965,6 +1965,25 @@ int udp_pre_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>>>> }
>>>> EXPORT_SYMBOL(udp_pre_connect);
>>>> +int __udp_disconnect_with_abort(struct sock *sk)
>>>> +{
>>>> + struct inet_sock *inet = inet_sk(sk);
>>>> +
>>>> + sk->sk_state = TCP_CLOSE;
>>>> + inet->inet_daddr = 0;
>>>> + inet->inet_dport = 0;
>>>> + sock_rps_reset_rxhash(sk);
>>>> + sk->sk_bound_dev_if = 0;
>>>> + inet_reset_saddr(sk);
>>>> + inet->inet_sport = 0;
>>>> + sk_dst_reset(sk);
>>>> + /* (TBD) In case of sockets listening on wildcard and specific address
>>>> + * with a common port, socket will be removed from {hash, hash2} table.
>>>> + */
>>>> + sk->sk_prot->unhash(sk);
>>>
>>> hmm... not sure if I understand the use case. The idea is to enforce the user space to bind() again when it gets error from read(fd) because the source ip/port needs to change when sending to another dst IP/port?
>>> Does it have a usage example in the selftests?
>> Yes, there is a new selftest case where I intend to exercise the UDP sockets batching changes (check the udp_server test case). Well, the Cilium use case is to destroy client sockets (the selftests from v1/v2 patch mirror the use case), but we would want to be able destroy listening sockets too since we don't have any code preventing that?
>> I expected when UDP listening server sockets are destroyed, they are removed from the hash table, and a subsequent bind on the overlapping port would succeed? At least, I observed similar behavior for TCP sockets (minus the bind part, of course) in the test, and the connected client sockets were reset when the server sockets were destroyed. That's not what I observed for UDP listening sockets though (shared the debugging notes in the v2 patch [1]).
>
> The tcp 'clien', from 'connect_to_fd()', was not bind() to a particular local ip and port. afaik, the tcp server which binds to a particular ip and port will observe similar behavior as the udp server.
>
> When the user space notices a read() error from a UDP server socket (because of udp_abort), should the user space close() this udp server socket first before opening a new one and then bind to a different src ip and src port?
> or I am still missing some pieces in the use case where there is other cgroup bpf programs doing bind?
I'm not sure if we are talking about the same selftests. It's possible that the new server related selftests are not validating the behavior in the right way.
Let's take an example of the test_udp_server test. It does the following -
1) Start SO_REUSEPORT servers. (I hit same issue for regular UDP listening sockets as well.)
2) Run BPF iterators that destroys the server sockets.
3) Start a regular server that binds on the same port as the ones from (1) with the expectation that it succeeds after (1) sockets were destroyed. The server fails to bind. Moreover, the UDP sockets were lingering in the kernel hash table without the fix in one of the commits.
Are you suggesting that (3) is expected? The destroyed sockets are expected to be also present in the hash table? Are you saying that userspace needs to first close the sockets that were destroyed in the kernel when they encounter read() error?
>
>
>> [1] https://lore.kernel.org/bpf/FB695169-4640-4E50-901D-84CF145765F2@xxxxxxxxxxxxx/T/#u
