Re: [PATCH v2 1/2] Add new RPC for pfinet network routes

[Samuel Thibault]

Hello,

The principle looks good, here are some comments.

Samuel

Damien Zammit, le ven. 02 sept. 2022 09:06:33 +0000, a ecrit:
> diff --git a/pfinet/linux-src/net/ipv4/fib_hash.c 
> b/pfinet/linux-src/net/ipv4/fib_hash.c
> index 074a36876..ca25377c9 100644
> --- a/pfinet/linux-src/net/ipv4/fib_hash.c
> +++ b/pfinet/linux-src/net/ipv4/fib_hash.c
> @@ -883,3 +883,87 @@ __initfunc(struct fib_table * fib_hash_init(int id))
>       memset(tb->tb_data, 0, sizeof(struct fn_hash));
>       return tb;
>  }
> +
> +/* DZ: Could not fit this anywhere else, due to excessive struct defs in .c 
> file */

Yes, that's fine.

> +static void
> +fib_node_get_route(int type, int dead, struct fib_info *fi, u32 prefix, u32 
> mask, ifrtreq_t *r)
> +{
> +  int len;
> +  static unsigned type2flags[RTN_MAX+1] = {
> +    0, 0, 0, 0, 0, 0, 0, RTF_REJECT, RTF_REJECT, 0, 0, 0

Rather use simply [RTN_UNREACHABLE] = RTF_REJECT, [RTN_PROHIBIT] =
RTF_REJECT, the rest will already be zero.

[...]
> +  if (fi && fi->fib_dev)
> +    sprintf (r->ifname, "%s", fi->fib_dev->name);

This should be snprintf to propertly truncate the name rather than
overflow.

> +int
> +fn_hash_get_routes(struct fib_table *tb, ifrtreq_t *routes, int first, int 
> count)
> +{
[...]
> +      for (i=0; i < maxslot; i++, fp++)
> +        {
> +          for (f = *fp; f; f = f->fn_next)
> +         {
> +              if (++pos <= first)
> +                continue;
> +              fib_node_get_route(f->fn_type,
> +                                 f->fn_state & FN_S_ZOMBIE,
> +                                 FIB_INFO(f),
> +                                 fz_prefix(f->fn_key, fz),
> +                                 FZ_MASK(fz), routes);
> +           routes++;

Take care of incoherent tab vs spaces.

> +              if (++n >= count)
> +                return n;
> +            }
> +        }
> +    }
> +  return n;
> +}

> diff --git a/pfinet/pfinet-ops.c b/pfinet/pfinet-ops.c
> index 5db669783..621721898 100644
> --- a/pfinet/pfinet-ops.c
> +++ b/pfinet/pfinet-ops.c
> @@ -22,6 +22,13 @@
> 
>  #include <linux/netdevice.h>
>  #include <linux/notifier.h>
> +#include <linux/inetdevice.h>
> +#include <linux/rtnetlink.h>
> +#include <linux/ip.h>
> +#include <net/route.h>
> +#include <net/sock.h>
> +#include <net/ip_fib.h>
> +#include <net/addrconf.h>
> 
>  #include "pfinet_S.h"
>  #include <netinet/in.h>
> @@ -32,7 +39,8 @@
>  #include <sys/mman.h>
> 
>  #include <sys/ioctl.h>
> -#include <net/if.h>
> +
> +#define MAX_ROUTES   255
> 
>  extern int dev_ifconf(char *arg);
> 
> @@ -91,3 +99,77 @@ S_pfinet_siocgifconf (io_t port,
>    pthread_mutex_unlock (&global_lock);
>    return err;
>  }
> +
> +int
> +get_routing_table(int start, int count, ifrtreq_t *routes)
> +{
> +  struct fib_table *tb;
> +
> +  if (!routes)
> +    return 0;
> +
> +  if ((tb = fib_get_table(RT_TABLE_MAIN)) == NULL)
> +    return 0;
> +
> +  return fn_hash_get_routes(tb, routes, start, count);
> +}
> +
> +
> +/* Return the routing table as a series of ifrtreq_t structs
> +   in routes, but don't return more then AMOUNT number of them.
> +   If AMOUNT is -1, we get the full table. */
> +error_t
> +S_pfinet_getroutes (io_t port,
> +                 vm_size_t amount,
> +                 data_t *routes,
> +                 mach_msg_type_number_t *len,
> +                 boolean_t *dealloc_data)
> +{
> +  error_t err = 0;
> +  ifrtreq_t rs[MAX_ROUTES];
> +  int n;
> +  ifrtreq_t *rtable;
> +
> +  pthread_mutex_lock (&global_lock);
> +
> +  if (dealloc_data)
> +    *dealloc_data = FALSE;
> +
> +  if (amount == (vm_size_t) -1)
> +    {
> +      /* Get all of them, and return the number we got.  */
> +      n = get_routing_table (0, MAX_ROUTES, rs);
> +      amount = n;
> +    }
> +  else
> +    n = amount;
> +
> +  if (amount > 0)
> +    {
> +      /* Possibly allocate a new buffer. */
> +      if (*len < amount * sizeof(ifrtreq_t))
> +     rtable = (ifrtreq_t *) mmap (0, amount * sizeof(ifrtreq_t), 
> PROT_READ|PROT_WRITE,
> +                                  MAP_ANON, 0, 0);

Then *dealloc_data should be set to TRUE for mig to deallocate it.

> +      memset(rtable, 0, n * sizeof(ifrtreq_t));
> +      n = get_routing_table (0, n, rtable);

Rather only memset to the remainder of the array?

> +    }
> +
> +  if (rtable == MAP_FAILED)
> +    {
> +      err = ENOMEM;

Rather take errno?

> +      *len = 0;
> +      if ((char *)rtable != *routes)
> +     munmap (rtable, amount * sizeof(ifrtreq_t));

Why unmapping? Here rtable is MAP_FAILED, munmaping it will just fail.

> +    }
> +  else
> +    {
> +      *len = n * sizeof(ifrtreq_t);
> +      *routes = (char *)rtable;
> +    }
> +
> +  pthread_mutex_unlock (&global_lock);
> +  return err;
> +}

> diff --git a/pfinet/route.h b/pfinet/route.h
> new file mode 100644
> index 000000000..8cb1b8a3f
> --- /dev/null
> +++ b/pfinet/route.h
> @@ -0,0 +1,26 @@

We need a copyright header.

> +#ifndef ROUTE_H_
> +#define ROUTE_H_
> +
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +#include <arpa/inet.h>
> +
> +#ifndef IFNAMSIZ
> +#define IFNAMSIZ 16
> +#endif

Can it happen that it's not defined? I'd rather not get a value randomly
defined here.

> +typedef struct ifrtreq {
> +  char ifname[IFNAMSIZ];
> +  in_addr_t rt_dest;
> +  in_addr_t rt_mask;
> +  in_addr_t rt_gateway;
> +  int rt_flags;
> +  int rt_metric;
> +  int rt_mtu;
> +  int rt_window;
> +  int rt_irtt;
> +  int rt_tos;
> +  int rt_class;
> +} ifrtreq_t;
> +
> +#endif