RE: [bitbake-devel] [PATCH] utils: Enable the loopback interface in disable_network()

[Peter Kjellerstedt <peter.kjellerstedt@axis.com>]

> -----Original Message-----
> From: Richard Purdie <richard.purdie@linuxfoundation.org>
> Sent: den 23 september 2022 15:17
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; bitbake-devel@lists.openembedded.org
> Subject: Re: [bitbake-devel] [PATCH] utils: Enable the loopback interface in disable_network()
> 
> On Fri, 2022-09-23 at 12:16 +0200, Peter Kjellerstedt wrote:
> > From: Mattias Jernberg <mattiasj@axis.com>
> >
> > This allows, e.g., gRPC within the host to be used even when
> > networking is disabled.
> >
> > Signed-off-by: Mattias Jernberg <mattias.jernberg@axis.com>
> > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > ---
> >
> > In our case, we have a wrapper for make (bear from
> > https://github.com/rizsotto/Bear) that is automatically enabled when
> > externalsrc is used. This creates a compile_commands.json file, which,
> > e.g., VS Code can make use of. The problem here is that bear uses gRPC
> > to communicate with itself and this does not work when all network
> > communications are disabled. Enabling the loopback interface resolves
> > this problem.
> >
> >  bitbake/lib/bb/utils.py | 41 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/bitbake/lib/bb/utils.py b/bitbake/lib/bb/utils.py
> > index 92d44c5260..2d37c50bac 100644
> > --- a/bitbake/lib/bb/utils.py
> > +++ b/bitbake/lib/bb/utils.py
> > @@ -29,6 +29,8 @@ import collections
> >  import copy
> >  import ctypes
> >  import random
> > +import socket
> > +import struct
> >  import tempfile
> >  from subprocess import getstatusoutput
> >  from contextlib import contextmanager
> > @@ -1603,6 +1605,41 @@ def set_process_name(name):
> >      except:
> >          pass
> >
> > +def loopback_up():
> > +    # From bits/ioctls.h
> > +    SIOCGIFFLAGS = 0x8913
> > +    SIOCSIFFLAGS = 0x8914
> > +    SIOCSIFADDR = 0x8916
> > +    SIOCSIFNETMASK = 0x891C
> > +
> > +    # if.h
> > +    IFF_UP = 0x1
> > +    IFF_RUNNING = 0x40
> > +
> > +    # bits/socket.h
> > +    AF_INET = 2
> > +
> > +    # char ifr_name[IFNAMSIZ=16]
> > +    ifr_name = struct.pack("@16s", b"lo")
> > +    def netdev_req(fd, req, data = b""):
> > +        # Pad and add interface name
> > +        data = ifr_name + data + (b'\x00' * (16 - len(data)))
> > +        # Return all data after interface name
> > +        return fcntl.ioctl(fd, req, data)[16:]
> > +
> > +    with socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_IP) as sock:
> > +        fd = sock.fileno()
> 
> > +        # struct sockaddr_in ifr_addr { unsigned short family; uint16_t sin_port ; uint32_t in_addr; }
> > +        req = struct.pack("@H", AF_INET) + struct.pack("=H4B", 0, 127, 0, 0, 1)
> > +        netdev_req(fd, SIOCSIFADDR, req)
> 
> > +        # short ifr_flags
> > +        flags = struct.unpack_from('@h', netdev_req(fd, SIOCGIFFLAGS))[0]
> > +        flags |= IFF_UP | IFF_RUNNING
> > +        netdev_req(fd, SIOCSIFFLAGS, struct.pack('@h', flags))
> 
> 
> > +        # struct sockaddr_in ifr_netmask
> > +        req = struct.pack("@H", AF_INET) + struct.pack("=H4B", 0, 255, 0, 0, 0)
> > +        netdev_req(fd, SIOCSIFNETMASK, req)
> > +
> 
> I found that quite hard to read/understand, it would probably help to
> put some whitespace between the netdev requests. Using those kinds of
> structs is pretty horrible but I guess there isn't much else we can do
> about it. Having the code in bitbake and hence made available via a
> function is probably better than the alternatives.

Ok.

> >  def disable_network(uid=None, gid=None):
> >      """
> >      Disable networking in the current process if the kernel supports it, else
> > @@ -1626,6 +1663,10 @@ def disable_network(uid=None, gid=None):
> >      if ret != 0:
> >          logger.debug("System doesn't suport disabling network without admin privs")
> >          return
> > +
> > +    # Enable the loopback interface
> > +    loopback_up()
> > +
> >      with open("/proc/self/uid_map", "w") as f:
> >          f.write("%s %s 1" % (uid, uid))
> >      with open("/proc/self/setgroups", "w") as f:
> 
> I think I'd probably prefer just to leave networking disabled and then
> allow places where it is specifically needed to add loopback back. You
> could probably do that in externalsrc where you add the bear
> dependency?

Ok. I hope it is then ok that I add a new task varflag "network-loopback" 
(or maybe even just "loopback"?) E.g.:

diff --git a/bitbake/bin/bitbake-worker b/bitbake/bin/bitbake-worker
index 7be39370b3..4115604d99 100755
--- a/bitbake/bin/bitbake-worker
+++ b/bitbake/bin/bitbake-worker
@@ -267,6 +267,8 @@ def fork_off_task(cfg, data, databuilder, workerdata, fn, task, taskname, taskha
                     if bb.utils.is_local_uid(uid):
                         logger.debug("Attempting to disable network for %s" % taskname)
                         bb.utils.disable_network(uid, gid)
+                        if the_data.getVarFlag(taskname, 'network-loopback', False):
+                            bb.utils.loopback_up()
                     else:
                         logger.debug("Skipping disable network for %s since %s is not a local uid." % (taskname, uid))

With that in place, I see no drawbacks compared to always 
enabling the loopback interface in disable_network() since 
setting, e.g., do_compile[network-loopback] = "1" will make 
sure it is only enabled when actually needed, compared to, 
e.g., doing it in an anonymous Python function. 

> 
> Cheers,
> 
> Richard

//Peter