netdev - Re: [PATCH net 1/1] bonding: fix PACKET_ORIGDEV regression on bonding masters

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: <CAHo-Ooya1gz-a=3bdZNDc3pDcETU+V7MGvdXq+kA0ALADrLn7g@mail.gmail.com>
Date:   Sun, 13 Jan 2019 18:01:39 -0800
From:   Maciej Żenczykowski <zenczykowski@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     soltys@....info, Linux NetDev <netdev@...r.kernel.org>,
        Jay Vosburgh <jay.vosburgh@...onical.com>, vincent@...nat.ch,
        Mahesh Bandewar <maheshb@...gle.com>,
        Chonggang Li <chonggangli@...gle.com>
Subject: Re: [PATCH net 1/1] bonding: fix PACKET_ORIGDEV regression on bonding masters

So I don't remember the specifics...

(note I'm writing this all from memory without looking it up/testing
it - I may be utterly wrong or dreaming)

But I seem to recall that the core problem we were trying to solve was
that a daemon listening
on an AF_PACKET ethertype 88CC [LLDP] socket not bound to any device
would not receive LLDP packets
arriving on inactive bond slaves (either active-backup or lag).

[inactive = link/carrier up, but not part of active aggregator]

This made monitoring for miscabling harder (IFIRC the only non kernel
fix was to get the daemon to create
a separate AF_PACKET/88CC socket bound to every physical interface in
the system, or monitor for
inactive slaves and add extra packet sockets as needed).

They would get re-parented to the master and then since the slave was
inactive they would be considered RX_HANDLER_EXACT match only and not
match the * interface.

Honestly I wasn't aware of PACKET_ORIGDEV, although I don't think it
helps in this case - AFAICR the packets never made it to the packet
socket.

Perhaps going from:
  /* don't change skb->dev for link-local packets */
  if (is_link_local_ether_addr(eth_hdr(skb)->h_dest)) return RX_HANDLER_PASS;
  if (bond_should_deliver_exact_match(skb, slave, bond)) return
RX_HANDLER_EXACT;

to something more like:
  if (bond_should_deliver_exact_match(skb, slave, bond)) {
    /* don't change skb->dev for link-local packets on inactive slaves */
    if (is_link_local_ether_addr(eth_hdr(skb)->h_dest)) return RX_HANDLER_PASS;
    return RX_HANDLER_EXACT;
  }

would fix both problems?

On Sun, Jan 13, 2019 at 3:05 PM David Miller <davem@...emloft.net> wrote:
>
> From: Michal Soltys <soltys@....info>
> Date: Mon,  7 Jan 2019 17:29:46 +0100
>
> > This patch reverts:
> >
> > b89f04c61efe bonding: deliver link-local packets with skb->dev set to link that packets arrived on
> >
> > And its subsequent fixups:
> >
> > 6a9e461f6fe4 bonding: pass link-local packets to bonding master also.
> > 0f3b914c9cfc bonding: fix warning message
> >
> > The intended functionality of the original patch (as explained by its
> > author) has been available in the kernel since v2.6.21-350-g80feaacb8a64
> > via PACKET_ORIGDEV socket option. The patch also broke that feature, as
> > it's now no longer possible to get the original incoming device. Quoting
> > the report:
> >
> >> Unfortunately, this doesn't completely restore the previous
> >> functionality as PACKET_ORIGDEV is broken for the copy: the original
> >> interface is lost through the call to netif_rx(). A LLDP daemon
> >> listening to the master interface won't get the original interface like
> >> it was able to before 4.12.
> >
> > The patch reverts to pre-b89f04c61efe state, so:
> >
> > - both master and original (via PACKET_ORIGDEV) devices are available
> >   when listening on the master
> > - original device is available when listening directly on one of its
> >   slaves
> >
> > Reported-by: Vincent Bernat <vincent@...nat.ch>
> > Signed-off-by: Michal Soltys <soltys@....info>
>
> Google folks I want to hear what you have to say about all of this.
>
> Thank you.