utils: Enable the loopback interface in disable_network()

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(+)

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.

>  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?

Cheers,

Richard

> -----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

On Fri, 2022-09-23 at 14:42 +0000, Peter Kjellerstedt wrote:
> 
> 
> 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. 

Setting it in a prefunc doesn't look that bad once you have a function
for it?

python enable_loopback () {
    bb.utils.loopback_up()
}

do_compile[prefuncs] += "enable_loopback"


On the subject of naming, it may be clearer as something like:

bb.utils.enable_loopback_networking()

as we need to think about how this looks without context we have in
these patches.

Cheers,

Richard

> -----Original Message-----
> From: Richard Purdie <richard.purdie@linuxfoundation.org>
> Sent: den 23 september 2022 16:54
> 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 14:42 +0000, Peter Kjellerstedt wrote:
> > 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.
> 
> Setting it in a prefunc doesn't look that bad once you have a function
> for it?
> 
> python enable_loopback () {
>     bb.utils.loopback_up()
> }
> 
> do_compile[prefuncs] += "enable_loopback"

Hmm. What worries me here is what happens when enable_loopback is 
called for a task that has network=1? Hopefully it only does some 
unnecessary work, but I do not know... That's why I find the 
varflag solution more appealing as I know it will only enable the 
loopback interface when networking has actually been disabled.

> On the subject of naming, it may be clearer as something like:
> 
> bb.utils.enable_loopback_networking()
> 
> as we need to think about how this looks without context we have in
> these patches.

Agreed.

> Cheers,
> 
> Richard

//Peter