Re: in_pcbbind_setup: wrong condition regarding INP_REUSEPORT ?

From: Andriy Gapon <avg_at_FreeBSD.org>
Date: Tue, 04 Oct 2022 11:46:51 UTC
On 04/10/2022 14:37, Sean Bruno wrote:
> 
> 
> On 10/3/22 04:14, Andriy Gapon wrote:
>>
>> I must admit that the condition in question is fairly long and non-trivial and 
>> I cannot decipher it, but these two lines look wrong to me:
>>
>>                                       (t->inp_flags2 & INP_REUSEPORT) ||
>>                                       (t->inp_flags2 & INP_REUSEPORT_LB) == 0) &&
>>
>> I'd expect that the check would be symmetric with respect to INP_REUSEPORT and 
>> INP_REUSEPORT_LB.
>> The problem seems to come from 1a43cff92a20d / r334719 / D11003.
>>
> 
> I think you are pointing at this absurd conditional?
> 
> https://cgit.freebsd.org/src/tree/sys/netinet/in_pcb.c#n1049
> 
> Besides the twisted logic, what problem are you trying to solve?

Yes, that conditional.
I pointed out the part of it that does not make sense to me.

Also, in my tests SO_REUSEPORT does not actually allow to share a port.
Test scenario:
- create a UDP socket
- setsockopt(SO_REUSEPORT)
- bind the socket to a port and wild card address
- success
- now repeat the previous steps with the same port *under a different user id*
- bind fails

I wonder if the following would be a correct change.

diff --git a/sys/netinet/in_pcb.c b/sys/netinet/in_pcb.c
index d9247f50d32b..f5e6e3932a96 100644
--- a/sys/netinet/in_pcb.c
+++ b/sys/netinet/in_pcb.c
@@ -1003,6 +1003,7 @@ in_pcbbind_setup(struct inpcb *inp, struct sockaddr *nam, 
in_addr_t *laddrp,
  	/*
  	 * XXX
  	 * This entire block sorely needs a rewrite.
+	 * And a good comment describing the rationale behind the conditions.
  	 */
  				if (t &&
  				    ((inp->inp_flags2 & INP_BINDMULTI) == 0) &&
@@ -1011,8 +1012,7 @@ in_pcbbind_setup(struct inpcb *inp, struct sockaddr *nam, 
in_addr_t *laddrp,
  				     ntohl(t->inp_faddr.s_addr) == INADDR_ANY) &&
  				    (ntohl(sin->sin_addr.s_addr) != INADDR_ANY ||
  				     ntohl(t->inp_laddr.s_addr) != INADDR_ANY ||
-				     (t->inp_flags2 & INP_REUSEPORT) ||
-				     (t->inp_flags2 & INP_REUSEPORT_LB) == 0) &&
+				     (t->inp_flags2 & (INP_REUSEPORT | INP_REUSEPORT_LB)) == 0) &&
  				    (inp->inp_cred->cr_uid !=
  				     t->inp_cred->cr_uid))
  					return (EADDRINUSE);

-- 
Andriy Gapon