svn commit: r360068 - in head/sys: kern net sys

[Kristof Provost]

On 19 Apr 2020, at 15:33, Ronald Klop wrote:
> On Sat, 18 Apr 2020 09:50:30 +0200, Kristof Provost <kp at 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