Hi Ondrej,
What do you think about splitting sk_setup() into different flavours?
See the example patch attached. This approach may be more beneficial,
because it does not count on different internal socket types, but uses
direct logic of calling specific initialization function depending how
the socket is obtained.
Regards,
Alexander
On Mon, Jul 29, 2024 at 4:25 AM Ondrej Zajicek <[email protected]> wrote:
>
> Hi
>
> Thanks for the patch. I agree that inheriting the iface-bind is the only
> way accept() on a VRF-bound scoket would make sense. I guess this
> ineriting behavior is true also for other socket properties, Therefore,
> sk_setup() likely does plenty of useless steps for sockets associated
> with incoming TCP connections.
>
> I would prefer the conditional set, you can just check (s->type == SK_TCP),
> (Listening socket has type SK_TCP_PASSIVE, while connecting socket has
> type SK_TCP_ACTIVE at sk_setup() time).
>
> Also, you noted that this is for BIRD running as non-root with no
> specific capability. Do you have some experiences and suggestions about
> running BIRD this way? I guess there are many other syscalls in BIRD that
> are CAP_NET_ADMIN privileged, like setsockopt(SO_DONTROUTE).
>
>
> On Sat, Jul 27, 2024 at 02:18:20PM +0200, Christian Svensson via Bird-users
> wrote:
> > Hi Alexander,
> >
> > On Sat, Jul 27, 2024 at 2:05 PM Alexander Zubkov <[email protected]> wrote:
> > > I wonder if it is necessary at all to set a vrf on an accepted
> > > connection? It seems to me that setting or checking vrf should be avoided
> > > instead for an accepted connection. What do you think?
> >
> > Indeed, this is what I set out to do in the beginning and is, if you
> > boil this patch down, the actual implication when using VRFs.
> >
> > The reason I chose to implement the patch as a get+set rather than a
> > conditional set was that the existing code structure assumes that
> > sk_setup is called on multiple types of sockets and I wasn't sure
> > exactly how to guard for specifically sockets that are connected.
> > In addition I tried to find a reference in the kernel to where exactly
> > it inherits the bound interface when a new socket is created from
> > accept() but I could not. It is evident from my experiments that it is
> > inherited, and that is the only way accept() on a VRF bind() would
> > make sense.
> > Doing a get+set seems like the least risky change that I felt safe to
> > propose.
> >
> > That said, if you believe it is better implemented as a conditional
> > and are able to nudge me how you'd want a check for the particular
> > socket type to look, I'd be happy to do a v2 patch.
> >
> > Regards,
>
> --
> Elen sila lumenn' omentielvo
>
> Ondrej 'Santiago' Zajicek (email: [email protected])
> "To err is human -- to blame it on a computer is even more so."
diff --git a/sysdep/unix/io.c b/sysdep/unix/io.c
index ba2e1661..0dcec17e 100644
--- a/sysdep/unix/io.c
+++ b/sysdep/unix/io.c
@@ -943,8 +943,11 @@ sock_new(pool *p)
return s;
}
+/**
+ * sk_setup_common - things to initialize for every socket
+ */
static int
-sk_setup(sock *s)
+sk_setup_common(sock *s)
{
int y = 1;
int fd = s->fd;
@@ -971,18 +974,6 @@ sk_setup(sock *s)
}
#endif
- if (s->vrf && !s->iface)
- {
- /* Bind socket to associated VRF interface.
- This is Linux-specific, but so is SO_BINDTODEVICE. */
-#ifdef SO_BINDTODEVICE
- struct ifreq ifr = {};
- strcpy(ifr.ifr_name, s->vrf->name);
- if (setsockopt(s->fd, SOL_SOCKET, SO_BINDTODEVICE, &ifr, sizeof(ifr)) < 0)
- ERR("SO_BINDTODEVICE");
-#endif
- }
-
if (s->iface)
{
#ifdef SO_BINDTODEVICE
@@ -1060,6 +1051,45 @@ sk_setup(sock *s)
return 0;
}
+/**
+ * sk_setup_accepted - initialize sockets for accepted connections
+ */
+static int
+sk_setup_accepted(sock *s)
+{
+ if (sk_setup_common(s) < 0)
+ return -1;
+
+ return 0;
+}
+
+/**
+ * sk_setup - initialize all other sockets (not for accepted connections)
+ */
+static int
+sk_setup(sock *s)
+{
+ int y = 1;
+ int fd = s->fd;
+
+ if (sk_setup_common(s) < 0)
+ return -1;
+
+ if (s->vrf && !s->iface)
+ {
+ /* Bind socket to associated VRF interface.
+ This is Linux-specific, but so is SO_BINDTODEVICE. */
+#ifdef SO_BINDTODEVICE
+ struct ifreq ifr = {};
+ strcpy(ifr.ifr_name, s->vrf->name);
+ if (setsockopt(s->fd, SOL_SOCKET, SO_BINDTODEVICE, &ifr, sizeof(ifr)) < 0)
+ ERR("SO_BINDTODEVICE");
+#endif
+ }
+
+ return 0;
+}
+
static void
sk_insert(sock *s)
{
@@ -1135,7 +1165,7 @@ sk_passive_connected(sock *s, int type)
log(L_WARN "SOCK: Cannot get remote IP address for TCP<");
}
- if (sk_setup(t) < 0)
+ if (sk_setup_accepted(t) < 0)
{
/* FIXME: Call err_hook instead ? */
log(L_ERR "SOCK: Incoming connection: %s%#m", t->err);
