Re: [PATCH v3 bpf-next 4/5] [RFC] udp: Fix destroying UDP listening sockets

[Martin KaFai Lau <martin.lau@linux.dev>]

On 3/21/23 5:59 PM, Aditi Ghag wrote:
> 
> 
>> On Mar 21, 2023, at 5:29 PM, Martin KaFai Lau <martin.lau@linux.dev> 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@isovalent.com>
>>> ---
>>>   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?


> 
> [1] https://lore.kernel.org/bpf/FB695169-4640-4E50-901D-84CF145765F2@isovalent.com/T/#u