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]

Date:   Tue, 02 Aug 2022 11:05:19 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Vladimir Oltean <vladimir.oltean@....com>,
        Jay Vosburgh <jay.vosburgh@...onical.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>, 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

On Tue, 2022-08-02 at 01:45 +0000, Vladimir Oltean wrote:
> On Mon, Aug 01, 2022 at 06:04:55PM -0700, Jay Vosburgh wrote:
> > 	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.
> 
> I don't see why it matters that my logic presumes that arp_xmit cannot fail.
> The bonding driver's logic doesn't put an equality sign anywhere between
> "arp_xmit succeeded" and "the packet actually reached the target".
> If it would, it would have a very broken understanding of Ethernet networks.
> For all practical purposes, updating last_tx in the bonding driver
> rather than letting the qdisc do it should make no perceivable
> difference at all, even if the driver was to drop all TX packets as you
> say.

I personally like Vladimir approach. I *think* it should be reasonably
safe.

If drops in arp_xmit() are really a concerns, what about let the latter
function return the NF error code as other output functions?

In any case, this looks like a significative rework, do you mind
consider it for the net-next, when it re-open?

Thanks!

Paolo