Re: [Bridge] [PATCH net-next v3 1/6] net: bridge: Set BR_FDB_ADDED_BY_USER early in fdb_add_entry
On Tue, 05 Sep 2023 13:47:18 +0200 Johannes Nixdorf wrote: > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -1056,7 +1056,7 @@ static int fdb_add_entry(struct net_bridge *br, struct > net_bridge_port *source, > if (!(flags & NLM_F_CREATE)) > return -ENOENT; > > - fdb = fdb_create(br, source, addr, vid, 0); > + fdb = fdb_create(br, source, addr, vid, > BIT(BR_FDB_ADDED_BY_USER)); Please try to wrap your code at 80 chars. Also: ## Form letter - net-next-closed The merge window for v6.6 has begun and therefore net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only. Please repost when net-next reopens after Sept 11th. RFC patches sent for review only are obviously welcome at any time. See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle -- pw-bot: defer
Re: [Bridge] [PATCH v2 00/14] sysctl: Add a size argument to register functions in sysctl
On Mon, 7 Aug 2023 14:44:11 -0700 Luis Chamberlain wrote: > > This is looking great thanks, I've taken the first 7 patches above > > to sysctl-next to get more exposure / testing and since we're already > > on rc4. > > Ok I havent't heard much more feedback from networking folks What did you expect to hear from us? My only comment is to merge the internal prep changes in and then send us the networking changes in the next release cycle.
Re: [Bridge] [PATCH net-next v2 1/8] skbuff: bridge: Add layer 2 miss indication
On Mon, 29 May 2023 14:48:28 +0300 Ido Schimmel wrote: > For EVPN non-DF (Designated Forwarder) filtering we need to be able to > prevent decapsulated traffic from being flooded to a multi-homed host. > Filtering of multicast and broadcast traffic can be achieved using the > following flower filter: > > # tc filter add dev bond0 egress pref 1 proto all flower indev vxlan0 > dst_mac 01:00:00:00:00:00/01:00:00:00:00:00 action drop > > Unlike broadcast and multicast traffic, it is not currently possible to > filter unknown unicast traffic. The classification into unknown unicast > is performed by the bridge driver, but is not visible to other layers > such as tc. > > Solve this by adding a new 'l2_miss' bit to the tc skb extension. Clear > the bit whenever a packet enters the bridge (received from a bridge port > or transmitted via the bridge) and set it if the packet did not match an > FDB or MDB entry. If there is no skb extension and the bit needs to be > cleared, then do not allocate one as no extension is equivalent to the > bit being cleared. The bit is not set for broadcast packets as they > never perform a lookup and therefore never incur a miss. Acked-by: Jakub Kicinski
Re: [Bridge] [PATCH net-next 1/5] skbuff: bridge: Add layer 2 miss indication
On Tue, 23 May 2023 11:10:38 +0300 Ido Schimmel wrote: > > Can we possibly put the new field at the end of the CB and then have TC > > look at it in the CB? We already do a bit of such CB juggling in strp > > (first member of struct sk_skb_cb). > > Using the CB between different layers is very fragile and I would like > to avoid it. Note that the skb can pass various layers until hitting the > classifier, each of which can decide to memset() the CB. > > Anyway, I think I have a better alternative. I added the 'l2_miss' bit > to the tc skb extension and adjusted the bridge to mark packets via this > extension. The entire thing is protected by the existing 'tc_skb_ext_tc' > static key, so overhead is kept to a minimum when feature is disabled. > Extended flower to enable / disable this key when filters that match on > 'l2_miss' are added / removed. > > bridge change to mark the packet: > https://github.com/idosch/linux/commit/3fab206492fcad9177f2340680f02ced1b9a0dec.patch > > flow_dissector change to dissect the info from the extension: > https://github.com/idosch/linux/commit/1533c078b02586547817a4e63989a0db62aa5315.patch > > flower change to enable / disable the key: > https://github.com/idosch/linux/commit/cf84b277511ec80fe565c41271abc6b2e2f629af.patch > > Advantages compared to the previous approach are that we do not need a > new bit in the skb and that overhead is kept to a minimum when feature > is disabled. Disadvantage is that overhead is higher when feature is > enabled. > > WDYT? Sounds good, yup. Thanks!
Re: [Bridge] [PATCH net-next 1/5] skbuff: bridge: Add layer 2 miss indication
On Fri, 19 May 2023 16:51:48 +0300 Ido Schimmel wrote: > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index fc17b9fd93e6..274e55455b15 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -46,6 +46,8 @@ static int br_pass_frame_up(struct sk_buff *skb) > */ > br_switchdev_frame_unmark(skb); > > + skb->l2_miss = BR_INPUT_SKB_CB(skb)->miss; > + > /* Bridge is just like any other port. Make sure the > * packet is allowed except in promisc mode when someone > * may be running packet capture. > > Ran these changes through the selftest and it seems to work. Can we possibly put the new field at the end of the CB and then have TC look at it in the CB? We already do a bit of such CB juggling in strp (first member of struct sk_skb_cb).
Re: [Bridge] [Question] Any plan to write/update the bridge doc?
On Mon, 24 Apr 2023 17:25:08 +0800 Hangbin Liu wrote: > Maybe someone already has asked. The only official Linux bridge document I > got is a very ancient wiki page[1] or the ip link man page[2][3]. As there are > many bridge stp/vlan/multicast paramegers. Should we add a detailed kernel > document about each parameter? The parameter showed in ip link page seems > a little brief. > > I'd like to help do this work. But apparently neither my English nor my > understanding of the code is good enough. Anyway, if you want, I can help > write a draft version first and you (bridge maintainers) keep working on this. > > [1] https://wiki.linuxfoundation.org/networking/bridge > [2] https://man7.org/linux/man-pages/man8/bridge.8.html > [3] https://man7.org/linux/man-pages/man8/ip-link.8.html Sounds like we have 2 votes for the CLI man pages but I'd like to register a vote for in-kernel documentation. I work at a large company so my perspective may differ but from what I see: - users who want to call the kernel API should not have to look at the CLI's man - man pages use archaic and arcane markup, I'd like to know how many people actually know how it works and how many copy / paste / look; ReST is prevalent, simple and commonly understood - in-kernel docs are rendered on the web as soon as they hit linux-next - we can make sure documentation is provided with the kernel changes, in an ideal world it doesn't matter but in practice the CLI support may never happen (no to mention that iproute does not hold all CLI) Obviously if Stephen and Ido prefer to document the bridge CLI that's perfectly fine, it's their call :) For new sections of uAPI, however, I personally find in-kernel docs superior.
Re: [Bridge] [PATCH net-next] net/bridge: add drop reasons for bridge forwarding
On Wed, 12 Apr 2023 09:33:10 +0800 xu xin wrote: > >You can return the reason from this function. That's the whole point of > >SKB_NOT_DROPPED_YET existing and being equal to 0. > > If returning the reasons, then the funtion will have to be renamed because > 'should_deliever()' is expected to return a non-zero value when it's ok to > deliever. I don't want to change the name here, and it's better to keep its > name and use the pointer to store the reasons. Sure. You have to touch all callers, anyway, you can as well adjust the name.
Re: [Bridge] [PATCH net-next] net/bridge: add drop reasons for bridge forwarding
On Thu, 6 Apr 2023 19:30:34 +0800 (CST) yang.yan...@zte.com.cn wrote:
> From: xu xin
>
> This creates six drop reasons as follows, which will help users know the
> specific reason why bridge drops the packets when forwarding.
>
> 1) SKB_DROP_REASON_BRIDGE_FWD_NO_BACKUP_PORT: failed to get a backup
>port link when the destination port is down.
>
> 2) SKB_DROP_REASON_BRIDGE_FWD_SAME_PORT: destination port is the same
>with originating port when forwarding by a bridge.
>
> 3) SKB_DROP_REASON_BRIDGE_NON_FORWARDING_STATE: the bridge's state is
>not forwarding.
>
> 4) SKB_DROP_REASON_BRIDGE_NOT_ALLOWED_EGRESS: the packet is not allowed
>to go out through the port due to vlan filtering.
>
> 5) SKB_DROP_REASON_BRIDGE_SWDEV_NOT_ALLOWED_EGRESS: the packet is not
>allowed to go out through the port which is offloaded by a hardware
>switchdev, checked by nbp_switchdev_allowed_egress().
>
> 6) SKB_DROP_REASON_BRIDGE_BOTH_PORT_ISOLATED: both source port and dest
>port are in BR_ISOLATED state when bridge forwarding.
> @@ -338,6 +344,33 @@ enum skb_drop_reason {
>* for another host.
>*/
> SKB_DROP_REASON_IPV6_NDISC_NS_OTHERHOST,
> + /** @SKB_DROP_REASON_BRIDGE_FWD_NO_BACKUP_PORT: failed to get a backup
> + * port link when the destination port is down.
> + */
That's not valid kdoc. Text can be on the same line as the value only
in one-line comments. Otherwise:
/**
* @VALUE: bla bla bla
* more blas.
*/
> +static inline bool should_deliver(const struct net_bridge_port *p, const
> struct sk_buff *skb,
> + enum skb_drop_reason *need_reason)
> {
> struct net_bridge_vlan_group *vg;
> + enum skb_drop_reason reason;
>
> vg = nbp_vlan_group_rcu(p);
> - return ((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&
> - p->state == BR_STATE_FORWARDING && br_allowed_egress(vg, skb) &&
> - nbp_switchdev_allowed_egress(p, skb) &&
> - !br_skb_isolated(p, skb);
> + if (!(p->flags & BR_HAIRPIN_MODE) && skb->dev == p->dev) {
> + reason = SKB_DROP_REASON_BRIDGE_FWD_SAME_PORT;
> + goto undeliverable;
> + }
> + if (p->state != BR_STATE_FORWARDING) {
> + reason = SKB_DROP_REASON_BRIDGE_NON_FORWARDING_STATE;
> + goto undeliverable;
> + }
> + if (!br_allowed_egress(vg, skb)) {
> + reason = SKB_DROP_REASON_BRIDGE_NOT_ALLOWED_EGRESS;
> + goto undeliverable;
> + }
> + if (!nbp_switchdev_allowed_egress(p, skb)) {
> + reason = SKB_DROP_REASON_BRIDGE_SWDEV_NOT_ALLOWED_EGRESS;
> + goto undeliverable;
> + }
> + if (br_skb_isolated(p, skb)) {
> + reason = SKB_DROP_REASON_BRIDGE_BOTH_PORT_ISOLATED;
> + goto undeliverable;
> + }
> + return true;
> +
> +undeliverable:
> + if (need_reason)
> + *need_reason = reason;
> + return false;
You can return the reason from this function. That's the whole point of
SKB_NOT_DROPPED_YET existing and being equal to 0.
Which is not to say that I know whether the reasons are worth adding
here. We'll need to hear from bridge experts on that.
Re: [Bridge] [PATCH 0/3] net: make kobj_type structures constant
On Sat, 11 Feb 2023 03:32:28 + Thomas Weißschuh wrote:
> Since commit ee6d3dd4ed48 ("driver core: make kobj_type constant.")
> the driver core allows the usage of const struct kobj_type.
>
> Take advantage of this to constify the structure definitions to prevent
> modification at runtime.
Could you resend just the first two with [PATCH net-next] in the
subject? Patch 3 needs to go to a different tree.
Re: [Bridge] [PATCH net-next 3/4] bridge: mcast: Move validation to a policy
On Thu, 9 Feb 2023 09:18:51 +0200 Ido Schimmel wrote:
> + if (nla_len(attr) != sizeof(struct br_mdb_entry)) {
> + NL_SET_ERR_MSG_MOD(extack, "Invalid MDBA_SET_ENTRY attribute
> length");
> + return -EINVAL;
Well, you're just moving it, but NL_SET_ERR_MSG_ATTR() would be better.
We shouldn't be adding _MOD() in the core implementation of the family.
Re: [Bridge] [PATCH net-next mlxsw v2 00/16] bridge: Limit number of MDB entries per port, port-vlan
On Wed, 1 Feb 2023 18:28:33 +0100 Petr Machata wrote: > Subject: [PATCH net-next mlxsw v2 00/16] bridge: Limit number of MDB entries > per port, port-vlan What do you mean by "net-next mlxsw"? Is there a tree called "net-next mlxsw" somewhere?
Re: [Bridge] [PATCH net-next] netlink: provide an ability to set default extack message
On Fri, 27 Jan 2023 07:26:13 +0200 Leon Romanovsky wrote: > > That'd be my preference too, FWIW. It's only the offload cases which > > need this sort of fallback. > > Of course not, almost any error unwind path which sets extack will need it. I guess we can come up with scenarios where the new behavior would be useful. But the fact is - your patch changes 4 places... > See devlink as an example I don't know what part of devlink you mean at a quick scroll.
Re: [Bridge] [PATCH net-next] netlink: provide an ability to set default extack message
On Fri, 27 Jan 2023 00:44:57 +0200 Vladimir Oltean wrote: > On Thu, Jan 26, 2023 at 02:37:23PM -0800, Jakub Kicinski wrote: > > > I would somewhat prefer not doing this, and instead introducing a new > > > NL_SET_ERR_MSG_WEAK() of sorts. > > > > That'd be my preference too, FWIW. It's only the offload cases which > > need this sort of fallback. > > > > BTW Vladimir, I remember us discussing this. I was searching the > > archive as you sent this, but can't find the thread. Mostly curious > > whether I flip flipped on this or I'm not completely useless :) > > What we discussed was on a patch of mine fixing "if (!extack->_msg)" to > "if (extack && !extack->_msg)". I never proposed a new macro wrapper > (you did), but I didn't do it at the time because it was a patch for > "net", and I forgot to put a reminder for the next net->net-next merge. > https://lore.kernel.org/netdev/20220822182523.6821e...@kernel.org/ > And from there, out of sight, out of mind. That explains it, I was running blame the message lines, not the if (). Thanks for digging it up!
Re: [Bridge] [PATCH net-next] netlink: provide an ability to set default extack message
On Fri, 27 Jan 2023 00:32:13 +0200 Vladimir Oltean wrote:
> On Thu, Jan 26, 2023 at 09:15:03PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky
> >
> > In netdev common pattern, xxtack pointer is forwarded to the drivers
> ~~
> extack
>
> > to be filled with error message. However, the caller can easily
> > overwrite the filled message.
> >
> > Instead of adding multiple "if (!extack->_msg)" checks before any
> > NL_SET_ERR_MSG() call, which appears after call to the driver, let's
> > add this check to common code.
> >
> > [1] https://lore.kernel.org/all/Y9Irgrgf3uxOjwUm@unreal
> > Signed-off-by: Leon Romanovsky
> > ---
>
> I would somewhat prefer not doing this, and instead introducing a new
> NL_SET_ERR_MSG_WEAK() of sorts.
That'd be my preference too, FWIW. It's only the offload cases which
need this sort of fallback.
BTW Vladimir, I remember us discussing this. I was searching the
archive as you sent this, but can't find the thread. Mostly curious
whether I flip flipped on this or I'm not completely useless :)
> The reason has to do with the fact that an extack is sometimes also
> used to convey warnings rather than hard errors, for example right here
> in net/dsa/slave.c:
>
> if (err == -EOPNOTSUPP) {
> if (extack && !extack->_msg)
> NL_SET_ERR_MSG_MOD(extack,
> "Offloading not supported");
> NL_SET_ERR_MSG_MOD(extack,
> "Offloading not supported");
> err = 0;
> }
Re: [Bridge] [PATCH 1/5] kobject: make kobject_get_ownership() take a constant kobject *
On Mon, 21 Nov 2022 10:46:45 +0100 Greg Kroah-Hartman wrote: > The call, kobject_get_ownership(), does not modify the kobject passed > into it, so make it const. This propagates down into the kobj_type > function callbacks so make the kobject passed into them also const, > ensuring that nothing in the kobject is being changed here. > > This helps make it more obvious what calls and callbacks do, and do not, > modify structures passed to them. Acked-by: Jakub Kicinski
Re: [Bridge] [RFC][PATCH v2 19/31] timers: net: Use del_timer_shutdown() before freeing timer
On Fri, 28 Oct 2022 18:31:49 -0400 Steven Rostedt wrote:
> Could someone from networking confirm (or deny) that the timer being
> removed in sk_stop_timer() will no longer be used even if del_timer()
> returns false?
>
> net/core/sock.c:
>
> void sk_stop_timer(struct sock *sk, struct timer_list* timer)
> {
> if (del_timer(timer))
> __sock_put(sk);
> }
>
> If this is the case, then I'll add the following interface:
>
>del_timer_sync_shutdown() // the common case which syncs
>
>del_timer_shutdown() // the uncommon case, that returns immediately
> // used for those cases that add extra code to
> // handle it, like sk_stop_timer()
Sorry too many bugs at once :)
FWIW Paolo was saying privately earlier today that he spotted some cases
of reuse, he gave an example of ccid2_hc_tx_packet_recv()
So we can't convert all cases of sk_stop_timer() in one fell swoop :(
> Which has the same semantics as del_timer_sync() and del_timer()
> respectively, but will prevent the timer from being rearmed again.
>
> This way we can convert the sk_stop_timer() to:
>
> void sk_stop_timer(struct sock *sk, struct timer_list* timer)
> {
> if (del_timer_shutdown(timer))
> __sock_put(sk);
> }
>
>
> We can also add the del_timer_shutdown() to other locations that need to
> put a timer into a shutdown state before freeing, and where it's in a
> context that can not call del_timer_sync_shutdown().
Re: [Bridge] [PATCH v8 net-next 02/12] net: bridge: add blackhole fdb entry flag
On Sun, 23 Oct 2022 07:32:02 +0200 net...@kapio-technology.com wrote:
> >> @@ -1140,7 +1148,7 @@ static int __br_fdb_add(struct ndmsg *ndm,
> >> struct net_bridge *br,
> >>err = br_fdb_external_learn_add(br, p, addr, vid, true);
> >>} else {
> >>spin_lock_bh(>hash_lock);
> >> - err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
> >> + err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid,
> >> ext_flags,
> >> nfea_tb);
> >
> > I believe the preference is to wrap to 80 columns when possible.
>
> Very strange... since I ran checkpatch.pl from the net-next kernel
> itself and it did not
> give me any warnings about 80 columns, but rather said 'patch is ready
> for submission'.
>
> As this is silent, could it be some missing python plugins or something
> to do with perl?
I run:
./scripts/checkpatch.pl --strict --max-line-length=80
Re: [Bridge] [PATCH v8 net-next 01/12] net: bridge: add locked entry fdb flag to extend locked port feature
On Thu, 20 Oct 2022 21:37:17 +0200 net...@kapio-technology.com wrote: > Okay, since Jakub says that this patch set must be resent, the question > remains to me if I shall make these changes and resend the patch set > as v8? If I understand the question right - since you'd be making changes the new posting should be a v9. If you got only acks and no change requests for this posting you could repost as "v8 RESEND", or also as v9, when in doubt err on the side of bumping the version...
Re: [Bridge] [PATCH v8 net-next 00/12] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)
On Tue, 18 Oct 2022 18:56:07 +0200 Hans J. Schultz wrote: > This patch set extends the locked port feature for devices > that are behind a locked port, but do not have the ability to > authorize themselves as a supplicant using IEEE 802.1X. > Such devices can be printers, meters or anything related to > fixed installations. Instead of 802.1X authorization, devices > can get access based on their MAC addresses being whitelisted. FWIW half of this posting got stuck on the "email pipes" for a day.. somehow. Let's give Ido and others a chance to have a look but you'll need to repost even if it's flawless because the build bots can't deal with a delay that long :(
Re: [Bridge] [RFC PATCH net-next 00/19] bridge: mcast: Extensions for EVPN
On Tue, 18 Oct 2022 15:04:01 +0300 Ido Schimmel wrote: > [ MDBE_ATTR_SRC_LIST ] // new > [ MDBE_SRC_LIST_ENTRY ] > [ MDBE_SRCATTR_ADDRESS ] > struct in_addr / struct in6_addr > [ ...] nit: I found that the MDBE_ATTR_SRC_LIST level of wrapping corresponds to how "sane" formats work, but in practice there is no need for it in netlink. You can put the entry nests directly in the outer. Saves one layer of parsing. Just thought I'd mention it, you can keep as is if you prefer.
Re: [Bridge] [PATCH v6 net-next 0/9] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)
On Fri, 30 Sep 2022 18:04:12 +0300 Ido Schimmel wrote: > On Fri, Sep 30, 2022 at 07:52:04AM -0700, Jakub Kicinski wrote: > > On Fri, 30 Sep 2022 17:05:20 +0300 Ido Schimmel wrote: > > > You can see build issues on patchwork: > > > > Overall a helpful response, but that part you got wrong. > > > > Do not point people to patchwork checks, please. It will only encourage > > people to post stuff they haven't build tested themselves. > > > > It's documented: > > > > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#running-all-the-builds-and-checks-locally-is-a-pain-can-i-post-my-patches-and-have-the-patchwork-bot-validate-them > > > > Did you read my reply? I specifically included this link so that you > won't tell me I'm encouraging people to build test their patches by > posting to netdev. Yeah, I noticed the link after, but I think my point stands. You show someone the patchwork checks and then at the end of the "also" section put a link on how it's not okay to abuse it. Not a clear enough instruction. *never* point people to the patchwork checks, *please*
Re: [Bridge] [PATCH v6 net-next 0/9] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)
On Fri, 30 Sep 2022 17:05:20 +0300 Ido Schimmel wrote: > You can see build issues on patchwork: Overall a helpful response, but that part you got wrong. Do not point people to patchwork checks, please. It will only encourage people to post stuff they haven't build tested themselves. It's documented: https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#running-all-the-builds-and-checks-locally-is-a-pain-can-i-post-my-patches-and-have-the-patchwork-bot-validate-them Only people who helped write the code and maintain the infra can decide how to use it which means me, Kees, or Hangbin. Please and thank you :S
Re: [Bridge] [PATCH v6 net-next 0/9] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)
On Thu, 29 Sep 2022 18:37:09 +0200 net...@kapio-technology.com wrote:
> On 2022-09-29 18:10, Jakub Kicinski wrote:
> > On Wed, 28 Sep 2022 17:02:47 +0200 Hans Schultz wrote:
> >> From: "Hans J. Schultz"
> >>
> >> This patch set extends the locked port feature for devices
> >> that are behind a locked port, but do not have the ability to
> >> authorize themselves as a supplicant using IEEE 802.1X.
> >> Such devices can be printers, meters or anything related to
> >> fixed installations. Instead of 802.1X authorization, devices
> >> can get access based on their MAC addresses being whitelisted.
> >
> > Try a allmodconfig build on latest net-next, seems broken.
>
> I have all different switch drivers enabled and I see no compile
> warnings or errors.
Just do what I told you - rebase on net-next, allmodconfig.
> I guess I will get a robot update if that is the
> case but please be specific as to what does not build.
The maintainers simply don't have time to hold everyone by the hand.
Sometimes I wish it was still okay to yell at people who post code
which does not build. Oh well.
../drivers/net/dsa/qca/qca8k-common.c:810:5: error: conflicting types for
‘qca8k_port_fdb_del’
int qca8k_port_fdb_del(struct dsa_switch *ds, int port,
^~
In file included from ../drivers/net/dsa/qca/qca8k-common.c:13:
../drivers/net/dsa/qca/qca8k.h:483:5: note: previous declaration of
‘qca8k_port_fdb_del’ was here
int qca8k_port_fdb_del(struct dsa_switch *ds, int port,
^~
../drivers/net/dsa/qca/qca8k-common.c: In function ‘qca8k_port_fdb_del’:
../drivers/net/dsa/qca/qca8k-common.c:818:6: error: ‘fdb_flags’ undeclared
(first use in this function); did you mean ‘tsq_flags’?
if (fdb_flags)
^
tsq_flags
../drivers/net/dsa/qca/qca8k-common.c:818:6: note: each undeclared identifier
is reported only once for each function it appears in
make[5]: *** [../scripts/Makefile.build:249:
drivers/net/dsa/qca/qca8k-common.o] Error 1
make[5]: *** Waiting for unfinished jobs
make[4]: *** [../scripts/Makefile.build:465: drivers/net/dsa/qca] Error 2
make[4]: *** Waiting for unfinished jobs
../drivers/net/dsa/sja1105/sja1105_main.c: In function ‘sja1105_fast_age’:
../drivers/net/dsa/sja1105/sja1105_main.c:1941:61: error: incompatible type for
argument 5 of ‘sja1105_fdb_del’
rc = sja1105_fdb_del(ds, port, macaddr, l2_lookup.vlanid, db);
^~
../drivers/net/dsa/sja1105/sja1105_main.c:1831:11: note: expected ‘u16’ {aka
‘short unsigned int’} but argument is of type ‘struct dsa_db’
u16 fdb_flags, struct dsa_db db)
^
../drivers/net/dsa/sja1105/sja1105_main.c:1941:8: error: too few arguments to
function ‘sja1105_fdb_del’
rc = sja1105_fdb_del(ds, port, macaddr, l2_lookup.vlanid, db);
^~~
../drivers/net/dsa/sja1105/sja1105_main.c:1829:12: note: declared here
static int sja1105_fdb_del(struct dsa_switch *ds, int port,
^~~
../drivers/net/dsa/sja1105/sja1105_main.c: In function ‘sja1105_mdb_del’:
../drivers/net/dsa/sja1105/sja1105_main.c:1962:56: error: incompatible type for
argument 5 of ‘sja1105_fdb_del’
return sja1105_fdb_del(ds, port, mdb->addr, mdb->vid, db);
^~
../drivers/net/dsa/sja1105/sja1105_main.c:1831:11: note: expected ‘u16’ {aka
‘short unsigned int’} but argument is of type ‘struct dsa_db’
u16 fdb_flags, struct dsa_db db)
^
../drivers/net/dsa/sja1105/sja1105_main.c:1962:9: error: too few arguments to
function ‘sja1105_fdb_del’
return sja1105_fdb_del(ds, port, mdb->addr, mdb->vid, db);
^~~
../drivers/net/dsa/sja1105/sja1105_main.c:1829:12: note: declared here
static int sja1105_fdb_del(struct dsa_switch *ds, int port,
^~~
../drivers/net/dsa/sja1105/sja1105_main.c:1963:1: error: control reaches end of
non-void function [-Werror=return-type]
}
^
cc1: some warnings being treated as errors
make[5]: *** [../scripts/Makefile.build:249:
drivers/net/dsa/sja1105/sja1105_main.o] Error 1
make[5]: *** Waiting for unfinished jobs
make[4]: *** [../scripts/Makefile.build:465: drivers/net/dsa/sja1105] Error 2
make[3]: *** [../scripts/Makefile.build:465: drivers/net/dsa] Error 2
make[3]: *** Waiting for unfinished jobs
make[2]: *** [../scripts/Makefile.build:465: drivers/net] Error 2
make[1]: *** [/home/kicinski/linux/Makefile:1852: drivers] Error 2
make[1]: *** Waiting for unfinished jobs
make: *** [Makefile:222: __sub-make] Error 2
Re: [Bridge] [PATCH v6 net-next 9/9] selftests: forwarding: add test of MAC-Auth Bypass to locked port tests
On Thu, 29 Sep 2022 18:17:40 +0200 net...@kapio-technology.com wrote: > > If you were trying to repost just the broken patches - that's not gonna > > work :( > > Sorry, I do not understand what 'broken' patches you are referring to? > > I think that the locked port tests should be working? Ignore it then. v6 does not build, see my other reply.
Re: [Bridge] [PATCH v6 net-next 9/9] selftests: forwarding: add test of MAC-Auth Bypass to locked port tests
On Wed, 28 Sep 2022 19:49:04 +0200 Hans Schultz wrote: > From: "Hans J. Schultz" > > Verify that the MAC-Auth mechanism works by adding a FDB entry with the > locked flag set, denying access until the FDB entry is replaced with a > FDB entry without the locked flag set. > > Add test of blackhole fdb entries, verifying that there is no forwarding > to a blackhole entry from any port, and that the blackhole entry can be > replaced. > > Also add a test that verifies that sticky FDB entries cannot roam (this > is not needed for now, but should in general be present anyhow for future > applications). If you were trying to repost just the broken patches - that's not gonna work :(
Re: [Bridge] [PATCH v6 net-next 0/9] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)
On Wed, 28 Sep 2022 17:02:47 +0200 Hans Schultz wrote: > From: "Hans J. Schultz" > > This patch set extends the locked port feature for devices > that are behind a locked port, but do not have the ability to > authorize themselves as a supplicant using IEEE 802.1X. > Such devices can be printers, meters or anything related to > fixed installations. Instead of 802.1X authorization, devices > can get access based on their MAC addresses being whitelisted. Try a allmodconfig build on latest net-next, seems broken.
Re: [Bridge] [PATCH RFC net-next 0/5] net: vlan: fix bridge binding behavior and add selftests
On Wed, 21 Sep 2022 07:45:07 +0300 Nikolay Aleksandrov wrote:
> > IDK, vlan knows it's calling the bridge:
> >
> > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
> > + netif_is_bridge_master(vlan->real_dev)) {
>
> This one is more of an optimization so notifications are sent only when the
> bridge
> is involved, it can be removed if other interested parties show up.
>
> > bridge knows it's vlan calling:
> >
> > + if (is_vlan_dev(dev)) {
> > + br_vlan_device_event(dev, event, ptr);
> >
> > going thru the generic NETDEV notifier seems odd.
> >
> > If this is just to avoid the dependency we can perhaps add a stub
> > like net/ipv4/udp_tunnel_stub.c ?
>
> I suggested the notifier to be more generic and be able to re-use it for
> other link types although
> I don't have other use cases in mind right now. Stubs are an alternative as
> long as they and
> their lifetime are properly managed. I don't have a strong preference here so
> if you prefer
> stubs I'm good.
Yup, stub seems simpler and more efficient to me. Only time will
tell if indeed this ntf type would have been reused further.. 路
Re: [Bridge] [PATCH RFC net-next 0/5] net: vlan: fix bridge binding behavior and add selftests
On Tue, 20 Sep 2022 12:16:26 +0300 Nikolay Aleksandrov wrote:
> The set looks good to me, the bridge and vlan direct dependency is gone and
> the new notification type is used for passing link type specific info.
IDK, vlan knows it's calling the bridge:
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
+ netif_is_bridge_master(vlan->real_dev)) {
bridge knows it's vlan calling:
+ if (is_vlan_dev(dev)) {
+ br_vlan_device_event(dev, event, ptr);
going thru the generic NETDEV notifier seems odd.
If this is just to avoid the dependency we can perhaps add a stub
like net/ipv4/udp_tunnel_stub.c ?
> If the others are ok with it I think you can send it as non-RFC, but I'd give
> it
> a few more days at least. :)
Re: [Bridge] [PATCH v4 net-next 0/6] Extend locked port feature with FDB locked flag (MAC-Auth/MAB)
On Thu, 7 Jul 2022 17:29:24 +0200 Hans Schultz wrote: > [PATCH v4 net-next 0/6] Extend locked port feature with FDB locked flag > (MAC-Auth/MAB) Let's give it a day or two for feedback but the series does not apply cleanly to net-next so a rebase & repost will be needed even if it's otherwise perfect.
Re: [Bridge] [PATCH net-next 1/1] net: dsa: mv88e6xxx: allow reading FID when handling ATU violations
On Thu, 7 Jul 2022 13:28:36 +0300 Vladimir Oltean wrote: > Make no mistake, the existing code doesn't disallow reading back the FID > during an ATU Get/Clear Violation operation, and your patch isn't > "allowing" something that wasn't disallowed. > > The documentation for the ATU FID register says that its contents is > ignored before the operation starts, and it contains the returned ATU > entry's FID after the operation completes. > > So the change simply says: don't bother to write the ATU FID register > with zero, it doesn't matter what this contains. This is probably true, > but the patch needs to do what's written on the box. > > Please note that this only even matters at all for switches with > mv88e6xxx_num_databases(chip) > 256, where MV88E6352_G1_ATU_FID is a > dedicated register which this patch avoids writing. For other switches, > the FID is embedded within MV88E6XXX_G1_ATU_CTL or MV88E6XXX_G1_ATU_OP. > So _practically_, for those switches, you are still emitting the > GET_CLR_VIOLATION ATU op with a FID of 0 whether you like it or not, and > this patch introduces a (most likely irrelevant) discrepancy between the > access methods for various switches. > > Please note that this observation is relevant for your future changes to > read back the FID too. As I said here: > https://patchwork.kernel.org/project/netdevbpf/patch/20220524152144.40527-4-schultz.hans+net...@gmail.com/#24912482 > you can't just assume that the FID lies within the MV88E6352_G1_ATU_FID > register, just look at the way it is packed within mv88e6xxx_g1_atu_op(). > You'll need to unpack it in the same way. I reckon it'll be useful to render some of this info into the commit message and adjust the subject so marking Changes Requested.
Re: [Bridge] [PATCH net-next v7 2/2] net: vxlan: Add extack support to vxlan_fdb_delete
On Thu, 12 May 2022 10:17:13 -0700 Roopa Prabhu wrote: > On 5/12/22 09:47, Jakub Kicinski wrote: > > Also the patches don't apply to net-next, again. > > that's probably because the patches were already applied. Ido just told > me abt it also > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=5dd6da25255a9d64622c693b99d7668da939a980 > > I have requested Alaa send an incremental fix (offline). Oh, I see, sorry about the confusion.
Re: [Bridge] [PATCH net-next v7 2/2] net: vxlan: Add extack support to vxlan_fdb_delete
On Thu, 12 May 2022 09:22:17 -0700 Roopa Prabhu wrote: > On 5/12/22 02:55, Alaa Mohamed wrote: > > This patch adds extack msg support to vxlan_fdb_delete and vxlan_fdb_parse. > > extack is used to propagate meaningful error msgs to the user of vxlan > > fdb netlink api > > > > Signed-off-by: Alaa Mohamed Also the patches don't apply to net-next, again.
Re: [Bridge] [PATCH net-next v6 2/2] net: vxlan: Add extack support to vxlan_fdb_delete
On Thu, 5 May 2022 17:09:58 +0200 Alaa Mohamed wrote: > + NL_SET_ERR_MSG(extack, > + "DST, VNI, ifindex and port > are mutually exclusive with NH_ID"); This continuation line still does not align with the opening bracket. Look here if your email client makes it hard to see: https://lore.kernel.org/all/ac4b6c650b6519cc56baa32ef20415460a5aa8ee.1651762830.git.eng.alaamohamedsoliman...@gmail.com/ Same story in patch 1: > static int ocelot_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[], > struct net_device *dev, > -const unsigned char *addr, u16 vid) > +const unsigned char *addr, u16 vid, > +struct netlink_ext_ack *extack) and here: > static int vxlan_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[], > struct net_device *dev, > - const unsigned char *addr, u16 vid) > + const unsigned char *addr, u16 vid, > + struct netlink_ext_ack *extack)
Re: [Bridge] [PATCH net-next v5 1/2] rtnetlink: add extack support in fdb del handlers
On Fri, 29 Apr 2022 14:49:06 +0200 Alaa Mohamed wrote: > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c > b/drivers/net/ethernet/intel/ice/ice_main.c > index fde839ef0613..3fccac358198 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -5678,7 +5678,7 @@ ice_fdb_add(struct ndmsg *ndm, struct nlattr > __always_unused *tb[], > static int > ice_fdb_del(struct ndmsg *ndm, __always_unused struct nlattr *tb[], > struct net_device *dev, const unsigned char *addr, > - __always_unused u16 vid) > + __always_unused u16 vid, struct netlink_ext_ack *extack) You need to update the kdoc on this one.
Re: [Bridge] [PATCH net-next v4 0/2] propagate extack to vxlan_fdb_delete
On Mon, 25 Apr 2022 16:25:05 +0200 Alaa Mohamed wrote: > In order to propagate extack to vxlan_fdb_delete and vxlan_fdb_parse, > add extack to .ndo_fdb_del and edit all fdb del handelers Your patches do not apply cleanly to net-next/master. Please rebase and repost. Please wait 24h between postings to allow additional feedback to come in.
Re: [Bridge] [PATCH net-next] net: bridge: switchdev: check br_vlan_group() return value
On Thu, 21 Apr 2022 13:17:51 +0300 Nikolay Aleksandrov wrote:
> On 21/04/2022 13:12, Clément Léger wrote:
> > br_vlan_group() can return NULL and thus return value must be checked
> > to avoid dereferencing a NULL pointer.
> >
> > Fixes: 6284c723d9b9 ("net: bridge: mst: Notify switchdev drivers of VLAN
> > MSTI migrations")
> > Signed-off-by: Clément Léger
> > ---
> > net/bridge/br_switchdev.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> > index 81400e0b26ac..8f3d76c751dd 100644
> > --- a/net/bridge/br_switchdev.c
> > +++ b/net/bridge/br_switchdev.c
> > @@ -354,6 +354,8 @@ static int br_switchdev_vlan_attr_replay(struct
> > net_device *br_dev,
> > attr.orig_dev = br_dev;
> >
> > vg = br_vlan_group(br);
> > + if (!vg)
> > + return 0;
> >
> > list_for_each_entry(v, >vlan_list, vlist) {
> > if (v->msti) {
>
> Acked-by: Nikolay Aleksandrov
Thanks! Applying to net tho, the patch in question is already
in Linus's tree.
Re: [Bridge] [PATCH net-next v2 0/8] net: bridge: add flush filtering support
On Tue, 12 Apr 2022 00:17:14 +0300 Nikolay Aleksandrov wrote: > > Yup, basically the policy is defined in the core, so the types are > > known. We can extract the fields from the message there, even if > > the exact meaning of the fields gets established in the callback. > > That sounds nice, but there are a few catches, f.e. some ndo_fdb > implementations > check if attributes were set, i.e. they can also interpret 0, so it will > require > additional state (either special value, bitfield or some other way of telling > them > it was actually present but 0). > Anyway I think that is orthogonal to adding the flush support, it's a nice > cleanup but can > be done separately because it will have to be done for all ndo_fdb callbacks > and I > suspect the change will grow considerably. > OTOH the flush implementation via delneigh doesn't require a new ndo_fdb call > way, > would you mind if I finish that up without the struct conversion? Not terribly, go ahead.
Re: [Bridge] [PATCH net-next v2 0/8] net: bridge: add flush filtering support
On Mon, 11 Apr 2022 23:34:23 +0300 Nikolay Aleksandrov wrote: > On 11/04/2022 22:49, Jakub Kicinski wrote: > >> all great points. My only reason to explore RTM_DELNEIGH is to see if we > >> can find a recipe to support similar bulk deletes of other objects > >> handled via rtm msgs in the future. Plus, it allows you to maintain > >> symmetry between flush requests and object delete notification msg types. > >> > >> Lets see if there are other opinions. > > > > I'd vote for reusing RTM_DELNEIGH, but that's purely based on > > OK, I'll look into the delneigh solution. Note that for backwards > compatibility > we won't be able to return proper error because rtnl_fdb_del will be called > without > a mac address, so for old kernels with new iproute2 fdb flush will return > "invalid > address" as an error. If only we had policy dump for rtnl :) Another todo item, I guess. > > intuition, I don't know this code. I'd also lean towards core > > creating struct net_bridge_fdb_flush_desc rather than piping > > raw netlink attrs thru. Lastly feels like fdb ops should find > > I don't think the struct can really be centralized, at least for the > bridge case it contains private fields which parsed attributes get mapped to, > specifically the ndm flags and state, and their maps are all mapped into > bridge-private flags. Or did you mean pass the raw attribute vals through a > struct instead of a nlattr table? Yup, basically the policy is defined in the core, so the types are known. We can extract the fields from the message there, even if the exact meaning of the fields gets established in the callback. BTW setting NLA_REJECT policy is not required, NLA_REJECT is 0 so it will be set automatically per C standard. > > a new home rather than ndos, but that's largely unrelated.. > > I like separating the ops idea. I'll add that to my bridge todo list. :) > > Thanks, > Nik >
Re: [Bridge] [PATCH net-next v2 0/8] net: bridge: add flush filtering support
On Mon, 11 Apr 2022 12:22:24 -0700 Roopa Prabhu wrote: > >> I thought about that option, but I didn't like overloading delneigh like > >> that. > >> del currently requires a mac address and we need to either signal the > >> device supports> a null mac, or we should push that verification to > >> ndo_fdb_del users. Also we'll have > > that's the only thing, overloading delneigh with a flush-behaviour > > (multi-del or whatever) > > would require to push the mac check to ndo_fdb_del implementers > > > > I don't mind going that road if others agree that we should do it through > > delneigh > > + a bit/option to signal flush, instead of a new rtm type. > > > >> attributes which are flush-specific and will work only when flushing as > >> opposed to when > >> deleting a specific mac, so handling them in the different cases can > >> become a pain. > > scratch the specific attributes, those can be adapted for both cases > > > >> MDBs will need DELMDB to be modified in a similar way. > >> > >> IMO a separate flush op is cleaner, but I don't have a strong preference. > >> This can very easily be adapted to delneigh with just a bit more > >> mechanical changes > >> if the mac check is pushed to the ndo implementers. > >> > >> FLUSHNEIGH can easily work for neighs, just need another address family > >> rtnl_register > >> that implements it, the new ndo is just for PF_BRIDGE. :) > > all great points. My only reason to explore RTM_DELNEIGH is to see if we > can find a recipe to support similar bulk deletes of other objects > handled via rtm msgs in the future. Plus, it allows you to maintain > symmetry between flush requests and object delete notification msg types. > > Lets see if there are other opinions. I'd vote for reusing RTM_DELNEIGH, but that's purely based on intuition, I don't know this code. I'd also lean towards core creating struct net_bridge_fdb_flush_desc rather than piping raw netlink attrs thru. Lastly feels like fdb ops should find a new home rather than ndos, but that's largely unrelated..
Re: [Bridge] [PATCH] decouple llc/bridge
On Thu, 7 Apr 2022 09:16:40 -0700 Stephen Hemminger wrote:
> > I was wondering why the llc code was getting compiled and it turned out
> > to be because I had bridging enabled. It turns out to only needs it for
> > a single function (llc_mac_hdr_init).
> > +static inline int llc_mac_hdr_init(struct sk_buff *skb,
> > + const unsigned char *sa, const unsigned char
> > *da)
> > +{
> > + int rc = -EINVAL;
> > +
> > + switch (skb->dev->type) {
> > + case ARPHRD_ETHER:
> > + case ARPHRD_LOOPBACK:
> > + rc = dev_hard_header(skb, skb->dev, ETH_P_802_2, da, sa,
> > +skb->len);
> > + if (rc > 0)
> > + rc = 0;
> > + break;
> > + default:
> > + break;
> > + }
> > + return rc;
> > +}
> > +
> >
nit: extra new line
> > -int llc_mac_hdr_init(struct sk_buff *skb,
> > -const unsigned char *sa, const unsigned char *da)
> > -{
> > - int rc = -EINVAL;
> > -
> > - switch (skb->dev->type) {
> > - case ARPHRD_ETHER:
> > - case ARPHRD_LOOPBACK:
> > - rc = dev_hard_header(skb, skb->dev, ETH_P_802_2, da, sa,
> > -skb->len);
> > - if (rc > 0)
> > - rc = 0;
> > - break;
> > - default:
> > - break;
> > - }
> > - return rc;
> > -}
There's also an EXPORT somewhere in this file that has to go.
> > /**
> > * llc_build_and_send_ui_pkt - unitdata request interface for upper layers
> > * @sap: sap to use
>
> You may break other uses of LLC.
>
> Why not open code as different function. I used the llc stuff because there
> were multiple copies of same code (DRY).
I didn't quite get what you mean, Stephen, would you mind restating?
Re: [Bridge] [PATCH net-next v2] veth: Support bonding events
On Wed, 30 Mar 2022 19:16:42 +0300 Nikolay Aleksandrov wrote: > > Maybe opt-out? But assuming the event is only generated on > > active/backup switch over - when would it be okay to ignore > > the notification? > > Let me just clarify, so I'm sure I've not misunderstood you. Do you mean > opt-out as in > make it default on? IMO that would be a problem, large scale setups would > suddenly > start propagating it to upper devices which would cause a lot of unnecessary > bcast. > I meant enable it only if needed, and only on specific ports (second part is > not > necessary, could be global, I think it's ok either way). I don't think any > setup > which has many upper vlans/macvlans would ever enable this. That may be. I don't have a good understanding of scenarios in which GARP is required and where it's not :) Goes without saying but the default should follow the more common scenario. > >> My concern was about the Hangbin's alternative proposal to notify all > >> bridge ports. I hope in my porposal I was able to avoid infinite loops. > > > > Possibly I'm confused as to where the notification for bridge master > > gets sent.. > > IIUC it bypasses the bridge and sends a notify peers for the veth peer so it > would > generate a grat arp (inetdev_event -> NETDEV_NOTIFY_PEERS). Ack, I was basically repeating the question of where does the notification with dev == br get generated. There is a protection in this patch to make sure the other end of the veth is not plugged into a bridge (i.e. is not a bridge port) but there can be a macvlan on top of that veth that is part of a bridge, so IIUC that check is either insufficient or unnecessary.
Re: [Bridge] [PATCH net-next v2] veth: Support bonding events
On Wed, 30 Mar 2022 13:14:12 +0200 Alexandra Winter wrote:
> >> This patch in no way addresses (2). But then, again, if we put
> >> a macvlan on top of a bridge master it will shotgun its GARPS all
> >> the same. So it's not like veth would be special in that regard.
> >>
> >> Nik, what am I missing?
> >
> > If we're talking about macvlan -> bridge -> bond then the bond flap's
> > notify peers shouldn't reach the macvlan.
Hm, right. I'm missing a step in my understanding. As you say bridge
does not seem to be re-broadcasting the event to its master. So how
does Alexandra catch this kind of an event? :S
case NETDEV_NOTIFY_PEERS:
/* propagate to peer of a bridge attached veth */
if (netif_is_bridge_master(dev)) {
IIUC bond will notify with dev == bond netdev. Where is the event with
dev == br generated?
> > Generally broadcast traffic
> > is quite expensive for the bridge, I have patches that improve on the
> > technical side (consider ports only for the same bcast domain), but you also
> > wouldn't want unnecessary bcast packets being sent around. :)
> > There are setups with tens of bond devices and propagating that to all
> > would be
> > very expensive, but most of all unnecessary. It would also hurt setups with
> > a lot of vlan devices on the bridge. There are setups with hundreds of vlans
> > and hundreds of macvlans on top, propagating it up would send it to all of
> > them and that wouldn't scale at all, these mostly have IP addresses too.
Ack.
> > Perhaps we can enable propagation on a per-port or per-bridge basis, then we
> > can avoid these walks. That is, make it opt-in.
Maybe opt-out? But assuming the event is only generated on
active/backup switch over - when would it be okay to ignore
the notification?
> >>> It also seems difficult to avoid re-bouncing the notifier.
> >>
> >> syzbot will make short work of this patch, I think the potential
> >> for infinite loops has to be addressed somehow. IIUC this is the
> >> first instance of forwarding those notifiers to a peer rather
> >> than within a upper <> lower device hierarchy which is a DAG.
>
> My concern was about the Hangbin's alternative proposal to notify all
> bridge ports. I hope in my porposal I was able to avoid infinite loops.
Possibly I'm confused as to where the notification for bridge master
gets sent..
Re: [Bridge] [PATCH net-next v2] veth: Support bonding events
Dropping the BPF people from CC and adding Hangbin, bridge and
bond/team. Please exercise some judgment when sending patches.
On Tue, 29 Mar 2022 13:40:52 +0200 Alexandra Winter wrote:
> Bonding drivers generate specific events during failover that trigger
> switch updates. When a veth device is attached to a bridge with a
> bond interface, we want external switches to learn about the veth
> devices as well.
>
> Example:
>
> | veth_a2 | veth_b2 | veth_c2 |
> --o---o--o--
> \ | /
> o oo
> veth_a1 veth_b1 veth_c1
> -
> |bridge |
> -
> bond0
> / \
>eth0 eth1
>
> In case of failover from eth0 to eth1, the netdev_notifier needs to be
> propagated, so e.g. veth_a2 can re-announce its MAC address to the
> external hardware attached to eth1.
>
> Without this patch we have seen cases where recovery after bond failover
> took an unacceptable amount of time (depending on timeout settings in the
> network).
>
> Due to the symmetric nature of veth special care is required to avoid
> endless notification loops. Therefore we only notify from a veth
> bridgeport to a peer that is not a bridgeport.
>
> References:
> Same handling as for macvlan:
> commit 4c9912556867 ("macvlan: Support bonding events")
> and vlan:
> commit 4aa5dee4d999 ("net: convert resend IGMP to notifier event")
>
> Alternatives:
> Propagate notifier events to all ports of a bridge. IIUC, this was
> rejected in https://www.spinics.net/lists/netdev/msg717292.html
My (likely flawed) reading of Nik's argument was that (1) he was
concerned about GARP storms; (2) he didn't want the GARP to be
broadcast to all ports, just the bond that originated the request.
I'm not sure I follow (1), as Hangbin said the event is rare, plus
GARP only comes from interfaces that have an IP addr, which IIUC
most bridge ports will not have.
This patch in no way addresses (2). But then, again, if we put
a macvlan on top of a bridge master it will shotgun its GARPS all
the same. So it's not like veth would be special in that regard.
Nik, what am I missing?
> It also seems difficult to avoid re-bouncing the notifier.
syzbot will make short work of this patch, I think the potential
for infinite loops has to be addressed somehow. IIUC this is the
first instance of forwarding those notifiers to a peer rather
than within a upper <> lower device hierarchy which is a DAG.
> Signed-off-by: Alexandra Winter
> ---
> drivers/net/veth.c | 53 ++
> 1 file changed, 53 insertions(+)
>
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index d29fb9759cc9..74b074453197 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -1579,6 +1579,57 @@ static void veth_setup(struct net_device *dev)
> dev->mpls_features = NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE;
> }
>
> +static bool netif_is_veth(const struct net_device *dev)
> +{
> + return (dev->netdev_ops == _netdev_ops);
brackets unnecessary
> +}
> +
> +static void veth_notify_peer(unsigned long event, const struct net_device
> *dev)
> +{
> + struct net_device *peer;
> + struct veth_priv *priv;
> +
> + priv = netdev_priv(dev);
> + peer = rtnl_dereference(priv->peer);
> + /* avoid re-bounce between 2 bridges */
> + if (!netif_is_bridge_port(peer))
> + call_netdevice_notifiers(event, peer);
> +}
> +
> +/* Called under rtnl_lock */
> +static int veth_device_event(struct notifier_block *unused,
> + unsigned long event, void *ptr)
> +{
> + struct net_device *dev, *lower;
> + struct list_head *iter;
> +
> + dev = netdev_notifier_info_to_dev(ptr);
> +
> + switch (event) {
> + case NETDEV_NOTIFY_PEERS:
> + case NETDEV_BONDING_FAILOVER:
> + case NETDEV_RESEND_IGMP:
> + /* propagate to peer of a bridge attached veth */
> + if (netif_is_bridge_master(dev)) {
Having veth sift thru bridge ports seems strange.
In fact it could be beneficial to filter the event based on
port state (whether it's forwarding, vlan etc). But looking
at details of port state outside the bridge would be even stranger.
> + iter = >adj_list.lower;
> + lower = netdev_next_lower_dev_rcu(dev, );
> + while (lower) {
> + if (netif_is_veth(lower))
> + veth_notify_peer(event, lower);
> + lower = netdev_next_lower_dev_rcu(dev, );
let's add netdev_for_each_lower_dev_rcu() rather than open-coding
> + }
> + }
> + break;
> + default:
> + break;
> + }
> + return NOTIFY_DONE;
> +}
> +
> +static struct
Re: [Bridge] [PATCH v2 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation
On Thu, 17 Mar 2022 10:39:01 +0100 Hans Schultz wrote: > This implementation for the Marvell mv88e6xxx chip series, is > based on handling ATU miss violations occurring when packets > ingress on a port that is locked. The mac address triggering > the ATU miss violation is communicated through switchdev to > the bridge module, which adds a fdb entry with the fdb locked > flag set. > Note: The locked port must have learning enabled for the ATU > miss violation to occur. > > Signed-off-by: Hans Schultz drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.c:32:5: warning: no previous prototype for ‘mv88e6xxx_switchdev_handle_atu_miss_violation’ [-Wmissing-prototypes] 32 | int mv88e6xxx_switchdev_handle_atu_miss_violation(struct mv88e6xxx_chip *chip, | ^
Re: [Bridge] [PATCH v4 net-next 04/15] net: bridge: mst: Notify switchdev drivers of MST mode changes
On Tue, 15 Mar 2022 01:25:32 +0100 Tobias Waldekranz wrote:
> Trigger a switchdev event whenever the bridge's MST mode is
> enabled/disabled. This allows constituent ports to either perform any
> required hardware config, or refuse the change if it not supported.
>
> Signed-off-by: Tobias Waldekranz
../net/bridge/br_mst.c: In function ‘br_mst_set_enabled’:
../net/bridge/br_mst.c:102:16: error: variable ‘attr’ has initializer but
incomplete type
102 | struct switchdev_attr attr = {
|^~
../net/bridge/br_mst.c:103:18: error: ‘struct switchdev_attr’ has no member
named ‘id’
103 | .id = SWITCHDEV_ATTR_ID_BRIDGE_MST,
| ^~
../net/bridge/br_mst.c:103:23: error: ‘SWITCHDEV_ATTR_ID_BRIDGE_MST’ undeclared
(first use in this function)
103 | .id = SWITCHDEV_ATTR_ID_BRIDGE_MST,
| ^~~~
../net/bridge/br_mst.c:103:23: note: each undeclared identifier is reported
only once for each function it appears in
../net/bridge/br_mst.c:103:23: warning: excess elements in struct initializer
../net/bridge/br_mst.c:103:23: note: (near initialization for ‘attr’)
../net/bridge/br_mst.c:104:18: error: ‘struct switchdev_attr’ has no member
named ‘orig_dev’
104 | .orig_dev = br->dev,
| ^~~~
../net/bridge/br_mst.c:104:29: warning: excess elements in struct initializer
104 | .orig_dev = br->dev,
| ^~
../net/bridge/br_mst.c:104:29: note: (near initialization for ‘attr’)
../net/bridge/br_mst.c:105:18: error: ‘struct switchdev_attr’ has no member
named ‘u’
105 | .u.mst = on,
| ^
../net/bridge/br_mst.c:105:26: warning: excess elements in struct initializer
105 | .u.mst = on,
| ^~
../net/bridge/br_mst.c:105:26: note: (near initialization for ‘attr’)
../net/bridge/br_mst.c:102:31: error: storage size of ‘attr’ isn’t known
102 | struct switchdev_attr attr = {
| ^~~~
../net/bridge/br_mst.c:125:15: error: implicit declaration of function
‘switchdev_port_attr_set’; did you mean ‘br_switchdev_port_vlan_del’?
[-Werror=implicit-function-declaration]
125 | err = switchdev_port_attr_set(br->dev, , extack);
| ^~~
| br_switchdev_port_vlan_del
../net/bridge/br_mst.c:102:31: warning: unused variable ‘attr’
[-Wunused-variable]
102 | struct switchdev_attr attr = {
| ^~~~
Re: [Bridge] [PATCH 1/1 net-next] net: bridge: add support for host l2 mdb entries
On Thu, 24 Feb 2022 17:29:35 +0100 Joachim Wiberg wrote: > On Thu, Feb 24, 2022 at 08:06, Jakub Kicinski wrote: > > On Thu, 24 Feb 2022 13:26:22 +0200 Nikolay Aleksandrov wrote: > >> On 23/02/2022 19:24, Joachim Wiberg wrote: > [...] > >> It would be nice to add a selftest for L2 entries. You can send it as a > >> follow-up. > > Let's wait for that, also checkpatch says you need to balance brackets > > to hold kernel coding style. > > Jakub, by "wait for that" do you mean you'd prefer I add the selftests > to this? Yes, add a selftest as a separate patch but in the same series.
Re: [Bridge] [PATCH 1/1 net-next] net: bridge: add support for host l2 mdb entries
On Thu, 24 Feb 2022 13:26:22 +0200 Nikolay Aleksandrov wrote: > On 23/02/2022 19:24, Joachim Wiberg wrote: > > This patch expands on the earlier work on layer-2 mdb entries by adding > > support for host entries. Due to the fact that host joined entries do > > not have any flag field, we infer the permanent flag when reporting the > > entries to userspace, which otherwise would be listed as 'temp'. > > > > Before patch: > > > > ~# bridge mdb add dev br0 port br0 grp 01:00:00:c0:ff:ee permanent > > Error: bridge: Flags are not allowed for host groups. > > ~# bridge mdb add dev br0 port br0 grp 01:00:00:c0:ff:ee > > Error: bridge: Only permanent L2 entries allowed. > > > > After patch: > > > > ~# bridge mdb add dev br0 port br0 grp 01:00:00:c0:ff:ee permanent > > ~# bridge mdb show > > dev br0 port br0 grp 01:00:00:c0:ff:ee permanent vid 1 > > > > Signed-off-by: Joachim Wiberg > > It would be nice to add a selftest for L2 entries. You can send it as a > follow-up. Let's wait for that, also checkpatch says you need to balance brackets to hold kernel coding style. > Acked-by: Nikolay Aleksandrov
Re: [Bridge] [PATCH net-next v4 0/5] Add support for locked bridge ports (for 802.1X)
On Wed, 23 Feb 2022 09:40:59 +0100 Hans Schultz wrote: > > You still haven't answer my question. Is the data plane clear text in > > the deployment you describe? > > Sorry, I didn't understand your question in the first instance. So as > 802.1X is only about authentication/authorization, the port when opened > for a host is like any other switch port and thus communication is in > the clear. Alright, thanks for clarifying! > I have not looked much into macsec (but know ipsec), and that is a > crypto (key) based connection mechanism, but that is a totally different > ballgame, and I think it would for most practical cases require hardware > encryption.
Re: [Bridge] [PATCH net-next v4 0/5] Add support for locked bridge ports (for 802.1X)
On Tue, 22 Feb 2022 14:28:13 +0100 Hans Schultz wrote: > This series starts by adding support for SA filtering to the bridge, > which is then allowed to be offloaded to switchdev devices. Furthermore > an offloading implementation is supplied for the mv88e6xxx driver. > > Public Local Area Networks are often deployed such that there is a > risk of unauthorized or unattended clients getting access to the LAN. > To prevent such access we introduce SA filtering, such that ports > designated as secure ports are set in locked mode, so that only > authorized source MAC addresses are given access by adding them to > the bridges forwarding database. Incoming packets with source MAC > addresses that are not in the forwarding database of the bridge are > discarded. It is then the task of user space daemons to populate the > bridge's forwarding database with static entries of authorized entities. > > The most common approach is to use the IEEE 802.1X protocol to take > care of the authorization of allowed users to gain access by opening > for the source address of the authorized host. > > With the current use of the bridge parameter in hostapd, there is > a limitation in using this for IEEE 802.1X port authentication. It > depends on hostapd attaching the port on which it has a successful > authentication to the bridge, but that only allows for a single > authentication per port. This patch set allows for the use of > IEEE 802.1X port authentication in a more general network context with > multiple 802.1X aware hosts behind a single port as depicted, which is > a commonly used commercial use-case, as it is only the number of > available entries in the forwarding database that limits the number of > authenticated clients. > > ++ > || > | Bridge/Authenticator | > || > +-+--+ >802.1X port | > | > | > +--+---+ > | | > | Hub/Switch | > | | > +-+--+-+ >| | > +--+--++--+--+ > | || | > Hosts | a || b | . . . > | || | > +-++-+ > > The 802.1X standard involves three different components, a Supplicant > (Host), an Authenticator (Network Access Point) and an Authentication > Server which is typically a Radius server. This patch set thus enables > the bridge module together with an authenticator application to serve > as an Authenticator on designated ports. > > > For the bridge to become an IEEE 802.1X Authenticator, a solution using > hostapd with the bridge driver can be found at > https://github.com/westermo/hostapd/tree/bridge_driver . > > > The relevant components work transparently in relation to if it is the > bridge module or the offloaded switchcore case that is in use. You still haven't answer my question. Is the data plane clear text in the deployment you describe?
Re: [Bridge] [PATCH] net: bridge: multicast: notify switchdev driver whenever MC processing gets disabled
On Fri, 11 Feb 2022 15:14:26 +0200 Oleksandr Mazur wrote: > Whenever bridge driver hits the max capacity of MDBs, it disables > the MC processing (by setting corresponding bridge option), but never > notifies switchdev about such change (the notifiers are called only upon > explicit setting of this option, through the registered netlink interface). > > This could lead to situation when Software MDB processing gets disabled, > but this event never gets offloaded to the underlying Hardware. > > Fix this by adding a notify message in such case. Any comments on this one? We have drivers offloading mdb so presumably this should have a Fixes tag and go to net, right?
Re: [Bridge] [PATCH bpf-next v2] net: don't include filter.h from net/sock.h
On Tue, 28 Dec 2021 17:33:39 -0800 Florian Fainelli wrote: > It would be nice if we used the number of files rebuilt because of a > header file change as another metric that the kernel is evaluated with > from release to release (or even on a commit by commit basis). Food for > thought. Maybe Andy has some thoughts, he has been working on dropping unnecessary includes of kernel.h, it seems. It'd be cool to plug something that'd warn us about significant increases in dependencies into the patchwork build bot. I have one more small series which un-includes uapi/bpf.h from netdevice.h at which point I hope we'll be largely in the clear from build bot performance perspective.
[Bridge] [PATCH bpf-next v2] net: don't include filter.h from net/sock.h
sock.h is pretty heavily used (5k objects rebuilt on x86 after it's touched). We can drop the include of filter.h from it and add a forward declaration of struct sk_filter instead. This decreases the number of rebuilt objects when bpf.h is touched from ~5k to ~1k. There's a lot of missing includes this was masking. Primarily in networking tho, this time. Acked-by: Marc Kleine-Budde Signed-off-by: Jakub Kicinski --- v2: https://lore.kernel.org/all/20211228192519.386913-1-k...@kernel.org/ - fix build in bond on ia64 - fix build in ip6_fib with randconfig CC: mar...@holtmann.org CC: johan.hedb...@gmail.com CC: luiz.de...@gmail.com CC: dledf...@redhat.com CC: j...@ziepe.ca CC: mustafa.ism...@intel.com CC: shiraz.sal...@intel.com CC: l...@kernel.org CC: ap420...@gmail.com CC: w...@grandegger.com CC: woojung@microchip.com CC: and...@lunn.ch CC: vivien.dide...@gmail.com CC: f.faine...@gmail.com CC: olte...@gmail.com CC: george.mccollis...@gmail.com CC: michael.c...@broadcom.com CC: jesse.brandeb...@intel.com CC: anthony.l.ngu...@intel.com CC: a...@kernel.org CC: dan...@iogearbox.net CC: h...@kernel.org CC: john.fastab...@gmail.com CC: tar...@nvidia.com CC: sae...@nvidia.com CC: ecree.xil...@gmail.com CC: habetsm.xil...@gmail.com CC: jreu...@yaina.de CC: dsah...@kernel.org CC: kv...@codeaurora.org CC: pks...@realtek.com CC: trond.mykleb...@hammerspace.com CC: anna.schuma...@netapp.com CC: v...@zeniv.linux.org.uk CC: and...@kernel.org CC: mcg...@kernel.org CC: keesc...@chromium.org CC: yzai...@google.com CC: niko...@nvidia.com CC: j...@nvidia.com CC: wint...@linux.ibm.com CC: wen...@linux.ibm.com CC: pa...@netfilter.org CC: kad...@netfilter.org CC: f...@strlen.de CC: r...@linux-mips.org CC: j...@mojatatu.com CC: xiyou.wangc...@gmail.com CC: kgr...@linux.ibm.com CC: sgarz...@redhat.com CC: steffen.klass...@secunet.com CC: herb...@gondor.apana.org.au CC: a...@arndb.de CC: linux-blueto...@vger.kernel.org CC: linux-r...@vger.kernel.org CC: linux-...@vger.kernel.org CC: intel-wired-...@lists.osuosl.org CC: b...@vger.kernel.org CC: linux-h...@vger.kernel.org CC: ath...@lists.infradead.org CC: linux-wirel...@vger.kernel.org CC: linux-...@vger.kernel.org CC: linux-fsde...@vger.kernel.org CC: bridge@lists.linux-foundation.org CC: linux-decnet-u...@lists.sourceforge.net CC: linux-s...@vger.kernel.org CC: netfilter-de...@vger.kernel.org CC: coret...@netfilter.org CC: virtualizat...@lists.linux-foundation.org --- drivers/bluetooth/btqca.c | 1 + drivers/infiniband/core/cache.c | 1 + drivers/infiniband/hw/irdma/ctrl.c| 2 ++ drivers/infiniband/hw/irdma/uda.c | 2 ++ drivers/infiniband/hw/mlx5/doorbell.c | 1 + drivers/infiniband/hw/mlx5/qp.c | 1 + drivers/net/amt.c | 1 + drivers/net/appletalk/ipddp.c | 1 + drivers/net/bonding/bond_main.c | 1 + drivers/net/can/usb/peak_usb/pcan_usb.c | 1 + drivers/net/dsa/microchip/ksz8795.c | 1 + drivers/net/dsa/xrs700x/xrs700x.c | 1 + drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 1 + drivers/net/ethernet/huawei/hinic/hinic_tx.c | 1 + drivers/net/ethernet/intel/ice/ice_devlink.c | 2 ++ drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 2 ++ drivers/net/ethernet/intel/igc/igc_xdp.c | 1 + drivers/net/ethernet/mellanox/mlx4/en_netdev.c| 1 + drivers/net/ethernet/mellanox/mlx5/core/en/qos.c | 1 + drivers/net/ethernet/sfc/efx.c| 1 + drivers/net/ethernet/sfc/efx_channels.c | 1 + drivers/net/ethernet/sfc/efx_common.c | 1 + drivers/net/hamradio/hdlcdrv.c| 1 + drivers/net/hamradio/scc.c| 1 + drivers/net/loopback.c| 1 + drivers/net/vrf.c | 1 + drivers/net/wireless/ath/ath11k/debugfs.c | 2 ++ drivers/net/wireless/realtek/rtw89/debug.c| 2 ++ fs/nfs/dir.c | 1 + fs/nfs/fs_context.c | 1 + fs/select.c | 1 + include/linux/bpf_local_storage.h | 1 + include/linux/dsa/loop.h | 1 + include/net/ipv6.h| 2 ++ include/net/route.h | 1 + include/net/sock.h| 2 +- include/net/xdp_sock.h| 1 + kernel/sysctl.c | 1 + net/bluetooth/bnep/sock.c | 1 + net/bluetooth/eir.h | 2 ++ net/bluetooth/hidp/sock.c | 1 + net/bluetooth/l2cap_sock.c| 1 + net/bridge/br_ioctl.c | 1 + net/caif/caif_socket.c| 1 + net/core/devlink.c
[Bridge] [PATCH bpf-next] net: don't include filter.h from net/sock.h
sock.h is pretty heavily used (5k objects rebuilt on x86 after it's touched). We can drop the include of filter.h from it and add a forward declaration of struct sk_filter instead. This decreases the number of rebuilt objects when bpf.h is touched from ~5k to ~1k. There's a lot of missing includes this was masking. Primarily in networking tho, this time. Signed-off-by: Jakub Kicinski --- CC: mar...@holtmann.org CC: johan.hedb...@gmail.com CC: luiz.de...@gmail.com CC: dledf...@redhat.com CC: j...@ziepe.ca CC: mustafa.ism...@intel.com CC: shiraz.sal...@intel.com CC: l...@kernel.org CC: ap420...@gmail.com CC: w...@grandegger.com CC: m...@pengutronix.de CC: woojung@microchip.com CC: unglinuxdri...@microchip.com CC: and...@lunn.ch CC: vivien.dide...@gmail.com CC: f.faine...@gmail.com CC: olte...@gmail.com CC: george.mccollis...@gmail.com CC: michael.c...@broadcom.com CC: jesse.brandeb...@intel.com CC: anthony.l.ngu...@intel.com CC: a...@kernel.org CC: dan...@iogearbox.net CC: h...@kernel.org CC: john.fastab...@gmail.com CC: tar...@nvidia.com CC: sae...@nvidia.com CC: ecree.xil...@gmail.com CC: habetsm.xil...@gmail.com CC: jreu...@yaina.de CC: dsah...@kernel.org CC: kv...@codeaurora.org CC: pks...@realtek.com CC: trond.mykleb...@hammerspace.com CC: anna.schuma...@netapp.com CC: v...@zeniv.linux.org.uk CC: and...@kernel.org CC: mcg...@kernel.org CC: keesc...@chromium.org CC: yzai...@google.com CC: niko...@nvidia.com CC: j...@nvidia.com CC: wint...@linux.ibm.com CC: wen...@linux.ibm.com CC: pa...@netfilter.org CC: kad...@netfilter.org CC: f...@strlen.de CC: r...@linux-mips.org CC: j...@mojatatu.com CC: xiyou.wangc...@gmail.com CC: kgr...@linux.ibm.com CC: sgarz...@redhat.com CC: steffen.klass...@secunet.com CC: herb...@gondor.apana.org.au CC: a...@arndb.de CC: linux-blueto...@vger.kernel.org CC: linux-r...@vger.kernel.org CC: linux-...@vger.kernel.org CC: intel-wired-...@lists.osuosl.org CC: b...@vger.kernel.org CC: linux-h...@vger.kernel.org CC: ath...@lists.infradead.org CC: linux-wirel...@vger.kernel.org CC: linux-...@vger.kernel.org CC: linux-fsde...@vger.kernel.org CC: bridge@lists.linux-foundation.org CC: linux-decnet-u...@lists.sourceforge.net CC: linux-s...@vger.kernel.org CC: netfilter-de...@vger.kernel.org CC: coret...@netfilter.org CC: virtualizat...@lists.linux-foundation.org --- drivers/bluetooth/btqca.c | 1 + drivers/infiniband/core/cache.c | 1 + drivers/infiniband/hw/irdma/ctrl.c| 2 ++ drivers/infiniband/hw/irdma/uda.c | 2 ++ drivers/infiniband/hw/mlx5/doorbell.c | 1 + drivers/infiniband/hw/mlx5/qp.c | 1 + drivers/net/amt.c | 1 + drivers/net/appletalk/ipddp.c | 1 + drivers/net/can/usb/peak_usb/pcan_usb.c | 1 + drivers/net/dsa/microchip/ksz8795.c | 1 + drivers/net/dsa/xrs700x/xrs700x.c | 1 + drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 1 + drivers/net/ethernet/huawei/hinic/hinic_tx.c | 1 + drivers/net/ethernet/intel/ice/ice_devlink.c | 2 ++ drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 2 ++ drivers/net/ethernet/intel/igc/igc_xdp.c | 1 + drivers/net/ethernet/mellanox/mlx4/en_netdev.c| 1 + drivers/net/ethernet/mellanox/mlx5/core/en/qos.c | 1 + drivers/net/ethernet/sfc/efx.c| 1 + drivers/net/ethernet/sfc/efx_channels.c | 1 + drivers/net/ethernet/sfc/efx_common.c | 1 + drivers/net/hamradio/hdlcdrv.c| 1 + drivers/net/hamradio/scc.c| 1 + drivers/net/loopback.c| 1 + drivers/net/vrf.c | 1 + drivers/net/wireless/ath/ath11k/debugfs.c | 2 ++ drivers/net/wireless/realtek/rtw89/debug.c| 2 ++ fs/nfs/dir.c | 1 + fs/nfs/fs_context.c | 1 + fs/select.c | 1 + include/linux/bpf_local_storage.h | 1 + include/linux/dsa/loop.h | 1 + include/net/ipv6.h| 2 ++ include/net/route.h | 1 + include/net/sock.h| 2 +- include/net/xdp_sock.h| 1 + kernel/sysctl.c | 1 + net/bluetooth/bnep/sock.c | 1 + net/bluetooth/eir.h | 2 ++ net/bluetooth/hidp/sock.c | 1 + net/bluetooth/l2cap_sock.c| 1 + net/bridge/br_ioctl.c | 1 + net/caif/caif_socket.c| 1 + net/core/devlink.c| 1 + net/core/flow_dissector.c | 1 + net/core/lwt_bpf.c| 1 + net/core/sock_diag.c
Re: [Bridge] [PATCH net 2/2] net: bridge: Get SIOCGIFBR/SIOCSIFBR ioctl working in compat mode
On Thu, 23 Dec 2021 18:50:30 +0100 Remi Pommarel wrote:
> On Thu, Dec 23, 2021 at 08:59:44AM -0800, Jakub Kicinski wrote:
> > On Thu, 23 Dec 2021 16:31:39 +0100 Remi Pommarel wrote:
> > > In compat mode SIOC{G,S}IFBR ioctls were only supporting
> > > BRCTL_GET_VERSION returning an artificially version to spur userland
> > > tool to use SIOCDEVPRIVATE instead. But some userland tools ignore that
> > > and use SIOC{G,S}IFBR unconditionally as seen with busybox's brctl.
> > >
> > > Example of non working 32-bit brctl with CONFIG_COMPAT=y:
> > > $ brctl show
> > > brctl: SIOCGIFBR: Invalid argument
> > >
> > > Example of fixed 32-bit brctl with CONFIG_COMPAT=y:
> > > $ brctl show
> > > bridge name bridge id STP enabled interfaces
> > > br0
> > >
> > > Signed-off-by: Remi Pommarel
> > > Co-developed-by: Arnd Bergmann
> > > Signed-off-by: Arnd Bergmann
> >
> > Since Arnd said this is not supposed to be backported I presume it
> > should go to net-next?
>
> Yes, out of curiosity, is it appropriate to mix "[PATCH net]" and
> "[PATCH net-next]" in the same serie ?
It's not, mixing makes it quite hard to know what's needed where.
Also hard to automate things on our end. Let me pick out the first
patch, I'll be sending a PR to Linus shortly and then merge net into
net-next. At which point you'll be able to rebase on top of net-next
and resend just the second patch for net-next..
Re: [Bridge] [PATCH net 2/2] net: bridge: Get SIOCGIFBR/SIOCSIFBR ioctl working in compat mode
On Thu, 23 Dec 2021 16:31:39 +0100 Remi Pommarel wrote:
> In compat mode SIOC{G,S}IFBR ioctls were only supporting
> BRCTL_GET_VERSION returning an artificially version to spur userland
> tool to use SIOCDEVPRIVATE instead. But some userland tools ignore that
> and use SIOC{G,S}IFBR unconditionally as seen with busybox's brctl.
>
> Example of non working 32-bit brctl with CONFIG_COMPAT=y:
> $ brctl show
> brctl: SIOCGIFBR: Invalid argument
>
> Example of fixed 32-bit brctl with CONFIG_COMPAT=y:
> $ brctl show
> bridge name bridge id STP enabled interfaces
> br0
>
> Signed-off-by: Remi Pommarel
> Co-developed-by: Arnd Bergmann
> Signed-off-by: Arnd Bergmann
Since Arnd said this is not supposed to be backported I presume it
should go to net-next?
Re: [Bridge] [PATCH] net/bridge: replace simple_strtoul to kstrtol
On Mon, 22 Nov 2021 12:17:39 +0200 Nikolay Aleksandrov wrote: > > # ip link add name br0 type bridge vlan_filtering 1 > > # echo "0x88a8" > /sys/class/net/br0/bridge/vlan_protocol > > bash: echo: write error: Invalid argument > > Good catch, Bernard please send a revert. Thanks. Doesn't look like he did, would you mind taking over?
Re: [Bridge] [PATCH net-next] net: bridge: make use of helper netif_is_bridge_master()
On Tue, 12 Oct 2021 23:18:09 +0900 Kyungrok Chung wrote: > Make use of netdev helper functions to improve code readability. > Replace 'dev->priv_flags & IFF_EBRIDGE' with netif_is_bridge_master(dev). > > Signed-off-by: Kyungrok Chung Why leave these out? net/batman-adv/multicast.c: } while (upper && !(upper->priv_flags & IFF_EBRIDGE)); net/core/rtnetlink.c: !(dev->priv_flags & IFF_EBRIDGE))
Re: [Bridge] [PATCH v3 net-next] net: bridge: change return type of br_handle_ingress_vlan_tunnel
On Mon, 23 Aug 2021 13:25:20 +0300 Nikolay Aleksandrov wrote: > On 23/08/2021 13:21, Kangmin Park wrote: > > br_handle_ingress_vlan_tunnel() is only referenced in > > br_handle_frame(). If br_handle_ingress_vlan_tunnel() is called and > > return non-zero value, goto drop in br_handle_frame(). > > > > But, br_handle_ingress_vlan_tunnel() always return 0. So, the > > routines that check the return value and goto drop has no meaning. > > > > Therefore, change return type of br_handle_ingress_vlan_tunnel() to > > void and remove if statement of br_handle_frame(). > > > > Signed-off-by: Kangmin Park > > Looks good to me, > Acked-by: Nikolay Aleksandrov Applied, thanks!
Re: [Bridge] [PATCH 0/4] Remove rtnetlink_send() in rtnetlink
On Mon, 19 Jul 2021 20:21:54 +0800, Yajun Deng wrote: > rtnetlink_send() is similar to rtnl_notify(), there is no need for two > functions to do the same thing. we can remove rtnetlink_send() and > modify rtnl_notify() to adapt more case. > > Patch1: remove rtnetlink_send() modify rtnl_notify() to adapt > more case in rtnetlink. > Path2,Patch3: Adjustment parameters in rtnl_notify(). > Path4: rtnetlink_send() already removed, use rtnl_notify() instead > of rtnetlink_send(). You can't break compilation in between patches. Each step of the series (each patch) must be self-contained, build, and work correctly. Otherwise bisection becomes a nightmare. Please also post series as a thread (patches in reply to the cover letter), it seems that patchwork did not group the patches correctly here.
Re: [Bridge] [PATCH net-next v2 0/2] net: bridge: mcast: simplify allow/block EHT code
On Mon, 15 Mar 2021 19:13:40 +0200 Nikolay Aleksandrov wrote: > There are no functional changes. That appears to indeed be the case: Reviewed-by: Jakub Kicinski
Re: [Bridge] [PATCH net-next v2 0/4] bridge: mrp: Extend br_mrp_switchdev_*
On Tue, 2 Feb 2021 21:06:49 +0100 Horatiu Vultur wrote: > The 02/02/2021 11:50, Jakub Kicinski wrote: > > On Tue, 2 Feb 2021 08:40:02 +0100 Rasmus Villemoes wrote: > > > I am planning to test these, but it's unlikely I'll get around to it > > > this week unfortunately. > > > > Horatiu are you okay with deferring the series until Rasmus validates? > > Given none of this HW is upstream now (AFAIU) this is an awkward set > > to handle. Having a confirmation from Rasmus would make us a little bit > > more comfortable. > > It is perfectly fine for me to wait for Rasmus to validate this series. > Also I have started to have a look how to implement the switchdev calls > for Ocelot driver. I might have something by the end of the week, but > lets see. Great, thanks! Please repost once we got the confirmation.
Re: [Bridge] [PATCH net-next v2 0/4] bridge: mrp: Extend br_mrp_switchdev_*
On Tue, 2 Feb 2021 08:40:02 +0100 Rasmus Villemoes wrote: > On 30/01/2021 04.01, Jakub Kicinski wrote: > > On Wed, 27 Jan 2021 21:52:37 +0100 Horatiu Vultur wrote: > >> This patch series extends MRP switchdev to allow the SW to have a better > >> understanding if the HW can implement the MRP functionality or it needs > >> to help the HW to run it. There are 3 cases: > > >> v2: > >> - fix typos in comments and in commit messages > >> - remove some of the comments > >> - move repeated code in helper function > >> - fix issue when deleting a node when sw_backup was true > > > > Folks who were involved in previous MRP conversations - does this look > > good to you? Anyone planning to test? > > I am planning to test these, but it's unlikely I'll get around to it > this week unfortunately. Horatiu are you okay with deferring the series until Rasmus validates? Given none of this HW is upstream now (AFAIU) this is an awkward set to handle. Having a confirmation from Rasmus would make us a little bit more comfortable.
Re: [Bridge] [PATCH net-next 0/2] net: bridge: drop hosts limit sysfs and add a comment
On Fri, 29 Jan 2021 13:55:24 +0200 Nikolay Aleksandrov wrote: > On 29/01/2021 13:51, Nikolay Aleksandrov wrote: > > From: Nikolay Aleksandrov > > > > Hi, > > As recently discussed[1] we should stop extending the bridge sysfs > > support for new options and move to using netlink only, so patch 01 > > drops the recently added hosts limit sysfs support which is still in > > net-next only and patch 02 adds comments in br_sysfs_br/if.c to warn > > against adding new sysfs options. > > > > Thanks, > > Nik > > > > Nikolay Aleksandrov (2): > > net: bridge: mcast: drop hosts limit sysfs support > > net: bridge: add warning comments to avoid extending sysfs > > > > net/bridge/br_sysfs_br.c | 4 > > net/bridge/br_sysfs_if.c | 30 -- > > 2 files changed, 8 insertions(+), 26 deletions(-) > > > > Oops :) the [1] addendum should be: > > [1] > https://lore.kernel.org/netdev/20210128105201.7c6be...@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/T/#mda7265b2e57b52bdab863f286efa85291cf83822 > > Since this is in the cover letter I don't think v2 is needed. > Please let me know if you'd like me to resend. Seems that vger ate the cover letter completely, but not the patches, so I was putting the merge commit message together by hand, anyway :) Applied, thanks!
Re: [Bridge] [PATCH net-next v2 0/4] bridge: mrp: Extend br_mrp_switchdev_*
On Wed, 27 Jan 2021 21:52:37 +0100 Horatiu Vultur wrote: > This patch series extends MRP switchdev to allow the SW to have a better > understanding if the HW can implement the MRP functionality or it needs > to help the HW to run it. There are 3 cases: > - when HW can't implement at all the functionality. > - when HW can implement a part of the functionality but needs the SW > implement the rest. For example if it can't detect when it stops > receiving MRP Test frames but it can copy the MRP frames to CPU to > allow the SW to determine this. Another example is generating the MRP > Test frames. If HW can't do that then the SW is used as backup. > - when HW can implement completely the functionality. > > So, initially the SW tries to offload the entire functionality in HW, if > that fails it tries offload parts of the functionality in HW and use the > SW as helper and if also this fails then MRP can't run on this HW. > > v2: > - fix typos in comments and in commit messages > - remove some of the comments > - move repeated code in helper function > - fix issue when deleting a node when sw_backup was true Folks who were involved in previous MRP conversations - does this look good to you? Anyone planning to test?
Re: [Bridge] [PATCH net-next v2 0/2] net: bridge: multicast: per-port EHT hosts limit
On Thu, 28 Jan 2021 11:12:26 +0200 Nikolay Aleksandrov wrote: > On 28/01/2021 03:42, Jakub Kicinski wrote: > > On Tue, 26 Jan 2021 11:35:31 +0200 Nikolay Aleksandrov wrote: > >> From: Nikolay Aleksandrov > >> > >> Hi, > >> This set adds a simple configurable per-port EHT tracked hosts limit. > >> Patch 01 adds a default limit of 512 tracked hosts per-port, since the EHT > >> changes are still only in net-next that shouldn't be a problem. Then > >> patch 02 adds the ability to configure and retrieve the hosts limit > >> and to retrieve the current number of tracked hosts per port. > >> Let's be on the safe side and limit the number of tracked hosts by > >> default while allowing the user to increase that limit if needed. > > > > Applied, thanks! > > > > I'm curious that you add those per-port sysfs files, is this a matter > > of policy for the bridge? Seems a bit like a waste of memory at this > > point. > > Indeed, that's how historically new port and bridge options are added. > They're all exposed via sysfs. I wonder if we should just draw the line > and continue with netlink-only attributes. Perhaps we should add a comment > about it for anyone adding new ones. > > Since this is in net-next I can send a follow up to drop the sysfs part > and another to add that comment. > > WDYT? SGTM!
Re: [Bridge] [PATCH net-next v2 0/2] net: bridge: multicast: per-port EHT hosts limit
On Tue, 26 Jan 2021 11:35:31 +0200 Nikolay Aleksandrov wrote: > From: Nikolay Aleksandrov > > Hi, > This set adds a simple configurable per-port EHT tracked hosts limit. > Patch 01 adds a default limit of 512 tracked hosts per-port, since the EHT > changes are still only in net-next that shouldn't be a problem. Then > patch 02 adds the ability to configure and retrieve the hosts limit > and to retrieve the current number of tracked hosts per port. > Let's be on the safe side and limit the number of tracked hosts by > default while allowing the user to increase that limit if needed. Applied, thanks! I'm curious that you add those per-port sysfs files, is this a matter of policy for the bridge? Seems a bit like a waste of memory at this point.
Re: [Bridge] [PATCH] bridge: Use PTR_ERR_OR_ZERO instead if(IS_ERR(...)) + PTR_ERR
On Mon, 25 Jan 2021 10:23:00 +0200 Nikolay Aleksandrov wrote: > On 25/01/2021 04:39, Jiapeng Zhong wrote: > > coccicheck suggested using PTR_ERR_OR_ZERO() and looking at the code. > > > > Fix the following coccicheck warnings: > > > > ./net/bridge/br_multicast.c:1295:7-13: WARNING: PTR_ERR_OR_ZERO can be > > used. > > > > Reported-by: Abaci > > Signed-off-by: Jiapeng Zhong > > --- > > net/bridge/br_multicast.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c > > index 257ac4e..2229d10 100644 > > --- a/net/bridge/br_multicast.c > > +++ b/net/bridge/br_multicast.c > > @@ -1292,7 +1292,7 @@ static int br_multicast_add_group(struct net_bridge > > *br, > > pg = __br_multicast_add_group(br, port, group, src, filter_mode, > > igmpv2_mldv1, false); > > /* NULL is considered valid for host joined groups */ > > - err = IS_ERR(pg) ? PTR_ERR(pg) : 0; > > + err = PTR_ERR_OR_ZERO(pg); > > spin_unlock(>multicast_lock); > > > > return err; > > > > This should be targeted at net-next. > Acked-by: Nikolay Aleksandrov Applied, thanks!
Re: [Bridge] [PATCH] bridge: Use PTR_ERR_OR_ZERO instead if(IS_ERR(...)) + PTR_ERR
On Fri, 22 Jan 2021 17:32:13 +0800 Jiapeng Zhong wrote: > coccicheck suggested using PTR_ERR_OR_ZERO() and looking at the code. > > Fix the following coccicheck warnings: > > ./net/bridge/br_multicast.c:1295:7-13: WARNING: PTR_ERR_OR_ZERO can be > used. > > Reported-by: Abaci > Signed-off-by: Jiapeng Zhong You need to CC netdev
Re: [Bridge] [PATCH v4 net-next] net: bridge: check vlan with eth_type_vlan() method
On Mon, 18 Jan 2021 13:55:11 +0200 Nikolay Aleksandrov wrote: > On 17/01/2021 10:09, menglong8.d...@gmail.com wrote: > > From: Menglong Dong > > > > Replace some checks for ETH_P_8021Q and ETH_P_8021AD with > > eth_type_vlan(). > > > > Signed-off-by: Menglong Dong > > Acked-by: Nikolay Aleksandrov Applied, thanks!
Re: [Bridge] [PATCH v3 net-next] net: bridge: check vlan with eth_type_vlan() method
On Thu, 14 Jan 2021 20:41:31 -0800 menglong8.d...@gmail.com wrote:
> - if (data[IFLA_BR_VLAN_PROTOCOL]) {
> - switch (nla_get_be16(data[IFLA_BR_VLAN_PROTOCOL])) {
> - case htons(ETH_P_8021Q):
> - case htons(ETH_P_8021AD):
> - break;
> - default:
> - return -EPROTONOSUPPORT;
> - }
> + if (data[IFLA_BR_VLAN_PROTOCOL] &&
> + !eth_type_vlan(nla_get_be16(data[IFLA_BR_VLAN_PROTOCOL]))) {
> + return -EPROTONOSUPPORT;
> }
The curly brackets are no longer necessary here, since it's a single
line expression.
Re: [Bridge] [PATCH v2 net-next] net: bridge: check vlan with eth_type_vlan() method
On Thu, 14 Jan 2021 18:23:44 -0800 menglong8.d...@gmail.com wrote: > From: Menglong Dong > > Replace some checks for ETH_P_8021Q and ETH_P_8021AD with > eth_type_vlan(). > > Signed-off-by: Menglong Dong This adds a new warning when built with W=1 C=1: net/bridge/br_vlan.c:920:28: warning: incorrect type in argument 1 (different base types) net/bridge/br_vlan.c:920:28:expected restricted __be16 [usertype] ethertype net/bridge/br_vlan.c:920:28:got unsigned long val
Re: [Bridge] [PATCH net-next] net/bridge: fix misspellings using codespell tool
On Thu, 7 Jan 2021 20:03:49 -0800 Randy Dunlap wrote: > On 1/7/21 6:53 PM, menglong8.d...@gmail.com wrote: > > From: Menglong Dong > > > > Some typos are found out by codespell tool: > > > > $ codespell ./net/bridge/ > > ./net/bridge/br_stp.c:604: permanant ==> permanent > > ./net/bridge/br_stp.c:605: persistance ==> persistence > > ./net/bridge/br.c:125: underlaying ==> underlying > > ./net/bridge/br_input.c:43: modue ==> mode > > ./net/bridge/br_mrp.c:828: Determin ==> Determine > > ./net/bridge/br_mrp.c:848: Determin ==> Determine > > ./net/bridge/br_mrp.c:897: Determin ==> Determine > > > > Fix typos found by codespell. > > > > Signed-off-by: Menglong Dong > > LGTM. Thanks. > > Acked-by: Randy Dunlap Applied, thanks!
Re: [Bridge] [PATCH v3] bridge: Fix a deadlock when enabling multicast snooping
On Sat, 5 Dec 2020 10:56:45 +0200 Nikolay Aleksandrov wrote:
> On 05/12/2020 01:56, Joseph Huang wrote:
> > When enabling multicast snooping, bridge module deadlocks on multicast_lock
> > if 1) IPv6 is enabled, and 2) there is an existing querier on the same L2
> > network.
> >
> > The deadlock was caused by the following sequence: While holding the lock,
> > br_multicast_open calls br_multicast_join_snoopers, which eventually causes
> > IP stack to (attempt to) send out a Listener Report (in igmp6_join_group).
> > Since the destination Ethernet address is a multicast address, br_dev_xmit
> > feeds the packet back to the bridge via br_multicast_rcv, which in turn
> > calls br_multicast_add_group, which then deadlocks on multicast_lock.
> >
> > The fix is to move the call br_multicast_join_snoopers outside of the
> > critical section. This works since br_multicast_join_snoopers only deals
> > with IP and does not modify any multicast data structures of the bridge,
> > so there's no need to hold the lock.
> >
> > Steps to reproduce:
> > 1. sysctl net.ipv6.conf.all.force_mld_version=1
> > 2. have another querier
> > 3. ip link set dev bridge type bridge mcast_snooping 0 && \
> >ip link set dev bridge type bridge mcast_snooping 1 < deadlock >
> >
> > A typical call trace looks like the following:
> > Fixes: 4effd28c1245 ("bridge: join all-snoopers multicast address")
> > Signed-off-by: Joseph Huang
>
> LGTM, thanks!
> Acked-by: Nikolay Aleksandrov
Applied, thank you!
Re: [Bridge] [PATCH] bridge: Fix a deadlock when enabling multicast snooping
On Fri, 4 Dec 2020 01:34:57 +0200 Nikolay Aleksandrov wrote: > Please add a comment why it's needed, so we won't wonder about it > later. And also include the trace in the commit message so we'd have > it. And please drop the empty line between the Fixes tag and your sign-off. We like tags all clustered together at the end, sort of reverse of email headers.
Re: [Bridge] [PATCH] bridge: Fix a deadlock when enabling multicast snooping
On Tue, 1 Dec 2020 16:40:47 -0500 Joseph Huang wrote:
> When enabling multicast snooping, bridge module deadlocks on multicast_lock
> if 1) IPv6 is enabled, and 2) there is an existing querier on the same L2
> network.
>
> The deadlock was caused by the following sequence: While holding the lock,
> br_multicast_open calls br_multicast_join_snoopers, which eventually causes
> IP stack to (attempt to) send out a Listener Report (in igmp6_join_group).
> Since the destination Ethernet address is a multicast address, br_dev_xmit
> feeds the packet back to the bridge via br_multicast_rcv, which in turn
> calls br_multicast_add_group, which then deadlocks on multicast_lock.
>
> The fix is to move the call br_multicast_join_snoopers outside of the
> critical section. This works since br_multicast_join_snoopers only deals
> with IP and does not modify any multicast data structures of the bridge,
> so there's no need to hold the lock.
>
> Fixes: 4effd28c1245 ("bridge: join all-snoopers multicast address")
>
> Signed-off-by: Joseph Huang
Nik, Linus - how does this one look?
Re: [Bridge] [PATCH net] net: bridge: Fix a warning when del bridge sysfs
On Tue, 1 Dec 2020 22:01:14 +0800 Wang Hai wrote: > If adding bridge sysfs fails, br->ifobj will be NULL, there is no > need to delete its non-existent sysfs when deleting the bridge device, > otherwise, it will cause a warning. So, when br->ifobj == NULL, > directly return can fix this bug. > > br_sysfs_addbr: can't create group bridge4/bridge > [ cut here ] > sysfs group 'bridge' not found for kobject 'bridge4' > WARNING: CPU: 2 PID: 9004 at fs/sysfs/group.c:279 sysfs_remove_group > fs/sysfs/group.c:279 [inline] > WARNING: CPU: 2 PID: 9004 at fs/sysfs/group.c:279 > sysfs_remove_group+0x153/0x1b0 fs/sysfs/group.c:270 > Modules linked in: iptable_nat > ... > Call Trace: > br_dev_delete+0x112/0x190 net/bridge/br_if.c:384 > br_dev_newlink net/bridge/br_netlink.c:1381 [inline] > br_dev_newlink+0xdb/0x100 net/bridge/br_netlink.c:1362 > __rtnl_newlink+0xe11/0x13f0 net/core/rtnetlink.c:3441 > rtnl_newlink+0x64/0xa0 net/core/rtnetlink.c:3500 > rtnetlink_rcv_msg+0x385/0x980 net/core/rtnetlink.c:5562 > netlink_rcv_skb+0x134/0x3d0 net/netlink/af_netlink.c:2494 > netlink_unicast_kernel net/netlink/af_netlink.c:1304 [inline] > netlink_unicast+0x4a0/0x6a0 net/netlink/af_netlink.c:1330 > netlink_sendmsg+0x793/0xc80 net/netlink/af_netlink.c:1919 > sock_sendmsg_nosec net/socket.c:651 [inline] > sock_sendmsg+0x139/0x170 net/socket.c:671 > sys_sendmsg+0x658/0x7d0 net/socket.c:2353 > ___sys_sendmsg+0xf8/0x170 net/socket.c:2407 > __sys_sendmsg+0xd3/0x190 net/socket.c:2440 > do_syscall_64+0x33/0x40 arch/x86/entry/common.c:46 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Reported-by: Hulk Robot > Signed-off-by: Wang Hai Nik, is this the way you want to handle this? Should the notifier not fail if sysfs files cannot be created? Currently br_sysfs_addbr() returns an int but the only caller ignores it. > diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c > index 7db06e3f642a..1e9cbf31d904 100644 > --- a/net/bridge/br_sysfs_br.c > +++ b/net/bridge/br_sysfs_br.c > @@ -991,6 +991,9 @@ void br_sysfs_delbr(struct net_device *dev) > struct kobject *kobj = >dev.kobj; > struct net_bridge *br = netdev_priv(dev); > > + if (!br->ifobj) > + return; > + > kobject_put(br->ifobj); > sysfs_remove_bin_file(kobj, _forward); > sysfs_remove_group(kobj, _group);
Re: [Bridge] [PATCH net-next v2] bridge: mrp: Implement LC mode for MRP
On Tue, 24 Nov 2020 09:25:25 +0100 Horatiu Vultur wrote: > Extend MRP to support LC mode(link check) for the interconnect port. > This applies only to the interconnect ring. > > Opposite to RC mode(ring check) the LC mode is using CFM frames to > detect when the link goes up or down and based on that the userspace > will need to react. > One advantage of the LC mode over RC mode is that there will be fewer > frames in the normal rings. Because RC mode generates InTest on all > ports while LC mode sends CFM frame only on the interconnect port. > > All 4 nodes part of the interconnect ring needs to have the same mode. > And it is not possible to have running LC and RC mode at the same time > on a node. > > Whenever the MIM starts it needs to detect the status of the other 3 > nodes in the interconnect ring so it would send a frame called > InLinkStatus, on which the clients needs to reply with their link > status. > > This patch adds InLinkStatus frame type and extends existing rules on > how to forward this frame. > > Acked-by: Nikolay Aleksandrov > Signed-off-by: Horatiu Vultur Applied, thanks!
Re: [Bridge] [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, 25 Nov 2020 04:24:27 -0800 Nick Desaulniers wrote: > I even agree that most of the churn comes from > > case 0: > ++x; > default: > break; And just to spell it out, case ENUM_VALUE1: bla(); break; case ENUM_VALUE2: bla(); default: break; is a fairly idiomatic way of indicating that not all values of the enum are expected to be handled by the switch statement. I really hope the Clang folks are reasonable and merge your patch. > If trivial patches are adding too much to your workload, consider > training a co-maintainer or asking for help from one of your reviewers > whom you trust. I don't doubt it's hard to find maintainers, but > existing maintainers should go out of their way to entrust > co-maintainers especially when they find their workload becomes too > high. And reviewing/picking up trivial patches is probably a great > way to get started. If we allow too much knowledge of any one > subsystem to collect with one maintainer, what happens when that > maintainer leaves the community (which, given a finite lifespan, is an > inevitability)? The burn out point is about enjoying your work and feeling that it matters. It really doesn't make much difference if you're doing something you don't like for 12 hours every day or only in shifts with another maintainer. You'll dislike it either way. Applying a real patch set and then getting a few follow ups the next day for trivial coding things like fallthrough missing or static missing, just because I didn't have the full range of compilers to check with before applying makes me feel pretty shitty, like I'm not doing a good job. YMMV.
Re: [Bridge] [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 23 Nov 2020 17:32:51 -0800 Nick Desaulniers wrote:
> On Sun, Nov 22, 2020 at 8:17 AM Kees Cook wrote:
> > On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > > If none of the 140 patches here fix a real bug, and there is no change
> > > to machine code then it sounds to me like a W=2 kind of a warning.
> >
> > FWIW, this series has found at least one bug so far:
> > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/
> >
>
> So looks like the bulk of these are:
> switch (x) {
> case 0:
> ++x;
> default:
> break;
> }
>
> I have a patch that fixes those up for clang:
> https://reviews.llvm.org/D91895
Re: [Bridge] [PATCH net-next] bridge: mrp: Implement LC mode for MRP
On Mon, 23 Nov 2020 23:31:48 +0100 Horatiu Vultur wrote: > > The existing structs are only present in net-next as well, so if you > > don't mind Horatiu it'd be good to follow up and remove the unused ones > > and move the ones (if any) which are only used by the kernel but not by > > the user space <-> kernel API communication out of include/uapi. > > Maybe we don't refer to the same structs, but I could see that they are > already in net and in Linus' tree. For example the struct > 'br_mrp_ring_topo_hdr'. Or am I missunderstanding something? Ah, scratch that, I thought this was HSR, I should have paid more attention. Nothing we can do about the existing ones, then.
Re: [Bridge] [PATCH net-next] bridge: mrp: Implement LC mode for MRP
On Mon, 23 Nov 2020 16:25:53 +0200 Nikolay Aleksandrov wrote:
> >>> @@ -156,4 +157,10 @@ struct br_mrp_in_link_hdr {
> >>> __be16 interval;
> >>> };
> >>>
> >>> +struct br_mrp_in_link_status_hdr {
> >>> + __u8 sa[ETH_ALEN];
> >>> + __be16 port_role;
> >>> + __be16 id;
> >>> +};
> >>> +
> >>
> >> I didn't see this struct used anywhere, am I missing anything?
> >
> > Yes, you are right, the struct is not used any. But I put it there as I
> > put the other frame types for MRP.
> >
>
> I see, we don't usually add unused code. The patch is fine as-is and since
> this is already the case for other MRP parts I'm not strictly against it, so:
>
> Acked-by: Nikolay Aleksandrov
>
> If Jakub decides to adhere to that rule you can keep my acked-by and just
> remove
> the struct for v2.
Yes, good catch, let's drop it, we don't want to make too much of
a precedent for using kernel uAPI headers as a place to provide
protocol-related structs if the kernel doesn't need them.
The existing structs are only present in net-next as well, so if you
don't mind Horatiu it'd be good to follow up and remove the unused ones
and move the ones (if any) which are only used by the kernel but not by
the user space <-> kernel API communication out of include/uapi.
Re: [Bridge] [PATCH net-next] net: bridge: switch to net core statistics counters handling
On Fri, 20 Nov 2020 12:22:23 +0100 Heiner Kallweit wrote: > Use netdev->tstats instead of a member of net_bridge for storing > a pointer to the per-cpu counters. This allows us to use core > functionality for statistics handling. > > Signed-off-by: Heiner Kallweit Applied, thanks!
Re: [Bridge] [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > This series aims to fix almost all remaining fall-through warnings in > > > order to enable -Wimplicit-fallthrough for Clang. > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > > add multiple break/goto/return/fallthrough statements instead of just > > > letting the code fall through to the next case. > > > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > > change[1] is meant to be reverted at some point. So, this patch helps > > > to move in that direction. > > > > > > Something important to mention is that there is currently a discrepancy > > > between GCC and Clang when dealing with switch fall-through to empty case > > > statements or to cases that only contain a break/continue/return > > > statement[2][3][4]. > > > > Are we sure we want to make this change? Was it discussed before? > > > > Are there any bugs Clangs puritanical definition of fallthrough helped > > find? > > > > IMVHO compiler warnings are supposed to warn about issues that could > > be bugs. Falling through to default: break; can hardly be a bug?! > > It's certainly a place where the intent is not always clear. I think > this makes all the cases unambiguous, and doesn't impact the machine > code, since the compiler will happily optimize away any behavioral > redundancy. If none of the 140 patches here fix a real bug, and there is no change to machine code then it sounds to me like a W=2 kind of a warning. I think clang is just being annoying here, but if I'm the only one who feels this way chances are I'm wrong :)
Re: [Bridge] [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > This series aims to fix almost all remaining fall-through warnings in > order to enable -Wimplicit-fallthrough for Clang. > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > add multiple break/goto/return/fallthrough statements instead of just > letting the code fall through to the next case. > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > change[1] is meant to be reverted at some point. So, this patch helps > to move in that direction. > > Something important to mention is that there is currently a discrepancy > between GCC and Clang when dealing with switch fall-through to empty case > statements or to cases that only contain a break/continue/return > statement[2][3][4]. Are we sure we want to make this change? Was it discussed before? Are there any bugs Clangs puritanical definition of fallthrough helped find? IMVHO compiler warnings are supposed to warn about issues that could be bugs. Falling through to default: break; can hardly be a bug?!
Re: [Bridge] [PATCH net-next] net: bridge: replace struct br_vlan_stats with pcpu_sw_netstats
On Tue, 17 Nov 2020 21:25:42 +0100 Heiner Kallweit wrote: > Struct br_vlan_stats duplicates pcpu_sw_netstats (apart from > br_vlan_stats not defining an alignment requirement), therefore > switch to using the latter one. > > Signed-off-by: Heiner Kallweit Applied, thanks!
Re: [Bridge] [PATCH net] net: bridge: add missing counters to ndo_get_stats64 callback
On Fri, 13 Nov 2020 10:27:27 +0100 Heiner Kallweit wrote:
> In br_forward.c and br_input.c fields dev->stats.tx_dropped and
> dev->stats.multicast are populated, but they are ignored in
> ndo_get_stats64.
>
> Fixes: 28172739f0a2 ("net: fix 64 bit counters on 32 bit arches")
> Signed-off-by: Heiner Kallweit
> ---
> Patch will not apply cleanly on kernel versions that don't have
> dev_fetch_sw_netstats() yet.
Looks straightforward enough, I'll backport manually.
Applied, thanks!
Re: [Bridge] [PATCH net-next] bridge: mrp: Use hlist_head instead of list_head for mrp
On Fri, 6 Nov 2020 22:50:49 +0100 Horatiu Vultur wrote: > Replace list_head with hlist_head for MRP list under the bridge. > There is no need for a circular list when a linear list will work. > This will also decrease the size of 'struct net_bridge'. > > Signed-off-by: Horatiu Vultur Applied, thanks!
Re: [Bridge] [RFC net-next 00/28] ndo_ioctl rework
On Fri, 6 Nov 2020 23:17:15 +0100 Arnd Bergmann wrote: > Any suggestions on how to proceed? I think the ndo_siocdevprivate > change is the most interesting here, and I would like to get > that merged. Splitting out / eliminating ioctl pass-thry in general seems like a nice clean up. How did you get the ndo_eth_ioctl patch done, was it manual work? > For the wireless drivers, removing the old drivers > instead of just the dead code might be an alternative, depending > on whether anyone thinks there might still be users. Dunno if you want to dig into removal with a series like this, anything using ioctls will be pretty old (with the exception of what you separated into ndo_eth_ioctl). You may get bogged down.
Re: [Bridge] [PATCH net-next 00/16] selftests: net: bridge: add tests for MLDv2
On Tue, 3 Nov 2020 19:23:56 +0200 Nikolay Aleksandrov wrote: > This is the second selftests patch-set for the new multicast functionality > which adds tests for the bridge's MLDv2 support. The tests use full > precooked packets which are sent via mausezahn and the resulting state > after each test is checked for proper X,Y sets, (*,G) source list, source > list entry timers, (S,G) existence and flags, packet forwarding and > blocking, exclude group expiration and (*,G) auto-add. The first 3 patches > factor out common functions which are used by IGMPv3 tests in lib.sh and > add support for IPv6 test UDP packet, then patch 4 adds the first test with > the initial MLDv2 setup. > The following new tests are added: > - base case: MLDv2 report ff02::cc is_include > - include -> allow report > - include -> is_include report > - include -> is_exclude report > - include -> to_exclude report > - exclude -> allow report > - exclude -> is_include report > - exclude -> is_exclude report > - exclude -> to_exclude report > - include -> block report > - exclude -> block report > - exclude timeout (move to include + entry deletion) > - S,G port entry automatic add to a *,G,exclude port > > The variable names and set notation are the same as per RFC 3810, > for more information check RFC 3810 sections 2.3 and 7. Applied, thank you!
Re: [Bridge] [PATCH net-next] net: bridge: explicitly convert between mdb entry state and port group flags
On Thu, 29 Oct 2020 01:48:15 +0200 Vladimir Oltean wrote: > When creating a new multicast port group, there is implicit conversion > between the __u8 state member of struct br_mdb_entry and the unsigned > char flags member of struct net_bridge_port_group. This implicit > conversion relies on the fact that MDB_PERMANENT is equal to > MDB_PG_FLAGS_PERMANENT. > > Let's be more explicit and convert the state to flags manually. > > Signed-off-by: Vladimir Oltean Applied, thanks!
Re: [Bridge] [PATCH v4 net-next] net: bridge: mcast: add support for raw L2 multicast groups
On Thu, 29 Oct 2020 01:38:31 +0200 Vladimir Oltean wrote: > From: Nikolay Aleksandrov > > Extend the bridge multicast control and data path to configure routes > for L2 (non-IP) multicast groups. > > The uapi struct br_mdb_entry union u is extended with another variant, > mac_addr, which does not change the structure size, and which is valid > when the proto field is zero. > > To be compatible with the forwarding code that is already in place, > which acts as an IGMP/MLD snooping bridge with querier capabilities, we > need to declare that for L2 MDB entries (for which there exists no such > thing as IGMP/MLD snooping/querying), that there is always a querier. > Otherwise, these entries would be flooded to all bridge ports and not > just to those that are members of the L2 multicast group. > > Needless to say, only permanent L2 multicast groups can be installed on > a bridge port. > > Signed-off-by: Nikolay Aleksandrov > Signed-off-by: Vladimir Oltean Applied, thanks!
Re: [Bridge] [PATCH net-next 00/16] selftests: net: bridge: add tests for IGMPv3
On Tue, 27 Oct 2020 20:59:18 +0200 Nikolay Aleksandrov wrote: > This set adds tests for the bridge's new IGMPv3 support. The tests use > precooked packets which are sent via mausezahn and the resulting state > after each test is checked for proper X,Y sets, (*,G) source list, source > list entry timers, (S,G) existence and flags, packet forwarding and > blocking, exclude group expiration and (*,G) auto-add. The first 3 patches > prepare the existing IGMPv2 tests, then patch 4 adds new helpers which are > used throughout the rest of the v3 tests. > The following new tests are added: > - base case: IGMPv3 report 239.10.10.10 is_include (A) > - include -> allow report > - include -> is_include report > - include -> is_exclude report > - include -> to_exclude report > - exclude -> allow report > - exclude -> is_include report > - exclude -> is_exclude report > - exclude -> to_exclude report > - include -> block report > - exclude -> block report > - exclude timeout (move to include + entry deletion) > - S,G port entry automatic add to a *,G,exclude port > > The variable names and set notation are the same as per RFC 3376, > for more information check RFC 3376 sections 4.2.15 and 6.4.1. > MLDv2 tests will be added by a separate patch-set. Applied, thanks Nik!
Re: [Bridge] [PATCH net-next v7 00/10] net: bridge: cfm: Add support for Connectivity Fault Management(CFM)
On Tue, 27 Oct 2020 10:02:41 + Henrik Bjoernlund wrote: > Connectivity Fault Management (CFM) is defined in 802.1Q > section 12.14. > > Connectivity Fault Management (CFM) comprises capabilities for > detecting, verifying, and isolating connectivity failures in Virtual > Bridged Networks. These capabilities can be used in networks > operated by multiple independent organizations, each with restricted > management access to each other’s equipment. Applied, thanks!
Re: [Bridge] [PATCH net-next v6 07/10] bridge: cfm: Netlink SET configuration Interface.
On Mon, 19 Oct 2020 08:51:04 + Henrik Bjoernlund wrote:
> Thank you for the review. Comments below.
>
> The 10/15/2020 10:34, Jakub Kicinski wrote:
> >
> > On Thu, 15 Oct 2020 11:54:15 + Henrik Bjoernlund wrote:
> > > + [IFLA_BRIDGE_CFM_MEP_CONFIG_MDLEVEL] = {
> > > + .type = NLA_U32, .validation_type = NLA_VALIDATE_MAX, .max = 7 },
> >
> > NLA_POLICY_MAX(NLA_U32, 7)
>
> I will change as requested.
>
> >
> > Also why did you keep the validation in the code in patch 4?
>
> In patch 4 there is no CFM NETLINK so I desided to keep the validation in the
> code until NETLINK was added that is now doing the check.
> I this a problem?
Nothing calls those functions until patch 7, so there's no need for
that code to be added.
Re: [Bridge] [PATCH net-next v6 07/10] bridge: cfm: Netlink SET configuration Interface.
On Thu, 15 Oct 2020 11:54:15 + Henrik Bjoernlund wrote:
> + [IFLA_BRIDGE_CFM_MEP_CONFIG_MDLEVEL] = {
> + .type = NLA_U32, .validation_type = NLA_VALIDATE_MAX, .max = 7 },
NLA_POLICY_MAX(NLA_U32, 7)
Also why did you keep the validation in the code in patch 4?
Re: [Bridge] [PATCH net-next v5 08/10] bridge: cfm: Netlink GET configuration Interface.
On Mon, 12 Oct 2020 14:04:26 + Henrik Bjoernlund wrote: > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_SEQ_NO_UPDATE, > + mep->cc_ccm_tx_info.seq_no_update)) > + goto nla_put_failure; > + > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_PERIOD, > + mep->cc_ccm_tx_info.period)) > + goto nla_put_failure; > + > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_IF_TLV, > + mep->cc_ccm_tx_info.if_tlv)) > + goto nla_put_failure; > + > + if (nla_put_u8(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_IF_TLV_VALUE, > +mep->cc_ccm_tx_info.if_tlv_value)) > + goto nla_put_failure; > + > + if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_PORT_TLV, > + mep->cc_ccm_tx_info.port_tlv)) > + goto nla_put_failure; > + > + if (nla_put_u8(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_PORT_TLV_VALUE, > +mep->cc_ccm_tx_info.port_tlv_value)) > + goto nla_put_failure; Consider collapsing writing related attrs in a nest into a single if statement: if (nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_SEQ_NO_UPDATE, mep->cc_ccm_tx_info.seq_no_update) || nla_put_u32(skb, IFLA_BRIDGE_CFM_CC_CCM_TX_PERIOD, mep->cc_ccm_tx_info.period) || ...
Re: [Bridge] [PATCH net-next v5 04/10] bridge: cfm: Kernel space implementation of CFM. MEP create/delete.
On Mon, 12 Oct 2020 14:04:22 + Henrik Bjoernlund wrote:
> + if (config->mdlevel > 7) {
> + NL_SET_ERR_MSG_MOD(extack,
> +"MD level is invalid");
> + return -EINVAL;
> + }
> + /* The MEP-ID is a 13 bit field in the CCM PDU identifying the MEP */
> + if (config->mepid > 0x1FFF) {
> + NL_SET_ERR_MSG_MOD(extack,
> +"MEP-ID is invalid");
> + return -EINVAL;
> + }
If I'm reading patch 7 correctly these parameters come from netlink,
right? In that case please use the netlink policies to check things
like ranges or correct values.
Re: [Bridge] [PATCH net-next v5 06/10] bridge: cfm: Kernel space implementation of CFM. CCM frame RX added.
On Mon, 12 Oct 2020 14:04:24 + Henrik Bjoernlund wrote: > + /* This CCM related status is based on the latest received CCM PDU. */ > + u8 port_tlv_value; /* Port Status TLV value */ > + u8 if_tlv_value; /* Interface Status TLV value */ > + > + /* CCM has not been received for 3.25 intervals */ > + bool ccm_defect; > + > + /* (RDI == 1) for last received CCM PDU */ > + bool rdi; > + > + /* Indications that a CCM PDU has been seen. */ > + bool seen; /* CCM PDU received */ > + bool tlv_seen; /* CCM PDU with TLV received */ > + /* CCM PDU with unexpected sequence number received */ > + bool seq_unexp_seen; Please consider using a u8 bitfield rather than a bunch of bools, if any of this structures are expected to have many instances. That'd save space.
Re: [Bridge] [PATCH net-next v5 06/10] bridge: cfm: Kernel space implementation of CFM. CCM frame RX added.
On Mon, 12 Oct 2020 14:04:24 + Henrik Bjoernlund wrote:
> +struct br_cfm_status_tlv {
> + __u8 type;
> + __be16 length;
> + __u8 value;
> +};
This structure is unused (and likely not what you want, since it will
have 2 1 byte while unless you mark length as __packed).
Re: [Bridge] [PATCH net-next v5 04/10] bridge: cfm: Kernel space implementation of CFM. MEP create/delete.
On Mon, 12 Oct 2020 14:04:22 + Henrik Bjoernlund wrote: > with restricted management access to each other<80><99>s equipment. Some Unicode funk in this line?
