Messages in this thread |  | | From | Alexander Duyck <> | Date | Sat, 17 Dec 2022 12:57:02 -0800 | Subject | Re: [PATCH] net/ncsi: Always use unicast source MAC address |
| |
On Fri, Dec 16, 2022 at 8:20 PM Peter Delevoryas <peter@pjd.dev> wrote: > > > > > On Dec 16, 2022, at 10:29 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote: > > > > On Thu, Dec 15, 2022 at 5:08 PM Peter Delevoryas <peter@pjd.dev> wrote: > >> > >> > >> > >>> On Dec 13, 2022, at 8:41 AM, Alexander H Duyck <alexander.duyck@gmail.com> wrote: > >>> > >>> On Mon, 2022-12-12 at 16:47 -0800, Peter Delevoryas wrote:
<...>
> > > >>> My main > >>> concern would be that the dev_addr is not initialized for those first > >>> few messages so you may be leaking information. > >>> > >>>> This might have the effect of causing the NIC to learn 2 MAC addresses from > >>>> an NC-SI link if the BMC uses OEM Get MAC Address commands to change its > >>>> initial MAC address, but it shouldn't really matter. Who knows if NIC's > >>>> even have MAC learning enabled from the out-of-band BMC link, lol. > >>>> > >>>> [1]: https://tinyurl.com/4933mhaj > >>>> [2]: https://tinyurl.com/mr3tyadb > >>> > >>> The thing is the OpenBMC approach initializes the value themselves to > >>> broadcast[3]. As a result the two code bases are essentially doing the > >>> same thing since mac_addr is defaulted to the broadcast address when > >>> the ncsi interface is registered. > >> > >> That’s a very good point, thanks for pointing that out, I hadn’t > >> even noticed that! > >> > >> Anyways, let me know what you think of the traces I added above. > >> Sorry for the delay, I’ve just been busy with some other stuff, > >> but I do really actually care about upstreaming this (and several > >> other NC-SI changes I’ll submit after this one, which are unrelated > >> but more useful). > >> > >> Thanks, > >> Peter > > > > So the NC-SI spec says any value can be used for the source MAC and > > that broadcast "may" be used. I would say there are some debugging > > advantages to using broadcast that will be obvious in a packet trace. > > Ehhhhh yeah I guess, but the ethertype is what I filter for. But sure, > a broadcast source MAC is pretty unique too. > > > I wonder if we couldn't look at doing something like requiring > > broadcast or LAA if the gma_flag isn't set. > > What is LAA? I’m out of the loop
Locally administered MAC address[4]. Basically it is a MAC address that is generated locally such as your random MAC address. Assuming the other end of the NC-SI link is using a MAC address with a vendor OUI there should be no risk of collisions on a point-to-point link. Essentially if you wanted to you could probably just generate a random MAC address for the NCSI protocol and then use that in place of the broadcast address.
> But also: aren’t we already using broadcast if the gma_flag isn’t set? > > - if (nca->ndp->gma_flag == 1) > - memcpy(eh->h_source, nca->ndp->ndev.dev->dev_addr, ETH_ALEN); > - else > - eth_broadcast_addr(eh->h_source); > + memcpy(eh->h_source, nca->ndp->ndev.dev->dev_addr, ETH_ALEN);
That I am not sure about. You were using this kernel without your patch right? With your patch it would make sense to see that behavior, but without I am not sure why you would see that address for any NC-SI commands before the gma_flag is set.
> > > With that we could at > > least advertise that we don't expect this packet to be going out in a > > real network as we cannot guarantee the MAC is unique. > > Yeah, but it probably wouldn’t help my simulation scenario. > > I guess it sounds like this patch is not a good idea, which to be fair, > is totally reasonable. > > I can just add some iptables rules to tunnel these packets with a different > source MAC, or fix the multicast socket issue I was having. It’s really > not a big deal, and like you’re saying, we probably don’t want to make > it harder to maintain _forever_.
Like I said before I would be good with either a Broadcast address OR a LAA address. The one thing we need to watch out for though is any sort of leak. One possible concern would be if for example you had 4 ports using 4 different MAC addresses but one BMC. You don't want to accidently leak the MAC address from one port onto the other one. With a LAA address if it were to leak and screw up ARP tables somewhere it wouldn't be a big deal since it isn't expected to be switched in the first place.
> I would just suggest praying for the next guy that tries to test NC-SI > stuff with QEMU and finds out NC-SI traffic gets dropped by bridges. > I had to resort to reading the source code and printing stuff with > BPF to identify this. Maybe it’s more obvious to other people this wouldn’t > work though.
Well it seems like NC-SI isn't meant to be bridged based on the fact that it is using a broadcast MAC address as a source. If nothing else I suppose you could try to work with the standards committee on that to see what can be done to make the protocol more portable.. :-)
[4]: https://macaddress.io/faq/what-are-a-universal-address-and-a-local-administered-address
|  |