On 12/02/16(Fri) 16:33, Florian Riehm wrote:
> Hi Tech,
>
> I have noticed that CARP IP-Balancing is broken, so I am testing and
> fixing it.
>
> The first problem came in with this commit:
> http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/if_ethersubr.c.diff?r1=1.176&r2=1.177
> It enforces that outgoing packets use the src mac of the carp interface
> instead
> the mac of its carpdev. That's a good idea for failover carp but it breaks
> balancing ip(-stealth), because the same source MAC is seen by multiple
> switchports now. That leads to continues MAC flapping at switches.
> My attached patch fixes the problem. Thanks Martin's refactoring, we can
> enforce the right MAC on a central place in carp_start() now.
Can't you completely get rid of the else chunk then? It looks like a
bad refactoring.
I'm also wondering can't we use "sc_realmac" here?
> A second problem was introduced two weeks ago. Since we always check the
> destination MAC address of received unicast packets, not only when in
> promiscuous mode, carp balancing ip is always broken, not only when in
> promiscuous mode. The new check collides with "Clear mcast if received on a
> carp IP balanced address." in carp_input().
Could you describe the problem? Which Destination MAC the packet
contains and why it doesn't match the interface? I still don't grok why
a carp(4) interface in balancing mode cannot have the right MAC
configured... Do you know? But if what you want is the parent's MAC,
then I'd rather go with the symmetric of your other change: modify
carp_input() to overwrite ``ether_dhost''.
In general we should try to avoid modifying the stack for carp.
> My fix in net/if_ethersubr.c is probably the wrong place to fix the problem,
> but I don't have a better idea for now. Maybe the fix is ok for now, because
> we
> don't have too much time before tagging 5.9.
>
> Both of my fixes are independent from each other. They can be separately
> committed.
>
> Thanks and Regards,
>
> Florian
>
>
>
> Index: net/if_ethersubr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_ethersubr.c,v
> retrieving revision 1.233
> diff -u -p -r1.233 if_ethersubr.c
> --- net/if_ethersubr.c 22 Jan 2016 17:09:05 -0000 1.233
> +++ net/if_ethersubr.c 6 Feb 2016 08:15:44 -0000
> @@ -354,7 +354,8 @@ ether_input(struct ifnet *ifp, struct mb
> * This check is required in promiscous mode, and for some hypervisors
> * where the MAC filter is 'best effort' only.
> */
> - if ((m->m_flags & (M_BCAST|M_MCAST)) == 0) {
> + if ((m->m_flags & (M_BCAST|M_MCAST)) == 0 &&
> + ifp->if_type != IFT_CARP) {
> if (memcmp(ac->ac_enaddr, eh->ether_dhost, ETHER_ADDR_LEN)) {
> m_freem(m);
> return (1);
> Index: netinet/ip_carp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> retrieving revision 1.286
> diff -u -p -r1.286 ip_carp.c
> --- netinet/ip_carp.c 21 Jan 2016 11:23:48 -0000 1.286
> +++ netinet/ip_carp.c 6 Feb 2016 08:15:46 -0000
> @@ -2275,9 +2275,15 @@ carp_start(struct ifnet *ifp)
> * advertisements in 'ip' and 'ip-stealth' balacing
> * modes.
> */
> - if (sc->sc_balancing != CARP_BAL_IPSTEALTH &&
> - sc->sc_balancing != CARP_BAL_IP &&
> - (sc->cur_vhe && !sc->cur_vhe->vhe_leader)) {
> + if (sc->sc_balancing == CARP_BAL_IP ||
> + sc->sc_balancing == CARP_BAL_IPSTEALTH) {
> + struct ether_header *eh;
> + uint8_t *esrc;
> +
> + eh = mtod(m, struct ether_header *);
> + esrc = ((struct arpcom*) ifp->if_carpdev)->ac_enaddr;
> + memcpy(eh->ether_shost, esrc,
> sizeof(eh->ether_shost));
> + } else if (sc->cur_vhe && !sc->cur_vhe->vhe_leader) {
> struct ether_header *eh;
> uint8_t *esrc;
>
>
>
>
