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

[sdf@google.com]

On 05/05, Andrey Ignatov wrote:
> 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?
Yes, it does, thanks! I somehow missed your 'only if port is zero' part.