Re: [PATCH] net/ncsi: Always use unicast source MAC address

[Alexander Duyck]

On Fri, Dec 16, 2022 at 8:20 PM Peter Delevoryas <peter@xxxxxxx> wrote:
>
>
>
> > On Dec 16, 2022, at 10:29 AM, Alexander Duyck <alexander.duyck@xxxxxxxxx> wrote:
> >
> > On Thu, Dec 15, 2022 at 5:08 PM Peter Delevoryas <peter@xxxxxxx> wrote:
> >>
> >>
> >>
> >>> On Dec 13, 2022, at 8:41 AM, Alexander H Duyck <alexander.duyck@xxxxxxxxx> 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