Re: in_pcbbind_setup: wrong condition regarding INP_REUSEPORT ?

On Tue, Oct 04, 2022 at 02:46:51PM +0300, Andriy Gapon wrote:
> 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.

Reviving this old thread, since I was confused by this code today.

I think you are right, the INP_REUSEPORT check was inverted in
commit https://cgit.freebsd.org/src/commit/?id=1a43cff92a20d

Before that, SO_REUSEPORT would let two sockets belonging to different
UIDs bind to the same port.  This was a consequence of commits
https://cgit.freebsd.org/src/commit/?id=52b65dbe85faf
and
https://cgit.freebsd.org/src/commit/?id=4049a042532f6

But, I can't tell if that old behaviour for unicast addresses was
actually intentional.  Bugzilla PR 7713 only discusses the problem in
the context of multicast addresses, where it makes more sense.

Note that the current code has the somewhat dubious property that
sockets with different creds can bind to the same port if they all have
SO_REUSEPORT_LB set.  I can't tell if that's intentional, but I doubt
it.  I'm inclined to completely remove the REUSEPORT* flag checks here.

> 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
> 
>