Re: [PATCH 4.14] usb: gadget: u_ether: fix race in setting MAC address in setup phase

[Greg KH <greg@kroah.com>]

On Sat, Dec 25, 2021 at 08:46:01PM +0200, Marian Postevca wrote:
> commit 890d5b40908bfd1a79be018d2d297cf9df60f4ee upstream.
> 
> When listening for notifications through netlink of a new interface being
> registered, sporadically, it is possible for the MAC to be read as zero.
> The zero MAC address lasts a short period of time and then switches to a
> valid random MAC address.
> 
> This causes problems for netd in Android, which assumes that the interface
> is malfunctioning and will not use it.
> 
> In the good case we get this log:
> InterfaceController::getCfg() ifName usb0
>  hwAddr 92:a8:f0:73:79:5b ipv4Addr 0.0.0.0 flags 0x1002
> 
> In the error case we get these logs:
> InterfaceController::getCfg() ifName usb0
>  hwAddr 00:00:00:00:00:00 ipv4Addr 0.0.0.0 flags 0x1002
> 
> netd : interfaceGetCfg("usb0")
> netd : interfaceSetCfg() -> ServiceSpecificException
>  (99, "[Cannot assign requested address] : ioctl() failed")
> 
> The reason for the issue is the order in which the interface is setup,
> it is first registered through register_netdev() and after the MAC
> address is set.
> 
> Fixed by first setting the MAC address of the net_device and after that
> calling register_netdev().
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Marian Postevca <posteuca@mutex.one>
> Fixes: bcd4a1c40bee885e ("usb: gadget: u_ether: construct with default values and add setters/getters")
> ---
>  drivers/usb/gadget/function/u_ether.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 38a35f57b22c0..f59c20457e658 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -864,19 +864,23 @@ int gether_register_netdev(struct net_device *net)
>  {
>  	struct eth_dev *dev;
>  	struct usb_gadget *g;
> -	struct sockaddr sa;
>  	int status;
>  
>  	if (!net->dev.parent)
>  		return -EINVAL;
>  	dev = netdev_priv(net);
>  	g = dev->gadget;
> +
> +	memcpy(net->dev_addr, dev->dev_mac, ETH_ALEN);
> +	net->addr_assign_type = NET_ADDR_RANDOM;
> +
>  	status = register_netdev(net);
>  	if (status < 0) {
>  		dev_dbg(&g->dev, "register_netdev failed, %d\n", status);
>  		return status;
>  	} else {
>  		INFO(dev, "HOST MAC %pM\n", dev->host_mac);
> +		INFO(dev, "MAC %pM\n", dev->dev_mac);
>  
>  		/* two kinds of host-initiated state changes:
>  		 *  - iff DATA transfer is active, carrier is "on"
> @@ -884,15 +888,6 @@ int gether_register_netdev(struct net_device *net)
>  		 */
>  		netif_carrier_off(net);
>  	}
> -	sa.sa_family = net->type;
> -	memcpy(sa.sa_data, dev->dev_mac, ETH_ALEN);
> -	rtnl_lock();
> -	status = dev_set_mac_address(net, &sa);
> -	rtnl_unlock();
> -	if (status)
> -		pr_warn("cannot set self ethernet address: %d\n", status);
> -	else
> -		INFO(dev, "MAC %pM\n", dev->dev_mac);
>  
>  	return status;
>  }
> -- 
> 2.32.0
> 

All now queued up, thanks.

greg k-h