Re: [Xen-devel] [PATCH] libxl: made vm mac address assignment deterministic

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Sep 05, 2018 at 12:25:55PM +0000, Joshua Perrett wrote:
> Uses MD5 on the host mac address, vm name and vif index to generate the
> last three bytes of the vm mac address (for each vm).
> 
> It uses the vif index to account for multiple vifs.
> 
> MD5 code is originally from the public domain (written by Colin Plumb in
> 1993), files found in xen/tools/blktap2/drivers/.
> 
> Reported-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Signed-off-by: Joshua Perrett <jperrett256@xxxxxxxxx>
[...]
>  int libxl_mac_to_device_nic(libxl_ctx *ctx, uint32_t domid,
>                              const char *mac, libxl_device_nic *nic)
>  {
> @@ -53,8 +65,41 @@ int libxl_mac_to_device_nic(libxl_ctx *ctx, uint32_t domid,
>      return rc;
>  }
>  
> +static int libxl__get_host_mac(libxl__gc *gc, unsigned char *buf)
> +{
> +    int rc = ERROR_FAIL;

Having a blank line makes things a bit clearer.

> +    #ifdef __linux__
> +    struct ifaddrs *iface_list;
> +    uint64_t largest = 0;
> +
> +    if (getifaddrs(&iface_list) == 0) {
> +        for (struct ifaddrs *iface = iface_list;
> +            iface != NULL; iface = iface->ifa_next) {
> +            if (iface->ifa_addr && iface->ifa_addr->sa_family == AF_PACKET) {
> +                struct sockaddr_ll *s = (struct sockaddr_ll 
> *)iface->ifa_addr;

Blank line here.

Generally please add a blank line between variable declarations and
statements.

> +                if (s->sll_halen == 6) {
> +                    uint64_t value = 0;

Blank line here.

> +                    memcpy(&value, s->sll_addr, 6);

Please use sizeof(s->sll_addr) instead of 6.

Also please add an assert before the memcpy

   assert(sizeof(value) >= sizeof(s->sll-addr));

This will help catch potential buffer overflow issue. Not that there is
one in your code, it is just good habit to have.

> +                    if (value > largest) {
> +                        memcpy(buf, s->sll_addr, 6);
> +                        largest = value;
> +                        rc = 0;

Interesting algorithm, but I don't know anything better at the moment.
:)

> +                    }
> +                }
> +            }
> +        }
> +        freeifaddrs(iface_list);
> +    } else {
> +        LOG(WARN, "getifaddrs\n");

Use LOGE here to log errno too. And please say

  getifaddrs failed

And there is no need to have '\n' at the end of the line. LOG* macros
add that themselves.

> +    }
> +    #endif
> +
> +    return rc;
> +}
> +
>  static int libxl__device_nic_setdefault(libxl__gc *gc, uint32_t domid,
> -                                        libxl_device_nic *nic, bool hotplug)
> +                                        libxl_device_nic *nic, const char 
> *name,
> +                                        const int nic_index, bool hotplug)
>  {
>      int rc;
>  
> @@ -65,11 +110,22 @@ static int libxl__device_nic_setdefault(libxl__gc *gc, 
> uint32_t domid,
>          if (!nic->model) return ERROR_NOMEM;
>      }
>      if (libxl__mac_is_default(&nic->mac)) {
> -        const uint8_t *r;
> -        libxl_uuid uuid;
> +        uint8_t r[16];
> +
> +        MD5_CTX ctx;
> +        MD5Init(&ctx);
> +
> +        uint8_t hostmac[6];
> +
> +        if(libxl__get_host_mac(gc, hostmac) == 0) {

Missing a space after `if' here.

> +            MD5Update(&ctx, hostmac, sizeof(hostmac));
> +        } else {
> +            LOGD(INFO, domid, "failed to get host mac address, will generate 
> vm mac address without\n");

Remove '\n'.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel