On 05/05, Andrey Ignatov wrote:
sdf@xxxxxxxxxx <sdf@xxxxxxxxxx> [Tue, 2020-05-05 10:09 -0700]:
> On 05/05, Stanislav Fomichev wrote:
> > On 05/04, Andrey Ignatov wrote:
> > > Stanislav Fomichev <sdf@xxxxxxxxxx> [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.
