On Mon, Sep 02, 2024 at 09:34:48AM +0200, Jonas Gorski wrote:
> Am So., 1. Sept. 2024 um 14:25 Uhr schrieb Nikolay Aleksandrov
> <razor@xxxxxxxxxxxxx>:
> >
> > On 01/09/2024 14:54, Ido Schimmel wrote:
> > > On Sat, Aug 31, 2024 at 11:31:50AM +0300, Nikolay Aleksandrov wrote:
> > >> On 30/08/2024 17:53, Jonas Gorski wrote:
> > >>> When userspace wants to take over a fdb entry by setting it as
> > >>> EXTERN_LEARNED, we set both flags BR_FDB_ADDED_BY_EXT_LEARN and
> > >>> BR_FDB_ADDED_BY_USER in br_fdb_external_learn_add().
> > >>>
> > >>> If the bridge updates the entry later because its port changed, we clear
> > >>> the BR_FDB_ADDED_BY_EXT_LEARN flag, but leave the BR_FDB_ADDED_BY_USER
> > >>> flag set.
> > >>>
> > >>> If userspace then wants to take over the entry again,
> > >>> br_fdb_external_learn_add() sees that BR_FDB_ADDED_BY_USER and skips
> > >>> setting the BR_FDB_ADDED_BY_EXT_LEARN flags, thus silently ignores the
> > >>> update:
> > >>>
> > >>> if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
> > >>> /* Refresh entry */
> > >>> fdb->used = jiffies;
> > >>> } else if (!test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
> > >>> /* Take over SW learned entry */
> > >>> set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
> > >>> modified = true;
> > >>> }
> > >>>
> > >>> Fix this by relaxing the condition for setting BR_FDB_ADDED_BY_EXT_LEARN
> > >>> by also allowing it if swdev_notify is true, which it will only be for
> > >>> user initiated updates.
> > >>>
> > >>> Fixes: 710ae7287737 ("net: bridge: Mark FDB entries that were added by user as such")
> > >>> Signed-off-by: Jonas Gorski <jonas.gorski@xxxxxxxx>
> > >>> ---
> > >>> net/bridge/br_fdb.c | 3 ++-
> > >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> > >>> index c77591e63841..c5d9ae13a6fb 100644
> > >>> --- a/net/bridge/br_fdb.c
> > >>> +++ b/net/bridge/br_fdb.c
> > >>> @@ -1472,7 +1472,8 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
> > >>> if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
> > >>> /* Refresh entry */
> > >>> fdb->used = jiffies;
> > >>> - } else if (!test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
> > >>> + } else if (swdev_notify ||
> > >>> + !test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
> > >>> /* Take over SW learned entry */
> > >>> set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
> > >>> modified = true;
> > >>
> > >> This literally means if added_by_user || !added_by_user, so you can probably
> > >> rewrite that whole block to be more straight-forward with test_and_set_bit -
> > >> if it was already set then refresh, if it wasn't modified = true
> > >
> > > Hi Nik,
> > >
> > > You mean like this [1]?
> > > I deleted the comment about "SW learned entry" since "extern_learn" flag
> > > not being set does not necessarily mean the entry was learned by SW.
> > >
> > > [1]
> > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> > > index c77591e63841..ad7a42b505ef 100644
> > > --- a/net/bridge/br_fdb.c
> > > +++ b/net/bridge/br_fdb.c
> > > @@ -1469,12 +1469,10 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
> > > modified = true;
> > > }
> > >
> > > - if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
> > > + if (test_and_set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
> > > /* Refresh entry */
> > > fdb->used = jiffies;
> > > - } else if (!test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
> > > - /* Take over SW learned entry */
> > > - set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
> > > + } else {
> > > modified = true;
> > > }
> >
> > Yeah, that's exactly what I meant. Since the added_by_user condition becomes
> > redundant we can just drop it.
>
> br_fdb_external_learn_add() is called from two places; once when
> userspace adds a EXT_LEARN flagged fdb entry (then swdev_nofity is
> set), and once when a switchdev driver reports it has learned an entry
> (then swdev_notify isn't).
>
> AFAIU the previous condition was to prevent user fdb entries from
> being taken over by hardware / switchdev events, which this would now
> allow to happen. OTOH, the switchdev notifications are a statement of
> fact, and the kernel really has a say into whether the hardware should
> keep the entry learned, so not allowing entries to be marked as
> learned by hardware would also result in a disconnect between hardware
> and kernel.
The entries were already learned by the hardware and the kernel even
updated their destination in br_fdb_external_learn_add(), it is just
that it didn't set the EXT_LEARN flag on them, which seems like a
mistake.
>
> My change was trying to accomodate for the former one, i.e. if the
> user bit is set, only the user may mark it as EXT_LEARN, but not any
> (switchdev) drivers.
>
> I have no strong feelings about what I think is right, so if this is
> the wanted direction, I can send a V2 doing that.
I prefer v2 as it means that an entry that was learned by the hardware
will now be marked as such regardless if it was previously added by user
space or not
