netdev - Re: [net-next PATCH 07/15] eth: fbnic: allocate a netdevice and napi vectors with queues

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKgT0UeO-Bv5twdgts0gSaO1qjd_Ze5ax5k0XYUPTeDcsDuyQA@mail.gmail.com>
Date: Wed, 3 Apr 2024 15:15:25 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, Alexander Duyck <alexanderduyck@...com>, kuba@...nel.org, 
	davem@...emloft.net, pabeni@...hat.com
Subject: Re: [net-next PATCH 07/15] eth: fbnic: allocate a netdevice and napi
 vectors with queues

On Wed, Apr 3, 2024 at 1:58 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > +static int fbnic_dsn_to_mac_addr(u64 dsn, char *addr)
> > +{
> > +     addr[0] = (dsn >> 56) & 0xFF;
> > +     addr[1] = (dsn >> 48) & 0xFF;
> > +     addr[2] = (dsn >> 40) & 0xFF;
> > +     addr[3] = (dsn >> 16) & 0xFF;
> > +     addr[4] = (dsn >> 8) & 0xFF;
> > +     addr[5] = dsn & 0xFF;
>
> u64_to_ether_addr() might work here.

Actually I think it is the opposite byte order. In addition we have to
skip over bytes 3 and 4 in the center of this as those are just {
0xff, 0xff } assuming the DSN is properly formed.

> > +
> > +     return is_valid_ether_addr(addr) ? 0 : -EINVAL;
> > +}
> > +
> > +/**
> > + * fbnic_netdev_register - Initialize general software structures
> > + * @netdev: Netdev containing structure to initialize and register
> > + *
> > + * Initialize the MAC address for the netdev and register it.
> > + **/
> > +int fbnic_netdev_register(struct net_device *netdev)
> > +{
> > +     struct fbnic_net *fbn = netdev_priv(netdev);
> > +     struct fbnic_dev *fbd = fbn->fbd;
> > +     u64 dsn = fbd->dsn;
> > +     u8 addr[ETH_ALEN];
> > +     int err;
> > +
> > +     err = fbnic_dsn_to_mac_addr(dsn, addr);
> > +     if (!err) {
> > +             ether_addr_copy(netdev->perm_addr, addr);
> > +             eth_hw_addr_set(netdev, addr);
> > +     } else {
> > +             dev_err(fbd->dev, "MAC addr %pM invalid\n", addr);
>
> Rather than fail, it is more normal to allocate a random MAC address.

If the MAC address is invalid we are probably looking at an EEPROM
corruption. If requested we could port over a module parameter we have
that enables fallback as you are mentioning. However for us it is
better to default to failing since the MAC address is used to identify
the system within the datacenter and if it is randomly assigned it
will make it hard to correctly provision the system anyway.

> > @@ -192,7 +266,6 @@ static int fbnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >
> >       fbnic_devlink_unregister(fbd);
> >       fbnic_devlink_free(fbd);
> > -
> >       return err;
> >  }
>
> That hunk should be somewhere else.
>
>      Andrew

Good catch. That wasn't supposed to be there. Must have accidentally
dropped that line.

Thanks,

- Alex