| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 23 May 2023 09:20:42 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [Intel-wired-lan] [PATCH iwl-next v3 01/10] ice: Minor switchdev
fixes
> -----Original Message-----
> From: Paul Menzel <pmenzel@...gen.mpg.de>
> Sent: poniedziałek, 22 maja 2023 12:06
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v3 01/10] ice: Minor switchdev fixes
>
> Dear Wojciech,
>
>
> Thank you for your patches.
Thanks for review Paul!
>
> Am 22.05.23 um 11:05 schrieb Wojciech Drewek:
> > Introduce a few fixes that are needed for bridge offload
> > to work properly.
> >
> > - Skip adv rule removal in ice_eswitch_disable_switchdev.
> > Advanced rules for ctrl VSI will be removed anyway when the
> > VSI will cleaned up, no need to do it explicitly.
> >
> > - Don't allow to change promisc mode in switchdev mode.
> > When switchdev is configured, PF netdev is set to be a
> > default VSI. This is needed for the slow-path to work correctly.
> > All the unmatched packets will be directed to PF netdev.
> >
> > It is possible that this setting might be overwritten by
> > ndo_set_rx_mode. Prevent this by checking if switchdev is
> > enabled in ice_set_rx_mode.
> >
> > - Disable vlan pruning for uplink VSI. In switchdev mode, uplink VSI
> > is configured to be default VSI which means it will receive all
> > unmatched packets. In order to receive vlan packets we need to
> > disable vlan pruning as well. This is done by dis_rx_filtering
> > vlan op.
> >
> > - There is possibility that ice_eswitch_port_start_xmit might be
> > called while some resources are still not allocated which might
> > cause NULL pointer dereference. Fix this by checking if switchdev
> > configuration was finished.
>
> If you enumerate/list changes in a commit message, it’s a good indicator
> to make one patch/commit for each item. ;-) Doing this also makes it
> easier to use a statement as the commit message summary, that means
> using a verb (in imperative mood) – Fix minor switchdev things – and
> making `git log --oneline`) more useful. Smaller commits are also easier
> to revert or to backport.
Originally this patchset had 12 patches so didn't want to have to many
patches in it. I agree that this patch should be splitted, since two of them were dropped,
>
> > Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>
> > ---
> > v2: enclose bitops into separate set of braces, move
> > ice_is_switchdev_running check to ice_set_rx_mode
> > from ice_vsi_sync_fltr
> > ---
> > drivers/net/ethernet/intel/ice/ice_eswitch.c | 14 +++++++++++++-
> > drivers/net/ethernet/intel/ice/ice_main.c | 4 ++--
> > 2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> > index ad0a007b7398..bfd003135fc8 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> > @@ -103,6 +103,10 @@ static int ice_eswitch_setup_env(struct ice_pf *pf)
> > rule_added = true;
> > }
> >
> > + vlan_ops = ice_get_compat_vsi_vlan_ops(uplink_vsi);
> > + if (vlan_ops->dis_rx_filtering(uplink_vsi))
> > + goto err_dis_rx;
> > +
> > if (ice_vsi_update_security(uplink_vsi, ice_vsi_ctx_set_allow_override))
> > goto err_override_uplink;
> >
> > @@ -114,6 +118,8 @@ static int ice_eswitch_setup_env(struct ice_pf *pf)
> > err_override_control:
> > ice_vsi_update_security(uplink_vsi, ice_vsi_ctx_clear_allow_override);
> > err_override_uplink:
> > + vlan_ops->ena_rx_filtering(uplink_vsi);
> > +err_dis_rx:
> > if (rule_added)
> > ice_clear_dflt_vsi(uplink_vsi);
> > err_def_rx:
> > @@ -331,6 +337,9 @@ ice_eswitch_port_start_xmit(struct sk_buff *skb, struct net_device *netdev)
> > np = netdev_priv(netdev);
> > vsi = np->vsi;
> >
> > + if (!vsi || !ice_is_switchdev_running(vsi->back))
> > + return NETDEV_TX_BUSY;
> > +
> > if (ice_is_reset_in_progress(vsi->back->state) ||
> > test_bit(ICE_VF_DIS, vsi->back->state))
> > return NETDEV_TX_BUSY;
> > @@ -378,9 +387,13 @@ static void ice_eswitch_release_env(struct ice_pf *pf)
> > {
> > struct ice_vsi *uplink_vsi = pf->switchdev.uplink_vsi;
> > struct ice_vsi *ctrl_vsi = pf->switchdev.control_vsi;
> > + struct ice_vsi_vlan_ops *vlan_ops;
> > +
> > + vlan_ops = ice_get_compat_vsi_vlan_ops(uplink_vsi);
> >
> > ice_vsi_update_security(ctrl_vsi, ice_vsi_ctx_clear_allow_override);
> > ice_vsi_update_security(uplink_vsi, ice_vsi_ctx_clear_allow_override);
> > + vlan_ops->ena_rx_filtering(uplink_vsi);
> > ice_clear_dflt_vsi(uplink_vsi);
> > ice_fltr_add_mac_and_broadcast(uplink_vsi,
> > uplink_vsi->port_info->mac.perm_addr,
> > @@ -503,7 +516,6 @@ static void ice_eswitch_disable_switchdev(struct ice_pf *pf)
> >
> > ice_eswitch_napi_disable(pf);
> > ice_eswitch_release_env(pf);
> > - ice_rem_adv_rule_for_vsi(&pf->hw, ctrl_vsi->idx);
> > ice_eswitch_release_reprs(pf, ctrl_vsi);
> > ice_vsi_release(ctrl_vsi);
> > ice_repr_rem_from_all_vfs(pf);
> > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> > index b0d1e6116eb9..80b2b4d39278 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > @@ -385,7 +385,7 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
> > }
> > err = 0;
> > /* check for changes in promiscuous modes */
> > - if (changed_flags & IFF_ALLMULTI) {
> > + if ((changed_flags & IFF_ALLMULTI)) {
> > if (vsi->current_netdev_flags & IFF_ALLMULTI) {
> > err = ice_set_promisc(vsi, ICE_MCAST_PROMISC_BITS);
> > if (err) {
> > @@ -5767,7 +5767,7 @@ static void ice_set_rx_mode(struct net_device *netdev)
> > struct ice_netdev_priv *np = netdev_priv(netdev);
> > struct ice_vsi *vsi = np->vsi;
> >
> > - if (!vsi)
> > + if (!vsi || ice_is_switchdev_running(vsi->back))
> > return;
> >
> > /* Set the flags to synchronize filters
>
> The diff itself looks good.
>
> Reviewed-by: Paul Menzel <pmenzel@...gen.mpg.de>
>
>
> Kind regards,
>
> Paul
Powered by blists - more mailing lists