| Message ID | 20210628220011.1910096-2-olteanv@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RX filtering in DSA | expand |
On 29/06/2021 00:59, Vladimir Oltean wrote: > From: Tobias Waldekranz <tobias@waldekranz.com> > > Treat addresses added to the bridge itself in the same way as regular > ports and send out a notification so that drivers may sync it down to > the hardware FDB. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > net/bridge/br_fdb.c | 4 ++-- > net/bridge/br_private.h | 7 ++++--- > net/bridge/br_switchdev.c | 11 +++++------ > 3 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index 16f9434fdb5d..0296d737a519 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -602,7 +602,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, > /* fastpath: update of existing entry */ > if (unlikely(source != fdb->dst && > !test_bit(BR_FDB_STICKY, &fdb->flags))) { > - br_switchdev_fdb_notify(fdb, RTM_DELNEIGH); > + br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH); > fdb->dst = source; > fdb_modified = true; > /* Take over HW learned entry */ > @@ -794,7 +794,7 @@ static void fdb_notify(struct net_bridge *br, > int err = -ENOBUFS; > > if (swdev_notify) > - br_switchdev_fdb_notify(fdb, type); > + br_switchdev_fdb_notify(br, fdb, type); > > skb = nlmsg_new(fdb_nlmsg_size(), GFP_ATOMIC); > if (skb == NULL) > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index a684d0cfc58c..2b48b204205e 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -1654,8 +1654,8 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p, > unsigned long flags, > unsigned long mask, > struct netlink_ext_ack *extack); > -void br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, > - int type); > +void br_switchdev_fdb_notify(struct net_bridge *br, > + const struct net_bridge_fdb_entry *fdb, int type); > int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, u16 flags, > struct netlink_ext_ack *extack); > int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid); > @@ -1702,7 +1702,8 @@ static inline int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid) > } > > static inline void > -br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) > +br_switchdev_fdb_notify(struct net_bridge *br, > + const struct net_bridge_fdb_entry *fdb, int type) > { > } > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > index a5e601e41cb9..9a707da79dfe 100644 > --- a/net/bridge/br_switchdev.c > +++ b/net/bridge/br_switchdev.c > @@ -108,7 +108,8 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p, > } > > void > -br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) > +br_switchdev_fdb_notify(struct net_bridge *br, > + const struct net_bridge_fdb_entry *fdb, int type) > { > struct switchdev_notifier_fdb_info info = { > .addr = fdb->key.addr.addr, > @@ -117,18 +118,16 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) > .is_local = test_bit(BR_FDB_LOCAL, &fdb->flags), > .offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags), > }; > - > - if (!fdb->dst) > - return; > + struct net_device *dev = fdb->dst ? fdb->dst->dev : br->dev; you should use READ_ONCE() for fdb->dst here to make sure it's read only once, to be fair the old code had the same issue :) > > switch (type) { > case RTM_DELNEIGH: > call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_DEVICE, > - fdb->dst->dev, &info.info, NULL); > + dev, &info.info, NULL); > break; > case RTM_NEWNEIGH: > call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_DEVICE, > - fdb->dst->dev, &info.info, NULL); > + dev, &info.info, NULL); > break; > } > } >
On Tue, Jun 29, 2021 at 01:40:20PM +0300, Nikolay Aleksandrov wrote: > > @@ -117,18 +118,16 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) > > .is_local = test_bit(BR_FDB_LOCAL, &fdb->flags), > > .offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags), > > }; > > - > > - if (!fdb->dst) > > - return; > > + struct net_device *dev = fdb->dst ? fdb->dst->dev : br->dev; > > you should use READ_ONCE() for fdb->dst here to make sure it's read only once, > to be fair the old code had the same issue :) Thanks for the comment. I still have budget for one patch until I hit the 15 limit, so I guess I'll do that separately before this one. Just trying to make sure I get it right. You want me to annotate fdb_create(), br_fdb_update(), fdb_add_entry() and br_fdb_external_learn_add() with WRITE_ONCE() too, right? Can I resend right away or did you notice other issues in the other patches?
On 29/06/2021 14:35, Vladimir Oltean wrote: > On Tue, Jun 29, 2021 at 01:40:20PM +0300, Nikolay Aleksandrov wrote: >>> @@ -117,18 +118,16 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) >>> .is_local = test_bit(BR_FDB_LOCAL, &fdb->flags), >>> .offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags), >>> }; >>> - >>> - if (!fdb->dst) >>> - return; >>> + struct net_device *dev = fdb->dst ? fdb->dst->dev : br->dev; >> >> you should use READ_ONCE() for fdb->dst here to make sure it's read only once, >> to be fair the old code had the same issue :) > > Thanks for the comment. I still have budget for one patch until I hit > the 15 limit, so I guess I'll do that separately before this one. > Just trying to make sure I get it right. You want me to annotate > fdb_create(), br_fdb_update(), fdb_add_entry() and > br_fdb_external_learn_add() with WRITE_ONCE() too, right? > Can I resend right away or did you notice other issues in the other > patches? > That would be best, yes. The rest of the changes look good to me. Thanks, Nik
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index 16f9434fdb5d..0296d737a519 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -602,7 +602,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, /* fastpath: update of existing entry */ if (unlikely(source != fdb->dst && !test_bit(BR_FDB_STICKY, &fdb->flags))) { - br_switchdev_fdb_notify(fdb, RTM_DELNEIGH); + br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH); fdb->dst = source; fdb_modified = true; /* Take over HW learned entry */ @@ -794,7 +794,7 @@ static void fdb_notify(struct net_bridge *br, int err = -ENOBUFS; if (swdev_notify) - br_switchdev_fdb_notify(fdb, type); + br_switchdev_fdb_notify(br, fdb, type); skb = nlmsg_new(fdb_nlmsg_size(), GFP_ATOMIC); if (skb == NULL) diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index a684d0cfc58c..2b48b204205e 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -1654,8 +1654,8 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p, unsigned long flags, unsigned long mask, struct netlink_ext_ack *extack); -void br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, - int type); +void br_switchdev_fdb_notify(struct net_bridge *br, + const struct net_bridge_fdb_entry *fdb, int type); int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, u16 flags, struct netlink_ext_ack *extack); int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid); @@ -1702,7 +1702,8 @@ static inline int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid) } static inline void -br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) +br_switchdev_fdb_notify(struct net_bridge *br, + const struct net_bridge_fdb_entry *fdb, int type) { } diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index a5e601e41cb9..9a707da79dfe 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -108,7 +108,8 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p, } void -br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) +br_switchdev_fdb_notify(struct net_bridge *br, + const struct net_bridge_fdb_entry *fdb, int type) { struct switchdev_notifier_fdb_info info = { .addr = fdb->key.addr.addr, @@ -117,18 +118,16 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type) .is_local = test_bit(BR_FDB_LOCAL, &fdb->flags), .offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags), }; - - if (!fdb->dst) - return; + struct net_device *dev = fdb->dst ? fdb->dst->dev : br->dev; switch (type) { case RTM_DELNEIGH: call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_DEVICE, - fdb->dst->dev, &info.info, NULL); + dev, &info.info, NULL); break; case RTM_NEWNEIGH: call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_DEVICE, - fdb->dst->dev, &info.info, NULL); + dev, &info.info, NULL); break; } }