Re: [PATCH bpf-next 4/4] bpf: allow any port in bpf_bind helper

[Andrey Ignatov <rdna@fb.com>]

sdf@google.com <sdf@google.com> [Tue, 2020-05-05 10:09 -0700]:
> On 05/05, Stanislav Fomichev wrote:
> > On 05/04, Andrey Ignatov wrote:
> > > Stanislav Fomichev <sdf@google.com> [Mon, 2020-05-04 10:34 -0700]:
> > > > [...]
> > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > index fa9ddab5dd1f..fc5161b9ff6a 100644
> > > > --- a/net/core/filter.c
> > > > +++ b/net/core/filter.c
> > > > @@ -4527,29 +4527,24 @@ BPF_CALL_3(bpf_bind, struct
> > bpf_sock_addr_kern *, ctx, struct sockaddr *, addr,
> > > >  	struct sock *sk = ctx->sk;
> > > >  	int err;
> > > >
> > > > -	/* Binding to port can be expensive so it's prohibited in the
> > helper.
> > > > -	 * Only binding to IP is supported.
> > > > -	 */
> > > >  	err = -EINVAL;
> > > >  	if (addr_len < offsetofend(struct sockaddr, sa_family))
> > > >  		return err;
> > > >  	if (addr->sa_family == AF_INET) {
> > > >  		if (addr_len < sizeof(struct sockaddr_in))
> > > >  			return err;
> > > > -		if (((struct sockaddr_in *)addr)->sin_port != htons(0))
> > > > -			return err;
> > > >  		return __inet_bind(sk, addr, addr_len,
> > > > +				   BIND_FROM_BPF |
> > > >  				   BIND_FORCE_ADDRESS_NO_PORT);
> > >
> > > Should BIND_FORCE_ADDRESS_NO_PORT be passed only if port is zero?
> > > Passing non zero port and BIND_FORCE_ADDRESS_NO_PORT at the same time
> > > looks confusing (even though it works).
> > Makes sense, will remove it here, thx.
> Looking at it some more, I think we need to always have that
> BIND_FORCE_ADDRESS_NO_PORT. Otherwise, it might regress your
> usecase with zero port:
> 
>   if (snum || !(inet->bind_address_no_port ||
>                (flags & BIND_FORCE_ADDRESS_NO_PORT)))
> 
> If snum == 0 we want to have either the flag on or
> IP_BIND_ADDRESS_NO_PORT being set on the socket to prevent the port
> allocation a bind time.

Yes, if snum == 0 then flag is needed, that's why my previous comment
has "only if port is zero" part.

> If snum != 0, BIND_FORCE_ADDRESS_NO_PORT doesn't matter and the port
> is passed as an argument. We don't need to search for a free one, just
> to confirm it's not used.

Yes, if snum != 0 then flag doesn't matter. So both cases are covered by
your current code and that's what I meant by "(even though it works)".

My point is in the "snum != 0" case it would look better not to pass the
flag since:

1) as we see the flag doesn't matter on one hand;

2) but passing both port number and flag that says "bind only to address,
   but not to port" can look confusing and raises a question "which
   options wins? the one that sets the port or the one that asks to
   ignore the port" and that question can be answered only by looking at
   __inet_bind implementation.

so basically what I mean is:

		flags = BIND_FROM_BPF;
		if (((struct sockaddr_in *)addr)->sin_port == htons(0))
			flags &= BIND_FORCE_ADDRESS_NO_PORT;

That won't change anything for "snum == 0" case, but it would make the
"snum != 0" case more readable IMO.

Does it clarify?

-- 
Andrey Ignatov