netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vladimir Oltean <olteanv@gmail.com>
To: Tobias Waldekranz <tobias@waldekranz.com>
Cc: davem@davemloft.net, kuba@kernel.org, andrew@lunn.ch,
	vivien.didelot@gmail.com, f.fainelli@gmail.com, roopa@nvidia.com,
	nikolay@nvidia.com, netdev@vger.kernel.org, jiri@resnulli.us,
	idosch@idosch.org, stephen@networkplumber.org
Subject: Re: [RFC net-next 2/7] net: bridge: switchdev: Include local flag in FDB notifications
Date: Mon, 18 Jan 2021 21:27:57 +0200	[thread overview]
Message-ID: <20210118192757.xpb4ad2af2xpetx3@skbuf> (raw)
In-Reply-To: <87turejclo.fsf@waldekranz.com>

On Mon, Jan 18, 2021 at 07:58:59PM +0100, Tobias Waldekranz wrote:
> Ah I see, no I was not aware of that. I just saw that the entry towards
> the CPU was added to the ATU, which it would in both cases. I.e. from
> the switch's POV, in this setup:
> 
>    br0
>    / \ (A)
> swp0 dummy0
>        (B)
> 
> A "local" entry like (A), or a "static" entry like (B) means the same
> thing to the switch: "it is somewhere behind my CPU-port".

Yes, except that if dummy0 was a real and non-switchdev interface, then
the "local" entry would probably break your traffic if what you meant
was "static".

> > So I think there is a very real issue in that the FDB entries with the
> > is_local bit was never specified to switchdev thus far, and now suddenly
> > is. I'm sorry, but what you're saying in the commit message, that
> > "!added_by_user has so far been indistinguishable from is_local" is
> > simply false.
> 
> Alright, so how do you do it? Here is the struct:
> 
>     struct switchdev_notifier_fdb_info {
> 	struct switchdev_notifier_info info; /* must be first */
> 	const unsigned char *addr;
> 	u16 vid;
> 	u8 added_by_user:1,
> 	   offloaded:1;
>     };
> 
> Which field separates a local address on swp0 from a dynamically learned
> address on swp0?

None, that's the problem. Local addresses are already presented to
switchdev without saying that they're local. Which is the entire reason
that users are misled into thinking that the addresses are not local.

I may have misread what you said, but to me, "!added_by_user has so far
been indistinguishable from is_local" means that:
- every struct switchdev_notifier_fdb_info with added_by_user == true
  also had an implicit is_local == false
- every struct switchdev_notifier_fdb_info with added_by_user == false
  also had an implicit is_local == true
It is _this_ that I deemed as clearly untrue.

The is_local flag is not indistinguishable from !added_by_user, it is
indistinguishable full stop. Which makes it hard to work with in a
backwards-compatible way.

> Ok, so just to see if I understand this correctly:
> 
> The situation today it that `bridge fdb add ADDR dev DEV master` results
> in flows towards ADDR being sent to:
> 
> 1. DEV if DEV belongs to a DSA switch.
> 2. To the host if DEV was a non-offloaded interface.

Not quite. In the bridge software FDB, the entry is marked as is_local
in both cases, doesn't matter if the interface is offloaded or not.
Just that switchdev does not propagate the is_local flag, which makes
the switchdev listeners think it is not local. The interpretation of
that will probably vary among switchdev drivers.

The subtlety is that for a non-offloading interface, the
misconfiguration (when you mean static but use local) is easy to catch.
Since only the entry from the software FDB will be hit, this means that
the frame will never be forwarded, so traffic will break.
But in the case of a switchdev offloading interface, the frames will hit
the hardware FDB entry more often than the software FDB entry. So
everything will work just fine and dandy even though it shouldn't.

> With this series applied both would result in (2) which, while
> idiosyncratic, is as intended. But this of course runs the risk of
> breaking existing scripts which rely on the current behavior.

Yes.

My only hope is that we could just offload the entries pointing towards
br0, and ignore the local ones. But for that I would need the bridge
maintainers to clarify what is the difference between then, as I asked
in your other patch.

  reply	other threads:[~2021-01-18 19:29 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-16  1:25 [RFC net-next 0/7] net: dsa: Sync local bridge FDB addresses to hardware Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 1/7] net: bridge: switchdev: Refactor br_switchdev_fdb_notify Tobias Waldekranz
2021-01-17 17:24   ` Vladimir Oltean
2021-01-16  1:25 ` [RFC net-next 2/7] net: bridge: switchdev: Include local flag in FDB notifications Tobias Waldekranz
2021-01-17 19:30   ` Vladimir Oltean
2021-01-18 18:58     ` Tobias Waldekranz
2021-01-18 19:27       ` Vladimir Oltean [this message]
2021-01-18 20:19         ` Tobias Waldekranz
2021-01-18 21:03           ` Vladimir Oltean
2021-01-18 21:17           ` Nikolay Aleksandrov
2021-01-18 21:22             ` Nikolay Aleksandrov
2021-01-18 21:39               ` Nikolay Aleksandrov
2021-01-18 21:50                 ` Vladimir Oltean
2021-01-18 21:53                   ` Nikolay Aleksandrov
2021-01-18 22:06                     ` Vladimir Oltean
2021-01-18 22:09                       ` Vladimir Oltean
2021-01-18 22:42                       ` Nikolay Aleksandrov
2021-01-19  0:42                         ` Vladimir Oltean
2021-01-19 10:14                           ` Nikolay Aleksandrov
2021-01-18 19:28     ` Ido Schimmel
2021-01-16  1:25 ` [RFC net-next 3/7] net: bridge: switchdev: Send FDB notifications for host addresses Tobias Waldekranz
2021-01-18 11:28   ` Vladimir Oltean
2021-01-16  1:25 ` [RFC net-next 4/7] net: dsa: Include local addresses in assisted CPU port learning Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 5/7] net: dsa: Include bridge " Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 6/7] net: dsa: Sync static FDB entries on foreign interfaces to hardware Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 7/7] net: dsa: mv88e6xxx: Request assisted learning on CPU port Tobias Waldekranz
2021-02-01  6:24 ` DENG Qingfang
2021-02-03  9:27   ` Tobias Waldekranz
2021-02-03 10:14     ` Vladimir Oltean
2021-02-03 10:42       ` Tobias Waldekranz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210118192757.xpb4ad2af2xpetx3@skbuf \
    --to=olteanv@gmail.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=idosch@idosch.org \
    --cc=jiri@resnulli.us \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=nikolay@nvidia.com \
    --cc=roopa@nvidia.com \
    --cc=stephen@networkplumber.org \
    --cc=tobias@waldekranz.com \
    --cc=vivien.didelot@gmail.com \
    --subject='Re: [RFC net-next 2/7] net: bridge: switchdev: Include local flag in FDB notifications' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).