On 19 Apr 2020, at 15:33, Ronald Klop wrote:
On Sat, 18 Apr 2020 09:50:30 +0200, Kristof Provost <k...@freebsd.org> wrote:

Author: kp
Date: Sat Apr 18 07:50:30 2020
New Revision: 360068
URL: https://svnweb.freebsd.org/changeset/base/360068

Log:
  ethersubr: Make the mac address generation more robust
If we create two (vnet) jails and create a bridge interface in each we end up
  with the same mac address on both bridge interfaces.
These very often conflicts, resulting in same mac address in both jails.
 Mitigate this problem by including the jail name in the mac address.
 Reviewed by:   kevans, melifaro
  MFC after:    1 week
  Differential Revision:        https://reviews.freebsd.org/D24383

Modified:
  head/sys/kern/kern_jail.c
  head/sys/net/if_ethersubr.c
  head/sys/sys/jail.h

Modified: head/sys/kern/kern_jail.c
==============================================================================
--- head/sys/kern/kern_jail.c   Sat Apr 18 03:14:16 2020        (r360067)
+++ head/sys/kern/kern_jail.c   Sat Apr 18 07:50:30 2020        (r360068)
@@ -2920,6 +2920,15 @@ getcredhostid(struct ucred *cred, unsigned long *hosti
        mtx_unlock(&cred->cr_prison->pr_mtx);
 }
+void
+getjailname(struct ucred *cred, char *name, size_t len)
+{
+
+       mtx_lock(&cred->cr_prison->pr_mtx);
+       strlcpy(name, cred->cr_prison->pr_name, len);
+       mtx_unlock(&cred->cr_prison->pr_mtx);
+}
+
 #ifdef VIMAGE
 /*
  * Determine whether the prison represented by cred owns

Modified: head/sys/net/if_ethersubr.c
==============================================================================
--- head/sys/net/if_ethersubr.c Sat Apr 18 03:14:16 2020        (r360067)
+++ head/sys/net/if_ethersubr.c Sat Apr 18 07:50:30 2020        (r360068)
@@ -1419,27 +1419,39 @@ ether_8021q_frame(struct mbuf **mp, struct ifnet *ife,
/*
  * Allocate an address from the FreeBSD Foundation OUI.  This uses a
- * cryptographic hash function on the containing jail's UUID and the interface - * name to attempt to provide a unique but stable address. Pseudo-interfaces
- * which require a MAC address should use this function to allocate
- * non-locally-administered addresses.
+ * cryptographic hash function on the containing jail's name, UUID and the
+ * interface name to attempt to provide a unique but stable address.
+ * Pseudo-interfaces which require a MAC address should use this function to
+ * allocate non-locally-administered addresses.
  */
 void
 ether_gen_addr(struct ifnet *ifp, struct ether_addr *hwaddr)
 {
-#define        ETHER_GEN_ADDR_BUFSIZ   HOSTUUIDLEN + IFNAMSIZ + 2
        SHA1_CTX ctx;
-       char buf[ETHER_GEN_ADDR_BUFSIZ];
+       char *buf;
        char uuid[HOSTUUIDLEN + 1];
        uint64_t addr;
        int i, sz;
        char digest[SHA1_RESULTLEN];
+       char jailname[MAXHOSTNAMELEN];
        getcredhostuuid(curthread->td_ucred, uuid, sizeof(uuid));
- sz = snprintf(buf, ETHER_GEN_ADDR_BUFSIZ, "%s-%s", uuid, ifp->if_xname); + /* If each (vnet) jail would also have a unique hostuuid this would not
+        * be necessary. */
+       getjailname(curthread->td_ucred, jailname, sizeof(jailname));
+       sz = asprintf(&buf, M_TEMP, "%s-%s-%s", uuid, if_name(ifp),
+           jailname);
+       if (sz < 0) {
+               /* Fall back to a random mac address. */


I was wondering if it would be valuable to give this fall back something like:

printf("%s: unable to create fixed mac address; using random mac address", if_name(ifp));

This will only be printed in rare circumstances. But in that case will provide valuable information.

That would potentially be valuable, yes. On the other hand, we traditionally don’t sprinkle a lot of printf()s around in the kernel. This is extremely unlikely to happen, and if it does odds are attaching the interface will fail at an earlier or later point, you may struggle to pass packets and run into any number of other issues. It’s also possible to diagnose absent the printf(), because the MAC address will be locally administered rather than within the FreeBSD OUI.

So, in short: not a bad idea. You can argue it both ways, and I find myself (weakly) on the opposite side.

Best regards,
Kristof
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Reply via email to