netdev - Re: [PATCH v3 net 1/4] net: bonding: replace dev_trans_start() with the jiffies of the last ARP/NS

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: <5679.1659402295@famine>
Date:   Mon, 01 Aug 2022 18:04:55 -0700
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jonathan Toppins <jtoppins@...hat.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        Hangbin Liu <liuhangbin@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [PATCH v3 net 1/4] net: bonding: replace dev_trans_start() with the jiffies of the last ARP/NS

Vladimir Oltean <vladimir.oltean@....com> wrote:

>Hello Jay,
>
>On Sun, Jul 31, 2022 at 11:53:55AM -0700, Jay Vosburgh wrote:
>> Vladimir Oltean <vladimir.oltean@....com> wrote:
>> 
>> >The bonding driver piggybacks on time stamps kept by the network stack
>> >for the purpose of the netdev TX watchdog, and this is problematic
>> >because it does not work with NETIF_F_LLTX devices.
>> >
>> >It is hard to say why the driver looks at dev_trans_start() of the
>> >slave->dev, considering that this is updated even by non-ARP/NS probes
>> >sent by us, and even by traffic not sent by us at all (for example PTP
>> >on physical slave devices). ARP monitoring in active-backup mode appears
>> >to still work even if we track only the last TX time of actual ARP
>> >probes.
>> 
>> 	Because it's the closest it can get to "have we sent an ARP," really.
>
>Does it really track this? It seems pretty easy to fool to me.
>I don't know why keeping a last_tx the way my patch does wouldn't be
>better.

	The ARP monitor in general is pretty easy to fool (which was
part of the impetus for adding the "arp_validate" logic).  Ultimately it
was simpler to have the ARP monitor logic be "interface sent something
AND received an appropriate ARP" since the "sent something" came for
free from ->trans_start (which over time became less useful for this
purpose).

	And, I'm not saying your patch isn't better, rather that what I
was intending to do is minimize the change in behavior.  My concern is
that some change in semantics will break existing configurations that
rely on the old behavior.  Then, the question becomes whether the broken
configuration was reasonable or not.

	I haven't thought of anything that seems reasonable thus far;
the major change looks to be that the new logic in your patch presumes
that arp_xmit cannot fail, so if some device was discarding all TX
packets, the new logic would update "last_rx" regardless.

>> The issue with LLTX is relatively new (the bonding driver has worked
>> this way for longer than I've been involved, so I don't know what the
>> original design decisions were).
>> 
>> 	FWIW, I've been working with the following, which is closer in
>> spirit to what Jakub and I discussed previously (i.e., inspecting the
>> device stats for virtual devices, relying on dev_trans_start for
>> physical devices with ndo_tx_timeout).
>> 
>> 	This WIP includes one unrelated change: including the ifindex in
>> the route lookup; that would be a separate patch if it ends up being
>> submitted (it handles the edge case of a route on an interface other
>> than the bond matching before the bond itself).
>
>The problem with dev_get_stats() is that it will contain hardware
>statistics, which may be completely unrelated to the number of packets
>software has sent. DSA can offload the Linux bridge and the bonding
>driver as a bridge port, so dev_get_stats() on a physical port will
>return the total number of packets that egressed that port, even without
>CPU intervention. Again, even easier to fool if "have we sent an ARP"
>is what the bonding driver actually wants to know.

	I'm not well versed in how DSA works, but if a physical port is,
well, discrete from the system to a degree, why would it be part of a
bond running on the host?

	Put another way, what sort of topology / plumbing makes sense
for DSA in conjunction with bonding?

	-J

---
	-Jay Vosburgh, jay.vosburgh@...onical.com