thr3ads.net - Linux Ethernet Bridging - [Bridge] [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation [Jul 2022]

If this information is useful, please help other people find it:
Share via:

Hans S

2022-Jul-06 10:12 UTC

[Bridge] [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation

On Wed, Jul 6, 2022 at 10:56 AM Vladimir Oltean <olteanv at gmail.com>
wrote:>
> On Tue, Jun 28, 2022 at 02:26:43PM +0200, Hans S wrote:
> > > Dumb question: if you only flush the locked entries at fast age
if the
> > > port is locked, then what happens with the existing locked
entries if
> > > the port becomes unlocked before an FDB flush takes place?
> > > Shouldn't mv88e6xxx_port_set_lock() call
mv88e6xxx_atu_locked_entry_flush()
> > > too?
BTW:>> @@ -919,6 +920,9 @@ static void mv88e6xxx_mac_link_down(struct
dsa_switch *ds, int port,
>>       if (err)
>>               dev_err(chip->dev,
>>                       "p%d: failed to force MAC link down\n",
port);
>> +     else
>> +             if (mv88e6xxx_port_is_locked(chip, port, true))
>> +                     mv88e6xxx_atu_locked_entry_flush(ds, port);
>
>This is superfluous, is it not? The bridge will transition a port whose
>link goes down to BR_STATE_DISABLED, which will make dsa_port_set_state()
>fast-age the dynamic FDB entries on the port, which you've already
>handled below.
I removed this code, but then on link down the locked entries were not
cleared out. Something not as thought?
> >
> > That was my first thought too, but the way the flags are handled with
the mask etc, does so that
> > mv88e6xxx_port_set_lock() is called when other flags change. It could
be done by the transition
> > from locked->unlocked by checking if the port is locked already.
>
> Why does mv88e6xxx_port_set_lock() get called when other flags change?
That is what seems to happen, but maybe I am wrong. Looking at the dsa
functions dsa_port_inherit_brport_flags() and
dsa_port_clear_brport_flags(), they set the mask for which underlying
function is called, and it looks to me that they call once for all the
flags: BR_LEARNING, BR_FLOOD, BR_MCAST_FLOOD, BR_BCAST_FLOOD,
BR_PORT_LOCKED?
>
> > On the other hand, the timers will timeout and the entries will be
removed anyhow.
>
> > > > +static void mv88e6xxx_atu_locked_entry_timer_work(struct
atu_locked_entry *ale)
> > >
> > > Please find a more adequate name for this function.
> >
> > Any suggestions?
>
> Not sure. It depends on whether you leave just the logic to delete a
> locked ATU entry, or also the switchdev FDB_DEL_TO_BRIDGE notifier.
> In any case, pick a name that reflects what it does. Something with
> locked_entry_delete() can't be too wrong.
>
> > > From the discussion with Ido and Nikolay I get the impression
that
> > > you're not doing the right thing here either, notifying a
> > > SWITCHDEV_FDB_DEL_TO_BRIDGE from what is effectively the
> > > SWITCHDEV_FDB_DEL_TO_DEVICE handler (port_fdb_del).
> >
> > Hmm, my experience tells me that much is opposite the normal
> > conventions when dealing with
> > locked ports, as there was never switchdev notifications from the
> > driver to the bridge before, but
> > that is needed to keep ATU and FDB entries in sync.
>
> On delete you mean? So the bridge signals switchdev a deletion of a
> locked FDB entry (as I pointed out, this function gets indirectly called
> from port_fdb_del), but it won't get deleted until switchdev signals it
> back, is what you're saying?
>
When added they are added with bridge FDB flags: extern_learn offload
locked, with a SWITCHDEV_FDB_ADD_TO_BRIDGE event. So they are owned by
the driver.
When the driver deletes the locked entry the bridge FDB entry is
removes by the SWITCHDEV_FDB_DEL_TO_BRIDGE event from the driver. That
seems quite fair?
> > > Why is the rtnl_unlock() outside the switch statement but the
rtnl_lock() inside?
> > > Not to mention, the dsa_port_to_bridge_port() call needs to be
under rtnl_lock().
> >
> > Just a small optimization as I also have another case of the switch
> > (only one switch case if
> > you didn't notice) belonging to the next patch set regarding
dynamic
> > ATU entries.
>
> What kind of optimization are you even talking about? Please get rid of
> coding patterns like this, sorry.
>Right!
> > > Please, no "if (chiplock) mutex_lock()" hacks. Just
lockdep_assert_held(&chip->reg_lock),
> > > which serves both for documentation and for validation purposes,
ensure
> > > the lock is always taken at the caller (which in this case is
super easy)
> > > and move on.
> >
> > As I am calling the function in if statement checks, it would make
> > that code more messy, while with
> > this approach the function can be called from anywhere. I also looked
> > at having two functions, with
> > one being a wrapper function taking the lock and calling the other...
>
> There are many functions in mv88e6xxx that require the reg_lock to be
> held, there's nothing new or special here.
>
Now I take the lock in the function regardless. No boolean.
> > >
> > > > +
> > > > +     if (mv88e6xxx_port_read(chip, port,
MV88E6XXX_PORT_CTL0, &reg))
> > > > +             goto out;
> > >
> > > It would be good to actually propagate the error to the caller
and
> > > "locked" via a pass-by-reference bool pointer argument,
not just say
> > > that I/O errors mean that the port is unlocked.
> >
> > Again the wish to be able to call it from if statement checks,.
> >
> > > > +     reg &= MV88E6XXX_PORT_ASSOC_VECTOR_PAV_MASK;
> > > > +     if (locked) {
> > > > +             reg |=
MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
> > > > +                    
MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
> > > > +                    
MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT |
> > > > +                     MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;
> > >
> > > I'd suggest aligning these macros vertically.
> >
> > They are according to the Linux kernel coding standard wrt indentation
afaik.
>
> Compare:
>
>                 reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
>                         MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
>                         MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT |
>                         MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;
>
> with:
>
>                 reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
>                        MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
>                        MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT |
>                        MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;
I cannot see the difference here...?

Another issue...

I have removed the timers as they are superfluous and now just use the
worker and jiffies. But I have found that the whole ageing time seems
to be broken on the 5.17 kernel I am running. I don't know if it has
been fixed, but the ageing timeout is supposed to be given in seconds.
Here is the output from various functions after the command "ip link
set dev br0 type bridge ageing_time 1500" (that is nominally 1500
seconds according to man page!):

dsa_switch_ageing_time: ageing_time 10000, ageing_time_min 1000,
ageing_time_max 3825000
mv88e6xxx_set_ageing_time: set ageing time to 10000
br0: failed (err=-34) to set attribute (id=6)
dsa_switch_ageing_time: ageing_time 15000, ageing_time_min 1000,
ageing_time_max 3825000
mv88e6xxx_set_ageing_time: set ageing time to 15000

The 15000 set corresponds to 150 seconds! (I hardcoded the dsa
ageing_time_min to 1000)

Hans S

2022-Jul-06 14:23 UTC

head link

[Bridge] [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation

> >> @@ -919,6 +920,9 @@ static void mv88e6xxx_mac_link_down(struct
dsa_switch *ds, int port,
> >>       if (err)
> >>               dev_err(chip->dev,
> >>                       "p%d: failed to force MAC link
down\n", port);
> >> +     else
> >> +             if (mv88e6xxx_port_is_locked(chip, port, true))
> >> +                     mv88e6xxx_atu_locked_entry_flush(ds, port);
> >
> >This is superfluous, is it not? The bridge will transition a port whose
> >link goes down to BR_STATE_DISABLED, which will make
dsa_port_set_state()
> >fast-age the dynamic FDB entries on the port, which you've already
> >handled below.
>
> I removed this code, but then on link down the locked entries were not
> cleared out. Something not as thought?
I don't see a fast ageing happening on link down. There is the two cases:
1. Soft link down

With iproute2 command the link is brought down and
mv88e6xxx_mac_link_down() is called with rtnl lock taken.

2. Hard link down

I remove the cable from the port and mv88e6xxx_mac_link_down() is
called without rtnl lock.

As the hard link down case calls without rtnl lock, either I trigger
the case you have mentioned or I have to use rtnl_is_locked()
somewhere along the line?

Vladimir Oltean

2022-Jul-06 14:33 UTC

head link

[Bridge] [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation

On Wed, Jul 06, 2022 at 12:12:01PM +0200, Hans S wrote:> On Wed, Jul 6, 2022 at 10:56 AM Vladimir Oltean <olteanv at
gmail.com> wrote:
> >
> > On Tue, Jun 28, 2022 at 02:26:43PM +0200, Hans S wrote:
> > > > Dumb question: if you only flush the locked entries at fast
age if the
> > > > port is locked, then what happens with the existing locked
entries if
> > > > the port becomes unlocked before an FDB flush takes place?
> > > > Shouldn't mv88e6xxx_port_set_lock() call
mv88e6xxx_atu_locked_entry_flush()
> > > > too?
> 
> BTW:
> >> @@ -919,6 +920,9 @@ static void mv88e6xxx_mac_link_down(struct
dsa_switch *ds, int port,
> >>       if (err)
> >>               dev_err(chip->dev,
> >>                       "p%d: failed to force MAC link
down\n", port);
> >> +     else
> >> +             if (mv88e6xxx_port_is_locked(chip, port, true))
> >> +                     mv88e6xxx_atu_locked_entry_flush(ds, port);
> >
> >This is superfluous, is it not? The bridge will transition a port whose
> >link goes down to BR_STATE_DISABLED, which will make
dsa_port_set_state()
> >fast-age the dynamic FDB entries on the port, which you've already
> >handled below.
> 
> I removed this code, but then on link down the locked entries were not
> cleared out. Something not as thought?
What was the port's STP state before the link down event, and did it
change after the link down?

The relevant code in DSA is:

int dsa_port_set_state(struct dsa_port *dp, u8 state, bool do_fast_age)
{
	struct dsa_switch *ds = dp->ds;
	int port = dp->index;

	if (!ds->ops->port_stp_state_set)
		return -EOPNOTSUPP;

	ds->ops->port_stp_state_set(ds, port, state);

	if (!dsa_port_can_configure_learning(dp) ||
	    (do_fast_age && dp->learning)) {
		/* Fast age FDB entries or flush appropriate forwarding database
		 * for the given port, if we are moving it from Learning or
		 * Forwarding state, to Disabled or Blocking or Listening state.
		 * Ports that were standalone before the STP state change don't
		 * need to fast age the FDB, since address learning is off in
		 * standalone mode.
		 */

		if ((dp->stp_state == BR_STATE_LEARNING ||
		     dp->stp_state == BR_STATE_FORWARDING) &&
		    (state == BR_STATE_DISABLED ||
		     state == BR_STATE_BLOCKING ||
		     state == BR_STATE_LISTENING))
			dsa_port_fast_age(dp);
	}

	dp->stp_state = state;

	return 0;
}

If the STP state wasn't LEARNING or FORWARDING, there weren't supposed
to be dynamic FDB entries on the port in the first place, so DSA says
there's nothing to flush, and doesn't call dsa_port_fast_age().
Are there dynamic FDB entries being installed on a port that isn't
in a state that's supposed to learn? I guess the answer is yes.
Is that what you want, or should the locked entries be recorded only in
the LEARNING or FORWARDING states, otherwise discarded?
> > > That was my first thought too, but the way the flags are handled
with the mask etc, does so that
> > > mv88e6xxx_port_set_lock() is called when other flags change. It
could be done by the transition
> > > from locked->unlocked by checking if the port is locked
already.
> >
> > Why does mv88e6xxx_port_set_lock() get called when other flags change?
> 
> That is what seems to happen, but maybe I am wrong. Looking at the dsa
> functions dsa_port_inherit_brport_flags() and
> dsa_port_clear_brport_flags(), they set the mask for which underlying
> function is called, and it looks to me that they call once for all the
> flags: BR_LEARNING, BR_FLOOD, BR_MCAST_FLOOD, BR_BCAST_FLOOD,
> BR_PORT_LOCKED?
What you actually want to say is: "mv88e6xxx_port_set_lock() is also
called when the DSA port joins a bridge, due to the switchdev attribute
replay logic present in dsa_port_switchdev_sync_attrs()".

Which, by the way, is logic that you've added yourself, in commit
b9e8b58fd2cb ("net: dsa: Include BR_PORT_LOCKED in the list of synced
brport flags") ;)

You are free to return early from mv88e6xxx_port_set_lock() if nothing has
changed. The DSA layer doesn't keep track of the locked state of the
port so it cannot deduce whether propagating to the switch driver is
necessary or not.
> > > > From the discussion with Ido and Nikolay I get the
impression that
> > > > you're not doing the right thing here either, notifying
a
> > > > SWITCHDEV_FDB_DEL_TO_BRIDGE from what is effectively the
> > > > SWITCHDEV_FDB_DEL_TO_DEVICE handler (port_fdb_del).
> > >
> > > Hmm, my experience tells me that much is opposite the normal
> > > conventions when dealing with
> > > locked ports, as there was never switchdev notifications from the
> > > driver to the bridge before, but
> > > that is needed to keep ATU and FDB entries in sync.
> >
> > On delete you mean? So the bridge signals switchdev a deletion of a
> > locked FDB entry (as I pointed out, this function gets indirectly
called
> > from port_fdb_del), but it won't get deleted until switchdev
signals it
> > back, is what you're saying?
> >
> 
> When added they are added with bridge FDB flags: extern_learn offload
> locked, with a SWITCHDEV_FDB_ADD_TO_BRIDGE event. So they are owned by
> the driver.
> When the driver deletes the locked entry the bridge FDB entry is
> removes by the SWITCHDEV_FDB_DEL_TO_BRIDGE event from the driver. That
> seems quite fair?
I'm just pointing out that you left other (probably unintended) code
paths for which the SWITCHDEV_FDB_DEL_TO_BRIDGE notifier is quite
useless. I haven't yet looked at your newest revision to see what
changed there.
> > > > Why is the rtnl_unlock() outside the switch statement but
the rtnl_lock() inside?
> > > > Not to mention, the dsa_port_to_bridge_port() call needs to
be under rtnl_lock().
> > >
> > > Just a small optimization as I also have another case of the
switch
> > > (only one switch case if
> > > you didn't notice) belonging to the next patch set regarding
dynamic
> > > ATU entries.
> >
> > What kind of optimization are you even talking about? Please get rid
of
> > coding patterns like this, sorry.
> >
> Right!
Right what? I'm genuinely curious what optimization are you talking about.
> > > > > +
> > > > > +     if (mv88e6xxx_port_read(chip, port,
MV88E6XXX_PORT_CTL0, &reg))
> > > > > +             goto out;
> > > >
> > > > It would be good to actually propagate the error to the
caller and
> > > > "locked" via a pass-by-reference bool pointer
argument, not just say
> > > > that I/O errors mean that the port is unlocked.
> > >
> > > Again the wish to be able to call it from if statement checks,.
> > >
> > > > > +     reg &= MV88E6XXX_PORT_ASSOC_VECTOR_PAV_MASK;
> > > > > +     if (locked) {
> > > > > +             reg |=
MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
> > > > > +                    
MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
> > > > > +                    
MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT |
> > > > > +                    
MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;
> > > >
> > > > I'd suggest aligning these macros vertically.
> > >
> > > They are according to the Linux kernel coding standard wrt
indentation afaik.
> >
> > Compare:
> >
> >                 reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
> >                         MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
> >                         MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT |
> >                         MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;
> >
> > with:
> >
> >                 reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
> >                        MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
> >                        MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT |
> >                        MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;
> 
> I cannot see the difference here...?
Just out of curiosity, are you even trying, are you looking at the
difference using a monospace font?
> Another issue...
> 
> I have removed the timers as they are superfluous and now just use the
> worker and jiffies. But I have found that the whole ageing time seems
> to be broken on the 5.17 kernel I am running. I don't know if it has
> been fixed, but the ageing timeout is supposed to be given in seconds.
> Here is the output from various functions after the command "ip link
> set dev br0 type bridge ageing_time 1500" (that is nominally 1500
> seconds according to man page!):
> 
> dsa_switch_ageing_time: ageing_time 10000, ageing_time_min 1000,
> ageing_time_max 3825000
> mv88e6xxx_set_ageing_time: set ageing time to 10000
> br0: failed (err=-34) to set attribute (id=6)
> dsa_switch_ageing_time: ageing_time 15000, ageing_time_min 1000,
> ageing_time_max 3825000
> mv88e6xxx_set_ageing_time: set ageing time to 15000
> 
> The 15000 set corresponds to 150 seconds! (I hardcoded the dsa
> ageing_time_min to 1000)
Are you talking about this known problem, that the ageing time values in
seconds need to be scaled up by a factor of USER_HZ when passed to the
kernel?
https://www.spinics.net/lists/netdev/msg672070.html
https://www.spinics.net/lists/netdev/msg567332.html

Linux Ethernet Bridging - Jul 2022 - [Bridge] [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Bridge] [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Bridge] [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Bridge] [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation