[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 5 May 2020 10:43:27 -0700
From: sdf@...gle.com
To: Andrey Ignatov <rdna@...com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, davem@...emloft.net,
ast@...nel.org, daniel@...earbox.net
Subject: Re: [PATCH bpf-next 4/4] bpf: allow any port in bpf_bind helper
On 05/05, Andrey Ignatov wrote:
> sdf@...gle.com <sdf@...gle.com> [Tue, 2020-05-05 10:09 -0700]:
> > On 05/05, Stanislav Fomichev wrote:
> > > On 05/04, Andrey Ignatov wrote:
> > > > Stanislav Fomichev <sdf@...gle.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.
Powered by blists - more mailing lists