netdev - Re: [PATCH net 1/3] net: bonding: Unsync device addresses on ndo

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: <Yw7lWrDODpu5pwDR@d3>
Date:   Wed, 31 Aug 2022 13:36:42 +0900
From:   Benjamin Poirier <bpoirier@...dia.com>
To:     Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:     netdev@...r.kernel.org, Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Jiri Pirko <jiri@...nulli.us>,
        Shuah Khan <shuah@...nel.org>,
        Jonathan Toppins <jtoppins@...hat.com>,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net 1/3] net: bonding: Unsync device addresses on ndo_stop

On 2022-08-30 20:25 -0700, Jay Vosburgh wrote:
> Benjamin Poirier <bpoirier@...dia.com> wrote:
> 
> >Netdev drivers are expected to call dev_{uc,mc}_sync() in their
> >ndo_set_rx_mode method and dev_{uc,mc}_unsync() in their ndo_stop method.
> >This is mentioned in the kerneldoc for those dev_* functions.
> >
> >The bonding driver calls dev_{uc,mc}_unsync() during ndo_uninit instead of
> >ndo_stop. This is ineffective because address lists (dev->{uc,mc}) have
> >already been emptied in unregister_netdevice_many() before ndo_uninit is
> >called. This mistake can result in addresses being leftover on former bond
> >slaves after a bond has been deleted; see test_LAG_cleanup() in the last
> >patch in this series.
> >
> >Add unsync calls, via bond_hw_addr_flush(), at their expected location,
> >bond_close().
> >Add dev_mc_add() call to bond_open() to match the above change.
> >The existing call __bond_release_one->bond_hw_addr_flush is left in place
> >because there are other call chains that lead to __bond_release_one(), not
> >just ndo_uninit.
> >
> >Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
> 	I'm just going from memory here, so I'm probably wrong, but
> didn't the sync/unsync stuff for HW addresses happen several years after
> the git transition?

Yes, you're right. The bonding driver was converted to dev addr list
functions in commit 303d1cbf610e ("bonding: Convert hw addr handling to
sync/unsync, support ucast addresses", v3.11-rc1). However, the problem
fixed by this patch ("addresses being leftover on former bond slaves
after a bond has been deleted") was present before the conversion (at
least for mc, uc was not handled at all before the conversion). Since
the problem was not introduced by 303d1cbf610e, that's why I chose
1da177e4c3f4 for the Fixes tag. Does that make sense?